From 61a003744fa9c79bdf6d63d9d50f58e2325229b4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Jul 2022 22:19:53 +0200 Subject: [PATCH 1/7] Use unchecked to improve gas usage of ERC721 --- contracts/token/ERC721/ERC721.sol | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 0a33e88841c..add790cde05 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -282,7 +282,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(address(0), to, tokenId); - _balances[to] += 1; + unchecked { + _balances[to] += 1; + } _owners[tokenId] = to; emit Transfer(address(0), to, tokenId); @@ -308,7 +310,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { // Clear approvals _approve(address(0), tokenId); - _balances[owner] -= 1; + unchecked { + _balances[owner] -= 1; + } delete _owners[tokenId]; emit Transfer(owner, address(0), tokenId); @@ -340,8 +344,10 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { // Clear approvals from the previous owner _approve(address(0), tokenId); - _balances[from] -= 1; - _balances[to] += 1; + unchecked { + _balances[from] -= 1; + _balances[to] += 1; + } _owners[tokenId] = to; emit Transfer(from, to, tokenId); From 48bea11415082fb8746c360525a188ff849b4b57 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 6 Jul 2022 10:11:02 +0200 Subject: [PATCH 2/7] add unchecked comments --- contracts/token/ERC721/ERC721.sol | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index add790cde05..bd3762b9c5a 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -283,6 +283,10 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(address(0), to, tokenId); 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 allow batch minting. + // The ERC fails to describe this case. _balances[to] += 1; } _owners[tokenId] = to; @@ -311,6 +315,8 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _approve(address(0), tokenId); unchecked { + // Cannot overflow, as that would require more tokens to be burned/transfered + // out then the owner initialy received through minting and transfering in. _balances[owner] -= 1; } delete _owners[tokenId]; @@ -345,6 +351,11 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _approve(address(0), tokenId); 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; } From ffae7c21256b05a780c2e8e204beaa4b86a58e2a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 6 Jul 2022 10:16:12 +0200 Subject: [PATCH 3/7] fix lint --- contracts/token/ERC721/ERC721.sol | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index bd3762b9c5a..7d43e4da863 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -283,10 +283,10 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(address(0), to, tokenId); 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 allow batch minting. - // The ERC fails to describe this case. + // 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 allow batch minting. + // The ERC fails to describe this case. _balances[to] += 1; } _owners[tokenId] = to; @@ -315,8 +315,8 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _approve(address(0), tokenId); unchecked { - // Cannot overflow, as that would require more tokens to be burned/transfered - // out then the owner initialy received through minting and transfering in. + // Cannot overflow, as that would require more tokens to be burned/transfered + // out then the owner initialy received through minting and transfering in. _balances[owner] -= 1; } delete _owners[tokenId]; @@ -351,11 +351,11 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _approve(address(0), tokenId); 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]` 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; } From 5db7f762318718691e22bca9cc9c921d0839e83e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 7 Jul 2022 22:32:12 +0200 Subject: [PATCH 4/7] Apply suggestions from code review Co-authored-by: Francisco --- contracts/token/ERC721/ERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 7d43e4da863..a86a0f36157 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -285,7 +285,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { 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 allow batch minting. + // this ever happens. Might change if we allow batch minting. // The ERC fails to describe this case. _balances[to] += 1; } From 073bb8fe0a9683650e071263909612e6bc0f5b15 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 22 Aug 2022 23:28:18 -0300 Subject: [PATCH 5/7] fix typo transfered -> transferred --- contracts/token/ERC721/ERC721.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 528c243d50c..bcdc6146472 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -316,14 +316,14 @@ 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]; unchecked { - // Cannot overflow, as that would require more tokens to be burned/transfered + // Cannot overflow, as that would require more tokens to be burned/transferred // out then the owner initialy received through minting and transfering in. _balances[owner] -= 1; } From d1004592549c354bb46e9c3bf28bc120fddab671 Mon Sep 17 00:00:00 2001 From: Francisco Date: Mon, 22 Aug 2022 23:29:24 -0300 Subject: [PATCH 6/7] fix typos --- contracts/token/ERC721/ERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index bcdc6146472..2932ab4a943 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -324,7 +324,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { unchecked { // Cannot overflow, as that would require more tokens to be burned/transferred - // out then the owner initialy received through minting and transfering in. + // out than the owner initialy received through minting and transferring in. _balances[owner] -= 1; } delete _owners[tokenId]; From 96f68577dd1f18ca4241f1cc964843a6f6e1a6cd Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 22 Aug 2022 23:38:33 -0300 Subject: [PATCH 7/7] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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))