Description

When an order is placed, the Trading contract calls getPrice on OstiumPriceRouter, which selects a OstiumPriceUpKeep deployment depending on the pair type:

function getPrice(uint16 pairIndex, IOstiumPriceUpKeep.OrderType orderType, uint256 timestamp)
    external
    onlyTrading
    returns (uint256)
{
    string memory priceUpkeepType = IOstiumPairsStorage(
        registry.getContractAddress('pairsStorage')).oracle(pairIndex);

    uint256 orderId = IOstiumPriceUpKeep(payable(
        registry.getContractAddress(bytes32(abi.encodePacked(
            priceUpkeepType, 'PriceUpkeep'))))
    ).getPrice(pairIndex, orderType, timestamp);

    return orderId;
}

On the other end of the call, getPrice on OstiumPriceUpKeep allocates a new order ID to use to identify the order:

function getPrice(uint16 pairIndex, OrderType orderType, uint256 timestamp) external onlyRouter returns (uint256) {
    ++currentOrderId;

    orders[currentOrderId] = Order(
        timestamp.toUint32(), pairIndex, orderType, true);

    emit PriceRequested(currentOrderId);

    return currentOrderId;
}

However, because the individual OstiumPriceUpKeep contracts have separate storages, the order ID returned by getPrice is not unique across Ostium. But, after the new order ID is returned back to the Trading contract, it is used to call storePendingMarketOrder in OstiumTradingStorage:

function storePendingMarketOrder(PendingMarketOrder memory _order, uint256 _id, bool _open) external onlyTrading {
    pendingOrderIds[_order.trade.trader].push(_id);

    reqID_pendingMarketOrder[_id] = _order;
    reqID_pendingMarketOrder[_id].block = ChainUtils.getBlockNumber();

    // [...]
}

And that assumes that the order ID is globally unique, since there is only one reqID_pendingMarketOrder mapping.

Impact

Multiple orders can have the same order ID. If multiple orders with the same order ID are pending, then when the forwarder fulfills the order, it will fulfill the later one even if the forwarder expects to be fulfilling the earlier one.

This causes the collateral sent for the first trade to be lost, locking funds in the Trading contract for users who accidentally reach this bug. Furthermore, if an attacker intentionally overlaps a market open on pair A with a market close on an existing trade on pair B, and then the open is upkept first, the "slippage" seen by the callback will be zero, causing the open to be cancelled, which returns all of the collateral of the existing trade for free.

Recommendations

Centrally maintain the currentOrderId instead of having the counter be in OstiumPriceUpKeep.

Remediation

This issue has been acknowledged by Ostium Labs, and a fix was implemented in commit 0dc9db66↗.