From a3526acdf233ecac6e23dcf13ed05b1901cf526b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 27 Apr 2023 16:37:40 +0200 Subject: [PATCH 01/47] Rebase ERC721._update on top of next-v5 --- .../token/ERC721ConsecutiveEnumerableMock.sol | 24 +-- .../mocks/token/ERC721ConsecutiveMock.sol | 24 +-- contracts/token/ERC721/ERC721.sol | 192 ++++++++---------- .../ERC721/extensions/ERC721Consecutive.sol | 51 ++--- .../ERC721/extensions/ERC721Enumerable.sol | 26 +-- .../ERC721/extensions/ERC721Pausable.sol | 14 +- .../token/ERC721/extensions/ERC721Royalty.sol | 13 +- .../ERC721/extensions/ERC721URIStorage.sol | 10 +- .../token/ERC721/extensions/ERC721Votes.sol | 16 +- 9 files changed, 147 insertions(+), 223 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index a2ee8199a0f..9219ebfca96 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -27,25 +27,11 @@ 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, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Enumerable) { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - } - - function _afterTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { + return super._update(to, tokenId, constraints); } } diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 4aae388e094..ed68ecce590 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -39,26 +39,12 @@ 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, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Pausable) { - super._beforeTokenTransfer(from, to, firstTokenId, batchSize); - } - - function _afterTokenTransfer( - address from, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { + return super._update(to, tokenId, constraints); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 21ed95813a6..207d2af5e44 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -151,12 +151,14 @@ 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 virtual override { + if (to == address(0)) { + revert ERC721InvalidReceiver(address(0)); + } + address owner = _update(to, tokenId, _constraintApprovedOrOwner); + if (owner != from) { + revert ERC721IncorrectOwner(from, tokenId, owner); } - - _transfer(from, to, tokenId); } /** @@ -257,6 +259,46 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } + /** + * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner + * (or `to`) is the zero address. + * + * The `constraints` argument is used to specify constraints, and eventually revert. For example this can be used + * to ensure that the current owner is what was expected. + + * All customizations to transfers, mints, and burns should be done by overriding this function. + * + * Emits a {Transfer} event. + */ + function _update( + address to, + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual returns (address) { + address from = _ownerOf(tokenId); + + constraints(from, to, tokenId); + + 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; + } + /** * @dev Mints `tokenId` and transfers it to `to`. * @@ -269,34 +311,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(to, tokenId, _constraintNotMinted); } /** @@ -310,26 +329,8 @@ 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 { + _update(address(0), tokenId, _constraintMinted); } /** @@ -343,41 +344,14 @@ 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); + address owner = _update(to, tokenId, _constraintMinted); 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; - } - - _owners[tokenId] = to; - - emit Transfer(from, to, tokenId); - - _afterTokenTransfer(from, to, tokenId, 1); } /** @@ -412,6 +386,34 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } + /** + * @dev Contraint: revert if token is already minted + */ + function _constraintNotMinted(address from, address, uint256) internal pure { + if (from != address(0)) { + revert ERC721InvalidSender(address(0)); + } + } + + /** + * @dev Contraint: revert if token is not yet minted + */ + function _constraintMinted(address from, address, uint256 tokenId) internal pure { + if (from == address(0)) { + revert ERC721NonexistentToken(tokenId); + } + } + + /** + * @dev Contraint: check that the caller is the current owner, or approved by the current owner + */ + function _constraintApprovedOrOwner(address owner, address, uint256 tokenId) internal view { + address spender = _msgSender(); + if (spender != owner && !isApprovedForAll(owner, spender) && getApproved(tokenId) != spender) { + revert ERC721InsufficientApproval(_msgSender(), tokenId); + } + } + /** * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address. * The call is not executed if the target address is not a contract. @@ -446,38 +448,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } - /** - * @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. * diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index f1308cdab1a..621b3676a99 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -120,9 +120,6 @@ 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)); @@ -132,49 +129,39 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { __unsafe_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, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); + + // only mint after construction + 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 { + // 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 from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 18e2ba5d626..36a8e552439 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -73,22 +73,10 @@ 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, function(address, address, uint256) view constraints) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); @@ -97,9 +85,11 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } if (to == address(0)) { _removeTokenFromAllTokensEnumeration(tokenId); - } else if (to != from) { + } else if (from != to) { _addTokenToOwnerEnumeration(to, tokenId); } + + return from; } /** @@ -108,7 +98,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; } @@ -134,7 +124,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 diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index a9472c5dc84..47990f407e8 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -21,20 +21,18 @@ import "../../../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); - + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { _requireNotPaused(); + return super._update(to, tokenId, constraints); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 7d6ef6c04e7..8b16cc9be9b 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,10 +29,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}. This override additionally clears the royalty information for the token. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); - _resetTokenRoyalty(tokenId); + function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); + + if (to == address(0)) { + _resetTokenRoyalty(tokenId); + } + + return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index ae625fc2890..53f118b070f 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -62,15 +62,17 @@ 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 checks 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 to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); - if (bytes(_tokenURIs[tokenId]).length != 0) { + if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } + + return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 89b2e073cee..eea2fd5157e 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -18,18 +18,18 @@ import "../../../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, + function _update( address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - _transferVotingUnits(from, to, batchSize); - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { + address from = super._update(to, tokenId, constraints); + _transferVotingUnits(from, to, 1); + return from; } /** From 1ed8f9ef2ced4ce5cc0476bb0953fe6c66fd9d6c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 21 Jun 2023 17:56:00 +0200 Subject: [PATCH 02/47] use __unsafe_increaseBalance to react to batch minting --- .../mocks/token/ERC721ConsecutiveEnumerableMock.sol | 4 ++++ contracts/mocks/token/ERC721ConsecutiveMock.sol | 4 ++++ contracts/token/ERC721/ERC721.sol | 2 +- .../token/ERC721/extensions/ERC721Enumerable.sol | 11 +++++++++++ contracts/token/ERC721/extensions/ERC721Votes.sol | 9 +++++++++ .../token/ERC721/extensions/ERC721Consecutive.test.js | 11 ----------- 6 files changed, 29 insertions(+), 12 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 9219ebfca96..a4588e61146 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -34,4 +34,8 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { return super._update(to, tokenId, constraints); } + + function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Enumerable) { + super.__unsafe_increaseBalance(account, amount); + } } diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index ed68ecce590..f7b718eedca 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -46,6 +46,10 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { return super._update(to, tokenId, constraints); } + + function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Votes) { + super.__unsafe_increaseBalance(account, amount); + } } contract ERC721ConsecutiveNoConstructorMintMock is ERC721Consecutive { diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 207d2af5e44..61d9db2dcf1 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -456,7 +456,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * that `ownerOf(tokenId)` is `a`. */ // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance(address account, uint256 amount) internal { + function __unsafe_increaseBalance(address account, uint256 amount) internal virtual { _balances[account] += amount; } } diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 36a8e552439..95035e0c9c8 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -164,4 +164,15 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { delete _allTokensIndex[tokenId]; _allTokens.pop(); } + + /** + * See {ERC721-__unsafe_increaseBalance}. We need that to account tokens that were minted in batch + */ + // solhint-disable-next-line func-name-mixedcase + function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override { + if (amount > 0) { + revert ERC721EnumerableForbiddenBatchMint(); + } + super.__unsafe_increaseBalance(account, amount); + } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index eea2fd5157e..f40d74a0173 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -40,4 +40,13 @@ abstract contract ERC721Votes is ERC721, Votes { function _getVotingUnits(address account) internal view virtual override returns (uint256) { return balanceOf(account); } + + /** + * See {ERC721-__unsafe_increaseBalance}. We need that to account tokens that were minted in batch + */ + // solhint-disable-next-line func-name-mixedcase + function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override { + super.__unsafe_increaseBalance(account, amount); + _transferVotingUnits(address(0), account, amount); + } } diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 55c26dffe08..c29226ecdbb 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -180,17 +180,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', - [], - ); - }); }); }); } From 7ec34355ae4e93855da9793e8297acb3e44b11e3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 21 Jun 2023 17:59:22 +0200 Subject: [PATCH 03/47] Apply suggestions from code review --- contracts/token/ERC721/ERC721.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 61d9db2dcf1..d0a2d0e5af2 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -387,7 +387,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @dev Contraint: revert if token is already minted + * @dev Constraint: revert if token is already minted */ function _constraintNotMinted(address from, address, uint256) internal pure { if (from != address(0)) { @@ -396,7 +396,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @dev Contraint: revert if token is not yet minted + * @dev Constraint: revert if token is not yet minted */ function _constraintMinted(address from, address, uint256 tokenId) internal pure { if (from == address(0)) { @@ -405,7 +405,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @dev Contraint: check that the caller is the current owner, or approved by the current owner + * @dev Constraint: check that the caller is the current owner, or approved by the current owner */ function _constraintApprovedOrOwner(address owner, address, uint256 tokenId) internal view { address spender = _msgSender(); From e2fdbacd63b44b25b1672c67c7aeb6ad5d3bd85d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 21 Jun 2023 22:09:50 +0200 Subject: [PATCH 04/47] fix lint --- contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol | 5 ++++- contracts/token/ERC721/extensions/ERC721Enumerable.sol | 6 +++++- contracts/token/ERC721/extensions/ERC721Royalty.sol | 6 +++++- contracts/token/ERC721/extensions/ERC721URIStorage.sol | 6 +++++- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index a4588e61146..d038f0e218a 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -35,7 +35,10 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._update(to, tokenId, constraints); } - function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Enumerable) { + function __unsafe_increaseBalance( + address account, + uint256 amount + ) internal virtual override(ERC721, ERC721Enumerable) { super.__unsafe_increaseBalance(account, amount); } } diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 95035e0c9c8..982ceb181b4 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -75,7 +75,11 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * @dev See {ERC721-_update}. */ - function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + function _update( + address to, + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { address from = super._update(to, tokenId, constraints); if (from == address(0)) { diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 8b16cc9be9b..8d68b514639 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -31,7 +31,11 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { /** * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token. */ - function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + function _update( + address to, + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { address from = super._update(to, tokenId, constraints); if (to == address(0)) { diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 53f118b070f..eb30f628911 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -66,7 +66,11 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _update(address to, uint256 tokenId, function(address, address, uint256) view constraints) internal virtual override returns (address) { + function _update( + address to, + uint256 tokenId, + function(address, address, uint256) view constraints + ) internal virtual override returns (address) { address from = super._update(to, tokenId, constraints); if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { From e9f03bd211c414458bc0e5b8123f270ad119b052 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 30 Jun 2023 12:09:15 -0300 Subject: [PATCH 05/47] Exclude address(0) in ERC721._isApprovedOrOwner --- contracts/token/ERC721/ERC721.sol | 33 +++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 4232c616eb7..fcaf04f2afb 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -114,9 +114,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er */ 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()); @@ -131,7 +128,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); } /** @@ -220,16 +217,22 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er return _ownerOf(tokenId) != address(0); } + /** + * @dev Returns the approved address for `tokenId`. Returns 0 if `tokenId` is not minted. + */ + function _getApproved(uint256 tokenId) internal view virtual returns (address) { + return _tokenApprovals[tokenId]; + } + /** * @dev Returns whether `spender` is allowed to manage `tokenId`. - * - * 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); + address owner = _ownerOf(tokenId); + return (owner != address(0) && + (spender == owner || + isApprovedForAll(owner, spender) || + (_getApproved(tokenId) == spender && spender != address(0)))); } /** @@ -386,8 +389,12 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits an {Approval} event. */ function _approve(address to, uint256 tokenId) internal virtual { + address owner = ownerOf(tokenId); + if (to == owner || to == address(0)) { + revert ERC721InvalidOperator(to); + } _tokenApprovals[tokenId] = to; - emit Approval(ownerOf(tokenId), to, tokenId); + emit Approval(owner, to, tokenId); } /** @@ -396,8 +403,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits an {ApprovalForAll} event. */ function _setApprovalForAll(address owner, address operator, bool approved) internal virtual { - if (owner == operator) { - revert ERC721InvalidOperator(owner); + if (operator == owner || operator == address(0)) { + revert ERC721InvalidOperator(operator); } _operatorApprovals[owner][operator] = approved; emit ApprovalForAll(owner, operator, approved); From c7303ec2ae96e12f8011a1621e6d0e47249acfe5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 3 Jul 2023 09:37:53 +0200 Subject: [PATCH 06/47] fix lint --- contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol | 1 + contracts/mocks/token/ERC721ConsecutiveMock.sol | 1 + 2 files changed, 2 insertions(+) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index cdfab3d175b..5771c3355d2 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -36,6 +36,7 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._update(to, tokenId, constraints); } + // solhint-disable-next-line func-name-mixedcase function __unsafe_increaseBalance( address account, uint256 amount diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 56ef577a977..b12e1cb6e4d 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -49,6 +49,7 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes return super._update(to, tokenId, constraints); } + // solhint-disable-next-line func-name-mixedcase function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Votes) { super.__unsafe_increaseBalance(account, amount); } From 562ddf566a02b70db0629f1d4a17f8704fbaf8d3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 5 Jul 2023 18:45:42 +0200 Subject: [PATCH 07/47] implement hybrid _update --- .../token/ERC721ConsecutiveEnumerableMock.sol | 8 +++--- .../mocks/token/ERC721ConsecutiveMock.sol | 8 +++--- contracts/token/ERC721/ERC721.sol | 27 ++++++++++++------- .../ERC721/extensions/ERC721Burnable.sol | 5 +--- .../ERC721/extensions/ERC721Consecutive.sol | 10 +++---- .../ERC721/extensions/ERC721Enumerable.sol | 10 +++---- .../ERC721/extensions/ERC721Pausable.sol | 8 +++--- .../token/ERC721/extensions/ERC721Royalty.sol | 10 +++---- .../ERC721/extensions/ERC721URIStorage.sol | 10 +++---- .../token/ERC721/extensions/ERC721Votes.sol | 9 +++---- .../token/ERC721/extensions/ERC721Wrapper.sol | 5 +--- 11 files changed, 51 insertions(+), 59 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 5771c3355d2..65bb135988f 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -29,11 +29,11 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable } function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { - return super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) { + super._update(from, to, tokenId); } // solhint-disable-next-line func-name-mixedcase diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index b12e1cb6e4d..f69c7056d0c 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -42,11 +42,11 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes } function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { - return super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) { + return super._update(from, to, tokenId); } // solhint-disable-next-line func-name-mixedcase diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 6c85d6ca609..d4d8d197824 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -155,7 +155,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _update(to, tokenId, _constraintApprovedOrOwner); + address owner = _updateWithConstraints(to, tokenId, _constraintApprovedOrOwner); if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } @@ -265,20 +265,29 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * The `constraints` argument is used to specify constraints, and eventually revert. For example this can be used * to ensure that the current owner is what was expected. - - * All customizations to transfers, mints, and burns should be done by overriding this function. * * Emits a {Transfer} event. */ - function _update( + function _updateWithConstraints( address to, uint256 tokenId, function(address, address, uint256) view constraints ) internal virtual returns (address) { address from = _ownerOf(tokenId); - constraints(from, to, tokenId); + _update(from, to, tokenId); + return from; + } + /** + * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner + * (or `to`) is the zero address. + * + * All customizations to transfers, mints, and burns should be done by overriding this function. + * + * Emits a {Transfer} event. + */ + function _update(address from, address to, uint256 tokenId) internal virtual { if (from != address(0)) { delete _tokenApprovals[tokenId]; unchecked { @@ -295,8 +304,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er _owners[tokenId] = to; emit Transfer(from, to, tokenId); - - return from; } /** @@ -315,7 +322,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - _update(to, tokenId, _constraintNotMinted); + _updateWithConstraints(to, tokenId, _constraintNotMinted); } /** @@ -330,7 +337,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits a {Transfer} event. */ function _burn(uint256 tokenId) internal { - _update(address(0), tokenId, _constraintMinted); + _updateWithConstraints(address(0), tokenId, _constraintMinted); } /** @@ -348,7 +355,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _update(to, tokenId, _constraintMinted); + address owner = _updateWithConstraints(to, tokenId, _constraintMinted); if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 607265328ec..c268d12d834 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,9 +19,6 @@ 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); + _updateWithConstraints(address(0), tokenId, _constraintApprovedOrOwner); } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 261c3041de1..de8d1b62985 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -141,11 +141,11 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * After construction, {_mintConsecutive} is no longer available and minting through {_update} becomes available. */ function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override returns (address) { - address from = super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override { + super._update(from, to, tokenId); // only mint after construction if (from == address(0) && address(this).code.length == 0) { @@ -160,8 +160,6 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { ) { _sequentialBurn.set(tokenId); } - - return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index ba864c81cb0..86fd5abff86 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -77,11 +77,11 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { * @dev See {ERC721-_update}. */ function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override returns (address) { - address from = super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override { + super._update(from, to, tokenId); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); @@ -93,8 +93,6 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } else if (from != to) { _addTokenToOwnerEnumeration(to, tokenId); } - - return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 672d11af144..229759c947e 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -28,11 +28,11 @@ abstract contract ERC721Pausable is ERC721, Pausable { * - the contract must not be paused. */ function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override returns (address) { + uint256 tokenId + ) internal virtual override { _requireNotPaused(); - return super._update(to, tokenId, constraints); + super._update(from, to, tokenId); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index a7d1b96f1e8..107fc6709b9 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -32,16 +32,14 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token. */ function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override returns (address) { - address from = super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override { + super._update(from, to, tokenId); if (to == address(0)) { _resetTokenRoyalty(tokenId); } - - return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 8fe503ac96d..9480ee122e0 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -69,16 +69,14 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * the storage mapping. */ function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override returns (address) { - address from = super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override { + super._update(from, to, tokenId); if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } - - return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 7447640f37a..ed958c7a6c9 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -23,13 +23,12 @@ abstract contract ERC721Votes is ERC721, Votes { * Emits a {IVotes-DelegateVotesChanged} event. */ function _update( + address from, address to, - uint256 tokenId, - function(address, address, uint256) view constraints - ) internal virtual override returns (address) { - address from = super._update(to, tokenId, constraints); + uint256 tokenId + ) internal virtual override { + super._update(from, to, tokenId); _transferVotingUnits(from, to, 1); - return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index b8c396c3e34..426461337f3 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -52,10 +52,7 @@ 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); + _updateWithConstraints(address(0), tokenId, _constraintApprovedOrOwner); // 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 From 5ab254cf95f13e379e288da68e884d4bccb67a7a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 7 Jul 2023 16:13:17 +0200 Subject: [PATCH 08/47] lint --- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 6 +----- contracts/token/ERC721/extensions/ERC721Enumerable.sol | 6 +----- contracts/token/ERC721/extensions/ERC721Pausable.sol | 6 +----- contracts/token/ERC721/extensions/ERC721Royalty.sol | 6 +----- contracts/token/ERC721/extensions/ERC721URIStorage.sol | 6 +----- contracts/token/ERC721/extensions/ERC721Votes.sol | 6 +----- 6 files changed, 6 insertions(+), 30 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 72947539032..56fefb3069c 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -138,11 +138,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * 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 _update( - address from, - address to, - uint256 tokenId - ) internal virtual override { + function _update(address from, address to, uint256 tokenId) internal virtual override { super._update(from, to, tokenId); // only mint after construction diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 86fd5abff86..b82469b94d9 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -76,11 +76,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * @dev See {ERC721-_update}. */ - function _update( - address from, - address to, - uint256 tokenId - ) internal virtual override { + function _update(address from, address to, uint256 tokenId) internal virtual override { super._update(from, to, tokenId); if (from == address(0)) { diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 229759c947e..1a596fd0090 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -27,11 +27,7 @@ abstract contract ERC721Pausable is ERC721, Pausable { * * - the contract must not be paused. */ - function _update( - address from, - address to, - uint256 tokenId - ) internal virtual override { + 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 becea403b82..dd2aa428307 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,11 +29,7 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { /** * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token. */ - function _update( - address from, - address to, - uint256 tokenId - ) internal virtual override { + function _update(address from, address to, uint256 tokenId) internal virtual override { super._update(from, to, tokenId); if (to == address(0)) { diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 9480ee122e0..6e08de3ac67 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -68,11 +68,7 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _update( - address from, - address to, - uint256 tokenId - ) internal virtual override { + function _update(address from, address to, uint256 tokenId) internal virtual override { super._update(from, to, tokenId); if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 9e93ff711f3..56e3525a56a 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -20,11 +20,7 @@ abstract contract ERC721Votes is ERC721, Votes { * * Emits a {IVotes-DelegateVotesChanged} event. */ - function _update( - address from, - address to, - uint256 tokenId - ) internal virtual override { + function _update(address from, address to, uint256 tokenId) internal virtual override { super._update(from, to, tokenId); _transferVotingUnits(from, to, 1); } From bd0c52e34a5503d30d089f18fb0611c1a9cd5f77 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 Jul 2023 18:06:29 +0200 Subject: [PATCH 09/47] refactor constraint into an optionalChecks bitmap --- .../token/ERC721ConsecutiveEnumerableMock.sol | 8 +- .../mocks/token/ERC721ConsecutiveMock.sol | 8 +- contracts/token/ERC721/ERC721.sol | 80 +++++++------------ .../ERC721/extensions/ERC721Burnable.sol | 2 +- .../ERC721/extensions/ERC721Consecutive.sol | 6 +- .../ERC721/extensions/ERC721Enumerable.sol | 6 +- .../ERC721/extensions/ERC721Pausable.sol | 4 +- .../token/ERC721/extensions/ERC721Royalty.sol | 6 +- .../ERC721/extensions/ERC721URIStorage.sol | 6 +- .../token/ERC721/extensions/ERC721Votes.sol | 7 +- .../token/ERC721/extensions/ERC721Wrapper.sol | 2 +- 11 files changed, 64 insertions(+), 71 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 65bb135988f..6a81ee622ab 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -29,11 +29,11 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable } function _update( - address from, address to, - uint256 tokenId - ) internal virtual override(ERC721Consecutive, ERC721Enumerable) { - super._update(from, to, tokenId); + uint256 tokenId, + bytes32 optionalChecks + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { + return super._update(to, tokenId, optionalChecks); } // solhint-disable-next-line func-name-mixedcase diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index f69c7056d0c..7f07b5260ca 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -42,11 +42,11 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes } function _update( - address from, address to, - uint256 tokenId - ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) { - return super._update(from, to, tokenId); + uint256 tokenId, + bytes32 optionalChecks + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { + return super._update(to, tokenId, optionalChecks); } // solhint-disable-next-line func-name-mixedcase diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d4d8d197824..770c0eeb8d4 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -19,6 +19,10 @@ import {IERC721Errors} from "../../interfaces/draft-IERC6093.sol"; abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Errors { using Strings for uint256; + bytes32 internal constant CONSTRAINT_MINTED = bytes32(uint256(1) << 0); + bytes32 internal constant CONSTRAINT_NOT_MINTED = bytes32(uint256(1) << 1); + bytes32 internal constant CONSTRAINT_SPENDER_APPROVED_OR_OWNER = bytes32(uint256(1) << 2); + // Token name string private _name; @@ -155,7 +159,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _updateWithConstraints(to, tokenId, _constraintApprovedOrOwner); + address owner = _update(to, tokenId, CONSTRAINT_MINTED | CONSTRAINT_SPENDER_APPROVED_OR_OWNER); if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } @@ -263,31 +267,35 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner * (or `to`) is the zero address. * - * The `constraints` argument is used to specify constraints, and eventually revert. For example this can be used - * to ensure that the current owner is what was expected. + * The `optionalChecks` argument is used to specify constraints, that the caller would want to be checked as part + * of the update. * * Emits a {Transfer} event. */ - function _updateWithConstraints( + function _update( address to, uint256 tokenId, - function(address, address, uint256) view constraints + bytes32 optionalChecks ) internal virtual returns (address) { address from = _ownerOf(tokenId); - constraints(from, to, tokenId); - _update(from, to, tokenId); - return from; - } - /** - * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner - * (or `to`) is the zero address. - * - * All customizations to transfers, mints, and burns should be done by overriding this function. - * - * Emits a {Transfer} event. - */ - function _update(address from, address to, uint256 tokenId) internal virtual { + // Perform optional checks + if (optionalChecks & CONSTRAINT_MINTED != 0 && from == address(0)) { + revert ERC721NonexistentToken(tokenId); + } + + if (optionalChecks & CONSTRAINT_NOT_MINTED != 0 && from != address(0)) { + revert ERC721InvalidSender(address(0)); + } + + if (optionalChecks & CONSTRAINT_SPENDER_APPROVED_OR_OWNER != 0) { + address spender = _msgSender(); + if (from != spender && !isApprovedForAll(from, spender) && getApproved(tokenId) != spender) { + revert ERC721InsufficientApproval(_msgSender(), tokenId); + } + } + + // Execute the update if (from != address(0)) { delete _tokenApprovals[tokenId]; unchecked { @@ -304,6 +312,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er _owners[tokenId] = to; emit Transfer(from, to, tokenId); + + return from; } /** @@ -322,7 +332,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - _updateWithConstraints(to, tokenId, _constraintNotMinted); + _update(to, tokenId, CONSTRAINT_NOT_MINTED); } /** @@ -337,7 +347,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits a {Transfer} event. */ function _burn(uint256 tokenId) internal { - _updateWithConstraints(address(0), tokenId, _constraintMinted); + _update(address(0), tokenId, CONSTRAINT_MINTED); } /** @@ -355,7 +365,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _updateWithConstraints(to, tokenId, _constraintMinted); + address owner = _update(to, tokenId, CONSTRAINT_MINTED); if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } @@ -393,34 +403,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } - /** - * @dev Constraint: revert if token is already minted - */ - function _constraintNotMinted(address from, address, uint256) internal pure { - if (from != address(0)) { - revert ERC721InvalidSender(address(0)); - } - } - - /** - * @dev Constraint: revert if token is not yet minted - */ - function _constraintMinted(address from, address, uint256 tokenId) internal pure { - if (from == address(0)) { - revert ERC721NonexistentToken(tokenId); - } - } - - /** - * @dev Constraint: check that the caller is the current owner, or approved by the current owner - */ - function _constraintApprovedOrOwner(address owner, address, uint256 tokenId) internal view { - address spender = _msgSender(); - if (spender != owner && !isApprovedForAll(owner, spender) && getApproved(tokenId) != spender) { - revert ERC721InsufficientApproval(_msgSender(), 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. diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index c268d12d834..93aa449b93c 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,6 +19,6 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - _updateWithConstraints(address(0), tokenId, _constraintApprovedOrOwner); + _update(address(0), tokenId, CONSTRAINT_MINTED | CONSTRAINT_SPENDER_APPROVED_OR_OWNER); } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 56fefb3069c..83c3b931c24 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -138,8 +138,8 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * 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 _update(address from, address to, uint256 tokenId) internal virtual override { - super._update(from, to, tokenId); + function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { + address from = super._update(to, tokenId, optionalChecks); // only mint after construction if (from == address(0) && address(this).code.length == 0) { @@ -154,6 +154,8 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { ) { _sequentialBurn.set(tokenId); } + + return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index b82469b94d9..9199b9b35bb 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -76,8 +76,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * @dev See {ERC721-_update}. */ - function _update(address from, address to, uint256 tokenId) internal virtual override { - super._update(from, to, tokenId); + function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { + address from = super._update(to, tokenId, optionalChecks); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); @@ -89,6 +89,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } else if (from != to) { _addTokenToOwnerEnumeration(to, tokenId); } + + return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 1a596fd0090..0de35ccefc9 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -27,8 +27,8 @@ abstract contract ERC721Pausable is ERC721, Pausable { * * - the contract must not be paused. */ - function _update(address from, address to, uint256 tokenId) internal virtual override { + function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { _requireNotPaused(); - super._update(from, to, tokenId); + return super._update(to, tokenId, optionalChecks); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index dd2aa428307..a5d5edd81ae 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,11 +29,13 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { /** * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token. */ - function _update(address from, address to, uint256 tokenId) internal virtual override { - super._update(from, to, tokenId); + function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { + address from = super._update(to, tokenId, optionalChecks); if (to == address(0)) { _resetTokenRoyalty(tokenId); } + + return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 6e08de3ac67..000f0bd9d4f 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -68,11 +68,13 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _update(address from, address to, uint256 tokenId) internal virtual override { - super._update(from, to, tokenId); + function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { + address from = super._update(to, tokenId, optionalChecks); if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } + + return from; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 56e3525a56a..42fb94572b8 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -20,9 +20,12 @@ abstract contract ERC721Votes is ERC721, Votes { * * Emits a {IVotes-DelegateVotesChanged} event. */ - function _update(address from, address to, uint256 tokenId) internal virtual override { - super._update(from, to, tokenId); + function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { + address from = super._update(to, tokenId, optionalChecks); + _transferVotingUnits(from, to, 1); + + return from; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index 395508b71a2..3fa9611a279 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,7 +50,7 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - _updateWithConstraints(address(0), tokenId, _constraintApprovedOrOwner); + _update(address(0), tokenId, CONSTRAINT_MINTED | CONSTRAINT_SPENDER_APPROVED_OR_OWNER); // 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 From 1a9552009b5e6bd3300f4d1f679cc7548fce5893 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 Jul 2023 21:47:23 +0200 Subject: [PATCH 10/47] replace constraints with a simple operator check --- .../token/ERC721ConsecutiveEnumerableMock.sol | 8 +- .../mocks/token/ERC721ConsecutiveMock.sol | 8 +- contracts/token/ERC721/ERC721.sol | 154 +++++++++--------- .../ERC721/extensions/ERC721Burnable.sol | 2 +- .../ERC721/extensions/ERC721Consecutive.sol | 6 +- .../ERC721/extensions/ERC721Enumerable.sol | 11 +- .../ERC721/extensions/ERC721Pausable.sol | 4 +- .../token/ERC721/extensions/ERC721Royalty.sol | 4 +- .../ERC721/extensions/ERC721URIStorage.sol | 4 +- .../token/ERC721/extensions/ERC721Votes.sol | 10 +- .../token/ERC721/extensions/ERC721Wrapper.sol | 2 +- 11 files changed, 103 insertions(+), 110 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 6a81ee622ab..79e8438b845 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -31,16 +31,16 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable function _update( address to, uint256 tokenId, - bytes32 optionalChecks + address operatorCheck ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { - return super._update(to, tokenId, optionalChecks); + return super._update(to, tokenId, operatorCheck); } // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance( + function _increaseBalance( address account, uint256 amount ) internal virtual override(ERC721, ERC721Enumerable) { - super.__unsafe_increaseBalance(account, amount); + super._increaseBalance(account, amount); } } diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 7f07b5260ca..0d79b15e78b 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -44,14 +44,14 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes function _update( address to, uint256 tokenId, - bytes32 optionalChecks + address operatorCheck ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { - return super._update(to, tokenId, optionalChecks); + return super._update(to, tokenId, operatorCheck); } // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Votes) { - super.__unsafe_increaseBalance(account, amount); + function _increaseBalance(address account, uint256 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 770c0eeb8d4..248c2f623fb 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -19,10 +19,6 @@ import {IERC721Errors} from "../../interfaces/draft-IERC6093.sol"; abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Errors { using Strings for uint256; - bytes32 internal constant CONSTRAINT_MINTED = bytes32(uint256(1) << 0); - bytes32 internal constant CONSTRAINT_NOT_MINTED = bytes32(uint256(1) << 1); - bytes32 internal constant CONSTRAINT_SPENDER_APPROVED_OR_OWNER = bytes32(uint256(1) << 2); - // Token name string private _name; @@ -159,8 +155,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _update(to, tokenId, CONSTRAINT_MINTED | CONSTRAINT_SPENDER_APPROVED_OR_OWNER); - if (owner != from) { + address owner = _update(to, tokenId, _msgSender()); + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } } @@ -176,32 +174,7 @@ 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); - } - - /** - * @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. - * - * Emits a {Transfer} event. - */ - function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual { - _transfer(from, to, tokenId); + transferFrom(from, to, tokenId); if (!_checkOnERC721Received(from, to, tokenId, data)) { revert ERC721InvalidReceiver(to); } @@ -238,61 +211,25 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender); } - /** - * @dev Safely mints `tokenId` and transfers it to `to`. - * - * 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 virtual { - _safeMint(to, tokenId, ""); - } - - /** - * @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); - if (!_checkOnERC721Received(address(0), to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } - } - /** * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner * (or `to`) is the zero address. * - * The `optionalChecks` argument is used to specify constraints, that the caller would want to be checked as part - * of the update. + * The `operatorCheck` argument is optional. If the value passed is non 0, then this function will check that + * `operatorCheck` is either the owner of the token, or approved to operate on the token (by the owner). * * Emits a {Transfer} event. */ function _update( address to, uint256 tokenId, - bytes32 optionalChecks + address operatorCheck ) internal virtual returns (address) { address from = _ownerOf(tokenId); - // Perform optional checks - if (optionalChecks & CONSTRAINT_MINTED != 0 && from == address(0)) { - revert ERC721NonexistentToken(tokenId); - } - - if (optionalChecks & CONSTRAINT_NOT_MINTED != 0 && from != address(0)) { - revert ERC721InvalidSender(address(0)); - } - - if (optionalChecks & CONSTRAINT_SPENDER_APPROVED_OR_OWNER != 0) { - address spender = _msgSender(); - if (from != spender && !isApprovedForAll(from, spender) && getApproved(tokenId) != spender) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); - } + // Perform (optional) operator check + if (operatorCheck != address(0) && from != operatorCheck && !isApprovedForAll(from, operatorCheck) && getApproved(tokenId) != operatorCheck) { + revert ERC721InsufficientApproval(operatorCheck, tokenId); } // Execute the update @@ -332,7 +269,35 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - _update(to, tokenId, CONSTRAINT_NOT_MINTED); + address from = _update(to, tokenId, address(0)); + if (from != address(0)) { + revert ERC721InvalidSender(address(0)); + } + } + + /** + * @dev Safely mints `tokenId` and transfers it to `to`. + * + * 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 virtual { + _safeMint(to, tokenId, ""); + } + + /** + * @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); + if (!_checkOnERC721Received(address(0), to, tokenId, data)) { + revert ERC721InvalidReceiver(to); + } } /** @@ -347,7 +312,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits a {Transfer} event. */ function _burn(uint256 tokenId) internal { - _update(address(0), tokenId, CONSTRAINT_MINTED); + address from = _update(address(0), tokenId, address(0)); + if (from == address(0)) { + revert ERC721NonexistentToken(tokenId); + } } /** @@ -365,12 +333,39 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _update(to, tokenId, CONSTRAINT_MINTED); - if (owner != from) { + address owner = _update(to, tokenId, address(0)); + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else if (owner != from) { revert ERC721IncorrectOwner(from, tokenId, owner); } } + /** + * @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. + * + * Emits a {Transfer} event. + */ + 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); + } + } + /** * @dev Approve `to` to operate on `tokenId` * @@ -444,8 +439,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * 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 virtual { + function _increaseBalance(address account, uint256 value) internal virtual { _balances[account] += value; } } diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 93aa449b93c..5be10ee09d2 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,6 +19,6 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - _update(address(0), tokenId, CONSTRAINT_MINTED | CONSTRAINT_SPENDER_APPROVED_OR_OWNER); + _update(address(0), tokenId, _msgSender()); } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 83c3b931c24..46a6fceb1b3 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -124,7 +124,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { // 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); } @@ -138,8 +138,8 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * 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 _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { - address from = super._update(to, tokenId, optionalChecks); + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { + address from = super._update(to, tokenId, operatorCheck); // only mint after construction if (from == address(0) && address(this).code.length == 0) { diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 9199b9b35bb..3e7ba6b69a3 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -76,8 +76,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * @dev See {ERC721-_update}. */ - function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { - address from = super._update(to, tokenId, optionalChecks); + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { + address from = super._update(to, tokenId, operatorCheck); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); @@ -167,13 +167,12 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { } /** - * See {ERC721-__unsafe_increaseBalance}. We need that to account tokens that were minted in batch + * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch */ - // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override { + function _increaseBalance(address account, uint256 amount) internal virtual override { if (amount > 0) { revert ERC721EnumerableForbiddenBatchMint(); } - super.__unsafe_increaseBalance(account, amount); + super._increaseBalance(account, amount); } } diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 0de35ccefc9..4f2f0bb4d6d 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -27,8 +27,8 @@ abstract contract ERC721Pausable is ERC721, Pausable { * * - the contract must not be paused. */ - function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { _requireNotPaused(); - return super._update(to, tokenId, optionalChecks); + return super._update(to, tokenId, operatorCheck); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index a5d5edd81ae..3ce0392167c 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,8 +29,8 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { /** * @dev See {ERC721-_update}. This override additionally clears the royalty information for the token. */ - function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { - address from = super._update(to, tokenId, optionalChecks); + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { + address from = super._update(to, tokenId, operatorCheck); if (to == address(0)) { _resetTokenRoyalty(tokenId); diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 000f0bd9d4f..62914f82f55 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -68,8 +68,8 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { - address from = super._update(to, tokenId, optionalChecks); + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { + address from = super._update(to, tokenId, operatorCheck); 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 42fb94572b8..901cbe52699 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -20,8 +20,8 @@ abstract contract ERC721Votes is ERC721, Votes { * * Emits a {IVotes-DelegateVotesChanged} event. */ - function _update(address to, uint256 tokenId, bytes32 optionalChecks) internal virtual override returns (address) { - address from = super._update(to, tokenId, optionalChecks); + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { + address from = super._update(to, tokenId, operatorCheck); _transferVotingUnits(from, to, 1); @@ -38,11 +38,11 @@ abstract contract ERC721Votes is ERC721, Votes { } /** - * See {ERC721-__unsafe_increaseBalance}. We need that to account tokens that were minted in batch + * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch */ // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance(address account, uint256 amount) internal virtual override { - super.__unsafe_increaseBalance(account, amount); + function _increaseBalance(address account, uint256 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 3fa9611a279..eb39df0fc72 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,7 +50,7 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - _update(address(0), tokenId, CONSTRAINT_MINTED | CONSTRAINT_SPENDER_APPROVED_OR_OWNER); + _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 From 7e9d024d08aed59a29fb05281910519ff900d888 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 09:31:18 +0200 Subject: [PATCH 11/47] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol | 1 - contracts/token/ERC721/ERC721.sol | 4 ++-- contracts/token/ERC721/extensions/ERC721Consecutive.sol | 2 +- contracts/token/ERC721/extensions/ERC721Royalty.sol | 2 +- contracts/token/ERC721/extensions/ERC721URIStorage.sol | 2 +- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 79e8438b845..af31b7efb8c 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -36,7 +36,6 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._update(to, tokenId, operatorCheck); } - // solhint-disable-next-line func-name-mixedcase function _increaseBalance( address account, uint256 amount diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 248c2f623fb..bdcb8eb47f1 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -151,7 +151,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev See {IERC721-transferFrom}. */ - function transferFrom(address from, address to, uint256 tokenId) public virtual override { + function transferFrom(address from, address to, uint256 tokenId) public virtual { if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } @@ -213,7 +213,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev Transfers `tokenId` from its current owner to `to`, or alternatively mints (or burns) if the current owner - * (or `to`) is the zero address. + * (or `to`) is the zero address. Returns the owner of the `tokenId` before the update. * * The `operatorCheck` argument is optional. If the value passed is non 0, then this function will check that * `operatorCheck` is either the owner of the token, or approved to operate on the token (by the owner). diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 46a6fceb1b3..5bec5125c96 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -135,7 +135,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { /** * @dev See {ERC721-_update}. Override version that restricts normal minting to after construction. * - * Warning: Using {ERC721Consecutive} prevents minting during construction in favor of {_mintConsecutive}. + * 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 _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 3ce0392167c..003c9f31f88 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -27,7 +27,7 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { } /** - * @dev See {ERC721-_update}. 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 _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { address from = super._update(to, tokenId, operatorCheck); diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 62914f82f55..90622553cb5 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -64,7 +64,7 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { } /** - * @dev See {ERC721-_update}. 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. */ From 16f2f156738c4b065accdfbaf60ee2a65234ee42 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 10:01:30 +0200 Subject: [PATCH 12/47] remove _isApproedOrOwner in favor of _isApproved + refactor _checkOnERC721Received --- .../token/ERC721ConsecutiveEnumerableMock.sol | 5 +- contracts/token/ERC721/ERC721.sol | 65 +++++++++---------- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index af31b7efb8c..bf7fc5018c7 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -36,10 +36,7 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._update(to, tokenId, operatorCheck); } - function _increaseBalance( - address account, - uint256 amount - ) internal virtual override(ERC721, ERC721Enumerable) { + function _increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Enumerable) { super._increaseBalance(account, amount); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index bdcb8eb47f1..a0e16cf36b5 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -175,9 +175,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er */ function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual { transferFrom(from, to, tokenId); - if (!_checkOnERC721Received(from, to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } + _checkOnERC721Received(from, to, tokenId, data); } /** @@ -200,15 +198,28 @@ 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: + * WARNING: + * - ownership is not checked. + * - spender = address(0) will lead to this function returning true in unexpected situations. + */ + 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`. * - * - `tokenId` must exist. + * WARNING: + * - ownership is not checked. */ - 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 _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual { + if (!_isApproved(owner, spender, tokenId)) { + revert ERC721InsufficientApproval(spender, tokenId); + } } /** @@ -220,16 +231,12 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ - function _update( - address to, - uint256 tokenId, - address operatorCheck - ) internal virtual returns (address) { + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual returns (address) { address from = _ownerOf(tokenId); // Perform (optional) operator check - if (operatorCheck != address(0) && from != operatorCheck && !isApprovedForAll(from, operatorCheck) && getApproved(tokenId) != operatorCheck) { - revert ERC721InsufficientApproval(operatorCheck, tokenId); + if (operatorCheck != address(0)) { + _checkApproved(from, operatorCheck, tokenId); } // Execute the update @@ -295,9 +302,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er */ function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual { _mint(to, tokenId); - if (!_checkOnERC721Received(address(0), to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } + _checkOnERC721Received(address(0), to, tokenId, data); } /** @@ -361,9 +366,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er */ 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); - } + _checkOnERC721Received(from, to, tokenId, data); } /** @@ -399,24 +402,20 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @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); @@ -427,8 +426,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } } - } else { - return true; } } From 2558c8fac82a914b89c79d6e1515ea849b712580 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 10:16:55 +0200 Subject: [PATCH 13/47] change _increaseBalance type to uint128 --- .../token/ERC721ConsecutiveEnumerableMock.sol | 2 +- contracts/mocks/token/ERC721ConsecutiveMock.sol | 2 +- contracts/token/ERC721/ERC721.sol | 14 ++++++++++---- .../token/ERC721/extensions/ERC721Enumerable.sol | 2 +- contracts/token/ERC721/extensions/ERC721Votes.sol | 3 +-- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index bf7fc5018c7..7e65d55e826 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -36,7 +36,7 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._update(to, tokenId, operatorCheck); } - function _increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Enumerable) { + 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 0d79b15e78b..971bc579638 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -50,7 +50,7 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes } // solhint-disable-next-line func-name-mixedcase - function _increaseBalance(address account, uint256 amount) internal virtual override(ERC721, ERC721Votes) { + 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 a0e16cf36b5..41351179329 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -180,6 +180,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist + * + * WARNING: Any override of this function that allocates token to accounts following a custom logic should go in + * pair with a call to {_increaseBalance} so that balances and ownerships remain consistent with one another. */ function _ownerOf(uint256 tokenId) internal view virtual returns (address) { return _owners[tokenId]; @@ -432,11 +435,14 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @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`. + * 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. + * + * WARNING: Increassing an account's balance using this function should go in pair with an override of the + * {_ownerOf} function that resolve the ownership of the corresponding tokens so that balances and ownerships + * remain consistent with one another. */ - function _increaseBalance(address account, uint256 value) internal virtual { + function _increaseBalance(address account, uint128 value) internal virtual { _balances[account] += value; } } diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 3e7ba6b69a3..7e625b188e2 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -169,7 +169,7 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch */ - function _increaseBalance(address account, uint256 amount) internal virtual override { + function _increaseBalance(address account, uint128 amount) internal virtual override { if (amount > 0) { revert ERC721EnumerableForbiddenBatchMint(); } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 901cbe52699..b7cc5020c38 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -40,8 +40,7 @@ abstract contract ERC721Votes is ERC721, Votes { /** * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch */ - // solhint-disable-next-line func-name-mixedcase - function _increaseBalance(address account, uint256 amount) internal virtual override { + function _increaseBalance(address account, uint128 amount) internal virtual override { super._increaseBalance(account, amount); _transferVotingUnits(address(0), account, amount); } From de570d0d145ea0d594a40924b2edddf61e86d1cb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 13:42:46 +0200 Subject: [PATCH 14/47] allow using approve/_approve to clean approval --- contracts/token/ERC721/ERC721.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index fcaf04f2afb..fbeb3d6414f 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -114,9 +114,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er */ function approve(address to, uint256 tokenId) public virtual { address owner = ownerOf(tokenId); + address caller = _msgSender(); - if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) { - revert ERC721InvalidApprover(_msgSender()); + if (owner != caller && !isApprovedForAll(owner, caller)) { + revert ERC721InvalidApprover(caller); } _approve(to, tokenId); @@ -390,7 +391,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er */ function _approve(address to, uint256 tokenId) internal virtual { address owner = ownerOf(tokenId); - if (to == owner || to == address(0)) { + if (to == owner) { revert ERC721InvalidOperator(to); } _tokenApprovals[tokenId] = to; From b973d985a45ddda9ab6dcbe7c201b0adad1c2d82 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 14:11:59 +0200 Subject: [PATCH 15/47] changesets --- .changeset/bright-tomatoes-sing.md | 2 +- .changeset/eighty-lemons-shake.md | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/eighty-lemons-shake.md 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..6a8ab6a0796 --- /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 londer allows setting address(0) as an operator. From e4b0e725df539ccbd122c2419d6fb5798ac44528 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 16:56:07 +0200 Subject: [PATCH 16/47] use whenNotPaused in ERC721Pausable --- contracts/token/ERC721/extensions/ERC721Pausable.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 4f2f0bb4d6d..0820cf183c2 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -27,8 +27,7 @@ abstract contract ERC721Pausable is ERC721, Pausable { * * - the contract must not be paused. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { - _requireNotPaused(); + function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override whenNotPaused returns (address) { return super._update(to, tokenId, operatorCheck); } } From 4516803058d453ac73af9b3304a21d3ef5f34ddf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 12 Jul 2023 17:15:48 +0200 Subject: [PATCH 17/47] make the safe function without a data field non virtual --- contracts/token/ERC721/ERC721.sol | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 65b6efb2d63..738a3240257 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -164,7 +164,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @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, ""); } @@ -300,7 +300,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, ""); } @@ -372,6 +372,15 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits a {Transfer} event. */ + function _safeTransfer(address from, address to, uint256 tokenId) internal { + _transfer(from, to, tokenId); + _checkOnERC721Received(from, to, tokenId, ""); + } + + /** + * @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); From 7c3f1615b09c417a0502d5721c692d17fd7d2892 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 12 Jul 2023 20:29:11 -0300 Subject: [PATCH 18/47] Update .changeset/eighty-lemons-shake.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- .changeset/eighty-lemons-shake.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/eighty-lemons-shake.md b/.changeset/eighty-lemons-shake.md index 6a8ab6a0796..4e53893f52b 100644 --- a/.changeset/eighty-lemons-shake.md +++ b/.changeset/eighty-lemons-shake.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`ERC721`: `_approve` no longer allows approving the owner of the tokenId. `_setApprovalForAll` no londer allows setting address(0) as an operator. +`ERC721`: `_approve` no longer allows approving the owner of the tokenId. `_setApprovalForAll` no longer allows setting address(0) as an operator. From 9ba012005fb7a501bf6fc1c6341a4d5be2320248 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 12 Jul 2023 20:28:50 -0600 Subject: [PATCH 19/47] Format _increaseBalance NatSpec --- contracts/token/ERC721/extensions/ERC721Votes.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index b7cc5020c38..c851ad563bb 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -38,7 +38,7 @@ abstract contract ERC721Votes is ERC721, Votes { } /** - * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch + * @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); From 10815081f7a1c5a630d49cc254c8d7894b880eaf Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 12 Jul 2023 21:09:18 -0600 Subject: [PATCH 20/47] Lint --- contracts/token/ERC721/extensions/ERC721Pausable.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 0820cf183c2..17e89b67d2a 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -27,7 +27,11 @@ abstract contract ERC721Pausable is ERC721, Pausable { * * - the contract must not be paused. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override whenNotPaused returns (address) { + function _update( + address to, + uint256 tokenId, + address operatorCheck + ) internal virtual override whenNotPaused returns (address) { return super._update(to, tokenId, operatorCheck); } } From fb4d9510de758ff7d4e047126977bfccc3d27ca8 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 10:00:39 +0200 Subject: [PATCH 21/47] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Francisco Co-authored-by: Ernesto García --- contracts/token/ERC721/ERC721.sol | 34 +++++++++++++++++-------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 738a3240257..576e02973d2 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -179,8 +179,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev Returns the owner of the `tokenId`. Does NOT revert if token doesn't exist * - * WARNING: Any override of this function that allocates token to accounts following a custom logic should go in - * pair with a call to {_increaseBalance} so that balances and ownerships remain consistent with one another. + * 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]; @@ -209,20 +211,17 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in * particular (ignoring whether it is owned by `owner`). * - * WARNING: - * - ownership is not checked. - * - spender = address(0) will lead to this function returning true in unexpected situations. + * WARNING: This function doesn't check that `owner` is the actual owner of the specified `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where needed. Consider checking for cases where `spender == address(0)` since they could lead to unexpected behavior. */ 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`. + * @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`. * - * WARNING: - * - ownership is not checked. + * WARNING: This function relies on {_isApproved}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` */ function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual { if (!_isApproved(owner, spender, tokenId)) { @@ -291,7 +290,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @dev Safely mints `tokenId` and transfers it to `to`. + * @dev Mints `tokenId`, transfers it to `to` and checks for `to` acceptance. * * Requirements: * @@ -355,8 +354,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } /** - * @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. + * @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`. * @@ -373,8 +372,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits a {Transfer} event. */ function _safeTransfer(address from, address to, uint256 tokenId) internal { - _transfer(from, to, tokenId); - _checkOnERC721Received(from, to, tokenId, ""); + _safeTransfer(from, to, tokenId, ""); } /** @@ -403,6 +401,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev Approve `operator` to operate on all of `owner` tokens * + * Requirements: + * - operator can't be the address zero. + * - owner can't approve himself. + * * Emits an {ApprovalForAll} event. */ function _setApprovalForAll(address owner, address operator, bool approved) internal virtual { @@ -461,6 +463,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * remain consistent with one another. */ function _increaseBalance(address account, uint128 value) internal virtual { - _balances[account] += value; + unchecked { + _balances[account] += value; + } } } From d7a6aaf41f87bc8b44ec839687b6eb1ec4420e13 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 10:00:50 +0200 Subject: [PATCH 22/47] remove _exists --- contracts/token/ERC721/ERC721.sol | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 738a3240257..af058c559f1 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -153,11 +153,11 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _update(to, tokenId, _msgSender()); - if (owner == address(0)) { + address previousOwner = _update(to, tokenId, _msgSender()); + if (previousOwner == address(0)) { revert ERC721NonexistentToken(tokenId); - } else if (owner != from) { - revert ERC721IncorrectOwner(from, tokenId, owner); + } else if (previousOwner != from) { + revert ERC721IncorrectOwner(from, tokenId, previousOwner); } } @@ -186,18 +186,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er return _owners[tokenId]; } - /** - * @dev Returns whether `tokenId` exists. - * - * Tokens can be managed by their owner or approved accounts via {approve} or {setApprovalForAll}. - * - * Tokens start existing when they are minted (`_mint`), - * and stop existing when they are burned (`_burn`). - */ - function _exists(uint256 tokenId) internal view virtual returns (bool) { - return _ownerOf(tokenId) != address(0); - } - /** * @dev Returns the approved address for `tokenId`. Returns 0 if `tokenId` is not minted. */ @@ -346,11 +334,11 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address owner = _update(to, tokenId, address(0)); - if (owner == address(0)) { + address previousOwner = _update(to, tokenId, address(0)); + if (previousOwner == address(0)) { revert ERC721NonexistentToken(tokenId); - } else if (owner != from) { - revert ERC721IncorrectOwner(from, tokenId, owner); + } else if (previousOwner != from) { + revert ERC721IncorrectOwner(from, tokenId, previousOwner); } } @@ -417,7 +405,7 @@ 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); } } From 20048ca3b9fd2006d4ad8116af5c635491662983 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 11:00:11 +0200 Subject: [PATCH 23/47] Changes suggested in the PR discussions --- .../token/ERC721ConsecutiveEnumerableMock.sol | 4 +- .../mocks/token/ERC721ConsecutiveMock.sol | 4 +- contracts/token/ERC721/ERC721.sol | 101 +++++++++--------- .../ERC721/extensions/ERC721Burnable.sol | 2 + .../ERC721/extensions/ERC721Consecutive.sol | 4 +- .../ERC721/extensions/ERC721Enumerable.sol | 4 +- .../ERC721/extensions/ERC721Pausable.sol | 4 +- .../token/ERC721/extensions/ERC721Royalty.sol | 4 +- .../ERC721/extensions/ERC721URIStorage.sol | 6 +- .../token/ERC721/extensions/ERC721Votes.sol | 4 +- .../token/ERC721/extensions/ERC721Wrapper.sol | 2 + test/token/ERC721/ERC721.behavior.js | 18 ---- .../extensions/ERC721Consecutive.test.js | 8 +- .../ERC721/extensions/ERC721Pausable.test.js | 6 -- .../extensions/ERC721URIStorage.test.js | 2 - 15 files changed, 75 insertions(+), 98 deletions(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 7e65d55e826..9cebf1a00b1 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -31,9 +31,9 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable function _update( address to, uint256 tokenId, - address operatorCheck + address auth ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { - return super._update(to, tokenId, operatorCheck); + return super._update(to, tokenId, auth); } function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Enumerable) { diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 971bc579638..0458943d693 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -44,9 +44,9 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes function _update( address to, uint256 tokenId, - address operatorCheck + address auth ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { - return super._update(to, tokenId, operatorCheck); + return super._update(to, tokenId, auth); } // solhint-disable-next-line func-name-mixedcase diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 7fbc2109a52..2d25da2aba4 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -113,14 +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); - address caller = _msgSender(); - - if (owner != caller && !isApprovedForAll(owner, caller)) { - revert ERC721InvalidApprover(caller); - } - - _approve(to, tokenId); + _approve(to, tokenId, _msgSender()); } /** @@ -153,10 +146,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } + // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), + // no need to duplicate that check here. address previousOwner = _update(to, tokenId, _msgSender()); - if (previousOwner == address(0)) { - revert ERC721NonexistentToken(tokenId); - } else if (previousOwner != from) { + if (previousOwner != from) { revert ERC721IncorrectOwner(from, tokenId, previousOwner); } } @@ -179,10 +172,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @dev Returns the owner of the `tokenId`. Does NOT revert 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`. + * 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]; @@ -201,37 +194,58 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * WARNING: This function doesn't check that `owner` is the actual owner of the specified `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where needed. Consider checking for cases where `spender == address(0)` since they could lead to unexpected behavior. */ - function _isApproved(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { - return owner == spender || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender; + function _isAuthorised(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { + return + spender != address(0) && + (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender); } /** * @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`. * - * WARNING: This function relies on {_isApproved}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` + * WARNING: This function relies on {_isAuthorised}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` */ - function _checkApproved(address owner, address spender, uint256 tokenId) internal view virtual { - if (!_isApproved(owner, spender, tokenId)) { + function _checkAuthorised(address owner, address spender, uint256 tokenId) internal view virtual { + // That first check is needed because the error is different, and should as precedence over insufficient approval + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else if (!_isAuthorised(owner, spender, tokenId)) { revert ERC721InsufficientApproval(spender, tokenId); } } + /** + * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. + * + * 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. + * + * WARNING: Increassing an account's balance using this function should go in pair with an override of the + * {_ownerOf} function that resolve the ownership of the corresponding tokens so that balances and ownerships + * remain consistent with one another. + */ + function _increaseBalance(address account, uint128 value) internal virtual { + unchecked { + _balances[account] += value; + } + } + /** * @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 `operatorCheck` argument is optional. If the value passed is non 0, then this function will check that - * `operatorCheck` is either the owner of the token, or approved to operate on the token (by the owner). + * 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. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual returns (address) { + function _update(address to, uint256 tokenId, address auth) internal virtual returns (address) { address from = _ownerOf(tokenId); // Perform (optional) operator check - if (operatorCheck != address(0)) { - _checkApproved(from, operatorCheck, tokenId); + if (auth != address(0)) { + _checkAuthorised(from, auth, tokenId); } // Execute the update @@ -271,8 +285,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - address from = _update(to, tokenId, address(0)); - if (from != address(0)) { + address previousOwner = _update(to, tokenId, address(0)); + if (previousOwner != address(0)) { revert ERC721InvalidSender(address(0)); } } @@ -312,8 +326,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits a {Transfer} event. */ function _burn(uint256 tokenId) internal { - address from = _update(address(0), tokenId, address(0)); - if (from == address(0)) { + address previousOwner = _update(address(0), tokenId, address(0)); + if (previousOwner == address(0)) { revert ERC721NonexistentToken(tokenId); } } @@ -375,13 +389,18 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er /** * @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 { address owner = ownerOf(tokenId); - if (to == owner) { - revert ERC721InvalidOperator(to); + + if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) { + revert ERC721InvalidApprover(auth); } + _tokenApprovals[tokenId] = to; emit Approval(owner, to, tokenId); } @@ -396,7 +415,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Emits an {ApprovalForAll} event. */ function _setApprovalForAll(address owner, address operator, bool approved) internal virtual { - if (operator == owner || operator == address(0)) { + if (operator == address(0)) { revert ERC721InvalidOperator(operator); } _operatorApprovals[owner][operator] = approved; @@ -439,20 +458,4 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } } - - /** - * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. - * - * 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. - * - * WARNING: Increassing an account's balance using this function should go in pair with an override of the - * {_ownerOf} function that resolve the ownership of the corresponding tokens so that balances and ownerships - * remain consistent with one another. - */ - function _increaseBalance(address account, uint128 value) internal virtual { - unchecked { - _balances[account] += value; - } - } } diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 5be10ee09d2..d9350b5e8c2 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,6 +19,8 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { + // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), + // no need to duplicate that check here. _update(address(0), tokenId, _msgSender()); } } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 5bec5125c96..4e24473f67d 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -138,8 +138,8 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * 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 _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { - address from = super._update(to, tokenId, operatorCheck); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address from = super._update(to, tokenId, auth); // only mint after construction if (from == address(0) && address(this).code.length == 0) { diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 7e625b188e2..08d0b3807de 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -76,8 +76,8 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { /** * @dev See {ERC721-_update}. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { - address from = super._update(to, tokenId, operatorCheck); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address from = super._update(to, tokenId, auth); if (from == address(0)) { _addTokenToAllTokensEnumeration(tokenId); diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 17e89b67d2a..b5b44e02d55 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -30,8 +30,8 @@ abstract contract ERC721Pausable is ERC721, Pausable { function _update( address to, uint256 tokenId, - address operatorCheck + address auth ) internal virtual override whenNotPaused returns (address) { - return super._update(to, tokenId, operatorCheck); + return super._update(to, tokenId, auth); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 003c9f31f88..3b7bb82c496 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -29,8 +29,8 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { /** * @dev See {ERC721-_update}. When burning, this override will additionally clear the royalty information for the token. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { - address from = super._update(to, tokenId, operatorCheck); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address from = super._update(to, tokenId, auth); if (to == address(0)) { _resetTokenRoyalty(tokenId); diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 90622553cb5..c334808220e 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; @@ -68,8 +68,8 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * token-specific URI was set for the token, and if so, it deletes the token URI from * the storage mapping. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { - address from = super._update(to, tokenId, operatorCheck); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address from = super._update(to, tokenId, auth); 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 c851ad563bb..b74199b8b94 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -20,8 +20,8 @@ abstract contract ERC721Votes is ERC721, Votes { * * Emits a {IVotes-DelegateVotesChanged} event. */ - function _update(address to, uint256 tokenId, address operatorCheck) internal virtual override returns (address) { - address from = super._update(to, tokenId, operatorCheck); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address from = super._update(to, tokenId, auth); _transferVotingUnits(from, to, 1); diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index eb39df0fc72..3297f06b60b 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,6 +50,8 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; + // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), + // no need to duplicate that check here. _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. diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 75700f6ab56..c9a2fa2a728 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -524,14 +524,6 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); - context('when the address that receives the approval is the owner', function () { - it('reverts', async function () { - await expectRevertCustomError(this.token.approve(owner, tokenId, { from: owner }), 'ERC721InvalidOperator', [ - owner, - ]); - }); - }); - context('when the sender does not own the given token ID', function () { it('reverts', async function () { await expectRevertCustomError( @@ -644,16 +636,6 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); }); - - context('when the operator is the owner', function () { - it('reverts', async function () { - await expectRevertCustomError( - this.token.setApprovalForAll(owner, true, { from: owner }), - 'ERC721InvalidOperator', - [owner], - ); - }); - }); }); describe('getApproved', async function () { diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index c29226ecdbb..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,7 +174,6 @@ contract('ERC721Consecutive', function (accounts) { tokenId, }); - expect(await this.token.$_exists(tokenId)).to.be.equal(true); expect(await this.token.ownerOf(tokenId), user2); }); }); 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]); }); }); From e996ba49d8fbb5844ddbf858886c3a0dd91c22c5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 16:00:38 +0200 Subject: [PATCH 24/47] add ERC721 specific details in the 'How to upgrade from 4.x' section of the CHANGELOG --- CHANGELOG.md | 14 +++++++++++++- contracts/token/ERC721/ERC721.sol | 8 +++++--- .../token/ERC721/extensions/ERC721Burnable.sol | 4 ++-- .../token/ERC721/extensions/ERC721Wrapper.sol | 4 ++-- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2936877e186..afc6231b70a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,7 @@ These removals were implemented in the following PRs: [#3637](https://github.com These breaking changes will require modifications to ERC20, ERC721, and ERC1155 contracts, since the `_afterTokenTransfer` and `_beforeTokenTransfer` functions were removed. Any customization made through those hooks should now be done overriding the new `_update` function instead. -Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies. +Minting and burning are implemented by `_update` and customizations should be done by overriding this function as well. `_transfer`, `_mint` and `_burn` are no longer virtual (meaning they are not overridable) to guard against possible inconsistencies. For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have to be changed in the following way. @@ -53,6 +53,18 @@ For example, a contract using `ERC20`'s `_beforeTokenTransfer` hook would have t } ``` +### More about ERC721 + +In the case of `ERC721`, the `_update` function does not include a `from` parameter, as the sender is implicitly the previous owner of the `tokenId`. The address of +this previous owner is returned by the `_update` function, so it can be used for a posteriori checks. In addition to `to` and `tokenId`, a third parameter (`auth`) is +present in this function. This parameter enabled an optional check that the caller/spender is approved to do the transfer. This check cannot be performed after the transfer (because the transfer resets the approval), and doing it before `_update` would require a duplicate call to `_ownerOf`. + +In this logic of removing hidden SLOADs, the `_isApprovedOrOwner` function was removed in favor of a new `_isAuthorised` function. Overrides that used to target the +`_isApprovedOrOwner` should now be performed on the `_isAuthorised` function. Calls to `_isApprovedOrOwner` that preceded a call to `_transfer`, `_burn` or `_approve` +should be removed in favor of using the `auth` argument in `_update` and `_approve`. This is showcased in `ERC721Burnable.burn` and in `ERC721Wrapper.withdrawTo`. + +The `_exist` function was removed. Calls to this function can be replaced by `_ownerOf(tokenId) != address(0)`. + #### ERC165Storage Users that were registering EIP-165 interfaces with `_registerInterface` from `ERC165Storage` should instead do so so by overriding the `supportsInterface` function as seen below: diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 2d25da2aba4..8fb4ec02a55 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -146,8 +146,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), - // no need to duplicate that check here. + // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. address previousOwner = _update(to, tokenId, _msgSender()); if (previousOwner != from) { revert ERC721IncorrectOwner(from, tokenId, previousOwner); @@ -394,7 +394,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Emits an {Approval} event. */ - function _approve(address to, uint256 tokenId, address auth) internal virtual { + function _approve(address to, uint256 tokenId, address auth) internal virtual returns (address) { address owner = ownerOf(tokenId); if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) { @@ -403,6 +403,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er _tokenApprovals[tokenId] = to; emit Approval(owner, to, tokenId); + + return owner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index d9350b5e8c2..4c761ed1644 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,8 +19,8 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), - // no need to duplicate that check here. + // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. _update(address(0), tokenId, _msgSender()); } } diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index 3297f06b60b..b2dbbd7f04a 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,8 +50,8 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - // Setting an "auth" arguments means that `_update` will check that the token exists (from != 0), - // no need to duplicate that check here. + // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // (from != 0). Therefore, it is not needed to verify that the return value is not 0 here. _update(address(0), tokenId, _msgSender()); // Checks were already performed at this point, and there's no way to retake ownership or approval from // the wrapped tokenId after this point, so it's safe to remove the reentrancy check for the next line. From b29e57338359832deabaec53ca7095bdf033db09 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 16:14:57 +0200 Subject: [PATCH 25/47] =?UTF-8?q?rename=20from=20=E2=86=92=20previousOwner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../token/ERC721/extensions/ERC721Consecutive.sol | 6 +++--- .../token/ERC721/extensions/ERC721Enumerable.sol | 12 ++++++------ contracts/token/ERC721/extensions/ERC721Royalty.sol | 4 ++-- .../token/ERC721/extensions/ERC721URIStorage.sol | 4 ++-- contracts/token/ERC721/extensions/ERC721Votes.sol | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 4e24473f67d..b014111ffc2 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -139,10 +139,10 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { * After construction, {_mintConsecutive} is no longer available and minting through {_update} becomes available. */ function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address from = super._update(to, tokenId, auth); + address previousOwner = super._update(to, tokenId, auth); // only mint after construction - if (from == address(0) && address(this).code.length == 0) { + if (previousOwner == address(0) && address(this).code.length == 0) { revert ERC721ForbiddenMint(); } @@ -155,7 +155,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { _sequentialBurn.set(tokenId); } - return from; + return previousOwner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index 08d0b3807de..3c62f429522 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -77,20 +77,20 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { * @dev See {ERC721-_update}. */ function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address from = super._update(to, tokenId, auth); + 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 (from != to) { + } else if (previousOwner != to) { _addTokenToOwnerEnumeration(to, tokenId); } - return from; + return previousOwner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 3b7bb82c496..ff9d31936ad 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -30,12 +30,12 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { * @dev See {ERC721-_update}. When burning, this override will additionally clear the royalty information for the token. */ function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address from = super._update(to, tokenId, auth); + address previousOwner = super._update(to, tokenId, auth); if (to == address(0)) { _resetTokenRoyalty(tokenId); } - return from; + return previousOwner; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index c334808220e..28440ef69a2 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -69,12 +69,12 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { * the storage mapping. */ function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address from = super._update(to, tokenId, auth); + address previousOwner = super._update(to, tokenId, auth); if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } - return from; + return previousOwner; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index b74199b8b94..ede3dff8e9e 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -21,11 +21,11 @@ abstract contract ERC721Votes is ERC721, Votes { * Emits a {IVotes-DelegateVotesChanged} event. */ function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address from = super._update(to, tokenId, auth); + address previousOwner = super._update(to, tokenId, auth); - _transferVotingUnits(from, to, 1); + _transferVotingUnits(previousOwner, to, 1); - return from; + return previousOwner; } /** From 328b16bf8cf20dc17f240b5599df17b566a19558 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 16:29:05 +0200 Subject: [PATCH 26/47] =?UTF-8?q?Authorised=20=E2=86=92=20Authorized?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/token/ERC721/ERC721.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 8fb4ec02a55..a315bf17cd4 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -194,7 +194,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * WARNING: This function doesn't check that `owner` is the actual owner of the specified `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where needed. Consider checking for cases where `spender == address(0)` since they could lead to unexpected behavior. */ - function _isAuthorised(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { + 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); @@ -204,13 +204,13 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @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`. * - * WARNING: This function relies on {_isAuthorised}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` + * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` */ - function _checkAuthorised(address owner, address spender, uint256 tokenId) internal view virtual { + function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual { // That first check is needed because the error is different, and should as precedence over insufficient approval if (owner == address(0)) { revert ERC721NonexistentToken(tokenId); - } else if (!_isAuthorised(owner, spender, tokenId)) { + } else if (!_isAuthorized(owner, spender, tokenId)) { revert ERC721InsufficientApproval(spender, tokenId); } } @@ -245,7 +245,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er // Perform (optional) operator check if (auth != address(0)) { - _checkAuthorised(from, auth, tokenId); + _checkAuthorized(from, auth, tokenId); } // Execute the update From 08da709ba7397d7a32676e228fc104332e87c7b7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 16:45:30 +0200 Subject: [PATCH 27/47] refactor _checkAuhtorized --- contracts/token/ERC721/ERC721.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index a315bf17cd4..22751b1eeb0 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -207,11 +207,12 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` */ function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual { - // That first check is needed because the error is different, and should as precedence over insufficient approval - if (owner == address(0)) { - revert ERC721NonexistentToken(tokenId); - } else if (!_isAuthorized(owner, spender, tokenId)) { - revert ERC721InsufficientApproval(spender, tokenId); + if (!_isAuthorized(owner, spender, tokenId)) { + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else { + revert ERC721InsufficientApproval(spender, tokenId); + } } } From 12f63b3b1b9d939b147a944c33e7f122543b21b7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 17:28:04 +0200 Subject: [PATCH 28/47] add test --- test/token/ERC721/ERC721.behavior.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index c9a2fa2a728..bb436fc88c6 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -635,6 +635,16 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); }); + + context('when the operator is address zero', function () { + it('reverts', async function () { + await expectRevertCustomError( + this.token.setApprovalForAll(constants.ZERO_ADDRESS, true, { from: owner }), + 'ERC721InvalidOperator', + [constants.ZERO_ADDRESS], + ); + }); + }); }); }); From 81aca964670e15762ad32d0f4c705cf5125b0831 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 13 Jul 2023 18:16:42 -0300 Subject: [PATCH 29/47] Update CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afc6231b70a..092571ec5c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,8 +59,8 @@ In the case of `ERC721`, the `_update` function does not include a `from` parame this previous owner is returned by the `_update` function, so it can be used for a posteriori checks. In addition to `to` and `tokenId`, a third parameter (`auth`) is present in this function. This parameter enabled an optional check that the caller/spender is approved to do the transfer. This check cannot be performed after the transfer (because the transfer resets the approval), and doing it before `_update` would require a duplicate call to `_ownerOf`. -In this logic of removing hidden SLOADs, the `_isApprovedOrOwner` function was removed in favor of a new `_isAuthorised` function. Overrides that used to target the -`_isApprovedOrOwner` should now be performed on the `_isAuthorised` function. Calls to `_isApprovedOrOwner` that preceded a call to `_transfer`, `_burn` or `_approve` +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 `_exist` function was removed. Calls to this function can be replaced by `_ownerOf(tokenId) != address(0)`. From d0375301f172beb57b75964c62bc4f893df5f392 Mon Sep 17 00:00:00 2001 From: Francisco Date: Thu, 13 Jul 2023 18:17:24 -0300 Subject: [PATCH 30/47] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 092571ec5c8..98d06de9980 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,7 +63,7 @@ In this logic of removing hidden SLOADs, the `_isApprovedOrOwner` function was r `_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 `_exist` function was removed. Calls to this function can be replaced by `_ownerOf(tokenId) != address(0)`. +The `_exists` function was removed. Calls to this function can be replaced by `_ownerOf(tokenId) != address(0)`. #### ERC165Storage From 5ce49a45fda8d809bd6497706db147faebff7930 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 13 Jul 2023 18:19:30 -0300 Subject: [PATCH 31/47] remove unnecessary solhint annotation --- contracts/mocks/token/ERC721ConsecutiveMock.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 0458943d693..2990076c70a 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -49,7 +49,6 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes return super._update(to, tokenId, auth); } - // solhint-disable-next-line func-name-mixedcase function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Votes) { super._increaseBalance(account, amount); } From a023cad591765f6753721e318a1534d560009ee2 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 13 Jul 2023 18:21:27 -0300 Subject: [PATCH 32/47] wrap long line --- contracts/token/ERC721/ERC721.sol | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 22751b1eeb0..eeda8934012 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -192,7 +192,10 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in * particular (ignoring whether it is owned by `owner`). * - * WARNING: This function doesn't check that `owner` is the actual owner of the specified `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where needed. Consider checking for cases where `spender == address(0)` since they could lead to unexpected behavior. + * WARNING: This function doesn't check that `owner` is the actual owner of the specified + * `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where + * needed. Consider checking for cases where `spender == address(0)` since they could lead to + * unexpected behavior. */ function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { return @@ -204,7 +207,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @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`. * - * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the actual owner of `tokenId` and will skip the check if `spender == owner` + * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the + * actual owner of `tokenId` and will skip the check if `spender == owner` */ function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual { if (!_isAuthorized(owner, spender, tokenId)) { From caabbf3c46376e8d30ccf5b1c2c818e2a1ddf895 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 13 Jul 2023 19:08:36 -0300 Subject: [PATCH 33/47] improve warnings and notes --- contracts/token/ERC721/ERC721.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index eeda8934012..0566ec63877 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -226,8 +226,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * 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. * - * WARNING: Increassing an account's balance using this function should go in pair with an override of the - * {_ownerOf} function that resolve the ownership of the corresponding tokens so that balances and ownerships + * 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 _increaseBalance(address account, uint128 value) internal virtual { @@ -244,6 +244,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * `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 _update(address to, uint256 tokenId, address auth) internal virtual returns (address) { address from = _ownerOf(tokenId); From ca32b459ec53428f456c03be5499fb7b25ac44e9 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 13 Jul 2023 19:14:15 -0300 Subject: [PATCH 34/47] fix _safeTransfer docs --- contracts/token/ERC721/ERC721.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 0566ec63877..0dab20f3ab9 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -368,7 +368,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * `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. + * 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: From b982e2a808eebc0e2aa7c4bc7dc621fb86fecc15 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jul 2023 08:38:46 +0200 Subject: [PATCH 35/47] Update ERC721.behavior.js --- test/token/ERC721/ERC721.behavior.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index bb436fc88c6..ee972766f02 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -635,15 +635,15 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); }); + }); - context('when the operator is address zero', function () { - it('reverts', async function () { - await expectRevertCustomError( - this.token.setApprovalForAll(constants.ZERO_ADDRESS, true, { from: owner }), - 'ERC721InvalidOperator', - [constants.ZERO_ADDRESS], - ); - }); + context('when the operator is address zero', function () { + it('reverts', async function () { + await expectRevertCustomError( + this.token.setApprovalForAll(constants.ZERO_ADDRESS, true, { from: owner }), + 'ERC721InvalidOperator', + [constants.ZERO_ADDRESS], + ); }); }); }); From f404802d5569a22f4b162d3b9dc4003e99f9c9b2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jul 2023 08:41:30 +0200 Subject: [PATCH 36/47] Update ERC721.sol --- contracts/token/ERC721/ERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 0dab20f3ab9..f5f75f4dd71 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -346,7 +346,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Requirements: * * - `to` cannot be the zero address. - * - `tokenId` token must be owned by `from`. + * - `tokenId` token must exists and be owned by `from`. * * Emits a {Transfer} event. */ From 20bb47f439a17946686bb3fa468ca12a94a89141 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jul 2023 08:43:14 +0200 Subject: [PATCH 37/47] Update contracts/token/ERC721/ERC721.sol --- contracts/token/ERC721/ERC721.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index f5f75f4dd71..256f66b4421 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -374,9 +374,8 @@ 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`. + * - `to` cannot be the zero address. * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer. * * Emits a {Transfer} event. From a475ffae0a0dfb94ac4aa85579aeff931c311976 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jul 2023 08:45:25 +0200 Subject: [PATCH 38/47] Update ERC721.sol --- contracts/token/ERC721/ERC721.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 256f66b4421..6d775ce02a6 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -419,7 +419,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * Requirements: * - operator can't be the address zero. - * - owner can't approve himself. * * Emits an {ApprovalForAll} event. */ From e26d5c0951ff2e5d1b3085e4a276b141aef6a73e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jul 2023 08:46:48 +0200 Subject: [PATCH 39/47] Update IERC721.sol --- contracts/token/ERC721/IERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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. */ From 2897abccc942f7720dcdcda4569fbb6431a0c917 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 14 Jul 2023 15:31:29 +0200 Subject: [PATCH 40/47] Update ERC721.sol --- contracts/token/ERC721/ERC721.sol | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 6d775ce02a6..afb2a03889f 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -192,10 +192,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in * particular (ignoring whether it is owned by `owner`). * - * WARNING: This function doesn't check that `owner` is the actual owner of the specified - * `tokenId`. Moreover, it returns true if `owner == spender` in order to skip the check where - * needed. Consider checking for cases where `spender == address(0)` since they could lead to - * unexpected behavior. + * WARNING: This function doesn't check that `owner` is the actual owner of `tokenId`. */ function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { return @@ -208,7 +205,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Reverts if `spender` has not approval for all assets of the provided `owner` nor the actual owner approved the `spender` for the specific `tokenId`. * * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the - * actual owner of `tokenId` and will skip the check if `spender == owner` + * actual owner of `tokenId`. */ function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual { if (!_isAuthorized(owner, spender, tokenId)) { From 52923d16757cae2df4949982b88a47f186832e38 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 4 Aug 2023 15:59:49 +0200 Subject: [PATCH 41/47] coverage for internal _transfer and _safeTransfer --- test/token/ERC721/ERC721.behavior.js | 256 ++++++++++++++------------- 1 file changed, 136 insertions(+), 120 deletions(-) diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index ee972766f02..e13f14db236 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,155 @@ 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, opts); - shouldTransferTokensByUsers(transferFun); + 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 () { From 42e17eef080c6919bba6253b8716216d0662f9fa Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 4 Aug 2023 17:16:32 +0200 Subject: [PATCH 42/47] mint --- test/token/ERC721/ERC721.behavior.js | 33 ++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index e13f14db236..4dd6e234c45 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -280,11 +280,11 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper ]) { 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); + 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); + return this.token.methods[fnName + '(address,address,uint256)'](from, to, tokenId, opts); }; describe('with data', function () { @@ -299,7 +299,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper 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 }), + this.token.methods[fnName + '(address,address,uint256)'](owner, invalidReceiver.address, tokenId, { + from: owner, + }), 'ERC721InvalidReceiver', [invalidReceiver.address], ); @@ -308,9 +310,14 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper describe('to a receiver contract that reverts with message', function () { it('reverts', async function () { - const revertingReceiver = await ERC721ReceiverMock.new(RECEIVER_MAGIC_VALUE, RevertType.RevertWithMessage); + 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 }), + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), 'ERC721ReceiverMock: reverting', ); }); @@ -323,7 +330,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper RevertType.RevertWithoutMessage, ); await expectRevertCustomError( - this.token.methods[fnName+'(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { from: owner }), + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), 'ERC721InvalidReceiver', [revertingReceiver.address], ); @@ -337,7 +346,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper RevertType.RevertWithCustomError, ); await expectRevertCustomError( - this.token.methods[fnName+'(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { from: owner }), + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), 'CustomError', [RECEIVER_MAGIC_VALUE], ); @@ -348,7 +359,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper 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 }), + this.token.methods[fnName + '(address,address,uint256)'](owner, revertingReceiver.address, tokenId, { + from: owner, + }), ); }); }); @@ -357,7 +370,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper it('reverts', async function () { const nonReceiver = await NonERC721ReceiverMock.new(); await expectRevertCustomError( - this.token.methods[fnName+'(address,address,uint256)'](owner, nonReceiver.address, tokenId, { from: owner }), + this.token.methods[fnName + '(address,address,uint256)'](owner, nonReceiver.address, tokenId, { + from: owner, + }), 'ERC721InvalidReceiver', [nonReceiver.address], ); From c2e1a5509bee7833a65300b3c19155581b7ac983 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 9 Aug 2023 12:55:36 -0300 Subject: [PATCH 43/47] fix comments _isApproved -> _isAuthorized --- contracts/token/ERC721/ERC721.sol | 2 +- contracts/token/ERC721/extensions/ERC721Burnable.sol | 2 +- contracts/token/ERC721/extensions/ERC721Wrapper.sol | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index afb2a03889f..6d9b4c8d9dd 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -146,7 +146,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er if (to == address(0)) { revert ERC721InvalidReceiver(address(0)); } - // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // 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) { diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 4c761ed1644..cfe7546686f 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,7 +19,7 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // 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/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index b2dbbd7f04a..cbb3a4b14f7 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,7 +50,7 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - // Setting an "auth" arguments enables the `_isApproved` check which verifies that the token exists + // 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 From a036284af03b7d4dde9964e723ea757acedc1d83 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 9 Aug 2023 12:58:58 -0300 Subject: [PATCH 44/47] extend warning for _isAuthorized --- contracts/token/ERC721/ERC721.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 6d9b4c8d9dd..37a0a17e11b 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -192,7 +192,8 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in * particular (ignoring whether it is owned by `owner`). * - * WARNING: This function doesn't check that `owner` is the actual owner of `tokenId`. + * WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not + * verify this assumption. */ function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { return From 1e4f3537805821f188df4eb8a7e9a52333d0fb79 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 9 Aug 2023 12:59:03 -0300 Subject: [PATCH 45/47] add comment to _approve --- contracts/token/ERC721/ERC721.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 37a0a17e11b..93e17da5870 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -402,6 +402,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er 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); } From 7b260301d736b7847ff6830e3d9f662ce3093f96 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 9 Aug 2023 13:16:25 -0300 Subject: [PATCH 46/47] Update contracts/token/ERC721/ERC721.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/token/ERC721/ERC721.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 93e17da5870..3b22cf5594a 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -374,6 +374,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * * - `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. From 7249414f9dc70aab2005ad60180e7225b14ad0cc Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 9 Aug 2023 13:16:39 -0300 Subject: [PATCH 47/47] Update contracts/token/ERC721/ERC721.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/token/ERC721/ERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 3b22cf5594a..1263f742f78 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -344,7 +344,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * Requirements: * * - `to` cannot be the zero address. - * - `tokenId` token must exists and be owned by `from`. + * - `tokenId` token must be owned by `from`. * * Emits a {Transfer} event. */