From 20048ca3b9fd2006d4ad8116af5c635491662983 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jul 2023 11:00:11 +0200 Subject: [PATCH] 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 7e65d55e8..9cebf1a00 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 971bc5796..0458943d6 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 7fbc2109a..2d25da2ab 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 5be10ee09..d9350b5e8 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 5bec5125c..4e24473f6 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 7e625b188..08d0b3807 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 17e89b67d..b5b44e02d 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 003c9f31f..3b7bb82c4 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 90622553c..c33480822 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 c851ad563..b74199b8b 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 eb39df0fc..3297f06b6 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 75700f6ab..c9a2fa2a7 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 c29226ecd..d9e33aff2 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -103,7 +103,7 @@ contract('ERC721Consecutive', function (accounts) { it('simple minting is possible after construction', async function () { const tokenId = sum(...batches.map(b => b.amount)) + offset; - expect(await this.token.$_exists(tokenId)).to.be.equal(false); + await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]); expectEvent(await this.token.$_mint(user1, tokenId), 'Transfer', { from: constants.ZERO_ADDRESS, @@ -115,7 +115,7 @@ contract('ERC721Consecutive', function (accounts) { it('cannot mint a token that has been batched minted', async function () { const tokenId = sum(...batches.map(b => b.amount)) + offset - 1; - expect(await this.token.$_exists(tokenId)).to.be.equal(true); + expect(await this.token.ownerOf(tokenId)).to.be.not.equal(constants.ZERO_ADDRESS); await expectRevertCustomError(this.token.$_mint(user1, tokenId), 'ERC721InvalidSender', [ZERO_ADDRESS]); }); @@ -151,13 +151,11 @@ contract('ERC721Consecutive', function (accounts) { it('tokens can be burned and re-minted #2', async function () { const tokenId = web3.utils.toBN(sum(...batches.map(({ amount }) => amount)) + offset); - expect(await this.token.$_exists(tokenId)).to.be.equal(false); await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]); // mint await this.token.$_mint(user1, tokenId); - expect(await this.token.$_exists(tokenId)).to.be.equal(true); expect(await this.token.ownerOf(tokenId), user1); // burn @@ -167,7 +165,6 @@ contract('ERC721Consecutive', function (accounts) { tokenId, }); - expect(await this.token.$_exists(tokenId)).to.be.equal(false); await expectRevertCustomError(this.token.ownerOf(tokenId), 'ERC721NonexistentToken', [tokenId]); // re-mint @@ -177,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 ec99dea96..5d77149f2 100644 --- a/test/token/ERC721/extensions/ERC721Pausable.test.js +++ b/test/token/ERC721/extensions/ERC721Pausable.test.js @@ -81,12 +81,6 @@ contract('ERC721Pausable', function (accounts) { }); }); - describe('exists', function () { - it('returns token existence', async function () { - expect(await this.token.$_exists(firstTokenId)).to.equal(true); - }); - }); - describe('isApprovedForAll', function () { it('returns the approval of the operator', async function () { expect(await this.token.isApprovedForAll(owner, operator)).to.equal(false); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index 34738cae1..129515514 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -86,7 +86,6 @@ contract('ERC721URIStorage', function (accounts) { it('tokens without URI can be burnt ', async function () { await this.token.$_burn(firstTokenId, { from: owner }); - expect(await this.token.$_exists(firstTokenId)).to.equal(false); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); @@ -95,7 +94,6 @@ contract('ERC721URIStorage', function (accounts) { await this.token.$_burn(firstTokenId, { from: owner }); - expect(await this.token.$_exists(firstTokenId)).to.equal(false); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); });