ShadowForce - User can game the order system

[sherlock-admin]

ShadowForce

high

User can game the order system

Summary

user can game order system by setting out of range trigger price

Vulnerability Detail

The protocol is aware of this attack vector and it is evident when we read the comments of the code snippet below.

  // freeze unfulfillable orders to prevent the order system from being gamed
            // an example of gaming would be if a user creates a limit order
            // with size greater than the available amount in the pool
            // the user waits for their limit price to be hit, and if price
            // moves in their favour after, they can deposit into the pool
            // to allow the order to be executed then close the order for a profit
            //
            // frozen order keepers will have additional validations before executing
            // frozen orders to prevent gaming
            //
            // alternatively, the user can call updateOrder to unfreeze the order
            OrderUtils.freezeOrder(
                dataStore,
                eventEmitter,
                orderVault,
                key,
                msg.sender,
                startingGas,
                reason,
                reasonBytes
            );

Although the protocol is aware of this attack, it is still possible in the upadateOrder function.

 function updateOrder(
        bytes32 key,
        uint256 sizeDeltaUsd,
        uint256 acceptablePrice,
        uint256 triggerPrice,
        uint256 minOutputAmount,
        Order.Props memory order
    ) external payable globalNonReentrant onlyController {
        FeatureUtils.validateFeature(dataStore, Keys.updateOrderFeatureDisabledKey(address(this), uint256(order.orderType())));

In the function above a malicious user can set and out of range trigger price to ensure the order cannot be executed.

When the malicious user sees the market has moved in a favorable position, he can simply update the trigger price.

Additionally the user can also front-run the order execution of the keeper and set the trigger price to ensure the order is not executed.

below we can observe how the trigger price is used when executing an order

 // @dev executes an order
    // @param params BaseOrderUtils.ExecuteOrderParams
    function executeOrder(BaseOrderUtils.ExecuteOrderParams memory params) external {
        BaseOrderUtils.validateNonEmptyOrder(params.order);

        BaseOrderUtils.setExactOrderPrice(
            params.contracts.oracle,
            params.market.indexToken,
            params.order.orderType(),
            params.order.triggerPrice(),
            params.order.isLong()
        );

The function above then calls into setExactOrderPrice

  if (shouldValidateAscendingPrice) {
                // check that the earlier price (primaryPrice) is smaller than the triggerPrice
                // and that the later price (secondaryPrice) is larger than the triggerPrice
                bool ok = primaryPrice <= triggerPrice && triggerPrice <= secondaryPrice;
                if (!ok) {
                    revert InvalidOrderPrices(primaryPrice, secondaryPrice, triggerPrice, shouldValidateAscendingPrice);
                }

                oracle.setCustomPrice(indexToken, Price.Props(
                    triggerPrice, // min price that order can be executed with
                    secondaryPrice // max price that order can be executed with
                ));
            } else {
                // check that the earlier price (primaryPrice) is larger than the triggerPrice
                // and that the later price (secondaryPrice) is smaller than the triggerPrice
                bool ok = primaryPrice >= triggerPrice && triggerPrice >= secondaryPrice;
                if (!ok) {
                    revert InvalidOrderPrices(primaryPrice, secondaryPrice, triggerPrice, shouldValidateAscendingPrice);
                }

                oracle.setCustomPrice(indexToken, Price.Props(
                    secondaryPrice, // min price that order can be executed with
                    triggerPrice // max price that order can be executed with
                ));
            }

            return;

From the logic above we can observe how if the trigger price is out of range, the order will not execute. Like i stated earlier the malicious user can then update trigger price if it is favorable for him, therefore creating a risk free trade.

Impact

Malicious user can game the order system in order to create risk free trades. This is a loss of funds for the protocol and this is unfair to all the other users who use the protocol correctly. Users will lose trust in GMX.

Code Snippet

https://github.com/gmx-io/gmx-synthetics/blob/91af13f93ee64e8cb50c37e4e8084037cbde15a7/contracts/exchange/OrderHandler.sol#L264-L286

https://github.com/gmx-io/gmx-synthetics/blob/91af13f93ee64e8cb50c37e4e8084037cbde15a7/contracts/exchange/OrderHandler.sol#L68-L76

https://github.com/gmx-io/gmx-synthetics/blob/91af13f93ee64e8cb50c37e4e8084037cbde15a7/contracts/order/OrderUtils.sol#L151-L162

https://github.com/gmx-io/gmx-synthetics/blob/91af13f93ee64e8cb50c37e4e8084037cbde15a7/contracts/order/BaseOrderUtils.sol#L258-L285

Tool used

Manual Review

Recommendation

I recommend to write logic in a way that mitigates this risk.

[Jiaren-tang]

Escalate for 10 USDC. This issue should either be marked as a duplicate of https://github.com/sherlock-audit/2023-02-gmx-judging/issues/130 or considered as a separate high severity issue. The basic idea is that a user can keep updating an order to ensure that it can only be executed when the price is favorable, creating a risk-free trade.

[sherlock-admin]

Escalate for 10 USDC. This issue should either be marked as a duplicate of https://github.com/sherlock-audit/2023-02-gmx-judging/issues/130 or considered as a separate high severity issue. The basic idea is that a user can keep updating an order to ensure that it can only be executed when the price is favorable, creating a risk-free trade.

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]

The updateOrder() function touches the order, which increases the stored block number, which is checked when executing the order. Once the stored block number is updated, the order is no longer able to use prices from when it was originally submitted, so the no risk free profit opportunity this issue describes is not there. - Invalid

[hrishibhat]

Escalation rejected

As pointed out by the Lead watson the only area where things could possibly be gamed is being able to modify the order without having it update the actual order's execution price, which isn't possible when the order is touched

[sherlock-admin]

Escalation rejected

As pointed out by the Lead watson the only area where things could possibly be gamed is being able to modify the order without having it update the actual order's execution price, which isn't possible when the order is touched

This issue's escalations have been rejected!

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