diff --git a/contracts/token/ERC721/extensions/ERC721Wrapper.sol b/contracts/token/ERC721/extensions/ERC721Wrapper.sol index f10d555645c..83e59ce88a6 100644 --- 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 index 0558dfa37cc..6e46d2e5ab3 100644 --- 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, });