ethcatherders / EIPIP

EIP Improvement Process
77 stars 38 forks source link

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

Closed SamWilsn closed 5 months ago

SamWilsn commented 8 months ago

Call for Input

Decision Do we merge https://github.com/ethereum/EIPs/pull/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 https://github.com/ethereum/EIPs/pull/7915 for the full details.

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

SamWilsn commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

@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 https://github.com/ethereum/EIPs/pull/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:

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 commented 8 months ago

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 commented 8 months ago

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.
SamWilsn commented 6 months ago

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

g11tech commented 6 months ago

Opposed, a new EIP proposing improvements can be done

SamWilsn commented 5 months ago

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.