From 2c711d0b0513d283b27a3b28df06d558a19c3aaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 14 Feb 2023 14:47:07 -0600 Subject: [PATCH] Restrict ERC721Wrapper wrap by direct transfer (#4043) Co-authored-by: Hadrien Croubois --- .../token/ERC721/extensions/ERC721Wrapper.sol | 17 +++----- .../ERC721/extensions/ERC721Wrapper.test.js | 39 ++----------------- 2 files changed, 9 insertions(+), 47 deletions(-) diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index 87729188a..f10d55564 100644 --- a/contracts/token/ERC721/extensions/ERC721Wrapper.sol +++ b/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 - * by checking a magic value (`WRAPPER_ACCEPT_MAGIC`) in the first 12 bytes. If it also matches, the rest 20 - * bytes are used as an address to send the tokens to. + * In case there's data attached, it validates that the operator is this contract, so only trusted data + * is accepted from {depositFor}. * * 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"); - from = address(bytes20(bytes32(data) << 96)); + 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 index def6ca7ee..0558dfa37 100644 --- a/test/token/ERC721/extensions/ERC721Wrapper.test.js +++ b/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 () {