047.md

Lone Midnight Stallion

High

An attacker can cause the order creator to lose money.

Summary

Currently, we do not use a nonce when generating the orderId. As a result, orders can be replaced by manipulating the orderId.

Root Cause

No response

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. An attacker monitors the network.
2. A user calls the createOrder function on the OracleLess contract with amountIn = 10 ether.
3. The attacker calls the createOrder function with amountIn = 9 ether.
4. The attacker calls the createOrder function with amountIn = 1 ether.
5. The attacker front-runs the user's transaction, executing all transactions in the same block. Since we only use the owner's address and block.timestamp to generate the orderId, all transactions execute with the sameorderId.

Impact

The owner loses their tokens without an additional order being created. Even if the owner cancels the order, 9 ether will be lost.

PoC

No response

Mitigation

1. Use msg.sender to ensure that the order is tied to the specific user. https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L38

    function createOrder(
        IERC20 tokenIn,
        IERC20 tokenOut,
        uint256 amountIn,
        uint256 minAmountOut,
-       address recipient,
        uint16 feeBips,
        bool permit,
        bytes calldata permitPayload
    ) external override returns (uint96 orderId) {
        //procure tokens
        procureTokens(tokenIn, amountIn, recipient, permit, permitPayload);

        //construct and store order
        orderId = MASTER.generateOrderId(recipient);
        orders[orderId] = Order({
            orderId: orderId,
            tokenIn: tokenIn,
            tokenOut: tokenOut,
            amountIn: amountIn,
            minAmountOut: minAmountOut,
-           recipient: recipient,
            feeBips: feeBips
        });

        //store pending order
        pendingOrderIds.push(orderId);

        emit OrderCreated(orderId);
    }

    function createOrder(
        IERC20 tokenIn,
        IERC20 tokenOut,
        uint256 amountIn,
        uint256 minAmountOut,
        uint16 feeBips,
        bool permit,
        bytes calldata permitPayload
    ) external override returns (uint96 orderId) {
        //procure tokens
        procureTokens(tokenIn, amountIn, recipient, permit, permitPayload);

        //construct and store order
        orderId = MASTER.generateOrderId(recipient);
        orders[orderId] = Order({
            orderId: orderId,
            tokenIn: tokenIn,
            tokenOut: tokenOut,
            amountIn: amountIn,
            minAmountOut: minAmountOut,
+           recipient: msg.sender,
            feeBips: feeBips
        });

        //store pending order
        pendingOrderIds.push(orderId);

        emit OrderCreated(orderId);
    }

2. Implement a nonce when generating the orderId to prevent order replacement. https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/AutomationMaster.sol#L90

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

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