From ba8e296915a8f69b478eb6c685b787c5e106f0a1 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 19 Jul 2023 13:11:33 -0600 Subject: [PATCH] ERC721 _update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commit 2897abccc942f7720dcdcda4569fbb6431a0c917 Author: Hadrien Croubois Date: Fri Jul 14 15:31:29 2023 +0200 Update ERC721.sol commit e26d5c0951ff2e5d1b3085e4a276b141aef6a73e Author: Hadrien Croubois Date: Fri Jul 14 08:46:48 2023 +0200 Update IERC721.sol commit a475ffae0a0dfb94ac4aa85579aeff931c311976 Author: Hadrien Croubois Date: Fri Jul 14 08:45:25 2023 +0200 Update ERC721.sol commit 20bb47f439a17946686bb3fa468ca12a94a89141 Author: Hadrien Croubois Date: Fri Jul 14 08:43:14 2023 +0200 Update contracts/token/ERC721/ERC721.sol commit f404802d5569a22f4b162d3b9dc4003e99f9c9b2 Author: Hadrien Croubois Date: Fri Jul 14 08:41:30 2023 +0200 Update ERC721.sol commit b982e2a808eebc0e2aa7c4bc7dc621fb86fecc15 Author: Hadrien Croubois Date: Fri Jul 14 08:38:46 2023 +0200 Update ERC721.behavior.js commit ca32b459ec53428f456c03be5499fb7b25ac44e9 Author: Francisco Giordano Date: Thu Jul 13 19:14:15 2023 -0300 fix _safeTransfer docs commit caabbf3c46376e8d30ccf5b1c2c818e2a1ddf895 Author: Francisco Giordano Date: Thu Jul 13 19:08:36 2023 -0300 improve warnings and notes commit a023cad591765f6753721e318a1534d560009ee2 Author: Francisco Giordano Date: Thu Jul 13 18:21:27 2023 -0300 wrap long line commit 5ce49a45fda8d809bd6497706db147faebff7930 Author: Francisco Giordano Date: Thu Jul 13 18:19:30 2023 -0300 remove unnecessary solhint annotation commit d0375301f172beb57b75964c62bc4f893df5f392 Author: Francisco Date: Thu Jul 13 18:17:24 2023 -0300 Apply suggestions from code review Co-authored-by: Ernesto García commit 81aca964670e15762ad32d0f4c705cf5125b0831 Author: Francisco Date: Thu Jul 13 18:16:42 2023 -0300 Update CHANGELOG.md Co-authored-by: Ernesto García commit 12f63b3b1b9d939b147a944c33e7f122543b21b7 Author: Hadrien Croubois Date: Thu Jul 13 17:28:04 2023 +0200 add test commit 08da709ba7397d7a32676e228fc104332e87c7b7 Author: Hadrien Croubois Date: Thu Jul 13 16:45:30 2023 +0200 refactor _checkAuhtorized commit 328b16bf8cf20dc17f240b5599df17b566a19558 Author: Hadrien Croubois Date: Thu Jul 13 16:29:05 2023 +0200 Authorised → Authorized commit b29e57338359832deabaec53ca7095bdf033db09 Author: Hadrien Croubois Date: Thu Jul 13 16:14:57 2023 +0200 rename from → previousOwner commit e996ba49d8fbb5844ddbf858886c3a0dd91c22c5 Author: Hadrien Croubois Date: Thu Jul 13 16:00:38 2023 +0200 add ERC721 specific details in the 'How to upgrade from 4.x' section of the CHANGELOG commit 20048ca3b9fd2006d4ad8116af5c635491662983 Author: Hadrien Croubois Date: Thu Jul 13 11:00:11 2023 +0200 Changes suggested in the PR discussions commit 4c25b488039cbccfd010e46238d711d2e64cc62f Merge: d7a6aaf4 fb4d9510 Author: Hadrien Croubois Date: Thu Jul 13 10:00:55 2023 +0200 Merge branch 'refactor/erc721-update-fnPointer' of https://github.com/Amxx/openzeppelin-contracts into refactor/erc721-update-fnPointer commit d7a6aaf41f87bc8b44ec839687b6eb1ec4420e13 Author: Hadrien Croubois Date: Thu Jul 13 10:00:50 2023 +0200 remove _exists commit fb4d9510de758ff7d4e047126977bfccc3d27ca8 Author: Hadrien Croubois Date: Thu Jul 13 10:00:39 2023 +0200 Apply suggestions from code review Co-authored-by: Francisco Co-authored-by: Ernesto García commit 10815081f7a1c5a630d49cc254c8d7894b880eaf Author: ernestognw Date: Wed Jul 12 21:09:18 2023 -0600 Lint commit 9ba012005fb7a501bf6fc1c6341a4d5be2320248 Author: ernestognw Date: Wed Jul 12 20:28:50 2023 -0600 Format _increaseBalance NatSpec commit 7c3f1615b09c417a0502d5721c692d17fd7d2892 Author: Francisco Date: Wed Jul 12 20:29:11 2023 -0300 Update .changeset/eighty-lemons-shake.md Co-authored-by: Ernesto García commit 4516803058d453ac73af9b3304a21d3ef5f34ddf Author: Hadrien Croubois Date: Wed Jul 12 17:15:48 2023 +0200 make the safe function without a data field non virtual commit e4b0e725df539ccbd122c2419d6fb5798ac44528 Author: Hadrien Croubois Date: Wed Jul 12 16:56:07 2023 +0200 use whenNotPaused in ERC721Pausable commit b973d985a45ddda9ab6dcbe7c201b0adad1c2d82 Author: Hadrien Croubois Date: Wed Jul 12 14:11:59 2023 +0200 changesets commit 7121ff7c5fdc60898ea49dfd919d5a63f0c9fa59 Merge: 2558c8fa de570d0d Author: Hadrien Croubois Date: Wed Jul 12 13:46:07 2023 +0200 Merge branch 'erc721-approve-0' into refactor/erc721-update-fnPointer commit de570d0d145ea0d594a40924b2edddf61e86d1cb Author: Hadrien Croubois Date: Wed Jul 12 13:42:46 2023 +0200 allow using approve/_approve to clean approval commit 2558c8fac82a914b89c79d6e1515ea849b712580 Author: Hadrien Croubois Date: Wed Jul 12 10:16:55 2023 +0200 change _increaseBalance type to uint128 commit 16f2f156738c4b065accdfbaf60ee2a65234ee42 Author: Hadrien Croubois Date: Wed Jul 12 10:01:30 2023 +0200 remove _isApproedOrOwner in favor of _isApproved + refactor _checkOnERC721Received commit 7e9d024d08aed59a29fb05281910519ff900d888 Author: Hadrien Croubois Date: Wed Jul 12 09:31:18 2023 +0200 Apply suggestions from code review Co-authored-by: Ernesto García commit 1a9552009b5e6bd3300f4d1f679cc7548fce5893 Author: Hadrien Croubois Date: Tue Jul 11 21:47:23 2023 +0200 replace constraints with a simple operator check commit bd0c52e34a5503d30d089f18fb0611c1a9cd5f77 Author: Hadrien Croubois Date: Tue Jul 11 18:06:29 2023 +0200 refactor constraint into an optionalChecks bitmap commit 5ab254cf95f13e379e288da68e884d4bccb67a7a Author: Hadrien Croubois Date: Fri Jul 7 16:13:17 2023 +0200 lint commit 0bb98cb8c647153ebdda75c19c9d2d3106fcdd61 Merge: 562ddf56 7ccea54d Author: Hadrien Croubois Date: Fri Jul 7 16:11:40 2023 +0200 Merge branch 'master' into feature/Governor-storage commit 562ddf566a02b70db0629f1d4a17f8704fbaf8d3 Author: Hadrien Croubois Date: Wed Jul 5 18:45:42 2023 +0200 implement hybrid _update commit 54cb3ca05f95a87ba82ce5e160142fd40ecbe310 Merge: c7303ec2 bb644589 Author: Hadrien Croubois Date: Mon Jul 3 21:09:30 2023 +0200 Merge branch 'master' into refactor/erc721-update-fnPointer commit c7303ec2ae96e12f8011a1621e6d0e47249acfe5 Author: Hadrien Croubois Date: Mon Jul 3 09:37:53 2023 +0200 fix lint commit 1cc7f54ab5319f51193e5f7110003fc54831cc79 Merge: 78c280b5 06861dce Author: Hadrien Croubois Date: Mon Jul 3 09:35:35 2023 +0200 Merge remote-tracking branch 'upstream' into refactor/erc721-update-fnPointer commit 78c280b5372be1167957f8f5f4507685db862ef8 Merge: e2fdbacd 04342118 Author: Hadrien Croubois Date: Fri Jun 30 18:40:55 2023 +0200 Merge branch 'master' into refactor/erc721-update-fnPointer commit e9f03bd211c414458bc0e5b8123f270ad119b052 Author: Francisco Giordano Date: Fri Jun 30 12:09:15 2023 -0300 Exclude address(0) in ERC721._isApprovedOrOwner commit e2fdbacd63b44b25b1672c67c7aeb6ad5d3bd85d Author: Hadrien Croubois Date: Wed Jun 21 22:09:50 2023 +0200 fix lint commit 7ec34355ae4e93855da9793e8297acb3e44b11e3 Author: Hadrien Croubois Date: Wed Jun 21 17:59:22 2023 +0200 Apply suggestions from code review commit 1ed8f9ef2ced4ce5cc0476bb0953fe6c66fd9d6c Author: Hadrien Croubois Date: Wed Jun 21 17:56:00 2023 +0200 use __unsafe_increaseBalance to react to batch minting commit a3526acdf233ecac6e23dcf13ed05b1901cf526b Author: Hadrien Croubois Date: Thu Apr 27 16:37:40 2023 +0200 Rebase ERC721._update on top of next-v5 --- .changeset/bright-tomatoes-sing.md | 2 +- .changeset/eighty-lemons-shake.md | 5 + CHANGELOG.md | 14 +- .../token/ERC721ConsecutiveEnumerableMock.sol | 24 +- .../mocks/token/ERC721ConsecutiveMock.sol | 24 +- contracts/token/ERC721/ERC721.sol | 382 ++++++++---------- contracts/token/ERC721/IERC721.sol | 2 +- .../ERC721/extensions/ERC721Burnable.sol | 7 +- .../ERC721/extensions/ERC721Consecutive.sol | 49 +-- .../ERC721/extensions/ERC721Enumerable.sol | 42 +- .../ERC721/extensions/ERC721Pausable.sol | 15 +- .../token/ERC721/extensions/ERC721Royalty.sol | 13 +- .../ERC721/extensions/ERC721URIStorage.sol | 12 +- .../token/ERC721/extensions/ERC721Votes.sol | 24 +- .../token/ERC721/extensions/ERC721Wrapper.sol | 7 +- test/token/ERC721/ERC721.behavior.js | 14 +- .../extensions/ERC721Consecutive.test.js | 19 +- .../ERC721/extensions/ERC721Pausable.test.js | 6 - .../extensions/ERC721URIStorage.test.js | 2 - 19 files changed, 297 insertions(+), 366 deletions(-) create mode 100644 .changeset/eighty-lemons-shake.md diff --git a/.changeset/bright-tomatoes-sing.md b/.changeset/bright-tomatoes-sing.md index dcd6d7ff0..7ef6d929a 100644 --- a/.changeset/bright-tomatoes-sing.md +++ b/.changeset/bright-tomatoes-sing.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`ERC20`, `ERC1155`: Deleted `_beforeTokenTransfer` and `_afterTokenTransfer` hooks, added a new internal `_update` function for customizations, and refactored all extensions using those hooks to use `_update` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838), [#3876](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876)) +`ERC20`, `ERC721`, `ERC1155`: Deleted `_beforeTokenTransfer` and `_afterTokenTransfer` hooks, added a new internal `_update` function for customizations, and refactored all extensions using those hooks to use `_update` instead. ([#3838](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3838), [#3876](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876), [#4377](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4377)) diff --git a/.changeset/eighty-lemons-shake.md b/.changeset/eighty-lemons-shake.md new file mode 100644 index 000000000..4e53893f5 --- /dev/null +++ b/.changeset/eighty-lemons-shake.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC721`: `_approve` no longer allows approving the owner of the tokenId. `_setApprovalForAll` no longer allows setting address(0) as an operator. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2936877e1..98d06de99 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 `_isAuthorized` function. Overrides that used to target the +`_isApprovedOrOwner` should now be performed on the `_isAuthorized` 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 `_exists` 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/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 081b8d99d..9cebf1a00 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -28,25 +28,15 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._ownerOf(tokenId); } - function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { - super._mint(to, tokenId); - } - - function _beforeTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Enumerable) { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + address auth + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { + return super._update(to, tokenId, auth); } - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Enumerable) { + super._increaseBalance(account, amount); } } diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 166959e92..2990076c7 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -41,26 +41,16 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes return super._ownerOf(tokenId); } - function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { - super._mint(to, tokenId); - } - - function _beforeTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Pausable) { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + address auth + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { + return super._update(to, tokenId, auth); } - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Votes) { + super._increaseBalance(account, amount); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 25ac69fa9..afb2a0388 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -113,16 +113,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev See {IERC721-approve}. */ function approve(address to, uint256 tokenId) public virtual { - address owner = ownerOf(tokenId); - if (to == owner) { - revert ERC721InvalidOperator(owner); - } - - if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) { - revert ERC721InvalidApprover(_msgSender()); - } - - _approve(to, tokenId); + _approve(to, tokenId, _msgSender()); } /** @@ -131,7 +122,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er function getApproved(uint256 tokenId) public view virtual returns (address) { _requireMinted(tokenId); - return _tokenApprovals[tokenId]; + return _getApproved(tokenId); } /** @@ -152,17 +143,21 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev See {IERC721-transferFrom}. */ function transferFrom(address from, address to, uint256 tokenId) public virtual { - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); + if (to == address(0)) { + revert ERC721InvalidReceiver(address(0)); + } + // 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); } - - _transfer(from, to, tokenId); } /** * @dev See {IERC721-safeTransferFrom}. */ - function safeTransferFrom(address from, address to, uint256 tokenId) public virtual { + function safeTransferFrom(address from, address to, uint256 tokenId) public { safeTransferFrom(from, to, tokenId, ""); } @@ -170,91 +165,112 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev See {IERC721-safeTransferFrom}. */ function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual { - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); - } - _safeTransfer(from, to, tokenId, data); + transferFrom(from, to, tokenId); + _checkOnERC721Received(from, to, tokenId, data); } /** - * @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients - * are aware of the ERC721 protocol to prevent tokens from being forever locked. - * - * `data` is additional data, it has no specified format and it is sent in call to `to`. - * - * This internal function is equivalent to {safeTransferFrom}, and can be used to e.g. - * implement alternative mechanisms to perform token transfer, such as signature-based. - * - * Requirements: - * - * - `from` cannot be the zero address. - * - `to` cannot be the zero address. - * - `tokenId` token must exist and be owned by `from`. - * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer. + * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist * - * Emits a {Transfer} event. + * IMPORTANT: Any overrides to this function that add ownership of tokens not tracked by the + * core ERC721 logic MUST be matched with the use of {_increaseBalance} to keep balances + * consistent with ownership. The invariant to preserve is that for any address `a` the value returned by + * `balanceOf(a)` must be equal to the number of tokens such that `_ownerOf(tokenId)` is `a`. */ - function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual { - _transfer(from, to, tokenId); - if (!_checkOnERC721Received(from, to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } + function _ownerOf(uint256 tokenId) internal view virtual returns (address) { + return _owners[tokenId]; } /** - * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist + * @dev Returns the approved address for `tokenId`. Returns 0 if `tokenId` is not minted. */ - function _ownerOf(uint256 tokenId) internal view virtual returns (address) { - return _owners[tokenId]; + function _getApproved(uint256 tokenId) internal view virtual returns (address) { + return _tokenApprovals[tokenId]; } /** - * @dev Returns whether `tokenId` exists. + * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in + * particular (ignoring whether it is owned by `owner`). * - * Tokens can be managed by their owner or approved accounts via {approve} or {setApprovalForAll}. - * - * Tokens start existing when they are minted (`_mint`), - * and stop existing when they are burned (`_burn`). + * WARNING: This function doesn't check that `owner` is the actual owner of `tokenId`. */ - function _exists(uint256 tokenId) internal view virtual returns (bool) { - return _ownerOf(tokenId) != address(0); + function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { + return + spender != address(0) && + (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender); } /** - * @dev Returns whether `spender` is allowed to manage `tokenId`. + * @dev Checks if `spender` can operate on `tokenId`, assuming the provided `owner` is the actual owner. + * Reverts if `spender` has not approval for all assets of the provided `owner` nor the actual owner approved the `spender` for the specific `tokenId`. * - * Requirements: - * - * - `tokenId` must exist. + * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the + * actual owner of `tokenId`. */ - function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) { - address owner = ownerOf(tokenId); - return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender); + function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual { + if (!_isAuthorized(owner, spender, tokenId)) { + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else { + revert ERC721InsufficientApproval(spender, tokenId); + } + } } /** - * @dev Safely mints `tokenId` and transfers it to `to`. - * - * Requirements: + * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. * - * - `tokenId` must not exist. - * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer. + * NOTE: the value is limited to type(uint128).max. This protect against _balance overflow. It is unrealistic that + * a uint256 would ever overflow from increments when these increments are bounded to uint128 values. * - * Emits a {Transfer} event. + * WARNING: Increasing an account's balance using this function tends to be paired with an override of the + * {_ownerOf} function to resolve the ownership of the corresponding tokens so that balances and ownership + * remain consistent with one another. */ - function _safeMint(address to, uint256 tokenId) internal virtual { - _safeMint(to, tokenId, ""); + function _increaseBalance(address account, uint128 value) internal virtual { + unchecked { + _balances[account] += value; + } } /** - * @dev Same as {xref-ERC721-_safeMint-address-uint256-}[`_safeMint`], with an additional `data` parameter which is - * forwarded in {IERC721Receiver-onERC721Received} to contract recipients. + * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner + * (or `to`) is the zero address. Returns the owner of the `tokenId` before the update. + * + * The `auth` argument is optional. If the value passed is non 0, then this function will check that + * `auth` is either the owner of the token, or approved to operate on the token (by the owner). + * + * Emits a {Transfer} event. + * + * NOTE: If overriding this function in a way that tracks balances, see also {_increaseBalance}. */ - function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual { - _mint(to, tokenId); - if (!_checkOnERC721Received(address(0), to, tokenId, data)) { - revert ERC721InvalidReceiver(to); + function _update(address to, uint256 tokenId, address auth) internal virtual returns (address) { + address from = _ownerOf(tokenId); + + // Perform (optional) operator check + if (auth != address(0)) { + _checkAuthorized(from, auth, tokenId); + } + + // Execute the update + if (from != address(0)) { + delete _tokenApprovals[tokenId]; + unchecked { + _balances[from] -= 1; + } + } + + if (to != address(0)) { + unchecked { + _balances[to] += 1; + } } + + _owners[tokenId] = to; + + emit Transfer(from, to, tokenId); + + return from; } /** @@ -269,34 +285,37 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _mint(address to, uint256 tokenId) internal virtual { + function _mint(address to, uint256 tokenId) internal { if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - if (_exists(tokenId)) { + address previousOwner = _update(to, tokenId, address(0)); + if (previousOwner != address(0)) { revert ERC721InvalidSender(address(0)); } + } - _beforeTokenTransfer(address(0), to, tokenId, 1); - - // Check that tokenId was not minted by `_beforeTokenTransfer` hook - if (_exists(tokenId)) { - revert ERC721InvalidSender(address(0)); - } - - unchecked { - // Will not overflow unless all 2**256 token ids are minted to the same owner. - // Given that tokens are minted one by one, it is impossible in practice that - // this ever happens. Might change if we allow batch minting. - // The ERC fails to describe this case. - _balances[to] += 1; - } - - _owners[tokenId] = to; - - emit Transfer(address(0), to, tokenId); + /** + * @dev Mints `tokenId`, transfers it to `to` and checks for `to` acceptance. + * + * Requirements: + * + * - `tokenId` must not exist. + * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer. + * + * Emits a {Transfer} event. + */ + function _safeMint(address to, uint256 tokenId) internal { + _safeMint(to, tokenId, ""); + } - _afterTokenTransfer(address(0), to, tokenId, 1); + /** + * @dev Same as {xref-ERC721-_safeMint-address-uint256-}[`_safeMint`], with an additional `data` parameter which is + * forwarded in {IERC721Receiver-onERC721Received} to contract recipients. + */ + function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual { + _mint(to, tokenId); + _checkOnERC721Received(address(0), to, tokenId, data); } /** @@ -310,26 +329,11 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _burn(uint256 tokenId) internal virtual { - address owner = ownerOf(tokenId); - - _beforeTokenTransfer(owner, address(0), tokenId, 1); - - // Update ownership in case tokenId was transferred by `_beforeTokenTransfer` hook - owner = ownerOf(tokenId); - - // Clear approvals - delete _tokenApprovals[tokenId]; - - // Decrease balance with checked arithmetic, because an `ownerOf` override may - // invalidate the assumption that `_balances[from] >= 1`. - _balances[owner] -= 1; - - delete _owners[tokenId]; - - emit Transfer(owner, address(0), tokenId); - - _afterTokenTransfer(owner, address(0), tokenId, 1); + function _burn(uint256 tokenId) internal { + address previousOwner = _update(address(0), tokenId, address(0)); + if (previousOwner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } } /** @@ -339,65 +343,85 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Requirements: * * - `to` cannot be the zero address. - * - `tokenId` token must be owned by `from`. + * - `tokenId` token must exists and be owned by `from`. * * Emits a {Transfer} event. */ - function _transfer(address from, address to, uint256 tokenId) internal virtual { - address owner = ownerOf(tokenId); - if (owner != from) { - revert ERC721IncorrectOwner(from, tokenId, owner); - } + function _transfer(address from, address to, uint256 tokenId) internal { if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - - _beforeTokenTransfer(from, to, tokenId, 1); - - // Check that tokenId was not transferred by `_beforeTokenTransfer` hook - owner = ownerOf(tokenId); - if (owner != from) { - revert ERC721IncorrectOwner(from, tokenId, owner); - } - - // Clear approvals from the previous owner - delete _tokenApprovals[tokenId]; - - // Decrease balance with checked arithmetic, because an `ownerOf` override may - // invalidate the assumption that `_balances[from] >= 1`. - _balances[from] -= 1; - - unchecked { - // `_balances[to]` could overflow in the conditions described in `_mint`. That would require - // all 2**256 token ids to be minted, which in practice is impossible. - _balances[to] += 1; + address previousOwner = _update(to, tokenId, address(0)); + if (previousOwner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else if (previousOwner != from) { + revert ERC721IncorrectOwner(from, tokenId, previousOwner); } + } - _owners[tokenId] = to; - - emit Transfer(from, to, tokenId); + /** + * @dev Safely transfers `tokenId` token from `from` to `to`, checking that contract recipients + * are aware of the ERC721 standard to prevent tokens from being forever locked. + * + * `data` is additional data, it has no specified format and it is sent in call to `to`. + * + * This internal function is like {safeTransferFrom} in the sense that it invokes + * {IERC721Receiver-onERC721Received} on the receiver, and can be used to e.g. + * implement alternative mechanisms to perform token transfer, such as signature-based. + * + * Requirements: + * + * - `tokenId` token must exist and be owned by `from`. + * - `to` cannot be the zero address. + * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer. + * + * Emits a {Transfer} event. + */ + function _safeTransfer(address from, address to, uint256 tokenId) internal { + _safeTransfer(from, to, tokenId, ""); + } - _afterTokenTransfer(from, to, tokenId, 1); + /** + * @dev Same as {xref-ERC721-_safeTransfer-address-address-uint256-}[`_safeTransfer`], with an additional `data` parameter which is + * forwarded in {IERC721Receiver-onERC721Received} to contract recipients. + */ + function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual { + _transfer(from, to, tokenId); + _checkOnERC721Received(from, to, tokenId, data); } /** * @dev Approve `to` to operate on `tokenId` * + * The `auth` argument is optional. If the value passed is non 0, then this function will check that `auth` is + * either the owner of the token, or approved to operate on all tokens held by this owner. + * * Emits an {Approval} event. */ - function _approve(address to, uint256 tokenId) 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)) { + revert ERC721InvalidApprover(auth); + } + _tokenApprovals[tokenId] = to; - emit Approval(ownerOf(tokenId), to, tokenId); + emit Approval(owner, to, tokenId); + + return owner; } /** * @dev Approve `operator` to operate on all of `owner` tokens * + * Requirements: + * - operator can't be the address zero. + * * Emits an {ApprovalForAll} event. */ function _setApprovalForAll(address owner, address operator, bool approved) internal virtual { - if (owner == operator) { - revert ERC721InvalidOperator(owner); + if (operator == address(0)) { + revert ERC721InvalidOperator(operator); } _operatorApprovals[owner][operator] = approved; emit ApprovalForAll(owner, operator, approved); @@ -407,30 +431,26 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev Reverts if the `tokenId` has not been minted yet. */ function _requireMinted(uint256 tokenId) internal view virtual { - if (!_exists(tokenId)) { + if (_ownerOf(tokenId) == address(0)) { revert ERC721NonexistentToken(tokenId); } } /** - * @dev Private function to invoke {IERC721Receiver-onERC721Received} on a target address. - * The call is not executed if the target address is not a contract. + * @dev Private function to invoke {IERC721Receiver-onERC721Received} on a target address. This will revert if the + * recipient doesn't accept the token transfer. The call is not executed if the target address is not a contract. * * @param from address representing the previous owner of the given token ID * @param to target address that will receive the tokens * @param tokenId uint256 ID of the token to be transferred * @param data bytes optional data to send along with the call - * @return bool whether the call correctly returned the expected magic value - */ - function _checkOnERC721Received( - address from, - address to, - uint256 tokenId, - bytes memory data - ) private returns (bool) { + */ + function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory data) private { if (to.code.length > 0) { try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) { - return retval == IERC721Receiver.onERC721Received.selector; + if (retval != IERC721Receiver.onERC721Received.selector) { + revert ERC721InvalidReceiver(to); + } } catch (bytes memory reason) { if (reason.length == 0) { revert ERC721InvalidReceiver(to); @@ -441,52 +461,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } } - } else { - return true; } } - - /** - * @dev Hook that is called before any token transfer. This includes minting and burning. If {ERC721Consecutive} is - * used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1. - * - * Calling conditions: - * - * - When `from` and `to` are both non-zero, ``from``'s tokens will be transferred to `to`. - * - When `from` is zero, the tokens will be minted for `to`. - * - When `to` is zero, ``from``'s tokens will be burned. - * - `from` and `to` are never both zero. - * - `batchSize` is non-zero. - * - * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. - */ - function _beforeTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {} - - /** - * @dev Hook that is called after any token transfer. This includes minting and burning. If {ERC721Consecutive} is - * used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1. - * - * Calling conditions: - * - * - When `from` and `to` are both non-zero, ``from``'s tokens were transferred to `to`. - * - When `from` is zero, the tokens were minted for `to`. - * - When `to` is zero, ``from``'s tokens were burned. - * - `from` and `to` are never both zero. - * - `batchSize` is non-zero. - * - * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. - */ - function _afterTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {} - - /** - * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. - * - * WARNING: Anyone calling this MUST ensure that the balances remain consistent with the ownership. The invariant - * being that for any address `a` the value returned by `balanceOf(a)` must be equal to the number of tokens such - * that `ownerOf(tokenId)` is `a`. - */ - // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance(address account, uint256 value) internal { - _balances[account] += value; - } } diff --git a/contracts/token/ERC721/IERC721.sol b/contracts/token/ERC721/IERC721.sol index 3b2db67cb..1eb423f63 100644 --- a/contracts/token/ERC721/IERC721.sol +++ b/contracts/token/ERC721/IERC721.sol @@ -108,7 +108,7 @@ interface IERC721 is IERC165 { * * Requirements: * - * - The `operator` cannot be the caller. + * - The `operator` cannot be the address zero. * * Emits an {ApprovalForAll} event. */ diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 607265328..4c761ed16 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,9 +19,8 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); - } - _burn(tokenId); + // 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/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 7c37e0764..b014111ff 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -118,61 +118,44 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { revert ERC721ExceededMaxBatchMint(batchSize, maxBatchSize); } - // hook before - _beforeTokenTransfer(address(0), to, next, batchSize); - // push an ownership checkpoint & emit event uint96 last = next + batchSize - 1; _sequentialOwnership.push(last, uint160(to)); // The invariant required by this function is preserved because the new sequentialOwnership checkpoint // is attributing ownership of `batchSize` new tokens to account `to`. - __unsafe_increaseBalance(to, batchSize); + _increaseBalance(to, batchSize); emit ConsecutiveTransfer(next, last, address(0), to); - - // hook after - _afterTokenTransfer(address(0), to, next, batchSize); } return next; } /** - * @dev See {ERC721-_mint}. Override version that restricts normal minting to after construction. + * @dev See {ERC721-_update}. Override version that restricts normal minting to after construction. * - * WARNING: Using {ERC721Consecutive} prevents using {_mint} during construction in favor of {_mintConsecutive}. - * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. + * WARNING: Using {ERC721Consecutive} prevents minting during construction in favor of {_mintConsecutive}. + * After construction, {_mintConsecutive} is no longer available and minting through {_update} becomes available. */ - function _mint(address to, uint256 tokenId) internal virtual override { - if (address(this).code.length == 0) { + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); + + // only mint after construction + if (previousOwner == address(0) && address(this).code.length == 0) { revert ERC721ForbiddenMint(); } - super._mint(to, tokenId); - } - /** - * @dev See {ERC721-_afterTokenTransfer}. Burning of tokens that have been sequentially minted must be explicit. - */ - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { + // record burn if ( to == address(0) && // if we burn - firstTokenId >= _firstConsecutiveId() && - firstTokenId < _nextConsecutiveId() && - !_sequentialBurn.get(firstTokenId) - ) // and the token was never marked as burnt - { - if (batchSize != 1) { - revert ERC721ForbiddenBatchBurn(); - } - _sequentialBurn.set(firstTokenId); + tokenId < _nextConsecutiveId() && // and the tokenId was minted in a batch + !_sequentialBurn.get(tokenId) // and the token was never marked as burnt + ) { + _sequentialBurn.set(tokenId); } - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + + return previousOwner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 2e33123b6..3c62f4295 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -74,33 +74,23 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } /** - * @dev See {ERC721-_beforeTokenTransfer}. + * @dev See {ERC721-_update}. */ - function _beforeTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - - if (batchSize > 1) { - // Will only trigger during construction. Batch transferring (minting) is not available afterwards. - revert ERC721EnumerableForbiddenBatchMint(); - } - - uint256 tokenId = firstTokenId; + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); - if (from == address(0)) { + if (previousOwner == address(0)) { _addTokenToAllTokensEnumeration(tokenId); - } else if (from != to) { - _removeTokenFromOwnerEnumeration(from, tokenId); + } else if (previousOwner != to) { + _removeTokenFromOwnerEnumeration(previousOwner, tokenId); } if (to == address(0)) { _removeTokenFromAllTokensEnumeration(tokenId); - } else if (to != from) { + } else if (previousOwner != to) { _addTokenToOwnerEnumeration(to, tokenId); } + + return previousOwner; } /** @@ -109,7 +99,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { * @param tokenId uint256 ID of the token to be added to the tokens list of the given address */ function _addTokenToOwnerEnumeration(address to, uint256 tokenId) private { - uint256 length = balanceOf(to); + uint256 length = balanceOf(to) - 1; _ownedTokens[to][length] = tokenId; _ownedTokensIndex[tokenId] = length; } @@ -135,7 +125,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { // To prevent a gap in from's tokens array, we store the last token in the index of the token to delete, and // then delete the last slot (swap and pop). - uint256 lastTokenIndex = balanceOf(from) - 1; + uint256 lastTokenIndex = balanceOf(from); uint256 tokenIndex = _ownedTokensIndex[tokenId]; // When the token to delete is the last token, the swap operation is unnecessary @@ -175,4 +165,14 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { delete _allTokensIndex[tokenId]; _allTokens.pop(); } + + /** + * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch + */ + function _increaseBalance(address account, uint128 amount) internal virtual override { + if (amount > 0) { + revert ERC721EnumerableForbiddenBatchMint(); + } + super._increaseBalance(account, amount); + } } diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 5777ac36e..b5b44e02d 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -21,20 +21,17 @@ import {Pausable} from "../../../security/Pausable.sol"; */ abstract contract ERC721Pausable is ERC721, Pausable { /** - * @dev See {ERC721-_beforeTokenTransfer}. + * @dev See {ERC721-_update}. * * Requirements: * * - the contract must not be paused. */ - function _beforeTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - - _requireNotPaused(); + uint256 tokenId, + address auth + ) internal virtual override whenNotPaused returns (address) { + return super._update(to, tokenId, auth); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index c4b8d371e..654f46ea5 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -26,10 +26,15 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { } /** - * @dev See {ERC721-_burn}. This override additionally clears the royalty information for the token. + * @dev See {ERC721-_update}. When burning, this override will additionally clear the royalty information for the token. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); - _resetTokenRoyalty(tokenId); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); + + if (to == address(0)) { + _resetTokenRoyalty(tokenId); + } + + return previousOwner; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 737d28e27..28440ef69 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -55,7 +55,7 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * - `tokenId` must exist. */ function _setTokenURI(uint256 tokenId, string memory _tokenURI) internal virtual { - if (!_exists(tokenId)) { + if (_ownerOf(tokenId) == address(0)) { revert ERC721NonexistentToken(tokenId); } _tokenURIs[tokenId] = _tokenURI; @@ -64,15 +64,17 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { } /** - * @dev See {ERC721-_burn}. This override additionally checks to see if a + * @dev See {ERC721-_update}. When burning, this override will additionally check if a * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); - if (bytes(_tokenURIs[tokenId]).length != 0) { + if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } + + return previousOwner; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 0838010eb..ede3dff8e 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -16,18 +16,16 @@ import {Votes} from "../../../governance/utils/Votes.sol"; */ abstract contract ERC721Votes is ERC721, Votes { /** - * @dev See {ERC721-_afterTokenTransfer}. Adjusts votes when tokens are transferred. + * @dev See {ERC721-_update}. Adjusts votes when tokens are transferred. * * Emits a {IVotes-DelegateVotesChanged} event. */ - function _afterTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - _transferVotingUnits(from, to, batchSize); - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); + + _transferVotingUnits(previousOwner, to, 1); + + return previousOwner; } /** @@ -38,4 +36,12 @@ abstract contract ERC721Votes is ERC721, Votes { function _getVotingUnits(address account) internal view virtual override returns (uint256) { return balanceOf(account); } + + /** + * @dev See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch. + */ + function _increaseBalance(address account, uint128 amount) internal virtual override { + super._increaseBalance(account, amount); + _transferVotingUnits(address(0), account, amount); + } } diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index f204c1079..b2dbbd7f0 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,10 +50,9 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); - } - _burn(tokenId); + // 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. // slither-disable-next-line reentrancy-no-eth diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 75700f6ab..ee972766f 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -524,14 +524,6 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); - context('when the address that receives the approval is the owner', function () { - it('reverts', async function () { - await expectRevertCustomError(this.token.approve(owner, tokenId, { from: owner }), 'ERC721InvalidOperator', [ - owner, - ]); - }); - }); - context('when the sender does not own the given token ID', function () { it('reverts', async function () { await expectRevertCustomError( @@ -645,12 +637,12 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); - context('when the operator is the owner', function () { + context('when the operator is address zero', function () { it('reverts', async function () { await expectRevertCustomError( - this.token.setApprovalForAll(owner, true, { from: owner }), + this.token.setApprovalForAll(constants.ZERO_ADDRESS, true, { from: owner }), 'ERC721InvalidOperator', - [owner], + [constants.ZERO_ADDRESS], ); }); }); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 55c26dffe..d9e33aff2 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -103,7 +103,7 @@ contract('ERC721Consecutive', function (accounts) { it('simple minting is possible after construction', async function () { const tokenId = sum(...batches.map(b => b.amount)) + offset; - expect(await this.token.$_exists(tokenId)).to.be.equal(false); + await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]); expectEvent(await this.token.$_mint(user1, tokenId), 'Transfer', { from: constants.ZERO_ADDRESS, @@ -115,7 +115,7 @@ contract('ERC721Consecutive', function (accounts) { it('cannot mint a token that has been batched minted', async function () { const tokenId = sum(...batches.map(b => b.amount)) + offset - 1; - expect(await this.token.$_exists(tokenId)).to.be.equal(true); + expect(await this.token.ownerOf(tokenId)).to.be.not.equal(constants.ZERO_ADDRESS); await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721InvalidSender', [ZERO_ADDRESS]); }); @@ -151,13 +151,11 @@ contract('ERC721Consecutive', function (accounts) { it('tokens can be burned and re-minted #2', async function () { const tokenId = web3.utils.toBN(sum(...batches.map(({ amount }) => amount)) + offset); - expect(await this.token.$_exists(tokenId)).to.be.equal(false); await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]); // mint await this.token.$_mint(user1, tokenId); - expect(await this.token.$_exists(tokenId)).to.be.equal(true); expect(await this.token.ownerOf(tokenId), user1); // burn @@ -167,7 +165,6 @@ contract('ERC721Consecutive', function (accounts) { tokenId, }); - expect(await this.token.$_exists(tokenId)).to.be.equal(false); await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]); // re-mint @@ -177,20 +174,8 @@ contract('ERC721Consecutive', function (accounts) { tokenId, }); - expect(await this.token.$_exists(tokenId)).to.be.equal(true); expect(await this.token.ownerOf(tokenId), user2); }); - - it('reverts burning batches of size != 1', async function () { - const tokenId = batches[0].amount + offset; - const receiver = batches[0].receiver; - - await expectRevertCustomError( - this.token.$_afterTokenTransfer(receiver, ZERO_ADDRESS, tokenId, 2), - 'ERC721ForbiddenBatchBurn', - [], - ); - }); }); }); } diff --git a/test/token/ERC721/extensions/ERC721Pausable.test.js b/test/token/ERC721/extensions/ERC721Pausable.test.js index ec99dea96..5d77149f2 100644 --- a/test/token/ERC721/extensions/ERC721Pausable.test.js +++ b/test/token/ERC721/extensions/ERC721Pausable.test.js @@ -81,12 +81,6 @@ contract('ERC721Pausable', function (accounts) { }); }); - describe('exists', function () { - it('returns token existence', async function () { - expect(await this.token.$_exists(firstTokenId)).to.equal(true); - }); - }); - describe('isApprovedForAll', function () { it('returns the approval of the operator', async function () { expect(await this.token.isApprovedForAll(owner, operator)).to.equal(false); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index 34738cae1..129515514 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -86,7 +86,6 @@ contract('ERC721URIStorage', function (accounts) { it('tokens without URI can be burnt ', async function () { await this.token.$_burn(firstTokenId, { from: owner }); - expect(await this.token.$_exists(firstTokenId)).to.equal(false); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); @@ -95,7 +94,6 @@ contract('ERC721URIStorage', function (accounts) { await this.token.$_burn(firstTokenId, { from: owner }); - expect(await this.token.$_exists(firstTokenId)).to.equal(false); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); });