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

Add EIP: Security Flaws in ERC-20 Transfer Patterns #7915

Closed
wants to merge 16 commits into from

Conversation

Dexaran
Copy link
Contributor

@Dexaran Dexaran commented Oct 24, 2023

This informational EIP describes a security flaw in ERC-20 standard as well as token standards that declare backwards compatibility with ERC-20 (and therefore inherit its security problems).

Previous discussions can be found here:

@Dexaran Dexaran requested a review from eth-bot as a code owner October 24, 2023 21:01
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-informational labels Oct 24, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 24, 2023

File EIPS/eip-7544.md

Requires 1 more reviewers from @axic, @gcolvin, @lightclient, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot changed the title EIP-?: Disclosure of a security flaw in ERC-20 transferring workflow Add EIP: Disclosure of a security flaw in ERC-20 transferring workflow Oct 24, 2023
@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Oct 24, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Oct 24, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Oct 24, 2023
@eth-bot eth-bot changed the title Add EIP: Disclosure of a security flaw in ERC-20 transferring workflow Add EIP: Security flaw disclosure in ERC-20 workflow Oct 24, 2023
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because I'd like to take a closer look at this one before assigning a number. Thanks!

EIPS/eip-7915.md Outdated Show resolved Hide resolved
EIPS/eip-7915.md Outdated Show resolved Hide resolved
Dexaran and others added 2 commits October 25, 2023 17:11
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
@Dexaran
Copy link
Contributor Author

Dexaran commented Oct 25, 2023

Typos & EIP number fixed.

@eth-bot eth-bot changed the title Add EIP: Security flaw disclosure in ERC-20 workflow Add EIP: Security Flaws in ERC-20 Transfer Patterns Oct 25, 2023
@github-actions
Copy link

The commit 58bd593 (as a parent of 894d75b) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Oct 25, 2023
@Dexaran
Copy link
Contributor Author

Dexaran commented Oct 25, 2023

@SamWilsn HTMLProofer failed for some unknown reason. Or at least it is not clear for me what has gone wrong.

Run bundle exec github-pages health-check
  bundle exec github-pages health-check
  shell: /usr/bin/bash -e {0}
To use retry middleware with Faraday v[2](https://github.com/ethereum/EIPs/actions/runs/6640971004/job/18042458972?pr=7915#step:6:2).0+, install `faraday-retry` gem
Checking domain eips.ethereum.org...
Uh oh. Looks like something's fishy: Domain's DNS record could not be retrieved. For more information, see [https://help.github.com/articles/setting-up-a-custom-domain-with-github-pages/.](https://help.github.com/articles/setting-up-a-custom-domain-with-github-pages/) (InvalidDNSError)
Error: Process completed with exit code 1.

@abcoathup
Copy link
Contributor

I think this informational ERC should be migrated to the ERC repo as it is application layer.
Though need confirmation from EIP/ERC editors.

@Dexaran
Copy link
Contributor Author

Dexaran commented Oct 27, 2023

@abcoathup technically this one is not a ERC (Ethereum Request for Comments), it's an Informational-type EIP (just an improvement proposal). However it is directly related to an ERC so it's a bit unclear to me if we want to keep it here or move.

I've asked about it on CatHerders discord but got no response so far https://discord.com/channels/916516304972824576/916713912970412103/1166854647013711872

EIPS/eip-7544.md Outdated Show resolved Hide resolved
@@ -0,0 +1,55 @@
---
eip: 7544
title: Security Flaws in ERC-20 Transfer Patterns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title: Security Flaws in ERC-20 Transfer Patterns
title: Security Considerations in ERC-20 Transfer Patterns

Hi, Dexaran, I think this is a very important issue to discuss, but also I think it's only proper that we discuss this issue in the broader and context: As far as I understand, this essay argues that without "transferAndCall" or "safeTransferFrom" that installs a hook on the receiver side, capital can be loss. If this understanding is correctly, I'd argue: that's true, but currently there are many security considerations that's no less severe than what's presented in this particular article.

For example, if you transfer native ethers to a Address of a "Contract Wallet" but on the wrong chainId, the same problem will occur, fund will get loss and unrecoverable. This also apply to ERC721.safeTransferFrom because if the wrong chain has no wallet deployment, the hook of safeTransferFrom will not have an effect.

Editorially: You have previously suggested to add an section to ERC20 to raise this awareness, and I don't know if it's relevant, to bring to people's attention to solutions like ERC223 or ERC1363, in which the ERC223 you are an author of. And you already stated your advice about ERC20 on your ERC223, I am not sure if having a separate ERC for this security consideration about ERC20 lack of hooks' issue will provide additional value. Instead it could create a false sentiment that this is a community consensus, despite that I personally fully support your argument about this particular security problem. The reason I have concern because if this is considered a "flaw", maybe $ethers transfer is also a flaw, an even bigger flaw.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xinbenlv

you already stated your advice about ERC20 on your ERC223, I am not sure if having a separate ERC for this security consideration about ERC20 lack of hooks' issue will provide additional value.

If something is particularly dangerous then the "DANGER" label must be placed on the dangerous object. We can't stop people getting bleach poisoning by placing a "Dont drink bleach" label on a bottle of coca-cola. If we want people to know that drinking bleach is dangerous then we must put a "Don't drink this" on the bottle of bleach.

Copy link
Contributor Author

@Dexaran Dexaran Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I have concern because if this is considered a "flaw", maybe $ethers transfer is also a flaw, an even bigger flaw.

Yes, there are problems with Ether transfers. However this is outside of the scope of this EIP. I'm not talking about the problems of ether here. I'm talking about the problems of ERC-20.

There are hundreds of millions of dollars lost due to this security flaw of ERC-20: https://dexaran.github.io/erc20-losses

If there are problems with Ether transfers and the money is lost - we can create a separate EIP for this. However it doesn't automatically mean that the problem that I'm describing is less of a problem and millions of dollars lost by users are less lost.

For example, if you transfer native ethers to a Address of a "Contract Wallet" but on the wrong chainId, the same problem will occur, fund will get loss and unrecoverable.

Yes this is a problem but it's not a problem with the token standard. It's a problem of Ethereum address generation that doesn't differentiate between "owned account" and "nobody knows if it's owned or not".

For example in EOS there is no such problem with cross-chain address collisions because there can be no accounts that are not owned by anyone in EOS. I wrote an article about "sending to wrong address" and "on-chain address assignment" few years ago: https://www.eosgo.io/blog/shadowed-advantage-of-eos-that-you-might-not-know/

This is a problem - and this problem is related to the Ethereum architecture. But it's totally a different problem. It's not an application-level problem at all. And it is definitely not a problem of any token standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, this essay argues that without "transferAndCall" or "safeTransferFrom" that installs a hook on the receiver side, capital can be loss.

No, this is not the main point of this proposal.

There are some well-known widely adopted principles of secure software development. One of the most important ones is "error handling is a must" which means - if there is something that is known to be incorrect then you must do something with it.

I can find you any number of research articles, guidelines and papers that say "error handling is important":

However, ERC-20 does not support error handling at all. <-- I'm saying that it's a critical security flaw.

Like if you don't place "onlyOwner" access restriction on a system function in your contract and someone can steal money because of this - it would be a security flaw. Lack of any musthave feature is a security flaw.

Co-authored-by: Andrew B Coathup <28278242+abcoathup@users.noreply.github.com>
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Nov 10, 2023
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure I don't want to accept this proposal, from two perspectives. Either:

  • EIP Editors are not technical experts, and have no business deciding whether a particular security flaw merits an informational EIP.
  • EIP Editors are technical experts, and I disagree that this is a security flaw in ERC-20.

Next step would be a formal Call for Input to get a ruling from the editors.


## Abstract

The following describes a security flaw in the transferring workflow of [ERC-20](./eip-20.md) token standard. It must be taken into account that all token standards that declare full backwards compatibility with [ERC-20](./eip-20.md) also inherit this security flaw, for example [ERC-1363](./eip-1363.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike an introduction, the abstract should be a terse technical summary of the body of the proposal. After reading the abstract, I should have a high level understanding of what the security flaw is.

EIPS/eip-7544.md Outdated Show resolved Hide resolved
EIPS/eip-7544.md Outdated Show resolved Hide resolved

## Rationale

Security flaw disclosures are an important part of software development. Increasing awareness of the problem helps the development community to implement solutions and minimize the damage that a particular flaw can deal to the users.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I know this is a first-of-its-kind proposal, but I think we should probably re-arrange a little bit. Keep a very dry description of the problem/reproduction steps in the Specification section, then describe why it's a problem in the rationale section.

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@Dexaran
Copy link
Contributor Author

Dexaran commented Nov 10, 2023

@SamWilsn

I'm pretty sure I don't want to accept this proposal, from two perspectives. Either:

  • EIP Editors are not technical experts, and have no business deciding whether a particular security flaw merits an informational EIP.
  • EIP Editors are technical experts, and I disagree that this is a security flaw in ERC-20.

Do you think that $90,000,000 lost over 2 years is fine and we don't need to do something to prevent it? https://dexaran.github.io/erc20-losses

Do you think that something that perfectly fits in the "Critical severity security flaw" criteria at OpenZeppelin bug bounty is not a security flaw? OpenZeppelin/openzeppelin-contracts#4474

Do you think that having a token standard that continuously causes users to lose money is better than having a description of its flaws?

What would you recommend if some other flaw is discovered and we need to somehow warn the developers & community about it? What would be the steps that the person who discovered it must take in order to solve the problem?

Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
@xinbenlv
Copy link
Contributor

@SamWilsn

I'm pretty sure I don't want to accept this proposal, from two perspectives. Either:

  • EIP Editors are not technical experts, and have no business deciding whether a particular security flaw merits an informational EIP.
  • EIP Editors are technical experts, and I disagree that this is a security flaw in ERC-20.

Do you think that $90,000,000 lost over 2 years is fine and we don't need to do something to prevent it? https://dexaran.github.io/erc20-losses

Do you think that something that perfectly fits in the "Critical severity security flaw" criteria at OpenZeppelin bug bounty is not a security flaw? OpenZeppelin/openzeppelin-contracts#4474

Do you think that having a token standard that continuously causes users to lose money is better than having a description of its flaws?

What would you recommend if some other flaw is discovered and we need to somehow warn the developers & community about it? What would be the steps that the person who discovered it must take in order to solve the problem?

When you ask "Do you think that $90,000,000 lost over 2 years is fine", I feel that what we suggested has not been well received and understood. Please allow me to reiterate: it's a problem that fund will be locked in this specific case. We are not saying it's fine. We are just saying that I am not convinced that this particular new Information ERC is the best format with editorial concern that I raised above and haven't get answer yet.

For example, the biggest lost of fund is probably lost of private keys, and yet we will not see it being very helpful to publish a separate informational ERC to warn people don't lose their private keys. As far as I know, having an ERC223 is already good enough for you to point people to your argument for raising the awareness. Unless this ERC shows extra merit in serving additional purpose, I think the best way to spend your time is to focus driving adoption of ERC223, such as build some widely used smart contracts, or convince projects to use ERC223, or to take an approach of ERC721A or OpenZepplin to be easily built-upon.

@Dexaran
Copy link
Contributor Author

Dexaran commented Nov 10, 2023

@xinbenlv in fact I've addressed all the raised concerns in my comments above. Let me repeat

the biggest lost of fund is probably lost of private keys, and yet we will not see it being very helpful to publish a separate informational ERC to warn people don't lose their private keys.

  1. I'm pretty sure there are other problems in the world but their presence does not automatically mean that the problem with ERC-20 tokens is not worth solving.
  2. Loss of a private key is not a security vulnerability or a violation of any widely known security rule. I admit it is a problem however and it has to be solved at some point. ERC-20 design has a cruel violation of one of the main principles of secure software design "Error handling is a must".
  3. Loss of a private key is not a problem with some smart-contract, it's a problem with Ethereum protocol. It's a different problem. This issue is about ERC-20 and it's an application-level problem. It is fine that protocol-level and application-level problems can have different solution approaches.

having an ERC223 is already good enough for you to point people to your argument for raising the awareness.

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

If you want to notify people that drinking bleach is dangerous - you put a danger warning on the bottle of bleach. Putting a label "don't drink bleach" on a bottle of coca-cola is not how a problem with bleach is solved.

We are just saying that I am not convinced that this particular new Information ERC is the best format

The current approach of describing it as an informational EIP was recommended by @gcolvin here https://github.com/ethereum-cat-herders/EIPIP/issues/257

EX27

If you guys can recommend me some process that will help me report the problem - I will follow your recommendations if it has a chance of succeeding.

Saying "ERC-223 should be the place to disclose ERC-20 flaws" is a way to silence the problem, not to solve it. If we want people to stop losing money - we don't need to tell devs who are reviewing ERC-223 about the problems of ERC-20 (they already know it). We need to let devs who are considering ERC-20 to know about its problems.

@g11tech
Copy link
Contributor

g11tech commented Nov 22, 2023

imo this is NOT a security flaw but DRAWBACKS... both mean and imply radically different things. loss of funds because of incorrect usage will not classify to a security flaw even though the concern raised is serious.

hence this also SHOULD NOT modify erc20 but setup its own erc standard which developers/eip readers could be made aware of regarding erc20

@Dexaran
Copy link
Contributor Author

Dexaran commented Nov 22, 2023

@g11tech

loss of funds because of incorrect usage will not classify to a security flaw

Loss of funds occurs not because of incorrect usage but because of the improper design of the standard. The root of the problem is not that a user can send tokens to wrong address but the fact that the recipient can't recognize and reject an incorrect transfer.

  • You can't lose ETH by sending to wrong contract address

  • You can't lose NFT by sending to wrong contract address

  • You can't lose ERC-223 / ERC-1155 by sending to wrong contract address

  • You can only lose ERC-20

In all the above cases the "incorrect usage" took place but only in case of ERC-20 it results in catastrophic consequences.

@Dexaran
Copy link
Contributor Author

Dexaran commented Nov 22, 2023

imo this is NOT a security flaw but DRAWBACKS

Idk what your opinion is based on but the described ERC-20 flaw falls into "Critical Security Vulnerability" criteria at OpenZeppelin bugbounty.

https://twitter.com/Dexaran/status/1682826924190494720

255351496-0246fb67-bf3d-494e-97f4-1efcff64a06e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-informational
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants