Restrict ERC721Wrapper wrap by direct transfer (#4043)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

contracts/token/ERC721/extensions/ERC721Wrapper.sol

@@ -17,10 +17,6 @@ import "../utils/ERC721Holder.sol";
 abstract contract ERC721Wrapper is ERC721, ERC721Holder {
     IERC721 private immutable _underlying;
 
-    // Kept as bytes12 so it can be packed with an address
-    // Equal to 0xb125e89df18e2ceac5fd2fa8
-    bytes12 public constant WRAPPER_ACCEPT_MAGIC = bytes12(keccak256("WRAPPER_ACCEPT_MAGIC"));
-
     constructor(IERC721 underlyingToken) {
         _underlying = underlyingToken;
     }
@@ -29,7 +25,7 @@ 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(WRAPPER_ACCEPT_MAGIC, account);
+        bytes memory data = abi.encodePacked(account);
 
         uint256 length = tokenIds.length;
         for (uint256 i = 0; i < length; ++i) {
@@ -61,23 +57,22 @@ abstract contract ERC721Wrapper is ERC721, ERC721Holder {
      * @dev Overrides {IERC721Receiver-onERC721Received} to allow minting on direct ERC721 transfers to
      * this contract.
      *
-     * In case there's data attached, it validates that the sender is aware of this contract's existence and behavior
+     * In case there's data attached, it validates that the operator is this contract, so only trusted data
-     * by checking a magic value (`WRAPPER_ACCEPT_MAGIC`) in the first 12 bytes. If it also matches, the rest 20
+     * is accepted from {depositFor}.
-     * bytes are used as an address to send the tokens to.
      *
      * WARNING: Doesn't work with unsafe transfers (eg. {IERC721-transferFrom}). Use {ERC721Wrapper-_recover}
      * for recovering in that scenario.
      */
     function onERC721Received(
-        address,
+        address operator,
         address from,
         uint256 tokenId,
         bytes memory data
     ) public override returns (bytes4) {
         require(address(underlying()) == _msgSender(), "ERC721Wrapper: caller is not underlying");
         if (data.length > 0) {
-            require(data.length == 32 && WRAPPER_ACCEPT_MAGIC == bytes12(data), "ERC721Wrapper: Invalid data format");
+            require(data.length == 20 && operator == address(this), "ERC721Wrapper: Invalid data format");
-            from = address(bytes20(bytes32(data) << 96));
+            from = address(bytes20(data));
         }
         _safeMint(from, tokenId);
         return IERC721Receiver.onERC721Received.selector;

test/token/ERC721/extensions/ERC721Wrapper.test.js

@@ -1,6 +1,5 @@
 const { BN, expectEvent, constants, expectRevert } = require('@openzeppelin/test-helpers');
 const { expect } = require('chai');
-const { keccakFromString, bufferToHex } = require('ethereumjs-util');
 
 const { shouldBehaveLikeERC721 } = require('../ERC721.behavior');
 
@@ -230,27 +229,13 @@ contract('ERC721Wrapper', function (accounts) {
   });
 
   describe('onERC721Received', function () {
-    const WRAPPER_ACCEPT_MAGIC = bufferToHex(keccakFromString('WRAPPER_ACCEPT_MAGIC')).slice(0, 26); // Include 0x
-
-    const magicWithAddresss = address =>
-      web3.utils.encodePacked(
-        {
-          value: WRAPPER_ACCEPT_MAGIC,
-          type: 'bytes12',
-        },
-        {
-          value: address,
-          type: 'address',
-        },
-      );
-
     it('only allows calls from underlying', async function () {
       await expectRevert(
         this.token.onERC721Received(
           initialHolder,
           this.token.address,
           firstTokenId,
-          magicWithAddresss(anotherAccount), // Correct data
+          anotherAccount, // Correct data
           { from: anotherAccount },
         ),
         'ERC721Wrapper: caller is not underlying',
@@ -273,13 +258,13 @@ contract('ERC721Wrapper', function (accounts) {
         );
       });
 
-      it('reverts with the magic value and data length different to 32', async function () {
+      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,
-            WRAPPER_ACCEPT_MAGIC, // Reverts for any non-32 bytes value
+            anotherAccount,
             {
               from: initialHolder,
             },
@@ -287,24 +272,6 @@ contract('ERC721Wrapper', function (accounts) {
           '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 () {