ERC1155 and ERC721 Floor Wrappers

.github/workflows/ci.yml

@@ -131,28 +131,6 @@ jobs:
       - name: Run tests
         run: FOUNDRY_FUZZ_RUNS=1024 forge test -vvv
 
-  build:
-    name: Contracts built
-    runs-on: ubuntu-latest
-    needs: [install]
-    steps:
-      - uses: actions/checkout@v3
-        with:
-          submodules: recursive
-      - uses: actions/setup-node@v1
-        with:
-          node-version: 18
-      - uses: actions/cache@master
-        id: yarn-cache
-        with:
-          path: |
-            node_modules
-            */*/node_modules
-          key: ${{ runner.os }}-lerna-${{ hashFiles('**/package.json', '**/yarn.lock') }}
-      - run: yarn build
-      - run: yarn format:ts
-      - run: git diff --exit-code
-
   # coverage:
   #   name: Coverage
   #   runs-on: ubuntu-latest

audits/2023_04_Wrappers_Ignacio_Mazzara.md

@@ -0,0 +1,169 @@
+# Niftyswap - Security Review
+
+**Responses to comments follow in bold.**
+
+## Table of contest
+
+- [Introduction](#introduction)
+- [General Notes](#general-notes)
+- [Interfaces](#interfaces)
+  - [IERC1155FloorFactory.sol](#IERC1155FloorFactory.sol)
+  - [IERC1155FloorWrapper.sol](#IERC1155FloorWrapper.sol)
+  - [IERC721.sol](#IERC721.sol)
+  - [IERC721FloorFactory.sol](#IERC721FloorFactory.sol)
+  - [IERC721FloorWrapper.sol](#IERC721FloorWrapper.sol)
+  - [IWrapAndNiftyswap.sol](#IWrapAndNiftyswap.sol)
+- [Utils](#utils)
+  - [Proxy.sol](#Proxy.sol)
+  - [WrapperErrors.sol](#WrapperErrors.sol)
+  - [WrapperProxyDeployer.sol](#WrapperProxyDeployer.sol)
+- [Wrappers](#wrappers)
+   - [ERC1155FloorFactory.sol](#ERC1155FloorFactory.sol)
+   - [ERC1155FloorWrapper.sol](#ERC1155FloorWrapper.sol)
+   - [ERC721FloorFactory.sol](#ERC721FloorFactory.sol)
+   - [ERC721FloorWrapper.sol](#ERC721FloorWrapper.sol)
+   - [WrapAndNiftyswap.sol](#WrapAndNiftyswap.sol) -> No
+
+## Introduction
+
+0xsequence team requested the review of the contracts under the repository **[niftyswap](https://github.com/0xsequence/niftyswap)** referenced by the commit [dc937eb9ba17e1d4886826fd0579febbe3ecb3ad](https://github.com/0xsequence/niftyswap/pull/79/commits/c89bf42b9585f0018f81803ec09fd2f628b0c52d). Spot at the [PR #79](https://github.com/0xsequence/niftyswap/pull/79/files#diff-447dff6610565178e797d7be963e0fe871eccb314da6a80fe9f6629a0f28184f) the following contracts: _IERC1155FloorFactory.sol_, _IERC1155FloorWrapper.sol_, _IERC721.sol_, _IERC721FloorFactory.sol_, _IERC721FloorWrapper.sol_, _IWrapAndNiftyswap.sol_, _Proxy.sol_, _WrapperErrors.sol_, _WrapperProxyDeployer.sol_, _ERC1155FloorFactory.sol_, _ERC1155FloorWrapper.sol_, _ERC721FloorFactory.sol_, _ERC721FloorWrapper.sol_, _WrapAndNiftyswap.sol_.
+
+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. **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). **IGNORED: Intentional, as stated.**
+
+## Interfaces
+
+### IERC1155FloorFactory.sol
+
+Nothing found.
+
+### IERC1155FloorWrapper.sol
+
+#### Notes
+
+- N1 - line 32 - Wrong return value in the dev notation. It says `bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))` and it should be bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) **ADDRESSED: Fixed docstring.**
+
+### IERC721.sol
+
+Nothing found.
+
+### IERC721FloorFactory.sol
+
+Nothing found.
+
+### IERC721FloorWrapper.sol
+
+Nothing found.
+
+### IWrapAndNiftyswap
+
+#### Notes
+
+- N1 - lines 12, 18, and 27 - Parameter names start with `_` while other interfaces use another convention. Consider removing the `_` or adding it to other parameter names interfaces. **ADDRESSED: Added _ to new interface function parameters.**
+
+## Utils
+
+### Proxy.sol
+
+### Low
+
+- L1 - line 8 - There is no check whether the implementation is a contract. Consider adding the `isContract` check to prevent human errors. **IGNORED: With CREATE2 it's possible that the proxy may be deployed pointing to a known address before the implementation has been deployed.**
+
+#### Notes
+
+- N1 - line 5 - Consider using the `immutable` keyword for the `implementation` variable since it is not intended to be changed during the contract's lifecycle. This change will reduce gas consumption at deployment and execution. **IGNORED: Assembly access to immutable variables is not supported in solidity 0.8.4.**
+
+### WrapperErrors.sol
+
+Nothing found.
+
+### WrapperProxyDeployer.sol
+
+#### Notes
+
+- N1 - lines 4, 5, 7, 9 - Unused imports. Consider removing them. **ADDRESSED: Removed unused imports.**
+
+- N2 - line 18 - Possibility to create a proxy for the zero address. Consider checking that `tokenAddr` is a contract or at least different from the zero address. **IGNORED: With CREATE2 it's possible that the proxy may be deployed pointing to a known address before the implementation has been deployed.**
+
+- N3 - lines 20, 21, 22, 23 - Consider removing some operations if there is no need to return specific errors. Checking whether the contract was already created can be done off-chain before and after sending the tx. The `WrapperCreationFailed` may be enough. **ADDRESSED: Removed additional checks.**
+
+- N4 - line 25 - Wrong comment. `getProxysalt` returns the salt needed for `create2`, not the resultant address. **ADDRESSED: Comment removed.**
+
+- N5 - lines 38, 43, 50, 54, and 58 - Missing dev notation. **ADDRESSED: Documentation added.**
+
+
+## Wrappers
+
+### ERC1155FloorFactory.sol
+
+#### Low
+
+
+- L1 - line 55 - There is no check if the contract being set supports the `IERC1155Metadata` interface and/or if it is at least a contract. Consider adding those checks to prevent human errors. **IGNORED: This can be performed off-chain. In the event of user error the function can be called again to fix the issue. This is consistent with the existing NiftyswapFactory20.sol**
+
+#### Notes
+
+- N1 - 15 and 55 - Function parameter names start with `_` while others do not. Consider removing the `_` or adding it to the rest. **ADDRESSED: Removed _ and updated variable name.**
+
+- N2 - line 15 - Missing dev notation. **ADDRESSED: Added documentation.**
+
+
+### ERC1155FloorWrapper.sol
+
+#### Low
+
+- L1 - line 30 - There is no check if the contract being set supports the `IERC1155` interface and/or if it is at least a contract. Consider adding those checks to prevent human errors. **IGNORED: With CREATE2 it's possible that the wrapper may be deployed pointing to a known address before the token has been deployed.**
+
+- L2 - line 161 - Possible griefing attack when withdrawing specific tokenIds. If the amount of one of the tokenIds desired is not available, the whole transaction will fail. Consider removing the usage of tokenIds. **IGNORED: This is desired behavour and mirrors existing Niftyswap Exchanges.**
+
+#### Notes
+
+- N1 - lines 23 and 27 - Missing dev notation. **ADDRESSED: Added documentation.**
+
+- N2 - 27 and 174 - Function parameter names start with `_` while others do not. Consider removing the `_` or adding it to the rest. **ADDRESSED: Added _ to all function parameters.**
+
+- N3 - lines 107 and 108 - Consider removing `FIXME` comments. **ADDRESSED: Removed comments.**
+
+- N4 - line 111 - Gas optimization. The `_deposit` function doesn't care about the `tokenIds` array. The `TokensDeposited` event can be emitted directly in `onERC1155Received` and  `onERC1155BatchReceived`. Also, consider moving the recipient check at the beginning of the `_deposit` function to prevent consuming more gas in the case of failure. **ADDRESSED: Followed advice.**
+
+
+### ERC721FloorFactory.sol
+
+#### Low
+
+
+- L1 - line 55 - There is no check if the contract being set supports the `IERC1155Metadata` interface and/or if it is at least a contract. Consider adding those checks to prevent human errors. **IGNORED: This can be performed off-chain. In the event of user error the function can be called again to fix the issue. This is consistent with the existing NiftyswapFactory20.sol**
+
+#### Notes
+
+- N1 - lines 15 and 55 - Function parameter names start with `_` while others do not. Consider removing the `_` or adding it to the rest. **ADDRESSED: Added _ to all function parameters.**
+
+- N2 - line 15 - Missing dev notation. **ADDRESSED: Added documentation.**
+
+
+### ERC721FloorWrapper
+
+### Low
+
+- L1 - line 30 - There is no check if the contract being set supports the `IERC721` interface and/or if it is at least a contract. Consider adding those checks to prevent human errors. **IGNORED: With CREATE2 it's possible that the wrapper may be deployed pointing to a known address before the token has been deployed.**
+
+- L2 - line 55 - Consider checking if the recipient address is not the zero address. **Addressed: Added zero address check.**
+
+- L3 - line 80 - Possible griefing attack when withdrawing specific tokenIds. If one of the tokenIds desired is not available, the whole transaction will fail. Consider removing the usage of tokenIds. **IGNORED: This is desired behavour and mirrors existing Niftyswap Exchanges.**
+
+#### Notes
+
+- N1 - lines 66, 76 and 78 - Consider re-using the `length` variable to reduce gas consumption. **ADDRESSED: Reused length variable.**
+
+- N2 - line 73 - Change ERC-1155 to ERC-721. **ADDRESSED: Fixed documentation.**
+
+- N3 - line 99 - `_id` is the only parameter that starts with `_`. Consider removing the `_` or adding it to other parameter names. **ADDRESSED: Added _ to all function parameters.**
+
+
+
+Ignacio Mazzara - April 2023.

src/contracts/exchange/NiftyswapExchange20.sol

@@ -85,9 +85,11 @@ contract NiftyswapExchange20 is
         FEE_MULTIPLIER = 1000 - _lpFee;
 
         // If global royalty, lets check for ERC-2981 support
+        bool supportsERC2981 = false;
         try IERC1155(_tokenAddr).supportsInterface(type(IERC2981).interfaceId) returns (bool supported) {
-            IS_ERC2981 = supported;
+            supportsERC2981 = supported;
         } catch {} // solhint-disable-line no-empty-blocks
+        IS_ERC2981 = supportsERC2981;
     }
 
     //

src/contracts/interfaces/IERC1155FloorFactory.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.4;
+
+interface IERC1155FloorFactory {
+    event NewERC1155FloorWrapper(address indexed token);
+    event MetadataContractChanged(address indexed metadataContract);
+
+    /**
+     * Creates an ERC-1155 Floor Wrapper for given token contract
+     * @param _tokenAddr The address of the ERC-1155 token contract
+     * @return The address of the ERC-1155 Floor Wrapper
+     */
+    function createWrapper(address _tokenAddr) external returns (address);
+
+    /**
+     * Return address of the ERC-1155 Floor Wrapper for a given token contract
+     * @param _tokenAddr The address of the ERC-1155 token contract
+     * @return The address of the ERC-1155 Floor Wrapper
+     */
+    function tokenToWrapper(address _tokenAddr) external view returns (address);
+}

src/contracts/interfaces/IERC1155FloorWrapper.sol

@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: Apache-2.0
+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";
+
+interface IERC1155FloorWrapper is IERC1155, IERC1155TokenReceiver {
+    event TokensDeposited(uint256[] tokenIds, uint256[] tokenAmounts);
+
+    event TokensWithdrawn(uint256[] tokenIds, uint256[] tokenAmounts);
+
+    struct DepositRequestObj {
+        address recipient;
+        bytes data;
+    }
+
+    struct WithdrawRequestObj {
+        uint256[] tokenIds;
+        uint256[] tokenAmounts;
+        address recipient;
+        bytes data;
+    }
+
+    /**
+     * Accepts ERC-1155 tokens to wrap and wrapped ERC-1155 tokens to unwrap.
+     * @notice Unwrapped ERC-1155 tokens are treated as deposits. Wrapped ERC-1155 tokens are treated as withdrawals.
+     * @param _operator The address which called `safeTransferFrom` function.
+     * @param _from The address which previously owned the token.
+     * @param _id The ID of the token being transferred.
+     * @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 _operator, address _from, uint256 _id, uint256 _amount, bytes calldata _data)
+        external
+        returns (bytes4);
+
+    /**
+     * Accepts ERC-1155 tokens to wrap and wrapped ERC-1155 tokens to unwrap.
+     * @notice Unwrapped ERC-1155 tokens are treated as deposits. Wrapped ERC-1155 tokens are treated as withdrawals.
+     * @param _operator The address which called `safeTransferFrom` function.
+     * @param _from The address which previously owned the token.
+     * @param _ids The IDs of the tokens being transferred.
+     * @param _amounts The amounts of tokens being transferred.
+     * @param _data Additional data with no specified format.
+     * @return `bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`
+     */
+    function onERC1155BatchReceived(
+        address _operator,
+        address _from,
+        uint256[] calldata _ids,
+        uint256[] calldata _amounts,
+        bytes calldata _data
+    ) external returns (bytes4);
+}

src/contracts/interfaces/IERC721.sol

@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.4;
+
+interface IERC721 {
+    event Transfer(address indexed from, address indexed to, uint256 indexed tokenId);
+    event Approval(address indexed owner, address indexed approved, uint256 indexed tokenId);
+    event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
+
+    function balanceOf(address _owner) external view returns (uint256 balance);
+    function ownerOf(uint256 _tokenId) external view returns (address owner);
+    function safeTransferFrom(address _from, address _to, uint256 _tokenId) external;
+    function transferFrom(address _from, address _to, uint256 _tokenId) external;
+    function approve(address _to, uint256 _tokenId) external;
+    function getApproved(uint256 _tokenId) external view returns (address operator);
+    function setApprovalForAll(address _operator, bool _approved) external;
+    function isApprovedForAll(address _owner, address _operator) external view returns (bool);
+    function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes calldata _data) external;
+}

src/contracts/interfaces/IERC721FloorFactory.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.4;
+
+interface IERC721FloorFactory {
+    event NewERC721FloorWrapper(address indexed token);
+    event MetadataContractChanged(address indexed metadataContract);
+
+    /**
+     * Creates an ERC-721 Floor Wrapper for given token contract
+     * @param _tokenAddr The address of the ERC-721 token contract
+     * @return The address of the ERC-721 Floor Wrapper
+     */
+    function createWrapper(address _tokenAddr) external returns (address);
+
+    /**
+     * Return address of the ERC-721 Floor Wrapper for a given token contract
+     * @param _tokenAddr The address of the ERC-721 token contract
+     * @return The address of the ERC-721 Floor Wrapper
+     */
+    function tokenToWrapper(address _tokenAddr) external view returns (address);
+}