Unchecked Loops and Use of `selfbalance()` Function Vulnerability in Smart Contract.

[code423n4]

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/lib/OrderCombiner.sol#L447-L482 https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/lib/OrderCombiner.sol#L725-L728 https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/lib/OrderCombiner.sol#L706-L713

Vulnerability details

Impact

Use of unchecked in-for loops.

unchecked {
            bytes32 orderHash;

            // Iterate over each order.
            for (uint256 i = 32; i < terminalMemoryOffset; i += 32) {
                assembly {
                    orderHash := mload(add(orderHashes, i))
                }

                // Do not emit an event if no order hash is present.
                if (orderHash == bytes32(0)) {
                    continue;
                }

                // Retrieve order using assembly to bypass out-of-range check.
                assembly {
                    advancedOrder := mload(add(advancedOrders, i))
                }

                // Retrieve parameters for the order in question.
                OrderParameters memory orderParameters = (
                    advancedOrder.parameters
                );

                // Emit an OrderFulfilled event.
                _emitOrderFulfilledEvent(
                    orderHash,
                    orderParameters.offerer,
                    orderParameters.zone,
                    recipient,
                    orderParameters.offer,
                    orderParameters.consideration
                );
            }
        }

unchecked {
            ++i;
        }

selfbalance() function in _performFinalChecksAndExecuteOrders function.

assembly {
                nativeTokenBalance := selfbalance()
            }

            // Ensure that sufficient native tokens are still available.
            if (item.amount > nativeTokenBalance) {
                _revertInsufficientEtherSupplied();
            }

Use of the unchecked keyword in several places in the code, specifically for loops and the _transfer function. This keyword is used to suppress overflow/underflow checks in Solidity, and its use can make the contract susceptible to attacks that exploit these conditions, a malicious actor could manipulate the input data to a for loop to cause it to run an excessive number of iterations, potentially causing the contract to run out of gas or execute unintended actions.

Use of the selfbalance() function in the _performFinalChecksAndExecuteOrders function. The selfbalance() function is used to retrieve the balance of the contract's own account, and its use can be exploited by malicious actors to manipulate the contract's behavior, an attacker could increase the contract's balance by sending a large amount of Ether to it, and then use this increased balance to perform unauthorized actions.

Proof of Concept

Exploit the use of unchecked in the for loops by creating a malicious contract that sends a large number of orders to the target contract. The loops that iterate over these orders do not have any overflow checks, allowing the attacker to overflow the index and access memory outside of the intended range. This will lead to unintended data access, or even worse, data modification.

Code.

// Malicious contract that sends a large number of orders to the target contract
contract Attacker {
    address target;

    constructor(address _target) public {
        target = _target;
    }

    function attack() public {
        // Create an array of orders that exceeds the maximum intended range
        AdvancedOrder[] memory orders = new AdvancedOrder[](2 ** 256);

        // Send the orders to the target contract
        IERC1155Mintable targetI = IERC1155Mintable(target);
        targetI.matchAdvancedOrders(orders, new CriteriaResolver[](), new Fulfillment[](), address(0));
    }
}

An attacker can exploit the use of selfbalance() function in _performFinalChecksAndExecuteOrders function by creating a malicious contract that manipulates the balance of the target contract, thus allowing the attacker to pass the check and get more token than they should have.

Code.

// Malicious contract that manipulates the balance of the target contract
contract Attacker {
    address target;

    constructor(address _target) public {
        target = _target;
    }

    function attack() public {
        // Send a large amount of Ether to the target contract
        target.transfer(2 ** 256);
    }
}

In both PoC, the attacker causes unintended behavior in the target contract, leading to loss of funds, or unauthorized access to data.

Tools Used

Manual audit.

Recommended Mitigation Steps

For the unchecked keyword in for loops, the developer should add overflow checks in the for loops by replacing the unchecked keyword with a check for the loop variable being less than the maximum value. For example, instead of using unchecked { for (uint256 i = 0; i < advancedOrders.length; ++i) {" the developer should use "for (uint256 i = 0; i < advancedOrders.length && i < uint256(2**256 -1); ++i) {

Add require(accumulator.length + 32 <= AccumulatorMaxLength,"Accumulator overflow"); before the transfer function.

As for the selfbalance() function in the _performFinalChecksAndExecuteOrders function, the developer can use msg.value instead of selfbalance() function. This will allow the developer to check the balance of the address that is calling the contract rather than the contract's own balance.

Code.

function _matchAdvancedOrders(
        AdvancedOrder[] memory advancedOrders,
        CriteriaResolver[] memory criteriaResolvers,
        Fulfillment[] memory fulfillments,
        address recipient
    ) internal returns (Execution[] memory /* executions */) {
        // Validate orders, update order status, and determine item amounts.
        bytes32[] memory orderHashes = _validateOrdersAndPrepareToFulfill(
            advancedOrders,
            criteriaResolvers,
            true, // Signifies that invalid orders should revert.
            advancedOrders.length,
            recipient
        );

        // Emit OrdersMatched event, providing an array of matched order hashes.
        _emitOrdersMatched(orderHashes);

        // Fulfill the orders using the supplied fulfillments and recipient.
        return
            _fulfillAdvancedOrders(
                advancedOrders,
                fulfillments,
                orderHashes,
                recipient
            );
    }

function _performFinalChecksAndExecuteOrders(
        AdvancedOrder[] memory advancedOrders,
        Execution[] memory executions,
        bytes32[] memory orderHashes,
        address recipient
    ) internal returns (bool[] memory /* availableOrders */) {
        // Declare a variable for the available native token balance.
        uint256 nativeTokenBalance;

        // Retrieve the length of the advanced orders array and place on stack.
        uint256 totalOrders = advancedOrders.length;

        // Initialize array for tracking available orders.
        bool[] memory availableOrders = new bool[](totalOrders);

        // Initialize an accumulator array. From this point forward, no new
        // memory regions can be safely allocated until the accumulator is no
        // longer being utilized, as the accumulator operates in an open-ended
        // fashion from this memory pointer; existing memory may still be
        // accessed and modified, however.
        bytes memory accumulator = new bytes(AccumulatorDisarmed);

        // Retrieve the length of the executions array and place on stack.
        uint256 totalExecutions = executions.length;

        // Iterate over each execution.
        for (uint256 i = 0; i < totalExec

[0age]

contested on both counts; we mask all lengths and pointers on the way in which prevents an attacker from even passing in an array length large enough to overflow any for loops; and an attacker sending Ether in is simply donating their eth to the caller to use at their discretion

[HickupHH3]

Insufficient proof on:

• terminalMemoryOffset being large enough to cause DoS
• selfBalance checks bricking functionality.

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Insufficient proof