diff --git a/.changeset/bright-tomatoes-sing.md b/.changeset/bright-tomatoes-sing.md index dcd6d7ff027..7ef6d929a30 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 00000000000..4e53893f52b --- /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 2936877e186..98d06de9980 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 081b8d99dee..9cebf1a00b1 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 166959e922b..2990076c70a 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 25ac69fa96a..1263f742f78 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 `_isAuthorized` 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,113 @@ 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. - * - * Tokens can be managed by their owner or approved accounts via {approve} or {setApprovalForAll}. + * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in + * particular (ignoring whether it is owned by `owner`). * - * Tokens start existing when they are minted (`_mint`), - * and stop existing when they are burned (`_burn`). + * WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not + * verify this assumption. */ - 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 +286,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 +330,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); + } } /** @@ -343,61 +348,83 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * 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. + * - `from` 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); + + // We do not use _isAuthorized because single-token approvals should not be able to call approve + 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 +434,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 +464,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 3b2db67cb97..1eb423f63bd 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 607265328ec..cfe7546686f 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 `_isAuthorized` 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 7c37e076415..b014111ffc2 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 2e33123b6cf..3c62f429522 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 5777ac36ea2..b5b44e02d55 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 eb128ac5860..ff9d31936ad 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -27,10 +27,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 737d28e27f2..28440ef69a2 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 0838010eb0a..ede3dff8e9e 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 f204c107986..cbb3a4b14f7 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 `_isAuthorized` 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 75700f6ab56..4dd6e234c45 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -104,7 +104,7 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }; - const shouldTransferTokensByUsers = function (transferFunction) { + const shouldTransferTokensByUsers = function (transferFunction, opts = {}) { context('when called by the owner', function () { beforeEach(async function () { receipt = await transferFunction.call(this, owner, this.toWhom, tokenId, { from: owner }); @@ -180,13 +180,19 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); context('when the sender is not authorized for the token id', function () { - it('reverts', async function () { - await expectRevertCustomError( - transferFunction.call(this, owner, other, tokenId, { from: other }), - 'ERC721InsufficientApproval', - [other, tokenId], - ); - }); + if (opts.unrestricted) { + it('does not revert', async function () { + await transferFunction.call(this, owner, other, tokenId, { from: other }); + }); + } else { + it('reverts', async function () { + await expectRevertCustomError( + transferFunction.call(this, owner, other, tokenId, { from: other }), + 'ERC721InsufficientApproval', + [other, tokenId], + ); + }); + } }); context('when the given token ID does not exist', function () { @@ -210,145 +216,170 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }; - describe('via transferFrom', function () { - shouldTransferTokensByUsers(function (from, to, tokenId, opts) { - return this.token.transferFrom(from, to, tokenId, opts); + const shouldTransferSafely = function (transferFun, data, opts = {}) { + describe('to a user account', function () { + shouldTransferTokensByUsers(transferFun, opts); }); - }); - - describe('via safeTransferFrom', function () { - const safeTransferFromWithData = function (from, to, tokenId, opts) { - return this.token.methods['safeTransferFrom(address,address,uint256,bytes)'](from, to, tokenId, data, opts); - }; - - const safeTransferFromWithoutData = function (from, to, tokenId, opts) { - return this.token.methods['safeTransferFrom(address,address,uint256)'](from, to, tokenId, opts); - }; - const shouldTransferSafely = function (transferFun, data) { - describe('to a user account', function () { - shouldTransferTokensByUsers(transferFun); + describe('to a valid receiver contract', function () { + beforeEach(async function () { + this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, RevertType.None); + this.toWhom = this.receiver.address; }); - describe('to a valid receiver contract', function () { - beforeEach(async function () { - this.receiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, RevertType.None); - this.toWhom = this.receiver.address; - }); - - shouldTransferTokensByUsers(transferFun); + shouldTransferTokensByUsers(transferFun, opts); - it('calls onERC721Received', async function () { - const receipt = await transferFun.call(this, owner, this.receiver.address, tokenId, { from: owner }); + it('calls onERC721Received', async function () { + const receipt = await transferFun.call(this, owner, this.receiver.address, tokenId, { from: owner }); - await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { - operator: owner, - from: owner, - tokenId: tokenId, - data: data, - }); + await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { + operator: owner, + from: owner, + tokenId: tokenId, + data: data, }); + }); - it('calls onERC721Received from approved', async function () { - const receipt = await transferFun.call(this, owner, this.receiver.address, tokenId, { from: approved }); + it('calls onERC721Received from approved', async function () { + const receipt = await transferFun.call(this, owner, this.receiver.address, tokenId, { from: approved }); - await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { - operator: approved, - from: owner, - tokenId: tokenId, - data: data, - }); + await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { + operator: approved, + from: owner, + tokenId: tokenId, + data: data, }); + }); - describe('with an invalid token id', function () { - it('reverts', async function () { - await expectRevertCustomError( - transferFun.call(this, owner, this.receiver.address, nonExistentTokenId, { from: owner }), - 'ERC721NonexistentToken', - [nonExistentTokenId], - ); - }); + describe('with an invalid token id', function () { + it('reverts', async function () { + await expectRevertCustomError( + transferFun.call(this, owner, this.receiver.address, nonExistentTokenId, { from: owner }), + 'ERC721NonexistentToken', + [nonExistentTokenId], + ); }); }); - }; - - describe('with data', function () { - shouldTransferSafely(safeTransferFromWithData, data); - }); - - describe('without data', function () { - shouldTransferSafely(safeTransferFromWithoutData, null); }); + }; - describe('to a receiver contract returning unexpected value', function () { - it('reverts', async function () { - const invalidReceiver = await ERC721ReceiverMock.new('0x42', RevertType.None); - await expectRevertCustomError( - this.token.safeTransferFrom(owner, invalidReceiver.address, tokenId, { from: owner }), - 'ERC721InvalidReceiver', - [invalidReceiver.address], - ); + for (const { fnName, opts } of [ + { fnName: 'transferFrom', opts: {} }, + { fnName: '$_transfer', opts: { unrestricted: true } }, + ]) { + describe(`via ${fnName}`, function () { + shouldTransferTokensByUsers(function (from, to, tokenId, opts) { + return this.token[fnName](from, to, tokenId, opts); + }, opts); + }); + } + + for (const { fnName, opts } of [ + { fnName: 'safeTransferFrom', opts: {} }, + { fnName: '$_safeTransfer', opts: { unrestricted: true } }, + ]) { + describe(`via ${fnName}`, function () { + const safeTransferFromWithData = function (from, to, tokenId, opts) { + return this.token.methods[fnName + '(address,address,uint256,bytes)'](from, to, tokenId, data, opts); + }; + + const safeTransferFromWithoutData = function (from, to, tokenId, opts) { + return this.token.methods[fnName + '(address,address,uint256)'](from, to, tokenId, opts); + }; + + describe('with data', function () { + shouldTransferSafely(safeTransferFromWithData, data, opts); + }); + + describe('without data', function () { + shouldTransferSafely(safeTransferFromWithoutData, null, opts); + }); + + describe('to a receiver contract returning unexpected value', function () { + it('reverts', async function () { + const invalidReceiver = await ERC721ReceiverMock.new('0x42', RevertType.None); + await expectRevertCustomError( + this.token.methods[fnName + '(address,address,uint256)'](owner, invalidReceiver.address, tokenId, { + from: owner, + }), + 'ERC721InvalidReceiver', + [invalidReceiver.address], + ); + }); }); - }); - describe('to a receiver contract that reverts with message', function () { - it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, RevertType.RevertWithMessage); - await expectRevert( - this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), - 'ERC721ReceiverMock: reverting', - ); + describe('to a receiver contract that reverts with message', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new( + RECEIVER_MAGIC_VALUE, + RevertType.RevertWithMessage, + ); + await expectRevert( + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), + 'ERC721ReceiverMock: reverting', + ); + }); }); - }); - describe('to a receiver contract that reverts without message', function () { - it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new( - RECEIVER_MAGIC_VALUE, - RevertType.RevertWithoutMessage, - ); - await expectRevertCustomError( - this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), - 'ERC721InvalidReceiver', - [revertingReceiver.address], - ); + describe('to a receiver contract that reverts without message', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new( + RECEIVER_MAGIC_VALUE, + RevertType.RevertWithoutMessage, + ); + await expectRevertCustomError( + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), + 'ERC721InvalidReceiver', + [revertingReceiver.address], + ); + }); }); - }); - describe('to a receiver contract that reverts with custom error', function () { - it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new( - RECEIVER_MAGIC_VALUE, - RevertType.RevertWithCustomError, - ); - await expectRevertCustomError( - this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), - 'CustomError', - [RECEIVER_MAGIC_VALUE], - ); + describe('to a receiver contract that reverts with custom error', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new( + RECEIVER_MAGIC_VALUE, + RevertType.RevertWithCustomError, + ); + await expectRevertCustomError( + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), + 'CustomError', + [RECEIVER_MAGIC_VALUE], + ); + }); }); - }); - describe('to a receiver contract that panics', function () { - it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, RevertType.Panic); - await expectRevert.unspecified( - this.token.safeTransferFrom(owner, revertingReceiver.address, tokenId, { from: owner }), - ); + describe('to a receiver contract that panics', function () { + it('reverts', async function () { + const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, RevertType.Panic); + await expectRevert.unspecified( + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), + ); + }); }); - }); - describe('to a contract that does not implement the required function', function () { - it('reverts', async function () { - const nonReceiver = await NonERC721ReceiverMock.new(); - await expectRevertCustomError( - this.token.safeTransferFrom(owner, nonReceiver.address, tokenId, { from: owner }), - 'ERC721InvalidReceiver', - [nonReceiver.address], - ); + describe('to a contract that does not implement the required function', function () { + it('reverts', async function () { + const nonReceiver = await NonERC721ReceiverMock.new(); + await expectRevertCustomError( + this.token.methods[fnName + '(address,address,uint256)'](owner, nonReceiver.address, tokenId, { + from: owner, + }), + 'ERC721InvalidReceiver', + [nonReceiver.address], + ); + }); }); }); - }); + } }); describe('safe mint', function () { @@ -524,14 +555,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 +668,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 55c26dffe08..d9e33aff29b 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 ec99dea96b9..5d77149f2b3 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 34738cae12a..129515514c8 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]); }); });