`SpentItem`/`ReceivedItem` fields can be dirty and unused (potentially useful for phishing)

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Executor.sol#L63 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Executor.sol#L66 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Consideration.sol#L215 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Consideration.sol#L300

Vulnerability details

Impact

These structs are used through Consideration.fulfillAvailableOrders() and Consideration.fulfillAvailableAdvancedOrders():

struct SpentItem {
    ItemType itemType;
    address token;
    uint256 identifier;
    uint256 amount;
}

struct ReceivedItem {
    ItemType itemType;
    address token;
    uint256 identifier;
    uint256 amount;
    address payable recipient;
}

These two structures are used to convey Ether/ERC-20/ERC-721/ERC-1155 transfers, however not all fields are always used:

• in case of Ether transfers token and identifier is not used and remain unchecked and can contain anything
• in case of ERC20 transfers the identifier is unused and unchecked

While this problem does not impact transfers, it can certainly impact the display of these order details to the user. A user may be mislead to think a transfer is about a ERC-721 token, but in reality they will receive an ERC-20. Since OpenSea users have a history of facing "phishing" attacks, we think a medium severity for this is appropriate.

This lack of checking is present in multiple places.

Real world impact example

As an example one could create an order transferring 1 Ether to look like it transfers 1 BAYC (or any other popular NFT). The transaction data would only differ in a single byte. While good data inspection tools are lacking on the wallet side, users are mostly familiar with at least spotting token addresses and tokenIds, and so could verify that it "looks like" the correct thing, but miss the single byte difference, which is the ItemType specific to Seaport.

Proof of Concept

Context: Executor.sol#L63, Executor.sol#L66, Consideration.sol#L215, Consideration.sol#L300

modified   test/foundry/FullfillAvailableOrder.t.sol
@@ -305,8 +305,8 @@ contract FulfillAvailableOrder is BaseOrderTest {
         considerationItems.push(
             ConsiderationItem(
                 ItemType.NATIVE,
-                address(0),
-                0,
+                address(0xbabe),
+                1000,
                 uint256(context.args.paymentAmts[0]),
                 uint256(context.args.paymentAmts[0]),
                 payable(alice)

Running

$ forge test -m "testSingleOrderViaFulfillAvailableOrdersEthToSingleErc721"
[PASS] testSingleOrderViaFulfillAvailableOrdersEthToSingleErc721((address,uint256,bytes32,uint248,uint128[3],bool)) (runs: 10, μ: 4168192, ~: 4175292)
Test result: ok. 1 passed; 0 failed; finished in 142.62ms

Tools Used

Manual review

Recommended Mitigation Steps

Insert checks that these unused fields are 0.

[0age]

This is a known limitation, but also adds a good deal of overhead to validate each ETH / ERC20 item (particularly for something that is ignored). We intend to reject orders that try to set any of these fields to non-zero values, and are working with wallets to improve the display of orders as well.

[HardlyDifficult]

This is not a vulnerability per se, but can be dangerously misleading for users. Lowering to a Low severity and grouping with the warden's QA report #203