Wrong items length assertion in basic order

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/BasicOrderFulfiller.sol#L346-L349

Vulnerability details

When fulfilling a basic order we need to assert that the parameter totalOriginalAdditionalRecipients is less or equal than the length of additionalRecipients written in calldata. However in _prepareBasicFulfillmentFromCalldata this assertion is incorrect (L346):

        // Ensure supplied consideration array length is not less than original.
        _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength(
            parameters.additionalRecipients.length + 1,
            parameters.totalOriginalAdditionalRecipients
        );

The way the function it's written (L75), it accepts also a length smaller than the original by 1 (basically there shouldn't be a + 1 in the first argument).

Interestingly enough, in the case additionalRecipients.length < totalOriginalAdditionalRecipients, the inline-assembly for-loop at (L506) will read consideration items out-of-bounds. This can be a vector of exploits, as illustrated below.

Proof of Concept

Alice makes the following offer: a basic order, with two considerationItems. The second item has the following data:

consideration[1] = {
    itemType: ...,
    token: ...,
    identifierOrCriteria: ...,
    startAmount: X,
    endAmount: X,
    recipient: Y,
}

The only quantities we need to track are the amounts X and recipient Y.

When fulfilling the order normally, the fulfiller will spend X tokens sending them to Y. It's possible however to exploit the previous bug in a way that the fulfiller won't need to make this transfer.

To do this, the fulfiller needs to craft the following calldata:

calldata pointer	correct calldata	exploit calldata
...	...	...
0x204	1 (tot original)	1 (tot original)
0x224	0x240 (head addRec)	0x240 (head addRec)
0x244	0x2a0 (head sign)	0x260 (head sign)
0x264	1 (length addRec)	0 (length addRec)
0x284	X (amount)	X (length sign)
0x2a4	Y (recipient)	Y (sign body)
0x2c4	0x40 (length sign)	0x00 (sign body)
0x2e4	[correct Alice sign]	...
0x304	[correct Alice sign]	...

Basically writing additionalRecipients = [] and making the signature length = X, with Y being the first 32 bytes. Of course this signature will be invalid; however it doesn't matter since the exploiter can call validate with the correct signature beforehand.

The transaction trace will look like this:

• the assertion _assertConsiderationLengthIsNotLessThanOriginalConsiderationLength passes;
• the orderHash calculated is the correct one, since the for-loop over original consideration items picks up calldata at pointers {0x284, 0x2a4} (L513);
• the order was already validated beforehand, so the signature isn't read;
• at the end, during the tokens transfers, only offer and consideration[0] are transferred, since the code looks at additionalRecipients which is empty.

Conclusion:

Every Order that is "basic" and has two or more consideration items can be fulfilled in a way to not trade the last consideration item in the list. The fulfiller spends less then normally, and a recipient doesn't get his due.

There's also an extra requirement which is stricter: this last item's startAmount (= endAmount) needs to be smallish (< 1e6). This is because this number becomes the signature bytes length, and we need to fill the calldata with extra zeroes to complete it. Realistically then the exploit will work only if the item is a ERC20 will low decimals.

I've made a hardhat test that exemplifies the exploit. (Link to gist).

Recommended Mitigation Steps

Remove the +1 at L347.

[0age]

Valid finding on the off-by-one error, this was already reported to us outside of c4 and we're going to fix — will mention that it's very difficult to find / craft exploitable payloads though, so severity should be lower

[0xleastwood]

While the issue outlines an exploit whereby an attacker can fulfill an order without paying the entire consideration amount, it does require a set of requirements, namely:

• The item is an ERC20 with low decimals.
• The order has considerationItems > 1.

Maximum extractable value for the most prevalent ERC20 token with low decimals, WBTC. This token uses 8 decimals and currently we know that calldata uses 16 gas for each byte used. Based on a block gas limit of 30,000,000, we can deduce that the calldata length has an upper bound of 1.875 MB. Based on this, the maximum extractable value would be (1,875,000 / 1e8) * $20,000 USD = $375 USD, assuming the price for each WBTC is $20,000 USD.

Relevant EIP detailing this is found at https://eips.ethereum.org/EIPS/eip-4488.

It is also important to note, that by utilising the entire available block space on Ethereum, it is very likely that the cost of the transaction will far exceed the amount received in the attack.

The attack does in fact leak value by allowing orders to be fulfilled at a slight discount. However, because this only affects very specific order types, I believe medium severity to be justified.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

This was one of the most interesting issues I've read, kudos to those who found it!

[liveactionllama]

Per @0age resolved via: https://github.com/ProjectOpenSea/seaport/pull/317