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

ERC20 standard implementation bug. #729

Closed
1 of 2 tasks
Dexaran opened this issue Feb 11, 2018 · 2 comments
Closed
1 of 2 tasks

ERC20 standard implementation bug. #729

Dexaran opened this issue Feb 11, 2018 · 2 comments

Comments

@Dexaran
Copy link

Dexaran commented Feb 11, 2018

🎉 Description

ERC20 token standard implementation assumes two ways of token transferring: (1) transfer function and (2) approve + transferFrom pattern.

It should be noted that event handling is a well-known and standard practice in programming. In case of token standard, a token transfer should be considered as an event. transfer function of ERC20 does not provide any opportunity to handle the transfer, i.e. it is silently increasing balance of the receiver. It is impossible for the receiver to recognize that transfer occurs if the receiver is a contract. As the result, the only correct way to make a token deposit to a contract is approve + transferFrom pattern.

It is obvious that in case of a mistake, the transfer function MUST throw an error and revert a wrong transaction. Otherwise it will cause negative consequences (lost tokens) for end user. This is a very common and default practice in programming, called Exception Handling: https://en.wikipedia.org/wiki/Exception_handling

This has already led to the loss of millions of dollars for the whole Ethereum ecosystem at the moment. For more information on money loss read Lost Tokens section.

  • 🐛 This is a bug report.
  • 📈 This is a feature request.

💻 Environment

Any version, any compiler, any environment, any network.

📝 Details

For more information: https://gist.github.com/Dexaran/ddb3e89fe64bf2e06ed15fbd5679bd20

👍 Other Information

Both ERC827 and ERC721 inherit ERC20 bug.

@spalladino
Copy link
Contributor

@Dexaran, as you mention in the comments of the gist, standards such as 223 and 777 do not suffer from this issue. Actually, 223 was specifically designed to address this problem. We are in discussions on whether to include either (or both) standards to OpenZeppelin, but regardless of what decision we make, we will not be modifying the current ERC20 standard, which is already finalized.

In other words, the solution for this problem is not to change the current standard, but to promote and adapt a new standard without this problem.

@Dexaran
Copy link
Author

Dexaran commented Feb 17, 2018

@spalladino Yes, but this does not negate the fact that the above contracts are insecure, thus are not recommended to use.

I mean this contracts in token folder in case of OpenZeppelin.

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

2 participants