DataRestrictedNFT (Both ERC721 and ERC1155) is not working with Seaport as seaport cannot send _data bytes on transfer

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/TokenTransferrer.sol#L220-L728 https://github.com/Chomtana/2022-05-opensea-seaport/blob/8b4009bc3d81fc753a0e8435470a22505f25c411/contracts/attack/DataRestrictedNFT721.sol#L1-L64 https://github.com/Chomtana/2022-05-opensea-seaport/blob/8b4009bc3d81fc753a0e8435470a22505f25c411/contracts/attack/DataRestrictedNFT1155.sol#L1-L28 https://github.com/Chomtana/2022-05-opensea-seaport/blob/8b4009bc3d81fc753a0e8435470a22505f25c411/test/index.js#L16210-L16363

Vulnerability details

Impact

DataRestrictedNFT (Both ERC721 and ERC1155) is not working with Seaport as seaport cannot send _data bytes on transfer

DataRestricted NFT is a NFT that can only be transferred if and only if valid _data bytes is specified. Seaport can't handle this kind of NFT at all cost because _data bytes is not specified anywhere in struct. Moreover, TokenTransferrer is clearly hardcode 0x into _data field for ERC1155 safeTransferFrom

Data restricted NFT has use case in GameFi where every transfer must be approved by the offchain backend. And it may has more use case in the future where centralized organization adopt NFT and wanted some centralized control on NFT changing hand.

You can find implementation of Data restricted NFT below

• DataRestrictedNft721: https://github.com/Chomtana/2022-05-opensea-seaport/blob/main/contracts/attack/DataRestrictedNFT721.sol
• DataRestrictedNft1155: https://github.com/Chomtana/2022-05-opensea-seaport/blob/main/contracts/attack/DataRestrictedNFT1155.sol

As a result, Seaport may loss flexibility to become singularity marketplace contract in the future as DataRestricted NFT can't be used with Seaport thus require new NFT marketplace contract to be developed independently of each organization.

Proof of Concept

1. No _data bytes field can be proven by missing _data bytes field on _performERC721Transfer, _performERC1155Transfer and _performERC1155BatchTransfers function in TokenTransferrer. Without _data bytes field TokenTransferrer cannot send extra data on transfer.

    function _performERC721Transfer(
        address token,
        address from,
        address to,
        uint256 identifier
    )

    function _performERC1155Transfer(
        address token,
        address from,
        address to,
        uint256 identifier,
        uint256 amount
    ) internal {

    function _performERC1155BatchTransfers(
        ConduitBatch1155Transfer[] calldata batchTransfers
    ) internal {

2. 0x is hardcoded into _data bytes field for ERC1155 transfer as seen below

            mstore(
                ERC1155_safeTransferFrom_data_offset_ptr,
                ERC1155_safeTransferFrom_data_length_offset
            )
            mstore(ERC1155_safeTransferFrom_data_length_ptr, 0)

3. My new testcases clearly illustrate what DataRestrictedNFT is and how it react with Seaport (reverted with reason "Forbidden")

https://github.com/Chomtana/2022-05-opensea-seaport/blob/main/test/index.js#L16210-L16363

    it("Should not be transferable without specific data bytes", async () => {
      // Buyer mints nft
      const nftId721 = await mint721(owner);
      const { nftId: nftId1155, amount } = await mint1155(owner);

      await expect(
        testERC721.connect(owner)["safeTransferFrom(address,address,uint256)"](owner.address, buyer.address, nftId721)
      ).to.be.revertedWith("Forbidden");

      await expect(
        testERC1155.connect(owner).safeTransferFrom(owner.address, buyer.address, nftId1155, amount, "0x")
      ).to.be.revertedWith("Forbidden");
    })

This testcase show that without extra data, the transfer is blocked

    it("Should be transferable with specific data bytes", async () => {
      // Buyer mints nft
      const nftId721 = await mint721(owner);
      const { nftId: nftId1155, amount } = await mint1155(owner);

      await testERC721.connect(owner)["safeTransferFrom(address,address,uint256,bytes)"](owner.address, buyer.address, nftId721, "0x1234")
      await testERC1155.connect(owner).safeTransferFrom(owner.address, buyer.address, nftId1155, amount, "0x1234")

      await expect(await testERC721.ownerOf(nftId721)).to.equal(buyer.address);
      await expect((await testERC1155.balanceOf(owner.address, nftId1155)).toNumber()).to.equal(0);
      await expect((await testERC1155.balanceOf(buyer.address, nftId1155)).toNumber()).to.equal(amount.toNumber());
    })

This testcase show that with a valid extra data bytes (0x1234 in this case) the transfer is permitted.

    it("DataRestricted ERC1155 CANNOT be traded using Seaport marketplace at all cost", async () => {
      // Seller mints nfts
      const { nftId, amount } = await mint1155(seller);

      // Seller approves marketplace contract to transfer NFTs
      await set1155ApprovalForAll(seller, marketplaceContract.address, true);

      const { root, proofs } = merkleTree([nftId]);

      const offer = [getTestItem1155WithCriteria(root, toBN(1), toBN(1))];

      const consideration = [
        getItemETH(parseEther("10"), parseEther("10"), seller.address),
        getItemETH(parseEther("1"), parseEther("1"), zone.address),
        getItemETH(parseEther("1"), parseEther("1"), owner.address),
      ];

      const criteriaResolvers = [
        buildResolver(0, 0, 0, nftId, proofs[nftId.toString()]),
      ];

      const { order, orderHash, value } = await createOrder(
        seller,
        zone,
        offer,
        consideration,
        0, // FULL_OPEN
        criteriaResolvers
      );

      await expect(
        marketplaceContract
          .connect(buyer)
          .fulfillAdvancedOrder(order, criteriaResolvers, toKey(false), {
            value,
          })
      ).to.be.revertedWith("Forbidden");
    })
  })

This testcase show that Seaport throw "Forbidden" for DataRestrictedNft1155

Tools Used

1. Fork code
2. Design special NFT that broke Seaport
3. Write DataRestricted NFT code for both ERC721 and ERC1155
4. Write testcase to illustrate the limitation
5. Using the result of testcases to write this report

Recommended Mitigation Steps

1. Add _data bytes field to _performERC721Transfer, _performERC1155Transfer and _performERC1155BatchTransfers function in TokenTransferrer
2. Add _data bytes field to all struct representing Item / Execution / Transfer
3. Use safeTransferFrom instead of transferFrom for ERC721
4. Always send _data bytes field on safeTransferFrom

But it may be gas consuming, alternative way is to deploy two separate version of Seaport. One which not support DataRestrictedNFT while another one supported it

[0age]

Conscious design decision, not a vulnerability; very uncommon pattern and (as indicated in the submission) can always create a new version that has it if it becomes a more common pattern

[HardlyDifficult]

This is a limitation that isn't expected to impact many NFTs today. Lowering this to a Low severity and grouping with the warden's QA report #188