Removing address != 0x0 checks from OpenZeppelin Contracts

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.

Let me know your thoughts!

3 Likes

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.

1 Like

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.

1 Like

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’:

  1. 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).

  2. 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.)

2 Likes

@frangio with regards ERC721 spec, throwing on 0 address in multiple circumstances is part of the spec.
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md
For example:

/// @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;

It’s true that many ERCs have made this a part of their spec. We’ll have to remain careful with respect to compliance.

1 Like

This is done by Solidity now? Any idea what version added this?

2 Likes

I think it was 0.5.0:

  • 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() .
2 Likes

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.

2 Likes

@frangio Thank you for pointing this out. I agree with you about removing checks for address != 0x0.

1 Like

@frangio

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.

1 Like

It wouldn’t be compliant to remove the check in our ERC721 implementation. We need to keep it there.

1 Like