ERC721Wrapper uses Receiver

audits/2023_04_Wrappers_Ignacio_Mazzara.md

@@ -32,9 +32,9 @@ The rest of the contracts in the repositories are assumed to be audited.
 
 ## General Notes
 
-The _ERC1155FloorWrapper_ wraps and unwraps tokens using the `onERC1155Received` and `onERC1155BatchReceived` functions, but the _ERC721FloorWrapper_ uses specific `deposit` and `withdraw` functions with no support to `onERC721Received`. Using different strategies for the same outcome makes the contract harder to follow and costly for the users in the case of the _ERC721FloorWrapper_. They need an extra transaction to `approve` each token or an `approvalForAll` before depositing. Consider using the same strategy for both wrappers.
+The _ERC1155FloorWrapper_ wraps and unwraps tokens using the `onERC1155Received` and `onERC1155BatchReceived` functions, but the _ERC721FloorWrapper_ uses specific `deposit` and `withdraw` functions with no support to `onERC721Received`. Using different strategies for the same outcome makes the contract harder to follow and costly for the users in the case of the _ERC721FloorWrapper_. They need an extra transaction to `approve` each token or an `approvalForAll` before depositing. Consider using the same strategy for both wrappers. **ADDRESSED: Updated ERC721FloorWrapper to use receiver functions. Have kept deposit to enable multiple deposits in a single transaction.**
 
-Another thing to have in mind is that the users can claim unwrapped tokens if someone, by mistake, doesn't use the wrappers as expected. This has been addressed before the audit started[here by the team](https://github.com/0xsequence/niftyswap/pull/79#pullrequestreview-1362003788)
+Another thing to have in mind is that the users can claim unwrapped tokens if someone, by mistake, doesn't use the wrappers as expected. This has been addressed before the audit started[here by the team](https://github.com/0xsequence/niftyswap/pull/79#pullrequestreview-1362003788). **IGNORED: Intentional, as stated.**
 
 ## Interfaces
 

src/contracts/interfaces/IERC721FloorWrapper.sol

@@ -2,27 +2,79 @@
 pragma solidity ^0.8.4;
 
 import {IERC1155} from "@0xsequence/erc-1155/contracts/interfaces/IERC1155.sol";
+import {IERC1155TokenReceiver} from "@0xsequence/erc-1155/contracts/interfaces/IERC1155TokenReceiver.sol";
+import {IERC721Receiver} from "./IERC721Receiver.sol";
 
-interface IERC721FloorWrapper is IERC1155 {
+interface IERC721FloorWrapper is IERC1155, IERC1155TokenReceiver, IERC721Receiver {
     event TokensDeposited(uint256[] tokenIds);
 
     event TokensWithdrawn(uint256[] tokenIds);
 
+    struct DepositRequestObj {
+        address recipient;
+        bytes data;
+    }
+
+    struct WithdrawRequestObj {
+        uint256[] tokenIds;
+        address recipient;
+        bytes data;
+    }
+
+    /**
+     * Accepts ERC-721 tokens to wrap.
+     * @param _operator The address which called `safeTransferFrom` function.
+     * @param _from The address which previously owned the token.
+     * @param _tokenId The ID of the token being transferred.
+     * @param _data Additional data formatted as DepositRequestObj.
+     * @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
+     * @notice This is the preferred method for wrapping a single token.
+     */
+    function onERC721Received(address _operator, address _from, uint256 _tokenId, bytes calldata _data)
+        external
+        returns (bytes4);
+
     /**
      * Deposit and wrap ERC-721 tokens.
      * @param _tokenIds The ERC-721 token ids to deposit.
      * @param _recipient The recipient of the wrapped tokens.
      * @param _data Data to pass to ERC-1155 receiver.
      * @notice Users must first approve this contract address on the ERC-721 contract.
-     * @dev This contract intentionally does not support IERC721Receiver for gas optimisations.
+     * @notice This function can wrap multiple ERC-721 tokens at once.
      */
     function deposit(uint256[] calldata _tokenIds, address _recipient, bytes calldata _data) external;
 
     /**
-     * Unwrap and withdraw ERC-721 tokens.
-     * @param _tokenIds The ERC-721 token ids to withdraw.
-     * @param _recipient The recipient of the unwrapped tokens.
-     * @param _data Data to pass to ERC-1155 receiver.
+     * Accepts wrapped ERC-1155 tokens to unwrap.
+     * @param _operator The address which called `safeTransferFrom` function.
+     * @param _from The address which previously owned the token.
+     * @param _tokenId The ID of the token being transferred.
+     * @param _amount The amount of tokens being transferred.
+     * @param _data Additional data formatted as WithdrawRequestObj.
+     * @return `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)`
+     */
+    function onERC1155Received(
+        address _operator,
+        address _from,
+        uint256 _tokenId,
+        uint256 _amount,
+        bytes calldata _data
+    ) external returns (bytes4);
+
+    /**
+     * Accepts wrapped ERC-1155 tokens to unwrap.
+     * @param _operator The address which called `safeTransferFrom` function.
+     * @param _from The address which previously owned the token.
+     * @param _tokenIds The IDs of the tokens being transferred.
+     * @param _amounts The amounts of tokens being transferred.
+     * @param _data Additional data formatted as WithdrawRequestObj.
+     * @return `bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`
      */
-    function withdraw(uint256[] calldata _tokenIds, address _recipient, bytes calldata _data) external;
+    function onERC1155BatchReceived(
+        address _operator,
+        address _from,
+        uint256[] calldata _tokenIds,
+        uint256[] calldata _amounts,
+        bytes calldata _data
+    ) external returns (bytes4);
 }

src/contracts/interfaces/IERC721Receiver.sol

@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.4;
+
+interface IERC721Receiver {
+    function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
+        external
+        returns (bytes4);
+}

src/contracts/utils/WrapperErrors.sol

@@ -8,7 +8,8 @@ abstract contract WrapperErrors {
     // Factories
     error WrapperCreationFailed(address tokenAddr);
 
-    // ERC1155
+    // Wrappers
+    error InvalidERC721Received();
     error InvalidERC1155Received();
     error InvalidDepositRequest();
     error InvalidWithdrawRequest();

src/contracts/wrappers/ERC1155FloorWrapper.sol

@@ -47,6 +47,10 @@ contract ERC1155FloorWrapper is IERC1155FloorWrapper, ERC1155MintBurn, IERC1155M
         revert UnsupportedMethod();
     }
 
+    //
+    // Tokens
+    //
+
     /**
      * Accepts ERC-1155 tokens to wrap and wrapped ERC-1155 tokens to unwrap.
      * @param _id The ID of the token being transferred.
@@ -168,6 +172,10 @@ contract ERC1155FloorWrapper is IERC1155FloorWrapper, ERC1155MintBurn, IERC1155M
         emit TokensWithdrawn(obj.tokenIds, obj.tokenAmounts);
     }
 
+    //
+    // Views
+    //
+
     /**
      * A distinct Uniform Resource Identifier (URI) for a given token.
      * @param _id The token id.

src/contracts/wrappers/ERC721FloorWrapper.sol

@@ -11,7 +11,7 @@ import {ERC1155MintBurn} from "@0xsequence/erc-1155/contracts/tokens/ERC1155/ERC
 import {WrapperErrors} from "../utils/WrapperErrors.sol";
 
 import {IERC721} from "../interfaces/IERC721.sol";
-import {IERC721FloorWrapper} from "../interfaces/IERC721FloorWrapper.sol";
+import {IERC721FloorWrapper, IERC1155TokenReceiver, IERC721Receiver} from "../interfaces/IERC721FloorWrapper.sol";
 
 /**
  * @notice Allows users to wrap any amount of a ERC-721 token with a 1:1 ratio.
@@ -51,15 +51,43 @@ contract ERC721FloorWrapper is IERC721FloorWrapper, ERC1155MintBurn, IERC1155Met
     }
 
     //
-    // Tokens
+    // Deposit
     //
 
+    /**
+     * Accepts ERC-721 tokens to wrap.
+     * @param _tokenId The ID of the token being transferred.
+     * @param _data Additional data formatted as DepositRequestObj.
+     * @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
+     * @notice This is the preferred method for wrapping a single token.
+     */
+    function onERC721Received(address, address, uint256 _tokenId, bytes calldata _data) external returns (bytes4) {
+        if (msg.sender != address(token)) {
+            revert InvalidERC721Received();
+        }
+        DepositRequestObj memory obj;
+        (obj) = abi.decode(_data, (DepositRequestObj));
+        if (obj.recipient == address(0)) {
+            // Don't allow deposits to the zero address
+            revert InvalidWithdrawRequest();
+        }
+
+        uint256[] memory tokenIds = new uint256[](1);
+        tokenIds[0] = _tokenId;
+        emit TokensDeposited(tokenIds);
+
+        _mint(obj.recipient, TOKEN_ID, 1, obj.data);
+
+        return IERC721Receiver.onERC721Received.selector;
+    }
+
     /**
      * Deposit and wrap ERC-721 tokens.
      * @param _tokenIds The ERC-721 token ids to deposit.
      * @param _recipient The recipient of the wrapped tokens.
      * @param _data Data to pass to ERC-1155 receiver.
      * @notice Users must first approve this contract address on the ERC-721 contract.
+     * @notice This function can wrap multiple ERC-721 tokens at once.
      */
     function deposit(uint256[] calldata _tokenIds, address _recipient, bytes calldata _data) external {
         if (_recipient == address(0)) {
@@ -79,22 +107,78 @@ contract ERC721FloorWrapper is IERC721FloorWrapper, ERC1155MintBurn, IERC1155Met
         _mint(_recipient, TOKEN_ID, length, _data);
     }
 
+    //
+    // Withdraw
+    //
+
+    /**
+     * Accepts wrapped ERC-1155 tokens to unwrap.
+     * @param _amount The amount of tokens being transferred.
+     * @param _data Additional data with no specified format.
+     * @return `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)`
+     */
+    function onERC1155Received(address, address, uint256, uint256 _amount, bytes calldata _data)
+        external
+        returns (bytes4)
+    {
+        if (msg.sender != address(this)) {
+            revert InvalidERC1155Received();
+        }
+        _withdraw(_amount, _data);
+
+        return IERC1155TokenReceiver.onERC1155Received.selector;
+    }
+
+    /**
+     * Accepts wrapped ERC-1155 tokens to unwrap.
+     * @param _ids The IDs of the tokens being transferred.
+     * @param _amounts The amounts of tokens being transferred.
+     * @param _data Additional data formatted as WithdrawRequestObj.
+     * @return `bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`
+     */
+    function onERC1155BatchReceived(
+        address,
+        address,
+        uint256[] calldata _ids,
+        uint256[] calldata _amounts,
+        bytes calldata _data
+    ) public returns (bytes4) {
+        if (msg.sender != address(this)) {
+            revert InvalidERC1155Received();
+        }
+
+        if (_ids.length != 1) {
+            revert InvalidERC1155Received();
+        }
+        // Either ids[0] == TOKEN_ID or amounts[0] == 0
+        _withdraw(_amounts[0], _data);
+
+        return IERC1155TokenReceiver.onERC1155BatchReceived.selector;
+    }
+
     /**
      * Unwrap and withdraw ERC-721 tokens.
-     * @param _tokenIds The ERC-721 token ids to withdraw.
-     * @param _recipient The recipient of the unwrapped tokens.
-     * @param _data Data to pass to ERC-721 receiver.
+     * @param _amount The amount of ERC-1155 tokens recieved.
+     * @param _data Additional data formatted as WithdrawRequestObj.
      */
-    function withdraw(uint256[] calldata _tokenIds, address _recipient, bytes calldata _data) external {
-        if (_recipient == address(0)) {
+    function _withdraw(uint256 _amount, bytes calldata _data) private {
+        WithdrawRequestObj memory obj;
+        (obj) = abi.decode(_data, (WithdrawRequestObj));
+        if (obj.recipient == address(0)) {
+            // Don't allow deposits to the zero address
+            revert InvalidWithdrawRequest();
+        }
+
+        uint256 length = obj.tokenIds.length;
+        if (_amount != length) {
+            // The amount of tokens received must match the amount of tokens being withdrawn
             revert InvalidWithdrawRequest();
         }
 
-        uint256 length = _tokenIds.length;
         _burn(msg.sender, TOKEN_ID, length);
-        emit TokensWithdrawn(_tokenIds);
+        emit TokensWithdrawn(obj.tokenIds);
         for (uint256 i; i < length;) {
-            token.safeTransferFrom(address(this), _recipient, _tokenIds[i], _data);
+            token.safeTransferFrom(address(this), obj.recipient, obj.tokenIds[i], obj.data);
             unchecked {
                 // Can never overflow
                 i++;
@@ -103,7 +187,7 @@ contract ERC721FloorWrapper is IERC721FloorWrapper, ERC1155MintBurn, IERC1155Met
     }
 
     //
-    // Metadata
+    // Views
     //
 
     /**

tests_foundry/ERC1155FloorWrapper.test.sol

@@ -278,6 +278,10 @@ contract ERC1155FloorWrapperTest is TestHelperBase, WrapperErrors {
         erc1155.safeBatchTransferFrom(USER, wrapperAddr, tokenIds, tokenAmounts, encodeDepositRequest(address(0), ""));
     }
 
+    //
+    // Deposit with Batch Receiver
+    //
+
     //
     // Withdraw
     //
@@ -456,6 +460,10 @@ contract ERC1155FloorWrapperTest is TestHelperBase, WrapperErrors {
         );
     }
 
+    //
+    // Withdraw with Batch Receiver
+    //
+
     //
     // Transfers
     //