OpenZeppelin Contracts has quite a strict policy of never allowing zero addresses, by placing checks like require(receiver != address(0)) everywhere.
For the upcoming 3.0 release I would like us to discuss the possibility of removing these checks.
One of the reasons that they were put there in the first place was that initially Solidity didn't verify the length of msg.data, and thus it was possible for a function to be called with missing arguments. If an address argument was missing it would be read as the zero address, and these checks served the purpose of protecting from that kind of accident. Nowadays Solidity inserts checks that msg.data is big enough, so this kind of error is not really possible anymore.
A different reason why these checks were put in place was to track situations where an asset was burnt or some capacity was being renounced. For example, it's not possible to transfer ERC20 tokens to the zero address because we want people to use burn so that we can update the totalSupply variable. On the other hand, transfering to the address 0x00...01 is also effectively burning the asset, and we don't update the total supply in that scenario.
Another example is Ownable, where we disallow transfering ownership to the zero address. This is meant more as a sanity check that you intend to renounce ownership, so we force you to use renounceOwnership. It can be argued though that transfering to the address 0x00...01 is just as dangerous, yet we don't do anything about that.
All in all, I'm not sure it makes sense to keep these checks in place. If we want to protect accidentally renouncing a capacity we should make transfering it more difficult by making it a two-step procedure, or something else. Keeping track of total supply in ERC20 transfer is not required by the spec and it's not that important (since people can burn through other means).
What do we gain by removing the checks? Mostly saving some gas. It may not be that much, but if this is only cruft from the past then there is also no reason to keep it.
I agree, the current zero-address checks are not useful for this purpose, and we shouldn't think of them as a security mechanism.
While transferring to 0x00..01 is effectively a burn (as is losing your crendentials), burn often has special semantics: many protocols have their tokens be destroyed when some condition is true (e.g. DAI is burned when a CDP is closed). Minting is also special, and no transfer operation is equivalent.
My main reason for keeping them is due to the 'hooks' pattern we're introducing in v3.0: by not letting to and from be the zero address in a transfer we can have a single hook that covers transferring, minting and burning, making extensions and overrides easier for the majority of cases.
We can still have a single hook, it just wouldn't be possible to infer that the total supply was reduced by looking at the transfer receiver. We can have an additional hook specifically for burn then. I guess ERC20Snapshot is the main contract affected by this.
Oh, actually one can read the totalSupply inside the after transfer hook if there's a need to keep track of it.
I have always thought of the non-zero address checks in the context of protecting against accidental burning of tokens and/or ownership via leaving an argument blank by mistake.
Since msg.data length is now checked, that is no longer a worry, and so I donât think we need the non-zero address checks to protect against that.
As for burning tokensâ, I think there are two different notions of âburningâ:
Sending coins to an unspendable address without reducing totalSupply. This increases the token balance of the âburnâ address. This is sometimes a âprovableâ burn (e.g.: address is provably unspendable) and sometimes not a âprovableâ burn (address is random and legitimately unspendable, but nobody other than the creator of the random address knows that the tokens are unspendable).
Reducing an accountâs token balance and the totalSupply by the same number.
It is not possible to prevent users from doing #1. The non-zero address check on transfer certainly doesnât accomplish it. As you mentioned they can just send to 0x0..01. So I donât think we should keep the non-zero address checks in the hopes of preventing #1.
I think the burn function is only intended to accomplish #2.
As far as I can tell, it is safe to remove the non-zero address checks.
(Though I cannot speak to any effects on âhooksâ in v3.0 because Iâm not familiar with them yet.)
/// @notice Transfer ownership of an NFT -- THE CALLER IS RESPONSIBLE
/// TO CONFIRM THAT _to IS CAPABLE OF RECEIVING NFTS OR ELSE
/// THEY MAY BE PERMANENTLY LOST
/// @dev Throws unless msg.sender is the current owner, an authorized
/// operator, or the approved address for this NFT. Throws if _from is
/// not the current owner. Throws if _to is the zero address. Throws if
/// _tokenId is not a valid NFT.
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
function transferFrom(address _from, address _to, uint256 _tokenId) external payable;
Code Generator: Revert at runtime if calldata is too short or points out of bounds. This is done inside the ABI decoder and therefore also applies to abi.decode() .
I havenât thought about this in a long time. Thanks @frangio for bringing it up.
I think you have convinced me that these kinds of checks should happen at a higher layer. It actually gives a false sense of security blocking only one of an almost infinite list of possible wrong values.
I hope that removing these checks from our library will put some pressure on the callers to make it harder for people to make mistakes.
As for our audits, instead of recommending our clients to validate that address != 0x0, we can suggest them to add some good natspec comments explaining what is a valid address and what is invalid, and what are the effects of using an invalid one by mistake.
That is a good point about ERC721. I think it is still safe to drop checking address != 0x0 if it doesnât cause any interoperability problems, and I donât know of any interoperability problems it would cause offhand.
Revisiting this thread as we start preparing the next major version of OpenZeppelin Contracts.
My thoughts on this have changed. I now believe that address != 0 checks do make sense and provide value. The reason is that address 0 is a clearly distinct address that can show up in at least two scenarios:
Read from an uninitialized address value in storage or memory.
Returned from ecrecover applied to an invalid signature.
Rejecting address 0 in interfaces as an invalid account provides a layer of protection against accidentally using these values.
The Nomad hack for example involved the use of address 0 which accidentally had a role assigned.
Re: totalSupply, I think a similar but more compelling point concerns tracking totalSupply through Transfer events. It's unfortunate that burn was never given its own event (would have even saved a little gas) because without a zero check burn(ali, amount) emits the same event as transferFrom(ali, address(0), amount). Makes it harder to track totalSupply correctly off-chain.
