Change execution order to avoid reentry through the _beforeTokenTransfer hook #3611
Conversation
Co-authored-by: Francisco <frangio.1@gmail.com>
contracts/token/ERC721/ERC721.sol
Outdated
| @@ -333,11 +336,11 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { | |||
| address to, | |||
| uint256 tokenId | |||
| ) internal virtual { | |||
| require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner"); | |||
There was a problem hiding this comment.
It is wrong to remove this from here... We need to check that from is the owner at the time of the transfer, don't we? We should do the same we did in _burn, and re-read the owner after the hook...
There was a problem hiding this comment.
My impression was that it would be checked after the hook, and that would revert in the owner is not correct.
The risk is that the hook would be executed with an invalid from, and the revert would come after.
There is a world in which the from is not valid, the before hook move the token from its current owner to the from, and then the test passes and we do what we have to do... The code would have to be really f****d up, but its possible.
My concern is that doing the test twice increase transfer cost by >100 gas (on probably the most common and scrutinized operation)
…fer hook (OpenZeppelin#3611) Co-authored-by: Francisco <frangio.1@gmail.com>
If a developer was to transfer/burn/mint the token during the
_beforeTokenTransfer, that would currently not be catched by checks, resulting in invalid event being emitted, and corrupted balances.This could in particular affect a voting instance.
This risk could become serious if the
_beforeTokenTransferhook was to perform an external call that could reenter!We eliminate this risk by applying strict "check→effect" logic to the
_mint,_transferand_burnfunction, at the risk of being more gas expensive. For_mintand_transfer, the increase in gas cost only affect reverting transaction (the revert latter). For_burn, an addition hot-sload (100gas) is needed, making all burns more exepensive. Hopefully burn is not as widelly and frequently used as mint and transfer.IMO this is one more reason to drop these hooks when possible (5.0)
PR Checklist