From 562ddf566a02b70db0629f1d4a17f8704fbaf8d3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 5 Jul 2023 18:45:42 +0200 Subject: [PATCH] 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 5771c3355..65bb13598 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 b12e1cb6e..f69c7056d 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 6c85d6ca6..d4d8d1978 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 607265328..c268d12d8 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 261c3041d..de8d1b629 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 ba864c81c..86fd5abff 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 672d11af1..229759c94 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 a7d1b96f1..107fc6709 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 8fe503ac9..9480ee122 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 7447640f3..ed958c7a6 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 b8c396c3e..426461337 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