Make onERC721Received more strict with the magic value

OpenZeppelin · Feb 8, 2023 · b49c187 · hixbex7 · Feb 9, 2023 · b49c187
1 parent 92d650b
commit b49c187
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 38 deletions.
diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol
@@ -75,7 +75,8 @@ abstract contract ERC721Wrapper is ERC721, ERC721Holder {
         bytes memory data
     ) public override returns (bytes4) {
         require(address(underlying()) == _msgSender(), "ERC721Wrapper: caller is not underlying");
-        if (data.length == 32 && WRAPPER_ACCEPT_MAGIC == bytes12(data)) {
+        if (data.length > 0) {
+            require(data.length == 32 && WRAPPER_ACCEPT_MAGIC == bytes12(data), "ERC721Wrapper: Invalid data format");
             from = address(bytes20(bytes32(data) << 96));
         }
         _safeMint(from, tokenId);

diff --git a/test/token/ERC721/extensions/ERC721Wrapper.test.js b/test/token/ERC721/extensions/ERC721Wrapper.test.js
@@ -234,43 +234,6 @@ contract('ERC721Wrapper', function (accounts) {
         },
       );
 
-    it('mints a token to sender with arbitrary data', async function () {
-      // `safeTransferFrom` guarantees `onERC721Received` check
-      const { tx } = await this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
-        initialHolder,
-        this.token.address,
-        firstTokenId,
-        '0x0123',
-        {
-          from: initialHolder,
-        },
-      );
-
-      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
-        from: constants.ZERO_ADDRESS,
-        to: initialHolder,
-        tokenId: firstTokenId,
-      });
-    });
-
-    it('mints token to specific holder with address after magic value', async function () {
-      const { tx } = await this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
-        initialHolder,
-        this.token.address,
-        firstTokenId,
-        magicWithAddresss(anotherAccount),
-        {
-          from: initialHolder,
-        },
-      );
-
-      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
-        from: constants.ZERO_ADDRESS,
-        to: anotherAccount,
-        tokenId: firstTokenId,
-      });
-    });
-
     it('only allows calls from underlying', async function () {
       await expectRevert(
         this.token.onERC721Received(
@@ -283,6 +246,68 @@ contract('ERC721Wrapper', function (accounts) {
         'ERC721Wrapper: caller is not underlying',
       );
     });
+
+    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 the magic value and data length different to 32', async function () {
+        await expectRevert(
+          this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
+            initialHolder,
+            this.token.address,
+            firstTokenId,
+            WRAPPER_ACCEPT_MAGIC, // Reverts for any non-32 bytes value
+            {
+              from: initialHolder,
+            },
+          ),
+          'ERC721Wrapper: Invalid data format',
+        );
+      });
+
+      it('mints token to specific holder with address after magic value', async function () {
+        const { tx } = await this.underlying.methods['safeTransferFrom(address,address,uint256,bytes)'](
+          initialHolder,
+          this.token.address,
+          firstTokenId,
+          magicWithAddresss(anotherAccount),
+          {
+            from: initialHolder,
+          },
+        );
+
+        await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+          from: constants.ZERO_ADDRESS,
+          to: anotherAccount,
+          tokenId: firstTokenId,
+        });
+      });
+    });
+
+    it('mints a token to from if no data is specified', async function () {
+      const { tx } = await this.underlying.safeTransferFrom(initialHolder, this.token.address, firstTokenId, {
+        from: initialHolder,
+      });
+
+      await expectEvent.inTransaction(tx, this.token, 'Transfer', {
+        from: constants.ZERO_ADDRESS,
+        to: initialHolder,
+        tokenId: firstTokenId,
+      });
+    });
   });
 
   describe('_recover', function () {