Synthetics Updates, June 2024, Part 2

createOrder checks every new decrease order for if it will pass over the max autoCancel gas limit using validateTotalCallbackGasLimitForAutoCancelOrders.

However the same check is missed inside updateOrder, enabling the user to:

1. Create 5 decrease orders with their max allowed callback gas, without putting them inside the

autoCancel list.

1. Call updateOrder for all of these order with autoCancel variable set to true.

With this method users can bypass 5 million maximum callback gas limit for Auto Cancel orders and reach up to 10 million. Which will result in reverting liquidations because gas required to liquidate will bypass block gas limit in avalanche.

Add the validateTotalCallbackGasLimitForAutoCancelOrders validation inside updateOrder.

GMX Team: Resolved.

When handling swap orders, the validation for the requestExpirationPeriod is meant to only be applied to MarketSwaps. Since it is applied to both swap types, it will revert for nearly all LimitSwaps.

This will occur because the majority of LimitSwaps will not be eligible to be executed until a later time has passed than the REQUEST_EXPIRATION_TIME.

Only perform this verification for MarketSwaps.

GMX Team: Resolved.

The callback gas amount is included inside an order’s executionFee, however the payExecutionFee function will refund the not used part of executionFee to the user.

Since this unused part includes callback gas, the keeper will not be remunerated for the gas spent during the cancellation callback.

Change the places for order cancellation callback call and execution fee payment.

GMX Team: Resolved.

When an order gets cancelled there is a check to see if the order has a cancellationReceiver. If it does the funds will be sent there, if not then the account will get the funds. This works well, however there is no way for a user to add a cancellationReceiver when creating an order. Preventing the use of this feature.

Set the cancellationReceiver when creating an order and validate that the address used is valid.

GMX Team: Resolved.

The sequencer uptime check is performed only in: Atomic Withdrawal, Normal Withdrawal and Liquidations.

If sequencer is down, while it won't be possible to execute these functions, rest of the protocol will continue functioning if they don't have a priceFeed to check for reference price.

Localize the sequencer checks to exactly where the Chainlink Aggregator Price is used.

GMX Team: Acknowledged.

validateGasLeftForCallback() verifies that the gas left in the transaction is enough to call the callback contract. However, validateGasLeftForCallback() checks gasLeft() and forgets to account that 1/64th of the gas is reserved when making an external call. Although this case is less likely to occur, it has the same impact as H-06.

Verify that the gasLeft() subtracted by the gas withheld from making an external call is greater than the callback gas limit.

GMX Team: Resolved.

MAX_TOTAL_CALLBACK_GAS_LIMIT_FOR_AUTO_CANCEL_ORDERS can change according to gas requirements of the system/chain. If this value decreases however, the position holders that already have maximum amount of callback gas used for their autoCancel orders can not call decrease order because the call will revert with MaxTotalCallbackGasLimitForAutoCancelOrdersExceeded.

Before reducing this variable inform users about this problem and let them prepare their positions to handle with this case. Additionally, this validation does not need to take place for MarketDecrease orders.

GMX Team: Acknowledged.

If a decreaseOrder will close the position altogether, the autoCancelList will be cleared out during that order's execution. But gas provided by the keeper won't be checked if it is sufficient to handle the gas required for cancellations of these orders that are in autoCancelList. If keepers don't provide enough gas for all auto-cancellation logic, since the gas provided is not validated to cover these auto-cancellations with validateExecutionGas, a position closing order created by the user will be cancelled instead of reverting. This can lead to the censoring of closing orders for users and can lead to unfair liquidations and loss of funds.

There are different possible solutions that comes with some caveats. 1- Query the position to see if order size is the entire position. If so, increase the result of estimateExecuteOrderGasLimit when the decrease order has auto cancel orders. 2- Before starting clearAutoCancelOrders() check if there is enough gas, if not revert such that order is not cancelled and the error is caught in the _handleOrderError function as a keeper mistake. Note that both of these first two solutions has a griefing vector whereby someone can frontrun the execution transaction and update an order to be an auto-cancel one such that the required gas for both will change. Which can lead to revert for keeper's execution error. 3- Separating the logic of auto cancellation from order execution. Emitting an event after position is completely closed and letting keepers to call clearAutoCancelOrders in a seperate transaction can solve the problem in a safer way, which will also address high gas usage concerns. The caveat for this is the execution logic change itself.

GMX Team: Resolved.

Proof of concept: PoC

Based on the estimated gas usage configurations, the gas required to execute some decrease and liquidation orders may exceed the Avalanche block gas limit of 15,000,000 gas:

• Liquidation's gas usage itself: 4,000,000 (Decrease order gas limit)
• afterOrderExecution callback gas: 2,000,000
• Main payExecutionFee: 500,000
• 5 autoCancel order cancellation: 5 x 600.000 = 3,000,000
• 5 autoCancel order cancellation callback: 5,000,000
• 5 autoCancel payExecutionFee: 5 x 500,000 = 2,500,000
• In total = 17,100,000 which is 2,100,000 more than avalanche block gas limit.

However in practice, it is unlikely that a liquidation will consume 15,000,000 or more gas units, refer to the attached PoC where we show that the rough maximum gas usage for a liquidation is around 14,000,000 gas.

If liquidation execution can consume more than 15,000,000 gas units this would result in unliquidatable positions on the Avalanche network, which will introduce bad debt into the system.

Carefully consider this limit when making future code updates and modifying the refundExecutionFeeGasLimit as well as other gas configurations.

GMX Team: Acknowledged.

The EXECUTION_GAS_FEE_BASE_AMOUNT key has been replaced with an EXECUTION_GAS_FEE_BASE_AMOUNT_V2_1 key to allow an increased base fee to be charged for additional gas expenditures in V2.1.

However the corresponding estimated fee which is required upon order creation is still based upon the ESTIMATED_GAS_FEE_BASE_AMOUNT which corresponds with the old estimated base gas fee amount.

As a result the estimated fee which users are required to pay upfront may be insufficient to cover the gas expenditure for order execution in the V2.1 system.

Consider implementing a ESTIMATED_GAS_FEE_BASE_AMOUNT_V2_1 which corresponds to the EXECUTION_GAS_FEE_BASE_AMOUNT_V2_1 value.

GMX Team: Resolved.

The file config/tokens.ts implements the configuration for all tokens and their oracles. In the GMX tokens, realtimeFeedId and realtimeFeedDecimals are used instead of the new dataStreamFeedId and dataStreamFeedDecimals. This will cause the setup to fail/revert.

Change the names of the two variables.

GMX Team: Resolved.

estimatedDepositOraclePriceCount(), estimatedWithdrawalOraclePriceCount(), estimateOrderOraclePriceCount(), & estimateShiftOraclePriceCount() make an assumption that the long, short, and index tokens will all be different tokens. This is not always the case since index token, long token, and short token can be the same. The oracle price count is then multiplied by EXECUTION_GAS_FEE_PER_ORACLE_PRICE and added to the Keeper’s fee. This will charge users unnecessary fees with each interaction to the protocol.

Store a variable that tracks the amount of tokens that have their prices set. Then when calculating the Keeper’s fee, utilize this value.

GMX Team: Acknowledged.

The maximum amount of auto cancels multiplied by the max callback gas limit is greater than the max total callback gas limit for auto cancels. This can be an issue for users and integrators who are not aware of this caveat, and attempt to add the maximum amount of auto cancels to a position.

Be sure to document this behavior to alert users and integrators of this scenario.

GMX Team: Acknowledged.

Because the funds are sent to the account instead of the callback contract when an order is cancelled it could be unexpected for users and integrating protocols, making it become difficult for the callback contract to handle these funds as they would receive the executionFee refund, but not the input token amount for deposits, withdrawals, or orders.

Document this behavior so integrators and users can build accordingly.

GMX Team: Acknowledged.

createDeposit() calls estimatedWithdrawalOraclePriceCount(). The logic is the same as estimatedDepositOraclePriceCount(), so there is no impact, however the naming convention is wrong.

Switch estimatedWithdrawalOraclePriceCount() to estimatedDepositOraclePriceCount() in createDeposit().

GMX Team: Resolved.

To verify update the auto cancel list an order must either be a LimitDecrease or StopLossDecrease with the new change to updateAutoCancelList(). However, isDecrease() is still used in createOrder. This will cause MarketDecrease orders to call validateTotalCallbackGasLimitForAutoCancelOrders() when it is unnecessary.

Use the same check from updateAutoCancelList() in createOrder().

GMX Team: Resolved.

The additionalBorrowingFactorPerSecond in the getKinkBorrowingFactor function is initialized to 0 and is only changed if aboveOptimalUsageBorrowingFactor is less than or equal to baseBorrowingFactor. Therefore the borrowingFactorPerSecond will not grow since multiplication by 0 will cause additionalBorrowingFactorPerSecond * diff / divisor to be 0.

Consider verifying that the aboveOptimalUsageBorrowingFactor is always be greater than baseBorrowingFactor upon configuration.

GMX Team: Acknowledged.

In the GasUtils file the oracle price estimation functions accept a swapCount and add this value to the resulting estimated oracle prices necessary. However the estimation does not accurately account for the prices required for swaps. For example:

• Consider a deposit to the USDC/WETH market
• longTokenSwapPath = [USDT/ATOM, ATOM/DAI, DAI/WETH]
• shortTokenSwapPath = [SOL/WBTC, WBTC/ARB, ARB/USDC]

In the worst case, all 6 of these markets in the swap path have a different index token, and the deposit market has a unique index token as well. Yielding 7 prices necessary just for index tokens. Then all tokens used in the swapPaths are necessary: [USDT, ATOM, DAI, WETH, SOL, WBTC, ARB, USDC] which adds 8 more potential prices.

While the existing validation assumes that the worst case is 8 prices as mentioned in the comments, and 9 prices as the maximum returnable by the estimatedDepositOraclePriceCount and estimatedWithdrawalOraclePriceCount functions — the actual worst case is 15 total prices.

Multiply the swapCount by 2 in each of the estimatedDepositOraclePriceCount, estimatedWithdrawalOraclePriceCount, and estimateOrderOraclePriceCount functions in order to accurately represent the worst case amount of oracle prices required for the action.

GMX Team: Acknowledged.