049.md

Fresh Topaz Flamingo

Medium

Stop Limit and Bracket Orders can cause ID collision and fund locking

Summary

The generateOrderId function in AutomationMaster uses keccak256(abi.encodePacked(sender, block.timestamp)) to create a unique identifier for orders. However, if a Stop Limit Order triggers the creation of a Bracket Order within the same block, both orders may generate the same orderId. This results in the overwriting of the old Bracket Order, effectively damaging the order and locking user funds.

https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/AutomationMaster.sol#L90-L95

This issue arises from the deterministic and collision-prone nature of using sender and block.timestamp as inputs for generating unique order IDs within the same block.

Root Cause

1. Shared Order ID Space:

   • Both Stop Limit and Bracket Orders use the same generateOrderId function, which relies on keccak256(abi.encodePacked(sender, block.timestamp)).

2. Timestamp Resolution:

   • block.timestamp has a resolution of seconds, making it insufficient to differentiate between two orders created in the same block by the same sender.

3. Order Overwriting:

   • When the Stop Limit Order is triggered and creates a new Bracket Order, the generated orderId might match an existing Bracket Order if they are created within the same block. This overwrites the existing order and disrupts the system's state.

Impact

1. Order Damage:

   • An existing Bracket Order can be unintentionally overwritten when a new Bracket Order is created from a Stop Limit Order in the same block.

2. Locked Funds:

   • Funds associated with the overwritten order become inaccessible as the overwritten order's data is corrupted or lost.

3. System Integrity:

   • The integrity of the contract is compromised, as orders can overwrite each other, leading to unintended behavior and potential loss of trust.

Proof of Concept

Scenario:

1. A user places a Bracket Order (sender = A), and an orderId is generated using generateOrderId.
2. A Stop Limit Order triggers within the same block (block.timestamp is the same).
3. The triggered Stop Limit Order creates a new Bracket Order with identical orderId as the existing Bracket Order.
4. The new order overwrites the old order, resulting in the loss of the original order's data and locking of funds.

Mitigation

1. Introduce Additional Entropy in generateOrderId

To prevent collisions, incorporate a nonce or counter to ensure uniqueness, even if the same sender and block.timestamp are used.

Updated generateOrderId:

function generateOrderId(address sender, uint256 nonce) external view override returns (uint96) {
    uint256 hashedValue = uint256(
        keccak256(abi.encodePacked(sender, block.timestamp, nonce))
    );
    return uint96(hashedValue);
}

• Nonce: A per-user counter that increments with every order created. This ensures that every order is unique, even within the same block.

2. Implement a Check for Existing Orders

function fillStopLimitOrder(
    bytes calldata swapPayload,
    uint256 takeProfit,
    uint256 stopPrice,
    uint256 amountIn,
    uint96 existingOrderId,
    IERC20 tokenIn,
    IERC20 tokenOut,
    address recipient,
    uint16 feeBips,
    uint16 takeProfitSlippage,
    uint16 stopSlippage,
    bool permit,
    bytes calldata permitPayload
) external override nonReentrant {
    // Check if the order with existingOrderId already exists
    Order memory existingOrder = orders[existingOrderId];
    require(existingOrder.orderId == 0, "Order ID Collision: Order already exists");
    ...
}