From 1a9552009b5e6bd3300f4d1f679cc7548fce5893 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 Jul 2023 21:47:23 +0200 Subject: [PATCH] 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 6a81ee622..79e8438b8 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 7f07b5260..0d79b15e7 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 770c0eeb8..248c2f623 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 93aa449b9..5be10ee09 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 83c3b931c..46a6fceb1 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 9199b9b35..3e7ba6b69 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 0de35ccef..4f2f0bb4d 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 a5d5edd81..3ce039216 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 000f0bd9d..62914f82f 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 42fb94572..901cbe526 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 3fa9611a2..eb39df0fc 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