Skip to content

fix(SeaportRouter): terminate tally loop on partially-available batches - #1415

Open
aeonframework wants to merge 1 commit into
ProjectOpenSea:mainfrom
aeonframework:fix/router-partial-availability-infinite-loop
Open

fix(SeaportRouter): terminate tally loop on partially-available batches#1415
aeonframework wants to merge 1 commit into
ProjectOpenSea:mainfrom
aeonframework:fix/router-partial-availability-infinite-loop

Conversation

@aeonframework

Copy link
Copy Markdown

Summary

SeaportRouter.fulfillAvailableAdvancedOrders never terminates on a partially-available batch, burning the caller's entire gas limit instead of returning a partial fill.

In the post-call tally at contracts/helpers/SeaportRouter.sol:146-154, the for header has no increment and the only ++j is nested inside if (newAvailableOrders[j]):

for (uint256 j = 0; j < newAvailableOrdersLength; ) {
    if (newAvailableOrders[j]) {
        unchecked {
            --fulfillmentsLeft;
            ++j;            // only advances when the flag is true
        }
    }
}

fulfillAvailableAdvancedOrders returns false for any order it skipped — cancelled, expired, already filled, zone-rejected, or beyond maximumFulfilled — and only reverts (NoSpecifiedOrdersAvailable) when no order is available. So a batch where some fill and some don't returns a mixed bool[] with at least one false — the exact case this router exists to serve. The first false pins j, and the loop spins to out-of-gas.

Impact

Any caller whose batch is partially available loses their whole gas limit rather than getting a cheap revert or a partial fill. It is cheaply and adversarially triggerable: a seller front-running one order in a pending batch with a cancel (~30-50k gas), or another buyer taking one listing first, is enough to force the victim's call to out-of-gas. No funds are stolen and nothing is stuck — this is a gas-griefing DoS.

SeaportRouter is not in the canonical deployment table, so the exposed parties are third-party integrators who deploy it themselves; reporting it as a defect in a shipped helper contract.

Fix

Move ++j into the loop body so it advances on every iteration regardless of the per-order flag. Fully-available batches are unaffected.

Reproduction

The defect is entirely in the router's tally of Seaport's return value, independent of order execution, so a mock Seaport isolates it. forge test at 080133906585 (solc 0.8.24): the [true, false] case out-of-gasses a 3M-gas inner call; the [true, true] control returns cleanly through the identical path.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import { Test } from "forge-std/Test.sol";
import { SeaportRouter } from "seaport/helpers/SeaportRouter.sol";
import { SeaportRouterInterface } from "seaport-types/src/interfaces/SeaportRouterInterface.sol";
import { AdvancedOrder, CriteriaResolver, FulfillmentComponent, Execution } from "seaport-types/src/lib/ConsiderationStructs.sol";

contract MockSeaport {
    bool[] private _result;
    constructor(bool[] memory result) { _result = result; }
    function fulfillAvailableAdvancedOrders(
        AdvancedOrder[] calldata, CriteriaResolver[] calldata,
        FulfillmentComponent[][] calldata, FulfillmentComponent[][] calldata,
        bytes32, address, uint256
    ) external payable returns (bool[] memory availableOrders, Execution[] memory executions) {
        availableOrders = _result;
        executions = new Execution[](0);
    }
}

contract RouterPartialAvailabilityTest is Test {
    function _params(address seaport, uint256 maxFulfilled)
        internal view returns (SeaportRouterInterface.FulfillAvailableAdvancedOrdersParams memory p)
    {
        address[] memory sc = new address[](1);
        sc[0] = seaport;
        SeaportRouterInterface.AdvancedOrderParams[] memory aop =
            new SeaportRouterInterface.AdvancedOrderParams[](1);
        p = SeaportRouterInterface.FulfillAvailableAdvancedOrdersParams({
            seaportContracts: sc, advancedOrderParams: aop,
            fulfillerConduitKey: bytes32(0), recipient: address(this),
            maximumFulfilled: maxFulfilled
        });
    }

    // BUG: [true, false] batch never terminates -> caller's whole gas limit is burned.
    function test_mixedBatch_neverTerminates() public {
        bool[] memory mixed = new bool[](2); mixed[0] = true; mixed[1] = false;
        MockSeaport mock = new MockSeaport(mixed);
        SeaportRouter router = new SeaportRouter(address(0xDEAD), address(mock));
        (bool ok, bytes memory ret) = address(router).call{ gas: 3_000_000 }(
            abi.encodeCall(SeaportRouterInterface.fulfillAvailableAdvancedOrders, (_params(address(mock), 2)))
        );
        assertFalse(ok, "expected out-of-gas: loop never terminates on a [true,false] batch");
        assertEq(ret.length, 0, "out-of-gas yields empty returndata");
    }

    // CONTROL: [true, true] batch terminates cleanly on the identical code path.
    function test_fullyAvailableBatch_terminates() public {
        bool[] memory allTrue = new bool[](2); allTrue[0] = true; allTrue[1] = true;
        MockSeaport mock = new MockSeaport(allTrue);
        SeaportRouter router = new SeaportRouter(address(0xDEAD), address(mock));
        (bool ok, ) = address(router).call{ gas: 3_000_000 }(
            abi.encodeCall(SeaportRouterInterface.fulfillAvailableAdvancedOrders, (_params(address(mock), 2)))
        );
        assertTrue(ok, "fully-available batch terminates cleanly");
    }
}

The existing test/router.spec.ts misses this because it puts one order behind each Seaport contract, so each call returns all-true or all-false — a mixed array within a single call is never produced. A regression test needs a single Seaport call whose batch is genuinely mixed (two orders on the same Seaport, one cancelled). Happy to add that in whichever harness you prefer.

fulfillAvailableAdvancedOrders returns a mixed bool[] whenever a batch is
partially available (some orders cancelled, expired, already filled, or beyond
maximumFulfilled). In SeaportRouter's post-call tally the ++j increment lived
inside `if (newAvailableOrders[j])`, so the first false entry pinned j and the
loop spun until it ran out of gas. Any caller with a partially-available batch
lost their whole gas limit instead of getting a partial fill, and it is cheaply
triggerable by a third party front-running one order in a pending batch with a
cancel.

Move ++j into the loop body so it advances on every iteration regardless of the
per-order flag. Behaviour on fully-available batches is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant