Oracle Updates

GMX reverts when the bid price is greater than the ask price, otherwise known as a crossed market. Crossed markets can typically happen in times of volatility, or when pricing is based on multiple venues/data providers. For example, if the highest bid is from Coinbase but the lowest ask is from Binance, the chance of a reported bid larger than the ask increases.

if (report.bid > report.ask) {
    revert Errors.InvalidRealtimeBidAsk(token, report.bid, report.ask);
}

Internal feeds cannot be used to regain protocol functionality because internal feeds cannot be enabled when a realtime feed is enabled for a particular token.

Verify whether the realtime feeds report crossed markets, if so consider an alternative to the strict validation or document and prepare for the risk of DoS.

GMX Team: With the realtime feeds bid should always be less than or equal to ask, will check with Chainlink about including this info in the docs.

The configuration of the Chainlink VerifierProxy and Verifier contracts pose a non-trivial threat to the GMX V2 system.

A malicious user may observe any of the following scenarios and leverage them to execute a risk free trade on the platform:

• A certain feed used by GMX V2 is deactivated with isDeactivated == true
• A verifier has been unset with the unsetVerifier function for a particular configDigest used by GMX V2
• A particular config that corresponds to the configDigest used by GMX is not active with isActive == false

In any of the above scenarios a malicious actor is able to submit a market order which is un-executable during the period where the feedId or configDigest is deactivated/misconfigured. If market prices move against the trader during this time, the trader can simply cancel their order.

GMX V2 offers no alternative means of execution for these orders until the feedId or configDigest is reactivated as they cannot be executed with the regular oracle system and changing the oracle configuration for the tokens used would take days using the Timelock contract.

Consider implementing an alternative pathway to provide prices for orders in the event that a feedId or configDigest is deactivated or misconfigured. Otherwise consider disallowing the creation of orders that rely on certain feedId’s or configDigest’s that are currently deactivated.

GMX Team: Added an option for the multisig to allow internal price feeds, will also check with Chainlink about this.

In the _executeDeposit function the pool amount is incremented by the positive price impact amount in the _params.tokenOut.

However only the _params.tokenIn is validated against the validatePoolAmountForDeposit validation. This could lead to the tokenOut balance exceeding the desired cap during deposits.

Validate that the increased tokenOut amount is also within the deposit cap with validatePoolAmountForDeposit.

GMX Team: The recommendation was implemented.

The getUncompactedOracleBlockNumbers function appends the min and max block numbers from realtime feeds to the end of the arrays. As a result, it is possible for the minBlockNumbers and maxBlockNumbers to be unsorted. This may cause ADL execution to be less predictable since the first minBlockNumber is selected to be the updatedAtBlock for the order:

cache.key = AdlUtils.createAdlOrder(
     AdlUtils.CreateAdlOrderParams(
           dataStore,
           . . .
           cache.minOracleBlockNumbers[0] <---
     )
);

For example:

ADL does not execute when the cache.minOracleBlockNumbers are [x, x+1] since OracleUtils.validateBlockNumberWithinRange prevent executions for the updatedAtBlock of x.

Now with the realtime feed block numbers appended, the cache.minOracleBlockNumbers can be unsorted as [x+1, x] with stale pricing used at block x and the ADL order will execute.

Consider having the block numbers sorted and/or document this potential behavior with the realtime reports.

GMX Team: As long as the ADL order's block number is within every block range of each oracle report, the oracle reports are considered valid and not stale.

The comment “the highest price the a buyer will pay” should read “the highest price that a buyer will pay”.

Implement the above recommended changes.

GMX Team: The recommendation was implemented.

The Errors.InvalidBlockNumber revert supplies the minOracleBlockNumber however it is the maxOracleBlockNumber which failed the block number validation.

Revert with the maxOracleBlockNumber as the invalid block number.

GMX Team: The recommendation was implemented.

In the _setRealtimeFeedActionKey function the “setPriceFeed” string is used to create the realtime feed action key. However following from the patterns of the other action keys, the realtime feed action key ought to use the string “setRealtimeFeed” to match the actionLabel.

Use the “setRealtimeFeed” string to construct the realtime feed action key in the _setRealtimeFeedActionKey function.

GMX Team: The recommendation was implemented.

The comment above the _setPrices function is outdated, it references initializing a SetPricesCache which does not happen immediately in the _setPrices function. Additionally there is no referenced signers param.

Update the documentation for the _setPrices function.

GMX Team: The documentation was updated for the _setPrices function.

The maxPriceAge validation is performed on the currentBlockTimestamp which is based on the upper bound block number. Therefore bid and ask values can technically come from before the maxPriceAge window, though perhaps a trivial amount of time.

Be aware that the maxPriceAge can be slightly exceeded and configure the maxPriceAge as such.

GMX Team: Acknowledged.

The NatSpec for the getUncompactedOracleBlockNumbers function is outdated, it includes the wrong name for the compactedOracleBlockNumbersLength parameter and lacks reference to the reports and oracleBlockNumberType parameters.

Update the NatSpec for the getUncompactedOracleBlockNumbers function.

GMX Team: The NatSpec for the getUncompactedOracleBlockNumbers was updated.

In the event that a new VerifierProxy is deployed by Chainlink the Oracle contract would have to be redeployed as the realtimeFeedVerifier variable is immutable.

However it may be more wieldy to allow the realtimeFeedVerifier implementation to be configurable in the event that a new proxy should be used.

Consider allowing the realtimeFeedVerifier implementation to be configurable in the dataStore rather than immutable.

GMX Team: Have checked with Chainlink that the VerifierProxy should not be changed.

In the event that there are no realtimeFeedTokens, the _validateRealtimeFeeds function logic can be shortcut to avoid expending gas on extra opcodes.

Consider implementing a realtimeFeedTokens.length == 0 early return statement at the beginning of the _validateRealtimeFeeds function.

GMX Team: Acknowledged.

In the _getOutputAmounts function the pool value information emitted with the MarketPoolValueUpdated event is obtained with the following call:

MarketPoolValueInfo.Props memory poolValueInfo = MarketUtils.getPoolValueInfo(
            params.dataStore,
            . . .
            Keys.MAX_PNL_FACTOR_FOR_WITHDRAWALS,
            false
        );

This differs from the call to MarketUtils.getPoolValueInfo in _executeWithdrawal since the pnlFactorType is set to Keys.MAX_PNL_FACTOR_FOR_DEPOSITS and the value is maximized:

MarketPoolValueInfo.Props memory poolValueInfo = MarketUtils.getPoolValueInfo(
            params.dataStore,
            . . .
            Keys.MAX_PNL_FACTOR_FOR_DEPOSITS, <---
            true <---
        );

Consider whether these parameters are desired for the data emitted in the MarketPoolValueUpdated event. If they are not, modify the pnlFactorType to Keys.MAX_PNL_FACTOR_FOR_WITHDRAWALS and set maximize to false.

GMX Team: The recommendation was implemented.

Execution prices with realtime feeds are based on bid/ask spreads from a collection of reference exchanges, however, they do not consider the liquidity at any given bid or ask price. Therefore, a malicious actor may manipulate the lowest ask or increase the highest bid with a relatively small liquidity on a reference exchange and trade based on that synthesized lowest ask or highest bid with a significant size on GMX V2.

Any potential manipulation is however limited in magnitude to the spread between the bids and asks.

Ensure that there is a sufficiently high minimum liquidity barrier to consider a bid or ask and that fees and price impact on the GMX V2 platform can effectively counteract the profitability of any such manipulation.

GMX Team: Have checked with Chainlink that liquidity will be taken into account for determining the bid and ask price.