From 6c3ea964e08ade02c3608ac598c475ddf8d260e0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 11 Aug 2022 17:39:28 +0200 Subject: [PATCH 1/9] Change execution order to avoid reentry through the beforeTokenTransfer hook --- contracts/token/ERC721/ERC721.sol | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index e33d2c79459..756268d4d5a 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -277,11 +277,11 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Emits a {Transfer} event. */ function _mint(address to, uint256 tokenId) internal virtual { + _beforeTokenTransfer(address(0), to, tokenId); + require(to != address(0), "ERC721: mint to the zero address"); require(!_exists(tokenId), "ERC721: token already minted"); - _beforeTokenTransfer(address(0), to, tokenId); - _balances[to] += 1; _owners[tokenId] = to; @@ -302,19 +302,21 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Emits a {Transfer} event. */ function _burn(uint256 tokenId) internal virtual { - address owner = ERC721.ownerOf(tokenId); + address from = ERC721.ownerOf(tokenId); + + _beforeTokenTransfer(from, address(0), tokenId); - _beforeTokenTransfer(owner, address(0), tokenId); + require(from == ERC721.ownerOf(tokenId), "ERC721: burn from incorrect owner"); // Clear approvals delete _tokenApprovals[tokenId]; - _balances[owner] -= 1; + _balances[from] -= 1; delete _owners[tokenId]; - emit Transfer(owner, address(0), tokenId); + emit Transfer(from, address(0), tokenId); - _afterTokenTransfer(owner, address(0), tokenId); + _afterTokenTransfer(from, address(0), tokenId); } /** @@ -333,11 +335,11 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { address to, uint256 tokenId ) internal virtual { - require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner"); - require(to != address(0), "ERC721: transfer to the zero address"); - _beforeTokenTransfer(from, to, tokenId); + require(from == ERC721.ownerOf(tokenId), "ERC721: transfer from incorrect owner"); + require(to != address(0), "ERC721: transfer to the zero address"); + // Clear approvals from the previous owner delete _tokenApprovals[tokenId]; From 52058b277c3cf507660dc850e7e14975f1fd5b86 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 11 Aug 2022 17:52:54 +0200 Subject: [PATCH 2/9] Add changelog entry --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5828a7b2193..b4963ba5d68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,15 +5,16 @@ * `ERC165Checker`: add `supportsERC165InterfaceUnchecked` for consulting individual interfaces without the full ERC165 protocol. ([#3339](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3339)) * `Address`: optimize `functionCall` by calling `functionCallWithValue` directly. ([#3468](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3468)) * `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) - * `GovernorCompatibilityBravo`: remove unused `using` statements ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506)) + * `GovernorCompatibilityBravo`: remove unused `using` statements. ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506)) * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) * `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`: Change execution order to avoid reentry through the `_beforeTokenTransfer` hook. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611)) * `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)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) - + ### Compatibility Note ERC-721 integrators that interpret contract state from events should make sure that they implement the clearing of approval that is implicit in every transfer according to the EIP. Previous versions of OpenZeppellin Contracts emitted an explicit `Approval` event even though it was not required by the specification, and this is no longer the case. From 2c5204a4d040ec7fd5f8b791a67fb6c78b44eb38 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 11 Aug 2022 19:00:24 +0200 Subject: [PATCH 3/9] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4963ba5d68..dc2ef5e3bf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) - + ### Compatibility Note ERC-721 integrators that interpret contract state from events should make sure that they implement the clearing of approval that is implicit in every transfer according to the EIP. Previous versions of OpenZeppellin Contracts emitted an explicit `Approval` event even though it was not required by the specification, and this is no longer the case. From 24996edbca67b87d0fb90b92c01db5635a59ab1a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Aug 2022 12:55:42 +0200 Subject: [PATCH 4/9] Update CHANGELOG.md Co-authored-by: Francisco --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc2ef5e3bf7..4e4292ec42b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) * `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`: Change execution order to avoid reentry through the `_beforeTokenTransfer` hook. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611)) + * `ERC721`: Change execution order to preserve correct balances with custom `_beforeTokenTransfer` hooks. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611)) * `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)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) From abae8ca620957ce672abf7c381ac09f832671535 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Aug 2022 22:11:45 +0200 Subject: [PATCH 5/9] Update ERC721.sol --- 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 756268d4d5a..88d607aff0d 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -306,7 +306,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(from, address(0), tokenId); - require(from == ERC721.ownerOf(tokenId), "ERC721: burn from incorrect owner"); + from = ERC721.ownerOf(tokenId); // Clear approvals delete _tokenApprovals[tokenId]; From 41f343b8bd31431300f8c8723ebe510f8ce3f80a Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 17 Aug 2022 20:37:49 -0300 Subject: [PATCH 6/9] add comment --- contracts/token/ERC721/ERC721.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 88d607aff0d..bff59f0f0fe 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -306,6 +306,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(from, address(0), tokenId); + // The token may have been transferred in the before hook so we have to re-read the current owner. from = ERC721.ownerOf(tokenId); // Clear approvals From 90f3c16ff38ca546cd5f749a659dc34bbced25a5 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 17 Aug 2022 20:52:01 -0300 Subject: [PATCH 7/9] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e4292ec42b..145c1a67e26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) * `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`: Change execution order to preserve correct balances with custom `_beforeTokenTransfer` hooks. ([#3611](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3611)) + * `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)) * `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)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) From ec24469d858e242825862272275964ce79d20db6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Aug 2022 14:45:10 +0200 Subject: [PATCH 8/9] Apply strict check-effect --- contracts/token/ERC721/ERC721.sol | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index bff59f0f0fe..3f88d11472b 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -277,9 +277,12 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Emits a {Transfer} event. */ function _mint(address to, uint256 tokenId) internal virtual { + require(!_exists(tokenId), "ERC721: token already minted"); + require(to != address(0), "ERC721: mint to the zero address"); + _beforeTokenTransfer(address(0), to, tokenId); - require(to != address(0), "ERC721: mint to the zero address"); + // Check that tokenId was not minted by `_beforeTokenTransfer` hook require(!_exists(tokenId), "ERC721: token already minted"); _balances[to] += 1; @@ -306,7 +309,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { _beforeTokenTransfer(from, address(0), tokenId); - // The token may have been transferred in the before hook so we have to re-read the current owner. + // Update ownership in case tokenId was transfered by `_beforeTokenTransfer` hook from = ERC721.ownerOf(tokenId); // Clear approvals @@ -336,10 +339,13 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { address to, uint256 tokenId ) internal virtual { + require(from == ERC721.ownerOf(tokenId), "ERC721: transfer from incorrect owner"); + require(to != address(0), "ERC721: transfer to the zero address"); + _beforeTokenTransfer(from, to, tokenId); + // Check that tokenId was not transfered by `_beforeTokenTransfer` hook require(from == ERC721.ownerOf(tokenId), "ERC721: transfer from incorrect owner"); - require(to != address(0), "ERC721: transfer to the zero address"); // Clear approvals from the previous owner delete _tokenApprovals[tokenId]; From b8fc567f01d22228fe70b455a12fc2b6210de377 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Aug 2022 14:50:33 +0200 Subject: [PATCH 9/9] minimize diff --- contracts/token/ERC721/ERC721.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 3f88d11472b..3c457801e49 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -277,8 +277,8 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Emits a {Transfer} event. */ function _mint(address to, uint256 tokenId) internal virtual { - require(!_exists(tokenId), "ERC721: token already minted"); require(to != address(0), "ERC721: mint to the zero address"); + require(!_exists(tokenId), "ERC721: token already minted"); _beforeTokenTransfer(address(0), to, tokenId); @@ -305,22 +305,22 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * Emits a {Transfer} event. */ function _burn(uint256 tokenId) internal virtual { - address from = ERC721.ownerOf(tokenId); + address owner = ERC721.ownerOf(tokenId); - _beforeTokenTransfer(from, address(0), tokenId); + _beforeTokenTransfer(owner, address(0), tokenId); // Update ownership in case tokenId was transfered by `_beforeTokenTransfer` hook - from = ERC721.ownerOf(tokenId); + owner = ERC721.ownerOf(tokenId); // Clear approvals delete _tokenApprovals[tokenId]; - _balances[from] -= 1; + _balances[owner] -= 1; delete _owners[tokenId]; - emit Transfer(from, address(0), tokenId); + emit Transfer(owner, address(0), tokenId); - _afterTokenTransfer(from, address(0), tokenId); + _afterTokenTransfer(owner, address(0), tokenId); } /** @@ -339,13 +339,13 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { address to, uint256 tokenId ) internal virtual { - require(from == ERC721.ownerOf(tokenId), "ERC721: transfer from incorrect owner"); + require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner"); require(to != address(0), "ERC721: transfer to the zero address"); _beforeTokenTransfer(from, to, tokenId); // Check that tokenId was not transfered by `_beforeTokenTransfer` hook - require(from == ERC721.ownerOf(tokenId), "ERC721: transfer from incorrect owner"); + require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner"); // Clear approvals from the previous owner delete _tokenApprovals[tokenId];