Simplify ERC721Wrapper.depositFor to save gas (#4048)

OpenZeppelin · Feb 17, 2023 · d5d9d4b · d5d9d4b
1 parent 5e76b26
commit d5d9d4b
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 45 deletions.
diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol
@@ -3,7 +3,6 @@
 pragma solidity ^0.8.0;
 
 import "../ERC721.sol";
-import "../utils/ERC721Holder.sol";
 
 /**
  * @dev Extension of the ERC721 token contract to support token wrapping.
@@ -14,7 +13,7 @@ import "../utils/ERC721Holder.sol";
  *
  * _Available since v4.9.0_
  */
-abstract contract ERC721Wrapper is ERC721, ERC721Holder {
+abstract contract ERC721Wrapper is ERC721, IERC721Receiver {
     IERC721 private immutable _underlying;
 
     constructor(IERC721 underlyingToken) {
@@ -25,11 +24,15 @@ abstract contract ERC721Wrapper is ERC721, ERC721Holder {
      * @dev Allow a user to deposit underlying tokens and mint the corresponding tokenIds.
      */
     function depositFor(address account, uint256[] memory tokenIds) public virtual returns (bool) {
-        bytes memory data = abi.encodePacked(account);
-
         uint256 length = tokenIds.length;
         for (uint256 i = 0; i < length; ++i) {
-            underlying().safeTransferFrom(_msgSender(), address(this), tokenIds[i], data);
+            uint256 tokenId = tokenIds[i];
+
+            // This is an "unsafe" transfer that doesn't call any hook on the receiver. With underlying() being trusted
+            // (by design of this contract) and no other contracts expected to be called from there, we are safe.
+            // slither-disable-next-line reentrancy-no-eth
+            underlying().transferFrom(_msgSender(), address(this), tokenId);
+            _safeMint(account, tokenId);
         }
 
         return true;
@@ -64,16 +67,12 @@ abstract contract ERC721Wrapper is ERC721, ERC721Holder {
      * for recovering in that scenario.
      */
     function onERC721Received(
-        address operator,
+        address,
         address from,
         uint256 tokenId,
-        bytes memory data
-    ) public override returns (bytes4) {
+        bytes memory
+    ) public virtual override returns (bytes4) {
         require(address(underlying()) == _msgSender(), "ERC721Wrapper: caller is not underlying");
-        if (data.length > 0) {
-            require(data.length == 20 && operator == address(this), "ERC721Wrapper: Invalid data format");
-            from = address(bytes20(data));
-        }
         _safeMint(from, tokenId);
         return IERC721Receiver.onERC721Received.selector;
     }

diff --git a/test/token/ERC721/extensions/ERC721Wrapper.test.js b/test/token/ERC721/extensions/ERC721Wrapper.test.js
@@ -242,39 +242,7 @@ contract('ERC721Wrapper', function (accounts) {
       );
     });
 
-    describe('when data length is > 0', function () {
-      it('reverts with arbitrary data', async function () {
-        await expectRevert(
-          this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
-            initialHolder,
-            this.token.address,
-            firstTokenId,
-            '0x0123',
-            {
-              from: initialHolder,
-            },
-          ),
-          'ERC721Wrapper: Invalid data format',
-        );
-      });
-
-      it('reverts with correct data from an untrusted operator', async function () {
-        await expectRevert(
-          this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
-            initialHolder,
-            this.token.address,
-            firstTokenId,
-            anotherAccount,
-            {
-              from: initialHolder,
-            },
-          ),
-          'ERC721Wrapper: Invalid data format',
-        );
-      });
-    });
-
-    it('mints a token to from if no data is specified', async function () {
+    it('mints a token to from', async function () {
       const { tx } = await this.underlying.safeTransferFrom(initialHolder, this.token.address, firstTokenId, {
         from: initialHolder,
       });