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.sol doesn't allow transfer to zero address, not standard compliant #1493

Closed
lightclient opened this issue Nov 9, 2018 · 3 comments
Closed

Comments

@lightclient
Copy link

lightclient commented Nov 9, 2018

The ERC-20 Token Standard describes the following for transfer:

transfer
Transfers _value amount of tokens to address _to, and MUST fire the Transfer event. The function SHOULD throw if the _from account balance does not have enough tokens to spend.

The OpenZeppelin ERC20.sol contract adds an additional requirement to this standard by blocking transfers to 0x0000000000000000000000000000000000000000. This opinionated decision should be removed from the base ERC20 implementation. The ERC20Burnable.sol contract is not a solution to this, because it is not compliant with the base standard and will not be interoperable with other contracts that accept any arbitrary ERC20.

As of right now, this token is not compatible with contracts that burn their tokens by transferring them to the zero address.

@lightclient lightclient changed the title ERC20.sol doesn't allow transfer to zero address ERC20.sol doesn't allow transfer to zero address, not standard compliant Nov 9, 2018
@cleanunicorn
Copy link
Contributor

As @zmitton suggested, keeping the require(to != address(0)) in place will prevent bad software that cause empty arg or improper address parsing from sending the funds into the void.

However there is a need to burn tokens, that could be achieved by calling a burn() function which can remove the funds from the user balance and emit a Transfer() event.

Now there's a discussion where the destination of the Transfer() event should be. It could either be address(0) or (another suggestion is) 0xdead, making it specific about sending the funds into the void and decreasing the likelihood of a software bug.

@lightclient
Copy link
Author

  1. I still disagree, the base OpenZeppelin implementation of ERC20 should not be opinionated (i.e. add a require statement that is not described in the standard). These kinds of opinions should live in SafeERC20 and it should be up to developers to make educated decisions on what token to use.
  2. The burn function is only useful if we can guarantee that the ERC20 we are interacting with implements burn. This is impossible in contracts that accept arbitrary ERC20 address from users.
  3. Using 0xdead is a much better place to burn funds too than 0x0.

@frangio
Copy link
Contributor

frangio commented Jul 13, 2019

@c-o-l-o-r Thank you for expressing this concern. Sorry for not replying to it earlier.

OpenZeppelin contracts are opinionated precisely because we don't want the burden of making these decisions to be on the developers. If one of our decisions turns out to be a bad one, we don't want it to be in the library at all, not even as an opt-in thing.

Disallowing a send to the address 0 is compliant with the ERC20 spec, and it's widespread behavior across deployed tokens. The OpenZeppelin implementation is even listed in the spec itself.

I understand that it sucks to not have a standard way to burn tokens in ERC20, but this is a feature that is missing in the spec and not a problem in the implementations.

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

3 participants