Add EIP: Security Flaws in ERC-20 Transfer Patterns #7915
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| --- | ||
| eip: 7544 | ||
| title: Security Flaws in ERC-20 Transfer Patterns | ||
| description: Description of a security flaw in ERC-20 transferring workflow that caused a loss of $201,690,741 as of 17.08.2023. | ||
| author: Dexaran (@Dexaran) <dexaran@ethereumclassic.org>, Vladimir Vencálek <vladimir@callisto.network>, Yuriy Kharytoshin (@yuriy77k) <yuriy@callisto.network>, Laurent Riche (@spatialiste) <tonton@callisto.network> | ||
| discussions-to: https://ethereum-magicians.org/t/disclosure-of-a-security-flaw-in-erc-20-transferring-workflow/16249 | ||
| status: Draft | ||
| type: Informational | ||
| created: 2023-10-24 | ||
| requires: 20, 1363 | ||
| --- | ||
|
|
||
| ## 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). | ||
|
There was a problem hiding this comment. 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. |
||
|
|
||
| ## Specification | ||
|
|
||
| ### [ERC-20](./eip-20.md) design overview | ||
Dexaran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| [ERC-20](./eip-20.md) standard declares two methods of transferring tokens: (1) `transfer` function and (2) `approve` & `transferFrom` pattern. `approve` & `transferFrom` is supposed to be used to deposit tokens to contracts. The `transfer` function is supposed to be used for transfers between externally owned addresses however this is not directly written in the specification. If the tokens are sent to a contract address via the `transfer` function then the recipient contract will not recognize the deposit. | ||
Dexaran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Security flaw | ||
|
|
||
| **The `transfer` function does not notify the recipient of an incoming transaction which makes error handling impossible.** Error handling is an essential part of secure software development. If tokens are sent to any contract via the `transfer` function and the recipient contract does not support extraction of tokens (i.e. it doesn't implement any functions which would allow to send tokens out) then it is a clear case of user error that must be reverted. For example if a user would send plain ether to a contract that does not explicitly declare that it is intended to accept ether deposits then such transaction would be reverted automatically. In case of [ERC-20](./eip-20.md) tokens a user can push the token contract into incorrect state where a user no longer controls the tokens by picking a wrong function when performing a transaction. | ||
|
|
||
| **A burden of determining the method of transferring tokens is placed on the user in a situation where one option is obviously wrong and will result in a loss of funds.** Prompting a user to make a decision on the internal logic of the contract combined with the lack of an implementation of error handling for users actions is another security failure that can result in incorrect token contract behavior and a loss of funds for the end user. | ||
|
|
||
| ### Token losses | ||
|
|
||
| As of 17.08.2023 $201,690,741 worth of [ERC-20](./eip-20.md) tokens were lost in top 50 (measured by transactions count) token contracts due to the described problem of the standard. | ||
|
|
||
| ## 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. | ||
|
There was a problem hiding this comment. 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. |
||
|
|
||
| ## Security Considerations | ||
|
|
||
| - The best option is to avoid using [ERC-20](./eip-20.md) tokens and to upgrade the ecosystem to a more secure token standard that (1) **properly supports error handling and notifies recipients** of incoming transfers, (2) **automatically determines the transferring method** if token transfers to externally owned accounts and to contracts are performed differently without prompring a user to make any decisions that can potentially push the contract in incorrect state or result in any errors, (3) **abstracts the user from the internal logic of the contract**. | ||
| - It could be possible to apply a patch on [ERC-20](./eip-20.md) standard by disallowing the use of `transfer` function to deposit tokens to contracts. This would introduce a breaking change to the "final" standard and therefore is not possible according to EIP process. | ||
| - Any implementor of a [ERC-20](./eip-20.md) token must be aware of the issue and describe it in the most visible form. Developers and users MUST be notified of an issue. | ||
| - Any developer of a UI that operates with tokens MUST perform additional checks when a user is transferring tokens via `transfer` function. If the recipient is a contract then the user must receive a wraning message stating that he/she is transferring tokens to a contract directly and it can result in tokens being stuck there without an option to recover them. It is a common user mistake that (for some reason) can't be prevented on the contract level. Ideally the UI must also check if the recipient is a contract that is known to not be intended to receive tokens such as a contract of another token, ENS contract or most other contracts except multisigs. | ||
| - Any contract deployed on Ethereum mainnet MUST expect that there would be "stuck" ERC-20 tokens accidentally deposited to the contract address. A special function to withdraw tokens MUST be implemented. | ||
|
|
||
| ``` | ||
| function rescueERC20(address _token) onlyOwner external | ||
| { | ||
| amount = IERC20(_token).balanceOf(address(this)); | ||
| IERC20(_token).transfer(msg.sender, amount); | ||
| } | ||
| ``` | ||
|
|
||
| ## Copyright | ||
|
|
||
| Copyright and related rights waived via [CC0](../LICENSE.md). | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xinbenlv
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.