code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

uint256 => uint120 silent overflow #132

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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).

0age commented 2 years ago

another partial fill finding

HardlyDifficult commented 2 years ago

Dupe of #77