From e996ba49d8fbb5844ddbf858886c3a0dd91c22c5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 16:00:38 +0200 Subject: [PATCH] add ERC721 specific details in the 'How to upgrade from 4.x' section of the CHANGELOG --- CHANGELOG.md | 14 +++++++++++++- contracts/token/ERC721/ERC721.sol | 8 +++++--- .../token/ERC721/extensions/ERC721Burnable.sol | 4 ++-- .../token/ERC721/extensions/ERC721Wrapper.sol | 4 ++-- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2936877e1..afc6231b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ These removals were implemented in the following PRs: [#3637](https://github.com These breaking changes will require modifications to ERC20, ERC721, and ERC1155 contracts, since the `_afterTokenTransfer` and `_beforeTokenTransfer` functions were removed. Any customization made through those hooks should now be done overriding the new `_update` function instead. -Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies. +Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_transfer`, `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies. For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have to be changed in the following way. @@ -53,6 +53,18 @@ For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have t } ``` +### More about ERC721 + +In the case of `ERC721`, the `_update` function does not include a `from` parameter, as the sender is implicitly the previous owner of the `tokenId`. The address of +this previous owner is returned by the `_update` function, so it can be used for a posteriori checks. In addition to `to` and `tokenId`, a third parameter (`auth`) is +present in this function. This parameter enabled an optional check that the caller/spender is approved to do the transfer. This check cannot be performed after the transfer (because the transfer resets the approval), and doing it before `_update` would require a duplicate call to `_ownerOf`. + +In this logic of removing hidden SLOADs, the `_isApprovedOrOwner` function was removed in favor of a new `_isAuthorised` function. Overrides that used to target the +`_isApprovedOrOwner` should now be performed on the `_isAuthorised` function. Calls to `_isApprovedOrOwner` that preceded a call to `_transfer`, `_burn` or `_approve` +should be removed in favor of using the `auth` argument in `_update` and `_approve`. This is showcased in `ERC721Burnable.burn` and in `ERC721Wrapper.withdrawTo`. + +The `_exist` function was removed. Calls to this function can be replaced by `_ownerOf(tokenId) != address(0)`. + #### ERC165Storage Users that were registering EIP-165 interfaces with `_registerInterface` from `ERC165Storage` should instead do so so by overriding the `supportsInterface` function as seen below: diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 2d25da2ab..8fb4ec02a 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -146,8 +146,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), - // no need to duplicate that check here. + // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. address previousOwner = _update(to, tokenId, _msgSender()); if (previousOwner != from) { revert ERC721IncorrectOwner(from, tokenId, previousOwner); @@ -394,7 +394,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits an {Approval} event. */ - function _approve(address to, uint256 tokenId, address auth) internal virtual { + function _approve(address to, uint256 tokenId, address auth) internal virtual returns (address) { address owner = ownerOf(tokenId); if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) { @@ -403,6 +403,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er _tokenApprovals[tokenId] = to; emit Approval(owner, to, tokenId); + + return owner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index d9350b5e8..4c761ed16 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,8 +19,8 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), - // no need to duplicate that check here. + // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. _update(address(0), tokenId, _msgSender()); } } diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index 3297f06b6..b2dbbd7f0 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,8 +50,8 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), - // no need to duplicate that check here. + // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. _update(address(0), tokenId, _msgSender()); // Checks were already performed at this point, and there's no way to retake ownership or approval from // the wrapped tokenId after this point, so it's safe to remove the reentrancy check for the next line.