Value Overflow in `FulfillmentApplier.sol`

[code423n4]

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
}

[0xleastwood]

The warden has identified an edge case which is incorrectly handled by the _aggregateValidFulfillmentOfferItems() and _aggregateValidFulfillmentConsiderationItems() functions. As such, these functions may accept an invalid input as it fails to revert even though an error is present. I do not believe funds are at risk, however, users may unintentionally fulfill an order with invalid inputs. This order fulfillment is likely to always revert due to an overflow/zero amount input. As a result, this would impact the protocol's overall user experience and for that reason, I think medium severity is justified.

Great find!

[HardlyDifficult]

Dupe https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/75