OpenSea Seaport contest - 0xsanson's results

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderValidator.sol#L228-L231

Vulnerability details

When converting a number from uint256 to a smaller type, solidity truncates it without raising errors. In our case, this operation is performed when saving numerator and denominator to the storage variable _orderStatus[orderHash] (code link).

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

The impact of this bug is that it's possible to fill an order over the maximum quantity the offerer set up.

Proof of Concept

Let's consider an order which is OPEN and PARTIAL. Without loss of generality, we can consider that all amounts are divisible by 10 (kinda common number).

Bob is a fulfiller who makes an advanced fulfill with:

// 1st fulfill
numerator = 2**120 / 4
denominator = 2**120 / 2

This correctly brings _orderStatus to the same numerator and denominator. Bob bought half the order.

Now Bob make another fulfillment with:

// 2nd fulfill
numerator = 2
denominator = 5

This brings _orderStatus to:

_orderStatus.numerator = uint120(5 * 2**120 / 4 + 2 * 2**120 / 2) = 2**120 / 4
_orderStatus.denominator = uint120(5 * 2**120 / 2) = 2**120 / 2

Surprisingly it doesn't change, even though Bob brings home another 2/5 of an order. Bob can then repeat filling 2/5 until he likes.

Example: Alice is a creator who made a ERC1155 token. She wants to make a super sale where she sells only 10 of them for only 1 ETH! Unexpectedly, Bob is a massive fan and he's able to scoop 100 of them (for a total 10 ETH of course) (assuming Alice's balance and allowance are greater than that).

I've proved this exploit with a hardhat test, link to gist.

Recommended Mitigation Steps

Add the following check before making the type conversion (around L222):

require(denominator <= type(uint120).max);

(we already know numerator < denominator).

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:

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.

Lines of code

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

Vulnerability details

Impact

The code currently moves ERC721 around using the transferFrom(from,to,id) function. According to EIP-721, when using this function "the caller is responsible to confirm that the recipient (to) is capable of receiving NFTs or else they may be permanently lost". Currently this check isn't performed.

The issue is of course that NFTs can be lost.

Proof of Concept

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/TokenTransferrer.sol#L239 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/TokenTransferrerConstants.sol#L85

Recommended Mitigation Steps

The standard is to use the safeTransferFrom function, which already makes the necessary check. To use the safe version, just change the constant ERC721_transferFrom_signature to ERC721_safeTransferFrom_signature = abi.encodeWithSignature("safeTransferFrom(address,address,uint256)").