diff --git a/.changeset/bright-tomatoes-sing.md b/.changeset/bright-tomatoes-sing.md index dcd6d7ff027..9eb230ad17d 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), [#4419](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/4419), [#3876](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3876)) diff --git a/contracts/interfaces/draft-IERC6093.sol b/contracts/interfaces/draft-IERC6093.sol index 08e77553c20..2f9374e5790 100644 --- a/contracts/interfaces/draft-IERC6093.sol +++ b/contracts/interfaces/draft-IERC6093.sol @@ -69,11 +69,11 @@ interface IERC721Errors { /** * @dev Indicates an error related to the ownership over a particular token. Used in transfers. - * @param sender Address whose tokens are being transferred. + * @param claimedOwner Address whose tokens are being transferred. * @param tokenId Identifier number of a token. - * @param owner Address of the current owner of a token. + * @param actualOwner Address of the current owner of a token. */ - error ERC721IncorrectOwner(address sender, uint256 tokenId, address owner); + error ERC721IncorrectOwner(address claimedOwner, uint256 tokenId, address actualOwner); /** * @dev Indicates a failure with the token `sender`. Used in transfers. diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 081b8d99dee..725aaaa5dda 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( + function _update( address from, address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Enumerable) { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) { + super._update(from, to, tokenId); } - 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 value) internal virtual override(ERC721, ERC721Enumerable) { + super._increaseBalance(account, value); } } diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 166959e922b..c6f0e67caf4 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( + function _update( address from, address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Pausable) { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) { + super._update(from, to, tokenId); } - 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 value) internal virtual override(ERC721, ERC721Votes) { + super._increaseBalance(account, value); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 25ac69fa96a..a872e7307a4 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -118,8 +118,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er revert ERC721InvalidOperator(owner); } - if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) { - revert ERC721InvalidApprover(_msgSender()); + address sender = _msgSender(); + if (sender != owner && !isApprovedForAll(owner, sender)) { + revert ERC721InvalidApprover(sender); } _approve(to, tokenId); @@ -151,28 +152,23 @@ 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); - } - + function transferFrom(address from, address to, uint256 tokenId) public { + _checkApproved(from, _msgSender(), tokenId); _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, ""); } /** * @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); - } + function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public { + _checkApproved(from, _msgSender(), tokenId); _safeTransfer(from, to, tokenId, data); } @@ -187,22 +183,24 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Requirements: * - * - `from` cannot be the zero address. - * - `to` cannot be the zero address. - * - `tokenId` token must exist and be owned by `from`. + * - `from` and `to` cannot be the zero address + * - `tokenId` token must exist `from` should be the current owner of the token or an approved operator. * - 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, bytes memory data) internal virtual { + function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal { _transfer(from, to, tokenId); - if (!_checkOnERC721Received(from, to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } + _checkOnERC721Received(from, to, tokenId, data); } /** - * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist + * @dev Returns the owner of the `tokenId`. Returns `address(0)` if token doesn't exist. + * + * 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 _ownerOf(uint256 tokenId) internal view virtual returns (address) { return _owners[tokenId]; @@ -221,15 +219,30 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @dev Returns whether `spender` is allowed to manage `tokenId`. + * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in + * particular (ignoring whether it is owned by `owner`). * * Requirements: * * - `tokenId` must exist. */ - 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 _isApproved(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { + return owner == spender || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender; + } + + /** + * @dev Checks whether `spender` was approved by `owner` to operate on `tokenId` or is the owner of `tokenId`. + * Reverts if `spender` is not approved nor owner of `tokenId`. + */ + function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual { + if (!_isApproved(owner, spender, tokenId)) { + address actualOwner = _ownerOf(tokenId); + if (owner == actualOwner) { + revert ERC721InsufficientApproval(spender, tokenId); + } else { + revert ERC721IncorrectOwner(owner, tokenId, actualOwner); + } + } } /** @@ -242,7 +255,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _safeMint(address to, uint256 tokenId) internal virtual { + function _safeMint(address to, uint256 tokenId) internal { _safeMint(to, tokenId, ""); } @@ -250,11 +263,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @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 { + function _safeMint(address to, uint256 tokenId, bytes memory data) internal { _mint(to, tokenId); - if (!_checkOnERC721Received(address(0), to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } + _checkOnERC721Received(address(0), to, tokenId, data); } /** @@ -269,34 +280,11 @@ 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)) { - 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); - - _afterTokenTransfer(address(0), to, tokenId, 1); + _update(address(0), to, tokenId); } /** @@ -306,78 +294,91 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Requirements: * - * - `tokenId` must exist. + * - `tokenId` must exist, so `from` cannot be the zero address. + * - `from` should be the current owner of the token or an approved operator. * * 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(address from, uint256 tokenId) internal { + if (from == address(0)) { + revert ERC721InvalidSender(address(0)); + } + _update(from, address(0), tokenId); } /** * @dev Transfers `tokenId` from `from` to `to`. - * As opposed to {transferFrom}, this imposes no restrictions on msg.sender. + * As opposed to {transferFrom}, this imposes no restrictions on msg.sender. * * Requirements: * - * - `to` cannot be the zero address. - * - `tokenId` token must be owned by `from`. + * - `from` and `to` cannot be the zero address + * - `from` should be the current owner of the token or an approved operator. * * 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 (from == address(0)) { + revert ERC721InvalidSender(address(0)); } if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } + _update(from, to, tokenId); + } - _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); + /** + * @dev Updates the ownership of token with id `tokenId` from `from` to `to`. + * It clears the approval for the token when is transferred. + * + * Requirements: + * + * - `from` should be the current owner of the token or an approved operator. + * + * NOTE: The `from` should be `address(0)` for mints and `to` should be `address(0)` for burns. + * + * Emits a {Transfer} event. + */ + function _update(address from, address to, uint256 tokenId) internal virtual { + address owner = _ownerOf(tokenId); + if (from != owner) { + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else { + 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; + if (from != address(0)) { + delete _tokenApprovals[tokenId]; + unchecked { + // Decrease balance with unchecked arithmetic because `owner == from` and + // therefore we can guarantee `_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; + if (to != address(0)) { + unchecked { + _balances[to] += 1; + } } _owners[tokenId] = to; emit Transfer(from, to, tokenId); + } - _afterTokenTransfer(from, to, tokenId, 1); + /** + * @dev Increases the balance of `account` by an arbitrary `value`. + * + * This function provides write access to the balances, which can be used by extensions that + * "mint" tokens using an {ownerOf} override. + */ + function _increaseBalance(address account, uint128 value) internal virtual { + unchecked { + _balances[account] += value; + } } /** @@ -420,17 +421,13 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @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 +438,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/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 607265328ec..11325b3f1dd 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -18,10 +18,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); + function burn(address from, uint256 tokenId) public virtual { + _checkApproved(from, _msgSender(), tokenId); + _burn(from, tokenId); } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index e25edfcd907..e82a31d037d 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -53,11 +53,6 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { */ error ERC721ForbiddenMint(); - /** - * @dev Batch burn is not supported. - */ - error ERC721ForbiddenBatchBurn(); - /** * @dev Maximum size of a batch of consecutive tokens. This is designed to limit stress on off-chain indexing * services that have to record one entry per token, and have protections against "unreasonably large" batches of @@ -120,61 +115,40 @@ 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); + // Preserve the invariant required by `_ownerOf`, given that the new sequentialOwnership + // checkpoint is attributing ownership of `batchSize` new tokens to account `to`. + _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. - * - * WARNING: Using {ERC721Consecutive} prevents using {_mint} during construction in favor of {_mintConsecutive}. - * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. + * @dev See {ERC721-_update}. Extends token updates so that non-consecutive minting is disabled + * during construction, and burning of tokens that have been sequentially minted are complete. */ - function _mint(address to, uint256 tokenId) internal virtual override { - if (address(this).code.length == 0) { + function _update(address from, address to, uint256 tokenId) internal virtual override { + if (from == 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 { + super._update(from, to, tokenId); + if ( to == address(0) && // if we burn - firstTokenId >= _firstConsecutiveId() && - firstTokenId < _nextConsecutiveId() && - !_sequentialBurn.get(firstTokenId) + tokenId >= _firstConsecutiveId() && + tokenId < _nextConsecutiveId() && + !_sequentialBurn.get(tokenId) ) // and the token was never marked as burnt { - if (batchSize != 1) { - revert ERC721ForbiddenBatchBurn(); - } - _sequentialBurn.set(firstTokenId); + _sequentialBurn.set(tokenId); } - super._afterTokenTransfer(from, to, firstTokenId, batchSize); } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 2e33123b6cf..ce0a0730e48 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -76,20 +76,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * @dev See {ERC721-_beforeTokenTransfer}. */ - 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 from, address to, uint256 tokenId) internal virtual override { + super._update(from, to, tokenId); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); @@ -109,7 +97,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; // Balance is already increased by super._update _ownedTokens[to][length] = tokenId; _ownedTokensIndex[tokenId] = length; } @@ -135,7 +123,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); // Balance is already decreased by super._update uint256 tokenIndex = _ownedTokensIndex[tokenId]; // When the token to delete is the last token, the swap operation is unnecessary @@ -175,4 +163,14 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { delete _allTokensIndex[tokenId]; _allTokens.pop(); } + + /** + * @dev See {ERC721-_increaseBalance}. + */ + function _increaseBalance(address account, uint128 value) internal virtual override { + if (value > 0) { + revert ERC721EnumerableForbiddenBatchMint(); + } + super._increaseBalance(account, value); + } } diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 5777ac36ea2..f549c6486d9 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -27,14 +27,8 @@ abstract contract ERC721Pausable is ERC721, Pausable { * * - the contract must not be paused. */ - function _beforeTokenTransfer( - address from, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - + function _update(address from, address to, uint256 tokenId) internal virtual override { _requireNotPaused(); + super._update(from, to, tokenId); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index b4518dc2482..79afc24927f 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,10 +29,13 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { } /** - * @dev See {ERC721-_burn}. This override additionally clears the royalty information for the token. + * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token on burns. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); - _resetTokenRoyalty(tokenId); + function _update(address from, address to, uint256 tokenId) internal virtual override { + super._update(from, to, tokenId); + + if (to == address(0)) { + _resetTokenRoyalty(tokenId); + } } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 737d28e27f2..6f7ed72246f 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -64,14 +64,14 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { } /** - * @dev See {ERC721-_burn}. This override additionally checks to see if a + * @dev See {ERC721-_update}. This override additionally extends burns to see 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 from, address to, uint256 tokenId) internal virtual override { + super._update(from, to, tokenId); - if (bytes(_tokenURIs[tokenId]).length != 0) { + if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 1e694e4c728..6e8894f5978 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -22,14 +22,17 @@ abstract contract ERC721Votes is ERC721, Votes { * * 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 from, address to, uint256 tokenId) internal virtual override { + super._update(from, to, tokenId); + _transferVotingUnits(from, to, 1); + } + + /** + * @dev See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch. + */ + function _increaseBalance(address account, uint128 value) internal virtual override { + super._increaseBalance(account, value); + _transferVotingUnits(address(0), account, value); } /** diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index b8c396c3e34..f50a33ac456 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -52,10 +52,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); + address owner = _ownerOf(tokenId); + _checkApproved(owner, _msgSender(), tokenId); + _burn(owner, tokenId); // 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/governance/utils/Votes.behavior.js b/test/governance/utils/Votes.behavior.js index 5836cc35154..77fff6abde6 100644 --- a/test/governance/utils/Votes.behavior.js +++ b/test/governance/utils/Votes.behavior.js @@ -269,16 +269,16 @@ function shouldBehaveLikeVotes(accounts, tokens, { mode = 'blocknumber', fungibl const t1 = await this.votes.$_mint(accounts[1], tokens[1]); await time.advanceBlock(); // t2 = burn #1 - const t2 = await this.votes.$_burn(...(fungible ? [accounts[1]] : []), tokens[1]); + const t2 = await this.votes.$_burn(accounts[1], tokens[1]); await time.advanceBlock(); // t3 = mint #2 const t3 = await this.votes.$_mint(accounts[1], tokens[2]); await time.advanceBlock(); // t4 = burn #0 - const t4 = await this.votes.$_burn(...(fungible ? [accounts[1]] : []), tokens[0]); + const t4 = await this.votes.$_burn(accounts[1], tokens[0]); await time.advanceBlock(); // t5 = burn #2 - const t5 = await this.votes.$_burn(...(fungible ? [accounts[1]] : []), tokens[2]); + const t5 = await this.votes.$_burn(accounts[1], tokens[2]); await time.advanceBlock(); t0.timepoint = await clockFromReceipt[mode](t0.receipt); diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 75700f6ab56..ec660d28754 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -705,14 +705,18 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); it('reverts when adding a token id that already exists', async function () { - await expectRevertCustomError(this.token.$_mint(owner, firstTokenId), 'ERC721InvalidSender', [ZERO_ADDRESS]); + await expectRevertCustomError(this.token.$_mint(owner, firstTokenId), 'ERC721IncorrectOwner', [ + ZERO_ADDRESS, + firstTokenId, + owner, + ]); }); }); }); describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { - await expectRevertCustomError(this.token.$_burn(nonExistentTokenId), 'ERC721NonexistentToken', [ + await expectRevertCustomError(this.token.$_burn(owner, nonExistentTokenId), 'ERC721NonexistentToken', [ nonExistentTokenId, ]); }); @@ -725,7 +729,7 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper context('with burnt token', function () { beforeEach(async function () { - this.receipt = await this.token.$_burn(firstTokenId); + this.receipt = await this.token.$_burn(owner, firstTokenId); }); it('emits a Transfer event', function () { @@ -738,7 +742,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); it('reverts when burning a token id that has been deleted', async function () { - await expectRevertCustomError(this.token.$_burn(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); + await expectRevertCustomError(this.token.$_burn(owner, firstTokenId), 'ERC721NonexistentToken', [ + firstTokenId, + ]); }); }); }); @@ -820,7 +826,7 @@ function shouldBehaveLikeERC721Enumerable(owner, newOwner, approved, anotherAppr const newTokenId = new BN(300); const anotherNewTokenId = new BN(400); - await this.token.$_burn(tokenId); + await this.token.$_burn(owner, tokenId); await this.token.$_mint(newOwner, newTokenId); await this.token.$_mint(newOwner, anotherNewTokenId); @@ -860,7 +866,7 @@ function shouldBehaveLikeERC721Enumerable(owner, newOwner, approved, anotherAppr describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { - await expectRevertCustomError(this.token.$_burn(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); + await expectRevertCustomError(this.token.$_burn(owner, firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); context('with minted tokens', function () { @@ -871,7 +877,7 @@ function shouldBehaveLikeERC721Enumerable(owner, newOwner, approved, anotherAppr context('with burnt token', function () { beforeEach(async function () { - this.receipt = await this.token.$_burn(firstTokenId); + this.receipt = await this.token.$_burn(owner, firstTokenId); }); it('removes that token from the token list of the owner', async function () { @@ -883,7 +889,7 @@ function shouldBehaveLikeERC721Enumerable(owner, newOwner, approved, anotherAppr }); it('burns all tokens', async function () { - await this.token.$_burn(secondTokenId, { from: owner }); + await this.token.$_burn(owner, secondTokenId, { from: owner }); expect(await this.token.totalSupply()).to.be.bignumber.equal('0'); await expectRevertCustomError(this.token.tokenByIndex(0), 'ERC721OutOfBoundsIndex', [ZERO_ADDRESS, 0]); }); diff --git a/test/token/ERC721/extensions/ERC721Burnable.test.js b/test/token/ERC721/extensions/ERC721Burnable.test.js index df059e09078..1779c8c3396 100644 --- a/test/token/ERC721/extensions/ERC721Burnable.test.js +++ b/test/token/ERC721/extensions/ERC721Burnable.test.js @@ -31,7 +31,7 @@ contract('ERC721Burnable', function (accounts) { describe('when successful', function () { beforeEach(async function () { - receipt = await this.token.burn(tokenId, { from: owner }); + receipt = await this.token.burn(owner, tokenId, { from: owner }); }); it('burns the given token ID and adjusts the balance of the owner', async function () { @@ -51,7 +51,7 @@ contract('ERC721Burnable', function (accounts) { describe('when there is a previous approval burned', function () { beforeEach(async function () { await this.token.approve(approved, tokenId, { from: owner }); - receipt = await this.token.burn(tokenId, { from: owner }); + receipt = await this.token.burn(owner, tokenId, { from: owner }); }); context('getApproved', function () { @@ -63,18 +63,21 @@ contract('ERC721Burnable', function (accounts) { describe('when there is no previous approval burned', function () { it('reverts', async function () { - await expectRevertCustomError(this.token.burn(tokenId, { from: another }), 'ERC721InsufficientApproval', [ - another, - tokenId, - ]); + await expectRevertCustomError( + this.token.burn(owner, tokenId, { from: another }), + 'ERC721InsufficientApproval', + [another, tokenId], + ); }); }); describe('when the given token ID was not tracked by this contract', function () { it('reverts', async function () { - await expectRevertCustomError(this.token.burn(unknownTokenId, { from: owner }), 'ERC721NonexistentToken', [ - unknownTokenId, - ]); + await expectRevertCustomError( + this.token.burn(owner, unknownTokenId, { from: owner }), + 'ERC721NonexistentToken', + [unknownTokenId], + ); }); }); }); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.t.sol b/test/token/ERC721/extensions/ERC721Consecutive.t.sol index 617b17a4669..fc03fc685d7 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.t.sol +++ b/test/token/ERC721/extensions/ERC721Consecutive.t.sol @@ -28,8 +28,8 @@ contract ERC721ConsecutiveTarget is StdUtils, ERC721Consecutive { } } - function burn(uint256 tokenId) public { - _burn(tokenId); + function burn(address from, uint256 tokenId) public { + _burn(from, tokenId); } function _firstConsecutiveId() internal view virtual override returns (uint96) { @@ -96,7 +96,7 @@ contract ERC721ConsecutiveTest is Test { // burn a token in [0; supply[ uint256 tokenId = bound(unboundedTokenId, startingTokenId, startingTokenId + supply - 1); - token.burn(tokenId); + token.burn(receiver, tokenId); // balance should have decreased assertEq(token.balanceOf(receiver), supply - 1); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 55c26dffe08..1166f211b72 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -117,7 +117,11 @@ contract('ERC721Consecutive', function (accounts) { expect(await this.token.$_exists(tokenId)).to.be.equal(true); - await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721InvalidSender', [ZERO_ADDRESS]); + await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721IncorrectOwner', [ + ZERO_ADDRESS, + tokenId, + await this.token.ownerOf(tokenId), + ]); }); }); @@ -131,7 +135,7 @@ contract('ERC721Consecutive', function (accounts) { }); it('tokens can be burned and re-minted #1', async function () { - expectEvent(await this.token.$_burn(tokenId, { from: user1 }), 'Transfer', { + expectEvent(await this.token.$_burn(user1, tokenId, { from: user1 }), 'Transfer', { from: user1, to: constants.ZERO_ADDRESS, tokenId, @@ -161,7 +165,7 @@ contract('ERC721Consecutive', function (accounts) { expect(await this.token.ownerOf(tokenId), user1); // burn - expectEvent(await this.token.$_burn(tokenId, { from: user1 }), 'Transfer', { + expectEvent(await this.token.$_burn(user1, tokenId, { from: user1 }), 'Transfer', { from: user1, to: constants.ZERO_ADDRESS, tokenId, @@ -180,17 +184,6 @@ contract('ERC721Consecutive', function (accounts) { 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', - [], - ); - }); }); }); } @@ -212,14 +205,6 @@ contract('ERC721Consecutive', function (accounts) { ); }); - it('cannot use single minting during construction', async function () { - await expectRevertCustomError( - ERC721ConsecutiveNoConstructorMintMock.new(name, symbol), - 'ERC721ForbiddenMint', - [], - ); - }); - it('consecutive mint not compatible with enumerability', async function () { await expectRevertCustomError( ERC721ConsecutiveEnumerableMock.new( diff --git a/test/token/ERC721/extensions/ERC721Pausable.test.js b/test/token/ERC721/extensions/ERC721Pausable.test.js index ec99dea96b9..2526e01bbbc 100644 --- a/test/token/ERC721/extensions/ERC721Pausable.test.js +++ b/test/token/ERC721/extensions/ERC721Pausable.test.js @@ -57,7 +57,7 @@ contract('ERC721Pausable', function (accounts) { }); it('reverts when trying to burn', async function () { - await expectRevertCustomError(this.token.$_burn(firstTokenId), 'EnforcedPause', []); + await expectRevertCustomError(this.token.$_burn(owner, firstTokenId), 'EnforcedPause', []); }); describe('getApproved', function () { diff --git a/test/token/ERC721/extensions/ERC721Royalty.test.js b/test/token/ERC721/extensions/ERC721Royalty.test.js index 1c0536bf5ff..b0b024b8714 100644 --- a/test/token/ERC721/extensions/ERC721Royalty.test.js +++ b/test/token/ERC721/extensions/ERC721Royalty.test.js @@ -29,7 +29,7 @@ contract('ERC721Royalty', function (accounts) { }); it('removes royalty information after burn', async function () { - await this.token.$_burn(tokenId1); + await this.token.$_burn(account1, tokenId1); const tokenInfo = await this.token.royaltyInfo(tokenId1, salePrice); expect(tokenInfo[0]).to.be.equal(constants.ZERO_ADDRESS); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index 34738cae12a..58fb77ad0db 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -84,7 +84,7 @@ contract('ERC721URIStorage', function (accounts) { }); it('tokens without URI can be burnt ', async function () { - await this.token.$_burn(firstTokenId, { from: owner }); + await this.token.$_burn(owner, firstTokenId, { from: owner }); expect(await this.token.$_exists(firstTokenId)).to.equal(false); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); @@ -93,7 +93,7 @@ contract('ERC721URIStorage', function (accounts) { it('tokens with URI can be burnt ', async function () { await this.token.$_setTokenURI(firstTokenId, sampleUri); - await this.token.$_burn(firstTokenId, { from: owner }); + await this.token.$_burn(owner, firstTokenId, { from: owner }); expect(await this.token.$_exists(firstTokenId)).to.equal(false); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]);