diff --git a/CHANGELOG.md b/CHANGELOG.md index b526dade7b0..24653c39dac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481)) * `ERC721`: optimize burn by making approval clearing implicit instead of emitting an event. ([#3538](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3538)) * `ERC721`: Fix balance accounting when a custom `_beforeTokenTransfer` hook results in a transfer of the token under consideration. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611)) + * `ERC721`: use unchecked arithmetic for balance updates. ([#3524](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3524)) * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515)) * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 3c457801e49..2932ab4a943 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -285,7 +285,14 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { // Check that tokenId was not minted by `_beforeTokenTransfer` hook require(!_exists(tokenId), "ERC721: token already minted"); - _balances[to] += 1; + 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); @@ -309,13 +316,17 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(owner, address(0), tokenId); - // Update ownership in case tokenId was transfered by `_beforeTokenTransfer` hook + // Update ownership in case tokenId was transferred by `_beforeTokenTransfer` hook owner = ERC721.ownerOf(tokenId); // Clear approvals delete _tokenApprovals[tokenId]; - _balances[owner] -= 1; + unchecked { + // Cannot overflow, as that would require more tokens to be burned/transferred + // out than the owner initialy received through minting and transferring in. + _balances[owner] -= 1; + } delete _owners[tokenId]; emit Transfer(owner, address(0), tokenId); @@ -350,8 +361,15 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { // Clear approvals from the previous owner delete _tokenApprovals[tokenId]; - _balances[from] -= 1; - _balances[to] += 1; + unchecked { + // `_balances[from]` cannot overflow for the same reason as described in `_burn`: + // `from`'s balance is the number of token held, which is at least one before the current + // transfer. + // `_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[from] -= 1; + _balances[to] += 1; + } _owners[tokenId] = to; emit Transfer(from, to, tokenId);