050.md

Lone Midnight Stallion

High

An Attacker Can Cause Owners and Users to Be Unable to Cancel Orders

Summary

An attacker can create an unlimited number of orders without any price restrictions. Additionally, the _cancelOrder function in the OracleLess contract uses an iterative statement, which exacerbates the issue.

Root Cause

No response

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. The attacker creates a large number of orders with minimal loss. (This might be also done without any financial investment from the attacker. This issue has been previously submitted "An attacker can cause the order creator to lose money.").

2. When the owner or users attempt to call cancelOrder or adminCancelOrder, the transaction always fails due to running out of gas.

Impact

Due to the excessive gas consumption, both the owner and users are unable to utilize the order cancellation feature.

PoC

No response

Mitigation

1. Limit the value of amountIn when creating orders to prevent excessive order creation.

Set the value of MINIMAL_ORDER_PRICE. 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) {
+       require(amountIn > MINIMAL_ORDER_PRICE, "Too low price");

        //procure tokens
        procureTokens(tokenIn, amountIn, recipient, permit, permitPayload);

2. Replace the iterative statement with a mapping variable to optimize the _cancelOrder function and reduce gas consumption.

We chose to use a dynamic array to track pending orders by their IDs, utilizing array mutation logic to remove specific indexes from the array as orders are filled. Because the checkUpkeep function must loop through all pending orders, a mapping is not ideal. While this implementation is not the most efficient, we like it's readability, and as this system will primarily operate on later 2, the increased gas usage is not an issue for us.

I understand and agree with your perspective, but we can track orders without using a for statement, as shown below.

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

+   mapping(uint96 => uint96) public indexFromOrderId;

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);
+       indexFromOrderId[orderId] = orders.length;
        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);
    }

And the _cancelOrder function should be redefined like this. https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L150

    function _cancelOrder(Order memory order) internal returns (bool) {
        uint96 indexOfOrder = pendingOrderIds[indexFromOrderId[order.orderId]];
        require(indexOfOrder, 'Does not exist');
        if(!indexOfOrder) return false;

        //remove from pending array
        pendingOrderIds = ArrayMutation.removeFromArray(
            indexOfOrder,
            pendingOrderIds
        );

        //refund tokenIn amountIn to recipient
        order.tokenIn.safeTransfer(order.recipient, order.amountIn);

        //emit event
        emit OrderCancelled(order.orderId);

        return true;
    }

No need delete indexFromOrderId[order.orderId];.