Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call for Input: Merge EIP Describing Flaws in ERC-20 #293

Closed
SamWilsn opened this issue Nov 10, 2023 · 9 comments
Closed

Call for Input: Merge EIP Describing Flaws in ERC-20 #293

SamWilsn opened this issue Nov 10, 2023 · 9 comments

Comments

@SamWilsn
Copy link
Collaborator

SamWilsn commented Nov 10, 2023

Call for Input

Decision

Do we merge ethereum/EIPs#7915 ?

If Affirmed

A new Informational EIP is created describing apparent flaws in the ERC-20 standard.

If Rejected No new EIP is created.
Method Rough Consensus
Deadline December 9, 2023

Background

Probably should read ethereum/EIPs#7915 for the full details.

tl;dr transfer allows tokens to be sent to contracts that don't support tokens.

@SamWilsn
Copy link
Collaborator Author

I am opposed to merging this pull request.


Please see https://ethereum-magicians.org/t/modification-of-eip-process-to-account-for-security-treatments/16265/8 for my reasoning.

@abcoathup
Copy link

abcoathup commented Nov 10, 2023

I am against merging (but I don't have a vote).

If this were to be merged it should be in the ERC repo as it is about app layer.

Though I would much prefer a wiki page for each ERC.
An ERC wiki page could have the latest information about security and implementations, be updated/maintained by the community. This would remove the issue of updating final EIPs/ERCs.

@Dexaran
Copy link

Dexaran commented Nov 10, 2023

@abcoathup

Though I would much prefer a wiki page for each ERC.
An ERC wiki page could have the latest information about security and implementations, be updated/maintained by the community. This would remove the issue of updating final EIPs/ERCs.

This is not a correct way to provide sensitive security information if we want to prioritize security of our users. And I think we MUST prioritize security as we are dealing with financial software.

If some object is particularly dangerous, a warning label must be placed on that object. Placing a warning label somewhere else won't help.

In order to prevent people from getting bleach poisoning the danger warning is placed on the bleach bottle. We can't create a separate web page where we list all the dangerous liquids and pretend that it is informative enough to solve the problem. I think you got the idea.

Thats why I'm advocating for security disclosures in EIPs.

@Dexaran
Copy link

Dexaran commented Nov 10, 2023

@SamWilsn

I am opposed to merging this pull request.
Please see https://ethereum-magicians.org/t/modification-of-eip-process-to-account-for-security-treatments/16265/8 for my reasoning.

In the linked thread he said the following:

"I object to including security considerations surfaced after a proposal becomes final within the body of the proposal itself. To be clear, I do believe that these security considerations should be published somewhere, just not within the EIP.
My objection stems from the question of who has the authority to determine if a security disclosure is worthy of publishing.

I would like to note that this PR ethereum/EIPs#7915 does not propose any changes to any EIP. It only creates a new informational EIP as it was suggested by the EIP editors before.

First, I would blame EIP editors for judging my proposal in the DRAFT status.

As I can read in the EIP-1 and EIP-5069 you should:

  • Read the EIP to check if it is ready: sound and complete. The ideas must make technical sense, even if they don’t seem likely to get to final status.
  • The title should accurately describe the content.
  • Check the EIP for language (spelling, grammar, sentence structure, etc.), markup (GitHub flavored Markdown), code style

And this is what you should not:

EIP5069

I don't see any mentions of any reasons that would prevent me from creating a proposal that does make technical sense. I think that it can be accepted as DRAFT as soon as the language is correct. I would be fine with EIP editors merging it as DRAFT and expressing their concerns regarding the correctness of the proposal during the PEER REVIEW stage. I would be fine if the proposal never gets finalized until all the expressed concerns are resolved.

However I would blame EIP editors for "deciding the winner" here. By not allowing me to publish sensitive info related to one of the proposals you are implying that the proposal is good - which in fact is not true because it caused $90,000,000 to $200,000,000 millions of dollars financial damage to our users.

Also, if you don't have a process or a security expert in your team then it would be much better to let all the security-related proposals to get merged as DRAFTs and never let them past REVIEW in long term. Because if a security expert will once join your team and you will assign him to security-related EIPs and ERCs they will all be there in the DRAFT statuses.

Second, I encourage EIP editors to think about the mission here

We are not discussing a random proposal. We are discussing a real problem. A problem that damages people, endangers their wellbeing and may be even ruins lives.

This problem can be solved technically. This has persisted for years because of everyone's reluctance to take responsibility for announcing it.

Imagine that you are a bank and you are using a database that contains a flaw that was discovered 6 years ago but nobody took responsibility of upgrading yours so you are still using it. As the result your customers lost $90,000,000 to software bugs but you decided to hide this fact. Does it look fair? Would you put your money in such bank? What will people think of it once it gets discovered?

EX26

How do you "foster transparency and ensure that valuable insights from past proposals are accessible" if you silence a disclosure of a problem that caused real financial damage that exceeds the scale of TheDAO hack?

You can probably call multiple reasons for simply ignoring the problem like "we EIP editors don't want to decide XYZ" but you know the problem will not be solved until someone will take responsibility for making some decision that can potentially lead to the solution. Publishing a description of the problem can (in theory) lead to the solution. Refusing to publish the description of the problem will not lead to the solution and in 2024 there will be even more money lost.

@xinbenlv
Copy link

xinbenlv commented Nov 10, 2023

I oppose to merging this pull request.


As I have posted on this comment
I don't think a new Information ERC is helpful in presenting the warning. I offered my recommendation of alternatives in my comment for author to pursue.

@Joeysantoro
Copy link

I am opposed to merging this pull request at this time:

  1. There is an ongoing discussion on how security considerations should fold into the EIP process that is far from resolved: https://ethereum-magicians.org/t/modification-of-eip-process-to-account-for-security-treatments/16265
  2. This particular proposal is too opinionated on the description and resolution of the problem. Many of the solutions proposed in the Security Considerations section are unworkable, force the modification of a Final EIP in nontrivial ways, or introduce other security/centralization concerns.

This was referenced Nov 15, 2023
@poojaranjan poojaranjan mentioned this issue Nov 29, 2023
11 tasks
@poojaranjan poojaranjan mentioned this issue Jan 3, 2024
7 tasks
@SamWilsn
Copy link
Collaborator Author

This is well past its deadline, so last call for comments from editors: @lightclient, @axic, @gcolvin, @g11tech.

@g11tech
Copy link

g11tech commented Jan 15, 2024

Opposed, a new EIP proposing improvements can be done

@poojaranjan poojaranjan mentioned this issue Feb 1, 2024
3 tasks
@SamWilsn
Copy link
Collaborator Author

SamWilsn commented Feb 7, 2024

While I'm not happy closing this issue without input from more Editors, the prevailing opinion in this thread is to not merge this pull request.

@SamWilsn SamWilsn closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants