diff --git a/.changeset/bright-tomatoes-sing.md b/.changeset/bright-tomatoes-sing.md index dcd6d7ff0..7ef6d929a 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 000000000..4e53893f5 --- /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 longer allows setting address(0) as an operator. diff --git a/.changeset/thick-pumpkins-exercise.md b/.changeset/thick-pumpkins-exercise.md new file mode 100644 index 000000000..8df8b51cc --- /dev/null +++ b/.changeset/thick-pumpkins-exercise.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Initializable`: Use the namespaced storage pattern to avoid putting critical variables in slot 0. Allow reinitializer versions greater than 256. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2936877e1..98d06de99 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 `_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 `_exists` 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/access/AccessControl.sol b/contracts/access/AccessControl.sol index a2be30de3..ccbfec66d 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -48,11 +48,11 @@ import {ERC165} from "../utils/introspection/ERC165.sol"; */ abstract contract AccessControl is Context, IAccessControl, ERC165 { struct RoleData { - mapping(address => bool) members; + mapping(address account => bool) hasRole; bytes32 adminRole; } - mapping(bytes32 => RoleData) private _roles; + mapping(bytes32 role => RoleData) private _roles; bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; @@ -76,7 +76,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { * @dev Returns `true` if `account` has been granted `role`. */ function hasRole(bytes32 role, address account) public view virtual returns (bool) { - return _roles[role].members[account]; + return _roles[role].hasRole[account]; } /** @@ -182,7 +182,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { */ function _grantRole(bytes32 role, address account) internal virtual returns (bool) { if (!hasRole(role, account)) { - _roles[role].members[account] = true; + _roles[role].hasRole[account] = true; emit RoleGranted(role, account, _msgSender()); return true; } else { @@ -199,7 +199,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { */ function _revokeRole(bytes32 role, address account) internal virtual returns (bool) { if (hasRole(role, account)) { - _roles[role].members[account] = false; + _roles[role].hasRole[account] = false; emit RoleRevoked(role, account, _msgSender()); return true; } else { diff --git a/contracts/access/extensions/AccessControlEnumerable.sol b/contracts/access/extensions/AccessControlEnumerable.sol index 707c5759b..8af95906d 100644 --- a/contracts/access/extensions/AccessControlEnumerable.sol +++ b/contracts/access/extensions/AccessControlEnumerable.sol @@ -13,7 +13,7 @@ import {EnumerableSet} from "../../utils/structs/EnumerableSet.sol"; abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessControl { using EnumerableSet for EnumerableSet.AddressSet; - mapping(bytes32 => EnumerableSet.AddressSet) private _roleMembers; + mapping(bytes32 role => EnumerableSet.AddressSet) private _roleMembers; /** * @dev See {IERC165-supportsInterface}. diff --git a/contracts/finance/VestingWallet.sol b/contracts/finance/VestingWallet.sol index 1edb0113e..7b50e699e 100644 --- a/contracts/finance/VestingWallet.sol +++ b/contracts/finance/VestingWallet.sol @@ -37,7 +37,7 @@ contract VestingWallet is Context, Ownable { error VestingWalletInvalidBeneficiary(address beneficiary); uint256 private _released; - mapping(address => uint256) private _erc20Released; + mapping(address token => uint256) private _erc20Released; uint64 private immutable _start; uint64 private immutable _duration; diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 9d9e2eb59..b149129dd 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -46,7 +46,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 bytes32 private constant _ALL_PROPOSAL_STATES_BITMAP = bytes32((2 ** (uint8(type(ProposalState).max) + 1)) - 1); string private _name; - mapping(uint256 => ProposalCore) private _proposals; + mapping(uint256 proposalId => ProposalCore) private _proposals; // This queue keeps track of the governor operating on itself. Calls to functions protected by the // {onlyGovernance} modifier needs to be whitelisted in this queue. Whitelisting is set in {_beforeExecute}, diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 798f84026..408feb6ca 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -27,7 +27,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { bytes32 public constant CANCELLER_ROLE = keccak256("CANCELLER_ROLE"); uint256 internal constant _DONE_TIMESTAMP = uint256(1); - mapping(bytes32 => uint256) private _timestamps; + mapping(bytes32 id => uint256) private _timestamps; uint256 private _minDelay; enum OperationState { diff --git a/contracts/governance/extensions/GovernorCountingSimple.sol b/contracts/governance/extensions/GovernorCountingSimple.sol index 26f97c8b9..87f9b39f5 100644 --- a/contracts/governance/extensions/GovernorCountingSimple.sol +++ b/contracts/governance/extensions/GovernorCountingSimple.sol @@ -22,10 +22,10 @@ abstract contract GovernorCountingSimple is Governor { uint256 againstVotes; uint256 forVotes; uint256 abstainVotes; - mapping(address => bool) hasVoted; + mapping(address voter => bool) hasVoted; } - mapping(uint256 => ProposalVote) private _proposalVotes; + mapping(uint256 proposalId => ProposalVote) private _proposalVotes; /** * @dev See {IGovernor-COUNTING_MODE}. diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol index eda22fbee..bcef4400f 100644 --- a/contracts/governance/extensions/GovernorPreventLateQuorum.sol +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -18,7 +18,7 @@ import {Math} from "../../utils/math/Math.sol"; abstract contract GovernorPreventLateQuorum is Governor { uint48 private _voteExtension; - mapping(uint256 => uint48) private _extendedDeadlines; + mapping(uint256 proposalId => uint48) private _extendedDeadlines; /// @dev Emitted when a proposal deadline is pushed back due to reaching quorum late in its voting period. event ProposalExtended(uint256 indexed proposalId, uint64 extendedDeadline); diff --git a/contracts/governance/extensions/GovernorStorage.sol b/contracts/governance/extensions/GovernorStorage.sol index 6443b7895..95aacb8c3 100644 --- a/contracts/governance/extensions/GovernorStorage.sol +++ b/contracts/governance/extensions/GovernorStorage.sol @@ -21,7 +21,7 @@ abstract contract GovernorStorage is Governor { } uint256[] private _proposalIds; - mapping(uint256 => ProposalDetails) private _proposalDetails; + mapping(uint256 proposalId => ProposalDetails) private _proposalDetails; /** * @dev Hook into the proposing mechanism diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index c468f328b..3dde36e23 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -24,7 +24,7 @@ import {SafeCast} from "../../utils/math/SafeCast.sol"; */ abstract contract GovernorTimelockControl is Governor { TimelockController private _timelock; - mapping(uint256 => bytes32) private _timelockIds; + mapping(uint256 proposalId => bytes32) private _timelockIds; /** * @dev Emitted when the timelock controller used for proposal execution is modified. diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index 69ef48d7d..96d89cb8e 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -34,9 +34,9 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); - mapping(address => address) private _delegation; + mapping(address account => address) private _delegatee; - mapping(address => Checkpoints.Trace224) private _delegateCheckpoints; + mapping(address delegatee => Checkpoints.Trace224) private _delegateCheckpoints; Checkpoints.Trace224 private _totalCheckpoints; @@ -124,7 +124,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { * @dev Returns the delegate that `account` has chosen. */ function delegates(address account) public view virtual returns (address) { - return _delegation[account]; + return _delegatee[account]; } /** @@ -166,7 +166,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { */ function _delegate(address account, address delegatee) internal virtual { address oldDelegate = delegates(account); - _delegation[account] = delegatee; + _delegatee[account] = delegatee; emit DelegateChanged(account, oldDelegate, delegatee); _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 6f8e000d2..f580f8729 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -27,10 +27,6 @@ import {Address} from "../utils/Address.sol"; * transactions in the mempool. In these cases the recommendation is to distribute the load among * multiple accounts. * - * WARNING: Do not approve this contract to spend tokens. Anyone can use this forwarder - * to execute calls with an arbitrary calldata to any address. Any form of approval may - * result in a loss of funds for the approving party. - * * NOTE: Batching requests includes an optional refund for unused `msg.value` that is achieved by * performing a call with empty calldata. While this is within the bounds of ERC-2771 compliance, * if the refund receiver happens to consider the forwarder a trusted forwarder, it MUST properly diff --git a/contracts/mocks/ERC165/ERC165InterfacesSupported.sol b/contracts/mocks/ERC165/ERC165InterfacesSupported.sol index b1efbd3e8..4010b2103 100644 --- a/contracts/mocks/ERC165/ERC165InterfacesSupported.sol +++ b/contracts/mocks/ERC165/ERC165InterfacesSupported.sol @@ -23,7 +23,7 @@ contract SupportsInterfaceWithLookupMock is IERC165 { /** * @dev A mapping of interface id to whether or not it's supported. */ - mapping(bytes4 => bool) private _supportedInterfaces; + mapping(bytes4 interfaceId => bool) private _supportedInterfaces; /** * @dev A contract implementing SupportsInterfaceWithLookup diff --git a/contracts/mocks/InitializableMock.sol b/contracts/mocks/InitializableMock.sol index 38959a5f5..7f76caacd 100644 --- a/contracts/mocks/InitializableMock.sol +++ b/contracts/mocks/InitializableMock.sol @@ -79,7 +79,7 @@ contract ChildConstructorInitializableMock is ConstructorInitializableMock { contract ReinitializerMock is Initializable { uint256 public counter; - function getInitializedVersion() public view returns (uint8) { + function getInitializedVersion() public view returns (uint64) { return _getInitializedVersion(); } @@ -87,15 +87,15 @@ contract ReinitializerMock is Initializable { doStuff(); } - function reinitialize(uint8 i) public reinitializer(i) { + function reinitialize(uint64 i) public reinitializer(i) { doStuff(); } - function nestedReinitialize(uint8 i, uint8 j) public reinitializer(i) { + function nestedReinitialize(uint64 i, uint64 j) public reinitializer(i) { reinitialize(j); } - function chainReinitialize(uint8 i, uint8 j) public { + function chainReinitialize(uint64 i, uint64 j) public { reinitialize(i); reinitialize(j); } diff --git a/contracts/mocks/StorageSlotMock.sol b/contracts/mocks/StorageSlotMock.sol index 7de9a732c..dbdad7a2a 100644 --- a/contracts/mocks/StorageSlotMock.sol +++ b/contracts/mocks/StorageSlotMock.sol @@ -39,7 +39,7 @@ contract StorageSlotMock { return slot.getUint256Slot().value; } - mapping(uint256 => string) public stringMap; + mapping(uint256 key => string) public stringMap; function setString(bytes32 slot, string calldata value) public { slot.getStringSlot().value = value; @@ -57,7 +57,7 @@ contract StorageSlotMock { return stringMap[key].getStringSlot().value; } - mapping(uint256 => bytes) public bytesMap; + mapping(uint256 key => bytes) public bytesMap; function setBytes(bytes32 slot, bytes calldata value) public { slot.getBytesSlot().value = value; diff --git a/contracts/mocks/VotesMock.sol b/contracts/mocks/VotesMock.sol index db9e9f52c..e28d6b557 100644 --- a/contracts/mocks/VotesMock.sol +++ b/contracts/mocks/VotesMock.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.20; import {Votes} from "../governance/utils/Votes.sol"; abstract contract VotesMock is Votes { - mapping(address => uint256) private _votingUnits; + mapping(address voter => uint256) private _votingUnits; function getTotalSupply() public view returns (uint256) { return _getTotalSupply(); diff --git a/contracts/mocks/token/ERC20VotesLegacyMock.sol b/contracts/mocks/token/ERC20VotesLegacyMock.sol index 2ddc25c53..192263e82 100644 --- a/contracts/mocks/token/ERC20VotesLegacyMock.sol +++ b/contracts/mocks/token/ERC20VotesLegacyMock.sol @@ -20,8 +20,8 @@ abstract contract ERC20VotesLegacyMock is IVotes, ERC20Permit { bytes32 private constant _DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); - mapping(address => address) private _delegates; - mapping(address => Checkpoint[]) private _checkpoints; + mapping(address account => address) private _delegatee; + mapping(address delegatee => Checkpoint[]) private _checkpoints; Checkpoint[] private _totalSupplyCheckpoints; /** @@ -42,7 +42,7 @@ abstract contract ERC20VotesLegacyMock is IVotes, ERC20Permit { * @dev Get the address `account` is currently delegating to. */ function delegates(address account) public view virtual returns (address) { - return _delegates[account]; + return _delegatee[account]; } /** @@ -188,7 +188,7 @@ abstract contract ERC20VotesLegacyMock is IVotes, ERC20Permit { function _delegate(address delegator, address delegatee) internal virtual { address currentDelegate = delegates(delegator); uint256 delegatorBalance = balanceOf(delegator); - _delegates[delegator] = delegatee; + _delegatee[delegator] = delegatee; emit DelegateChanged(delegator, currentDelegate, delegatee); diff --git a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol index 9c81549be..7732ae4a5 100644 --- a/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveEnumerableMock.sol @@ -28,25 +28,15 @@ contract ERC721ConsecutiveEnumerableMock is ERC721Consecutive, ERC721Enumerable return super._ownerOf(tokenId); } - function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { - super._mint(to, tokenId); + function _update( + address to, + uint256 tokenId, + address auth + ) internal virtual override(ERC721Consecutive, ERC721Enumerable) returns (address) { + return super._update(to, tokenId, auth); } - 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, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + 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 0054d7511..109864718 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -41,26 +41,16 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes return super._ownerOf(tokenId); } - function _mint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Consecutive) { - super._mint(to, tokenId); + function _update( + address to, + uint256 tokenId, + address auth + ) internal virtual override(ERC721Consecutive, ERC721Pausable, ERC721Votes) returns (address) { + return super._update(to, tokenId, auth); } - 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, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override(ERC721, ERC721Votes, ERC721Consecutive) { - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + function _increaseBalance(address account, uint128 amount) internal virtual override(ERC721, ERC721Votes) { + super._increaseBalance(account, amount); } } diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 53a34b3fe..7f33a6267 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -55,14 +55,27 @@ pragma solidity ^0.8.20; */ abstract contract Initializable { /** - * @dev Indicates that the contract has been initialized. + * @dev Storage of the initializable contract. + * + * It's implemented on a custom ERC-7201 namespace to reduce the risk of storage collisions + * when using with upgradeable contracts. + * + * @custom:storage-location erc7201:openzeppelin.storage.Initializable */ - uint8 private _initialized; + struct InitializableStorage { + /** + * @dev Indicates that the contract has been initialized. + */ + uint64 _initialized; + /** + * @dev Indicates that the contract is in the process of being initialized. + */ + bool _initializing; + } - /** - * @dev Indicates that the contract is in the process of being initialized. - */ - bool private _initializing; + // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) + bytes32 private constant _INITIALIZABLE_STORAGE = + 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a0e; /** * @dev The contract is already initialized. @@ -77,7 +90,7 @@ abstract contract Initializable { /** * @dev Triggered when the contract has been initialized or reinitialized. */ - event Initialized(uint8 version); + event Initialized(uint64 version); /** * @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope, @@ -89,17 +102,20 @@ abstract contract Initializable { * Emits an {Initialized} event. */ modifier initializer() { - bool isTopLevelCall = !_initializing; - if (!(isTopLevelCall && _initialized < 1) && !(address(this).code.length == 0 && _initialized == 1)) { + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + bool isTopLevelCall = !$._initializing; + if (!(isTopLevelCall && $._initialized < 1) && !(address(this).code.length == 0 && $._initialized == 1)) { revert AlreadyInitialized(); } - _initialized = 1; + $._initialized = 1; if (isTopLevelCall) { - _initializing = true; + $._initializing = true; } _; if (isTopLevelCall) { - _initializing = false; + $._initializing = false; emit Initialized(1); } } @@ -122,14 +138,17 @@ abstract contract Initializable { * * Emits an {Initialized} event. */ - modifier reinitializer(uint8 version) { - if (_initializing || _initialized >= version) { + modifier reinitializer(uint64 version) { + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + if ($._initializing || $._initialized >= version) { revert AlreadyInitialized(); } - _initialized = version; - _initializing = true; + $._initialized = version; + $._initializing = true; _; - _initializing = false; + $._initializing = false; emit Initialized(version); } @@ -146,7 +165,7 @@ abstract contract Initializable { * @dev Reverts if the contract is not in an initializing state. See {onlyInitializing}. */ function _checkInitializing() internal view virtual { - if (!_initializing) { + if (!_isInitializing()) { revert NotInitializing(); } } @@ -160,26 +179,39 @@ abstract contract Initializable { * Emits an {Initialized} event the first time it is successfully executed. */ function _disableInitializers() internal virtual { - if (_initializing) { + // solhint-disable-next-line var-name-mixedcase + InitializableStorage storage $ = _getInitializableStorage(); + + if ($._initializing) { revert AlreadyInitialized(); } - if (_initialized != type(uint8).max) { - _initialized = type(uint8).max; - emit Initialized(type(uint8).max); + if ($._initialized != type(uint64).max) { + $._initialized = type(uint64).max; + emit Initialized(type(uint64).max); } } /** * @dev Returns the highest version that has been initialized. See {reinitializer}. */ - function _getInitializedVersion() internal view returns (uint8) { - return _initialized; + function _getInitializedVersion() internal view returns (uint64) { + return _getInitializableStorage()._initialized; } /** * @dev Returns `true` if the contract is currently initializing. See {onlyInitializing}. */ function _isInitializing() internal view returns (bool) { - return _initializing; + return _getInitializableStorage()._initializing; + } + + /** + * @dev Returns a pointer to the storage namespace. + */ + // solhint-disable-next-line var-name-mixedcase + function _getInitializableStorage() private pure returns (InitializableStorage storage $) { + assembly { + $.slot := _INITIALIZABLE_STORAGE + } } } diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol index 6a65a3296..a89c35d2d 100644 --- a/contracts/token/ERC1155/ERC1155.sol +++ b/contracts/token/ERC1155/ERC1155.sol @@ -20,11 +20,9 @@ abstract contract ERC1155 is Context, ERC165, IERC1155, IERC1155MetadataURI, IER using Arrays for uint256[]; using Arrays for address[]; - // Mapping from token ID to account balances - mapping(uint256 => mapping(address => uint256)) private _balances; + mapping(uint256 id => mapping(address account => uint256)) private _balances; - // Mapping from account to operator approvals - mapping(address => mapping(address => bool)) private _operatorApprovals; + mapping(address account => mapping(address operator => bool)) private _operatorApprovals; // Used as the URI for all token types by relying on ID substitution, e.g. https://token-cdn-domain/{id}.json string private _uri; diff --git a/contracts/token/ERC1155/extensions/ERC1155Supply.sol b/contracts/token/ERC1155/extensions/ERC1155Supply.sol index 8553b356a..bbf5e2a8f 100644 --- a/contracts/token/ERC1155/extensions/ERC1155Supply.sol +++ b/contracts/token/ERC1155/extensions/ERC1155Supply.sol @@ -19,7 +19,7 @@ import {ERC1155} from "../ERC1155.sol"; * CAUTION: This extension should not be added in an upgrade to an already deployed contract. */ abstract contract ERC1155Supply is ERC1155 { - mapping(uint256 => uint256) private _totalSupply; + mapping(uint256 id => uint256) private _totalSupply; uint256 private _totalSupplyAll; /** diff --git a/contracts/token/ERC1155/extensions/ERC1155URIStorage.sol b/contracts/token/ERC1155/extensions/ERC1155URIStorage.sol index 09f777082..b766d9adc 100644 --- a/contracts/token/ERC1155/extensions/ERC1155URIStorage.sol +++ b/contracts/token/ERC1155/extensions/ERC1155URIStorage.sol @@ -17,7 +17,7 @@ abstract contract ERC1155URIStorage is ERC1155 { string private _baseURI = ""; // Optional mapping for token URIs - mapping(uint256 => string) private _tokenURIs; + mapping(uint256 tokenId => string) private _tokenURIs; /** * @dev See {IERC1155MetadataURI-uri}. diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index 8d4d60465..e8212e92b 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -36,9 +36,9 @@ import {IERC20Errors} from "../../interfaces/draft-IERC6093.sol"; * allowances. See {IERC20-approve}. */ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { - mapping(address => uint256) private _balances; + mapping(address account => uint256) private _balances; - mapping(address => mapping(address => uint256)) private _allowances; + mapping(address account => mapping(address spender => uint256)) private _allowances; uint256 private _totalSupply; diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 932671bbd..d56e33775 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -25,17 +25,13 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er // Token symbol string private _symbol; - // Mapping from token ID to owner address - mapping(uint256 => address) private _owners; + mapping(uint256 tokenId => address) private _owners; - // Mapping owner address to token count - mapping(address => uint256) private _balances; + mapping(address owner => uint256) private _balances; - // Mapping from token ID to approved address - mapping(uint256 => address) private _tokenApprovals; + mapping(uint256 tokenId => address) private _tokenApprovals; - // Mapping from owner to operator approvals - mapping(address => mapping(address => bool)) private _operatorApprovals; + mapping(address owner => mapping(address operator => bool)) private _operatorApprovals; /** * @dev Initializes the contract by setting a `name` and a `symbol` to the token collection. @@ -113,16 +109,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); - if (to == owner) { - revert ERC721InvalidOperator(owner); - } - - if (_msgSender() != owner && !isApprovedForAll(owner, _msgSender())) { - revert ERC721InvalidApprover(_msgSender()); - } - - _approve(to, tokenId); + _approve(to, tokenId, _msgSender()); } /** @@ -131,7 +118,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); } /** @@ -152,17 +139,21 @@ 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); + if (to == address(0)) { + revert ERC721InvalidReceiver(address(0)); + } + // 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) { + revert ERC721IncorrectOwner(from, tokenId, previousOwner); } - - _transfer(from, to, tokenId); } /** * @dev See {IERC721-safeTransferFrom}. */ - function safeTransferFrom(address from, address to, uint256 tokenId) public virtual { + function safeTransferFrom(address from, address to, uint256 tokenId) public { safeTransferFrom(from, to, tokenId, ""); } @@ -170,91 +161,113 @@ 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); - if (!_checkOnERC721Received(from, to, tokenId, data)) { - revert ERC721InvalidReceiver(to); - } + transferFrom(from, to, tokenId); + _checkOnERC721Received(from, to, tokenId, data); } /** * @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`. */ function _ownerOf(uint256 tokenId) internal view virtual returns (address) { 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`). + * @dev Returns the approved address for `tokenId`. Returns 0 if `tokenId` is not minted. */ - function _exists(uint256 tokenId) internal view virtual returns (bool) { - return _ownerOf(tokenId) != address(0); + function _getApproved(uint256 tokenId) internal view virtual returns (address) { + return _tokenApprovals[tokenId]; } /** - * @dev Returns whether `spender` is allowed to manage `tokenId`. + * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in + * particular (ignoring whether it is owned by `owner`). * - * Requirements: - * - * - `tokenId` must exist. + * WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not + * verify this assumption. */ - 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 _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) { + return + spender != address(0) && + (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender); } /** - * @dev Safely mints `tokenId` and transfers it to `to`. + * @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`. * - * Requirements: + * WARNING: This function relies on {_isAuthorized}, so it doesn't check whether `owner` is the + * actual owner of `tokenId`. + */ + function _checkAuthorized(address owner, address spender, uint256 tokenId) internal view virtual { + if (!_isAuthorized(owner, spender, tokenId)) { + if (owner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else { + revert ERC721InsufficientApproval(spender, tokenId); + } + } + } + + /** + * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. * - * - `tokenId` must not exist. - * - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer. + * 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: 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 { + 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 `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. + * + * NOTE: If overriding this function in a way that tracks balances, see also {_increaseBalance}. */ - function _safeMint(address to, uint256 tokenId) internal virtual { - _safeMint(to, tokenId, ""); - } + function _update(address to, uint256 tokenId, address auth) internal virtual returns (address) { + address from = _ownerOf(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); + // Perform (optional) operator check + if (auth != address(0)) { + _checkAuthorized(from, auth, tokenId); } + + // Execute the update + 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; } /** @@ -269,34 +282,37 @@ 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)) { + address previousOwner = _update(to, tokenId, address(0)); + if (previousOwner != address(0)) { revert ERC721InvalidSender(address(0)); } + } - _beforeTokenTransfer(address(0), to, tokenId, 1); + /** + * @dev Mints `tokenId`, transfers it to `to` and checks for `to` acceptance. + * + * 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 { + _safeMint(to, tokenId, ""); + } - // 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); + /** + * @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); + _checkOnERC721Received(address(0), to, tokenId, data); } /** @@ -310,26 +326,11 @@ 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 { + address previousOwner = _update(address(0), tokenId, address(0)); + if (previousOwner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } } /** @@ -343,61 +344,83 @@ 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); - if (owner != from) { - revert ERC721IncorrectOwner(from, tokenId, owner); + address previousOwner = _update(to, tokenId, address(0)); + if (previousOwner == address(0)) { + revert ERC721NonexistentToken(tokenId); + } else if (previousOwner != from) { + revert ERC721IncorrectOwner(from, tokenId, previousOwner); } + } - // Clear approvals from the previous owner - delete _tokenApprovals[tokenId]; + /** + * @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`. + * + * 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: + * + * - `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. + */ + function _safeTransfer(address from, address to, uint256 tokenId) internal { + _safeTransfer(from, to, 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); + /** + * @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); } /** * @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 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); + } + _tokenApprovals[tokenId] = to; - emit Approval(ownerOf(tokenId), to, tokenId); + emit Approval(owner, to, tokenId); + + return owner; } /** * @dev Approve `operator` to operate on all of `owner` tokens * + * Requirements: + * - operator can't be the address zero. + * * Emits an {ApprovalForAll} event. */ function _setApprovalForAll(address owner, address operator, bool approved) internal virtual { - if (owner == operator) { - revert ERC721InvalidOperator(owner); + if (operator == address(0)) { + revert ERC721InvalidOperator(operator); } _operatorApprovals[owner][operator] = approved; emit ApprovalForAll(owner, operator, approved); @@ -407,30 +430,26 @@ 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); } } /** - * @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); @@ -441,52 +460,6 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er } } } - } else { - return true; } } - - /** - * @dev Hook that is called before any token transfer. This includes minting and burning. If {ERC721Consecutive} is - * used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1. - * - * Calling conditions: - * - * - When `from` and `to` are both non-zero, ``from``'s tokens will be transferred to `to`. - * - When `from` is zero, the tokens will be minted for `to`. - * - When `to` is zero, ``from``'s tokens will be burned. - * - `from` and `to` are never both zero. - * - `batchSize` is non-zero. - * - * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. - */ - function _beforeTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {} - - /** - * @dev Hook that is called after any token transfer. This includes minting and burning. If {ERC721Consecutive} is - * used, the hook may be called as part of a consecutive (batch) mint, as indicated by `batchSize` greater than 1. - * - * Calling conditions: - * - * - When `from` and `to` are both non-zero, ``from``'s tokens were transferred to `to`. - * - When `from` is zero, the tokens were minted for `to`. - * - When `to` is zero, ``from``'s tokens were burned. - * - `from` and `to` are never both zero. - * - `batchSize` is non-zero. - * - * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks]. - */ - function _afterTokenTransfer(address from, address to, uint256 firstTokenId, uint256 batchSize) internal virtual {} - - /** - * @dev Unsafe write access to the balances, used by extensions that "mint" tokens using an {ownerOf} override. - * - * WARNING: Anyone calling this MUST ensure that the balances remain consistent with the ownership. The invariant - * being that for any address `a` the value returned by `balanceOf(a)` must be equal to the number of tokens such - * that `ownerOf(tokenId)` is `a`. - */ - // solhint-disable-next-line func-name-mixedcase - function __unsafe_increaseBalance(address account, uint256 value) internal { - _balances[account] += value; - } } diff --git a/contracts/token/ERC721/IERC721.sol b/contracts/token/ERC721/IERC721.sol index 49471fe77..cea9c1ca5 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. */ diff --git a/contracts/token/ERC721/extensions/ERC721Burnable.sol b/contracts/token/ERC721/extensions/ERC721Burnable.sol index 299a12536..eecbd6435 100644 --- a/contracts/token/ERC721/extensions/ERC721Burnable.sol +++ b/contracts/token/ERC721/extensions/ERC721Burnable.sol @@ -19,9 +19,8 @@ abstract contract ERC721Burnable is Context, ERC721 { * - The caller must own `tokenId` or be an approved operator. */ function burn(uint256 tokenId) public virtual { - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); - } - _burn(tokenId); + // 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/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 4540d295b..1cf88dbf9 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -118,61 +118,44 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { revert ERC721ExceededMaxBatchMint(batchSize, maxBatchSize); } - // hook before - _beforeTokenTransfer(address(0), to, next, batchSize); - // push an ownership checkpoint & emit event uint96 last = next + batchSize - 1; _sequentialOwnership.push(last, uint160(to)); // The invariant required by this function is preserved because the new sequentialOwnership checkpoint // is attributing ownership of `batchSize` new tokens to account `to`. - __unsafe_increaseBalance(to, batchSize); + _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, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); + + // only mint after construction + if (previousOwner == 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 previousOwner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Enumerable.sol b/contracts/token/ERC721/extensions/ERC721Enumerable.sol index acb57a7be..1b7cb650f 100644 --- a/contracts/token/ERC721/extensions/ERC721Enumerable.sol +++ b/contracts/token/ERC721/extensions/ERC721Enumerable.sol @@ -15,17 +15,11 @@ import {IERC165} from "../../../utils/introspection/ERC165.sol"; * interfere with enumerability and should not be used together with `ERC721Enumerable`. */ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { - // Mapping from owner to list of owned token IDs - mapping(address => mapping(uint256 => uint256)) private _ownedTokens; + mapping(address owner => mapping(uint256 index => uint256)) private _ownedTokens; + mapping(uint256 tokenId => uint256) private _ownedTokensIndex; - // Mapping from token ID to index of the owner tokens list - mapping(uint256 => uint256) private _ownedTokensIndex; - - // Array with all token ids, used for enumeration uint256[] private _allTokens; - - // Mapping from token id to position in the allTokens array - mapping(uint256 => uint256) private _allTokensIndex; + mapping(uint256 tokenId => uint256) private _allTokensIndex; /** * @dev An `owner`'s token query was out of bounds for `index`. @@ -74,33 +68,23 @@ 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); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); - if (batchSize > 1) { - // Will only trigger during construction. Batch transferring (minting) is not available afterwards. - revert ERC721EnumerableForbiddenBatchMint(); - } - - uint256 tokenId = firstTokenId; - - 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 (to != from) { + } else if (previousOwner != to) { _addTokenToOwnerEnumeration(to, tokenId); } + + return previousOwner; } /** @@ -109,7 +93,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; } @@ -135,7 +119,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 @@ -175,4 +159,14 @@ abstract contract ERC721Enumerable is ERC721, IERC721Enumerable { delete _allTokensIndex[tokenId]; _allTokens.pop(); } + + /** + * See {ERC721-_increaseBalance}. We need that to account tokens that were minted in batch + */ + function _increaseBalance(address account, uint128 amount) internal virtual override { + if (amount > 0) { + revert ERC721EnumerableForbiddenBatchMint(); + } + super._increaseBalance(account, amount); + } } diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 50fb7d1ea..301469d0a 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -21,20 +21,17 @@ import {Pausable} from "../../../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); - - _requireNotPaused(); + uint256 tokenId, + address auth + ) internal virtual override whenNotPaused returns (address) { + return super._update(to, tokenId, auth); } } diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 30906e605..5b889b90d 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -26,10 +26,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}. When burning, this override will additionally clear the royalty information for the token. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); - _resetTokenRoyalty(tokenId); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); + + if (to == address(0)) { + _resetTokenRoyalty(tokenId); + } + + return previousOwner; } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index f32babe0f..cd4845b78 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -15,7 +15,7 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { using Strings for uint256; // Optional mapping for token URIs - mapping(uint256 => string) private _tokenURIs; + mapping(uint256 tokenId => string) private _tokenURIs; /** * @dev See {IERC165-supportsInterface} @@ -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; @@ -64,15 +64,17 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { } /** - * @dev See {ERC721-_burn}. 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. */ - function _burn(uint256 tokenId) internal virtual override { - super._burn(tokenId); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); - if (bytes(_tokenURIs[tokenId]).length != 0) { + if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { delete _tokenURIs[tokenId]; } + + return previousOwner; } } diff --git a/contracts/token/ERC721/extensions/ERC721Votes.sol b/contracts/token/ERC721/extensions/ERC721Votes.sol index 4bef6ac5a..91e464173 100644 --- a/contracts/token/ERC721/extensions/ERC721Votes.sol +++ b/contracts/token/ERC721/extensions/ERC721Votes.sol @@ -16,18 +16,16 @@ import {Votes} from "../../../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, - address to, - uint256 firstTokenId, - uint256 batchSize - ) internal virtual override { - _transferVotingUnits(from, to, batchSize); - super._afterTokenTransfer(from, to, firstTokenId, batchSize); + function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { + address previousOwner = super._update(to, tokenId, auth); + + _transferVotingUnits(previousOwner, to, 1); + + return previousOwner; } /** @@ -38,4 +36,12 @@ abstract contract ERC721Votes is ERC721, Votes { function _getVotingUnits(address account) internal view virtual override returns (uint256) { return balanceOf(account); } + + /** + * @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); + _transferVotingUnits(address(0), account, amount); + } } diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index aea8e58d7..15844ea6f 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/contracts/token/ERC721/extensions/ERC721Wrapper.sol @@ -50,10 +50,9 @@ abstract contract ERC721Wrapper is ERC721, IERC721Receiver { uint256 length = tokenIds.length; for (uint256 i = 0; i < length; ++i) { uint256 tokenId = tokenIds[i]; - if (!_isApprovedOrOwner(_msgSender(), tokenId)) { - revert ERC721InsufficientApproval(_msgSender(), tokenId); - } - _burn(tokenId); + // 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 // the wrapped tokenId after this point, so it's safe to remove the reentrancy check for the next line. // slither-disable-next-line reentrancy-no-eth diff --git a/contracts/token/common/ERC2981.sol b/contracts/token/common/ERC2981.sol index b56e5f956..bdcbf2f74 100644 --- a/contracts/token/common/ERC2981.sol +++ b/contracts/token/common/ERC2981.sol @@ -26,7 +26,7 @@ abstract contract ERC2981 is IERC2981, ERC165 { } RoyaltyInfo private _defaultRoyaltyInfo; - mapping(uint256 => RoyaltyInfo) private _tokenRoyaltyInfo; + mapping(uint256 tokenId => RoyaltyInfo) private _tokenRoyaltyInfo; /** * @dev The default royalty set is invalid (eg. (numerator / denominator) >= 1). diff --git a/contracts/utils/Nonces.sol b/contracts/utils/Nonces.sol index eb756b5e6..b4681371a 100644 --- a/contracts/utils/Nonces.sol +++ b/contracts/utils/Nonces.sol @@ -10,7 +10,7 @@ abstract contract Nonces { */ error InvalidAccountNonce(address account, uint256 currentNonce); - mapping(address => uint256) private _nonces; + mapping(address account => uint256) private _nonces; /** * @dev Returns an the next unused nonce for an address. diff --git a/contracts/utils/structs/BitMaps.sol b/contracts/utils/structs/BitMaps.sol index 413b47e0c..b9d3de671 100644 --- a/contracts/utils/structs/BitMaps.sol +++ b/contracts/utils/structs/BitMaps.sol @@ -17,7 +17,7 @@ pragma solidity ^0.8.20; */ library BitMaps { struct BitMap { - mapping(uint256 => uint256) _data; + mapping(uint256 bucket => uint256) _data; } /** diff --git a/contracts/utils/structs/DoubleEndedQueue.sol b/contracts/utils/structs/DoubleEndedQueue.sol index 93f1b2d01..1827a5cf4 100644 --- a/contracts/utils/structs/DoubleEndedQueue.sol +++ b/contracts/utils/structs/DoubleEndedQueue.sol @@ -45,7 +45,7 @@ library DoubleEndedQueue { struct Bytes32Deque { uint128 _begin; uint128 _end; - mapping(uint128 => bytes32) _data; + mapping(uint128 index => bytes32) _data; } /** diff --git a/contracts/utils/structs/EnumerableMap.sol b/contracts/utils/structs/EnumerableMap.sol index 017072075..030ed1eef 100644 --- a/contracts/utils/structs/EnumerableMap.sol +++ b/contracts/utils/structs/EnumerableMap.sol @@ -65,7 +65,7 @@ library EnumerableMap { struct Bytes32ToBytes32Map { // Storage of keys EnumerableSet.Bytes32Set _keys; - mapping(bytes32 => bytes32) _values; + mapping(bytes32 key => bytes32) _values; } /** diff --git a/contracts/utils/structs/EnumerableSet.sol b/contracts/utils/structs/EnumerableSet.sol index 272851b2f..6df060b55 100644 --- a/contracts/utils/structs/EnumerableSet.sol +++ b/contracts/utils/structs/EnumerableSet.sol @@ -53,7 +53,7 @@ library EnumerableSet { bytes32[] _values; // Position of the value in the `values` array, plus 1 because index 0 // means a value is not in the set. - mapping(bytes32 => uint256) _indexes; + mapping(bytes32 value => uint256) _indexes; } /** diff --git a/docs/modules/ROOT/pages/index.adoc b/docs/modules/ROOT/pages/index.adoc index ff720f93c..905ce231d 100644 --- a/docs/modules/ROOT/pages/index.adoc +++ b/docs/modules/ROOT/pages/index.adoc @@ -11,11 +11,25 @@ [[install]] === Installation +==== Hardhat, Truffle (npm) + ```console $ npm install @openzeppelin/contracts ``` -OpenZeppelin Contracts features a xref:releases-stability.adoc#api-stability[stable API], which means your contracts won't break unexpectedly when upgrading to a newer minor version. +OpenZeppelin Contracts features a xref:releases-stability.adoc#api-stability[stable API], which means that your contracts won't break unexpectedly when upgrading to a newer minor version. + +==== Foundry (git) + +WARNING: When installing via git, it is a common error to use the `master` branch. This is a development branch that should be avoided in favor of tagged releases. The release process involves security measures that the `master` branch does not guarantee. + +WARNING: Foundry installs the latest version initially, but subsequent `forge update` commands will use the `master` branch. + +```console +$ forge install OpenZeppelin/openzeppelin-contracts +``` + +Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.` [[usage]] === Usage diff --git a/remappings.txt b/remappings.txt index 2479e3d26..304d1386a 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1 +1 @@ -openzeppelin/=contracts/ +@openzeppelin/contracts/=contracts/ diff --git a/scripts/generate/templates/EnumerableMap.js b/scripts/generate/templates/EnumerableMap.js index 2582b1206..8184bfe18 100644 --- a/scripts/generate/templates/EnumerableMap.js +++ b/scripts/generate/templates/EnumerableMap.js @@ -74,7 +74,7 @@ error EnumerableMapNonexistentKey(bytes32 key); struct Bytes32ToBytes32Map { // Storage of keys EnumerableSet.Bytes32Set _keys; - mapping(bytes32 => bytes32) _values; + mapping(bytes32 key => bytes32) _values; } /** diff --git a/scripts/generate/templates/EnumerableSet.js b/scripts/generate/templates/EnumerableSet.js index 4079b3718..57dd9d8a3 100644 --- a/scripts/generate/templates/EnumerableSet.js +++ b/scripts/generate/templates/EnumerableSet.js @@ -63,7 +63,7 @@ struct Set { bytes32[] _values; // Position of the value in the \`values\` array, plus 1 because index 0 // means a value is not in the set. - mapping(bytes32 => uint256) _indexes; + mapping(bytes32 value => uint256) _indexes; } /** diff --git a/test/governance/Governor.t.sol b/test/governance/Governor.t.sol index eaa4eefe5..f63124536 100644 --- a/test/governance/Governor.t.sol +++ b/test/governance/Governor.t.sol @@ -3,8 +3,8 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {Strings} from "../../contracts/utils/Strings.sol"; -import {Governor} from "../../contracts/governance/Governor.sol"; +import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; +import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; contract GovernorInternalTest is Test, Governor { constructor() Governor("") {} diff --git a/test/helpers/constants.js b/test/helpers/constants.js new file mode 100644 index 000000000..0f4d028cf --- /dev/null +++ b/test/helpers/constants.js @@ -0,0 +1,7 @@ +const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); +const MAX_UINT64 = web3.utils.toBN(1).shln(64).subn(1).toString(); + +module.exports = { + MAX_UINT48, + MAX_UINT64, +}; diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 522b05cd1..b0ebccca8 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -1,6 +1,7 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { getDomain, domainType } = require('../helpers/eip712'); +const { MAX_UINT48 } = require('../helpers/constants'); const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); @@ -14,8 +15,6 @@ const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); contract('ERC2771Context', function (accounts) { const [, trustedForwarder] = accounts; - const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); - beforeEach(async function () { this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder'); this.recipient = await ERC2771ContextMock.new(this.forwarder.address); diff --git a/test/metatx/ERC2771Forwarder.t.sol b/test/metatx/ERC2771Forwarder.t.sol index 3256289ea..d69b4750a 100644 --- a/test/metatx/ERC2771Forwarder.t.sol +++ b/test/metatx/ERC2771Forwarder.t.sol @@ -3,8 +3,8 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {ERC2771Forwarder} from "contracts/metatx/ERC2771Forwarder.sol"; -import {CallReceiverMockTrustingForwarder, CallReceiverMock} from "contracts/mocks/CallReceiverMock.sol"; +import {ERC2771Forwarder} from "@openzeppelin/contracts/metatx/ERC2771Forwarder.sol"; +import {CallReceiverMockTrustingForwarder, CallReceiverMock} from "@openzeppelin/contracts/mocks/CallReceiverMock.sol"; struct ForwardRequest { address from; diff --git a/test/proxy/utils/Initializable.test.js b/test/proxy/utils/Initializable.test.js index e3e0fc02f..98ed19d36 100644 --- a/test/proxy/utils/Initializable.test.js +++ b/test/proxy/utils/Initializable.test.js @@ -1,6 +1,7 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const { expectRevertCustomError } = require('../../helpers/customError'); +const { MAX_UINT64 } = require('../../helpers/constants'); const InitializableMock = artifacts.require('InitializableMock'); const ConstructorInitializableMock = artifacts.require('ConstructorInitializableMock'); @@ -213,7 +214,7 @@ contract('Initializable', function () { it('old and new patterns in good sequence', async function () { const ok = await DisableOk.new(); await expectEvent.inConstruction(ok, 'Initialized', { version: '1' }); - await expectEvent.inConstruction(ok, 'Initialized', { version: '255' }); + await expectEvent.inConstruction(ok, 'Initialized', { version: MAX_UINT64 }); }); }); }); diff --git a/test/token/ERC20/extensions/ERC4626.t.sol b/test/token/ERC20/extensions/ERC4626.t.sol index d4b9e2af1..72b0daca1 100644 --- a/test/token/ERC20/extensions/ERC4626.t.sol +++ b/test/token/ERC20/extensions/ERC4626.t.sol @@ -3,12 +3,12 @@ pragma solidity ^0.8.20; import {ERC4626Test} from "erc4626-tests/ERC4626.test.sol"; -import {ERC20} from "openzeppelin/token/ERC20/ERC20.sol"; -import {ERC4626} from "openzeppelin/token/ERC20/extensions/ERC4626.sol"; +import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import {ERC4626} from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; -import {ERC20Mock} from "openzeppelin/mocks/token/ERC20Mock.sol"; -import {ERC4626Mock} from "openzeppelin/mocks/token/ERC4626Mock.sol"; -import {ERC4626OffsetMock} from "openzeppelin/mocks/token/ERC4626OffsetMock.sol"; +import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol"; +import {ERC4626Mock} from "@openzeppelin/contracts/mocks/token/ERC4626Mock.sol"; +import {ERC4626OffsetMock} from "@openzeppelin/contracts/mocks/token/ERC4626OffsetMock.sol"; contract ERC4626VaultOffsetMock is ERC4626OffsetMock { constructor( diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 75700f6ab..4dd6e234c 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,170 @@ 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, - }); - }); - - 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, - }); - }); - - 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], - ); - }); + await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { + operator: owner, + from: owner, + tokenId: tokenId, + data: data, }); }); - }; - describe('with data', function () { - shouldTransferSafely(safeTransferFromWithData, data); - }); + it('calls onERC721Received from approved', async function () { + const receipt = await transferFun.call(this, owner, this.receiver.address, tokenId, { from: approved }); - describe('without data', function () { - shouldTransferSafely(safeTransferFromWithoutData, null); - }); + await expectEvent.inTransaction(receipt.tx, ERC721ReceiverMock, 'Received', { + operator: approved, + from: owner, + tokenId: tokenId, + data: data, + }); + }); - 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], - ); + 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('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', - ); + 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.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.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.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.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.methods[fnName + '(address,address,uint256)'](owner, nonReceiver.address, tokenId, { + from: owner, + }), + 'ERC721InvalidReceiver', + [nonReceiver.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.safeTransferFrom(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 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 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('safe mint', function () { @@ -524,14 +555,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( @@ -645,12 +668,12 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper }); }); - context('when the operator is the owner', function () { + context('when the operator is address zero', function () { it('reverts', async function () { await expectRevertCustomError( - this.token.setApprovalForAll(owner, true, { from: owner }), + this.token.setApprovalForAll(constants.ZERO_ADDRESS, true, { from: owner }), 'ERC721InvalidOperator', - [owner], + [constants.ZERO_ADDRESS], ); }); }); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.t.sol b/test/token/ERC721/extensions/ERC721Consecutive.t.sol index fc164558b..eca15e717 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.t.sol +++ b/test/token/ERC721/extensions/ERC721Consecutive.t.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.20; // solhint-disable func-name-mixedcase -import {ERC721} from "../../../../contracts/token/ERC721/ERC721.sol"; -import {ERC721Consecutive} from "../../../../contracts/token/ERC721/extensions/ERC721Consecutive.sol"; +import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; +import {ERC721Consecutive} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721Consecutive.sol"; import {Test, StdUtils} from "forge-std/Test.sol"; function toSingleton(address account) pure returns (address[] memory) { diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index 55c26dffe..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,20 +174,8 @@ contract('ERC721Consecutive', function (accounts) { tokenId, }); - 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', - [], - ); - }); }); }); } 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]); }); }); diff --git a/test/utils/ShortStrings.t.sol b/test/utils/ShortStrings.t.sol index e7e6b1960..b854d273c 100644 --- a/test/utils/ShortStrings.t.sol +++ b/test/utils/ShortStrings.t.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {ShortStrings, ShortString} from "../../contracts/utils/ShortStrings.sol"; +import {ShortStrings, ShortString} from "@openzeppelin/contracts/utils/ShortStrings.sol"; contract ShortStringsTest is Test { string _fallback; diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index d5c7e5c32..0b497a858 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; -import {Math} from "../../../contracts/utils/math/Math.sol"; +import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; contract MathTest is Test { // CEILDIV