Synthetics V2, Review 4

Several critical keys are missing from the initAllowedBaseKeys function:

• MIN_POSITION_SIZE_USD
• MAX_PNL_FACTOR_FOR_DEPOSITS
• MAX_PNL_FACTOR_FOR_ADL

Add the missing keys to initAllowedBaseKeys.

GMX Team: The recommendation was implemented in commit 7bbadf29.

Proof of concept: PoC

In the getLiquidationValues function the values.pnlAmountForPool is reset to a new value, although the previous value may have been used in a swap from the pnlToken to the collateralToken.

This causes mis-accounting in the market and causes a revert with the market token balance check upon liquidation. Therefore positions can be unliquidatable, yielding a potentially catastrophic amount of bad-debt for the market.

The solution is to account for the previous values.pnlAmountForPool in the event that this swap was made. e.g. add:

if (wasSwapped) {
   MarketUtils.applyDeltaToPoolAmount(
       params.contracts.dataStore,
       params.contracts.eventEmitter,
       params.market.marketToken,
       values.pnlTokenForPool,
       values.pnlAmountForPool
   );
}

to the else branch in getLiquidationValues.

GMX Team: The recommendation was implemented in commit cd9f9c2.

Positions in profit with unpaid borrowing/funding fees that are greater than the position’s collateral open the exchange up to several high-impact issues when the swapProfitToCollateralToken swap fails. These positions are able to exist since the isPositionLiquidatable check factors positive PnL as collateral that would purportedly always be able to cover these fees.

However swapProfitToCollateralToken will commonly fail whenever the validatePoolAmount, validateReserve, or validateMaxPnl checks fail as a result of the swap, causing the following issues:

• Positions in large profit would be un-ADL-able since the ADL order would revert on line 224 as the collateral is not sufficient to cover the fees alone.
• Liquidations for these positions result in the user losing all of their profit since the execution enters getLiquidationValues.
• Liquidations for these positions result in the protocol having to cover a potentially large deficit between the position’s collateral and the unpaid funding fees.
• The pool value for market depositors sees a stepwise jump down from the potentially large unpaid borrowing fees.
• Decrease orders expecting to be able to use their profit to pay fees will be cancelled/frozen

Do not allow positions to exist when their fees are greater than the actual collateral backing the position. Allow positions to be liquidated when their fees negate the collateral backing amount.

GMX Team: The team acknowledged this scenario and made the unswapped PnL claimable in commit c1d9428.

Proof of concept: PoC

The uint poolValue is decreased by the impact pool value before the PnL is added to the poolValue. Additionally, the impact pool value is not capped to avoid underflow.

Because of this, there are some cases where the market can be entirely bricked when the value of the impact pool surpasses the value of the backing tokens — even if it was meant to offset a positive pool PnL.

A malicious actor can engineer this outcome in certain scenarios, especially when the pool is initially deployed.

Consider moving the poolValue -= result.impactPoolAmount * indexTokenPrice.pickPrice(maximize); line to after the PnL is added to the poolValue, as the impactPoolAmount is meant to offset initial positive pnl. Otherwise, consider making the poolValue an int within the getPoolValueInfo function.

Additionally, consider capping the value of the impact pool (similarly to the capping of PnL) that is subtracted from the poolValue to avoid any cases where the market is bricked.

GMX Team: The recommendation was implemented in commit aea23c6.

In the getExpectedMinTokenBalance function, the collateralForLongs and collateralForShorts is included in the resulting expectedMinBalance.

Therefore users who are paid out funding fees will be unable to claim them until the users who are paying the funding fees update their position. There is no requirement for users to frequently update their position and therefore funding fees can go a long time without being claimable for users.

Adjust getExpectedMinTokenBalance such that funding fees that will be paid from user’s collateral can be immediately claimed without affecting the validation.

GMX Team: The recommendation was implemented in commit 5b1be2f.

When a remove oracle signer is signaled, the action key used is:

bytes32 actionKey = _removeOracleSignerActionKey(account);

However, removeOracleSignerAfterSignal uses an incorrect action key to validate the signal. Specifically, it uses _addOracleSignerActionKey instead of _removeOracleSignerActionKey.

Therefore a signer cannot be removed using the removeOracleSignerAfterSignal function.

Use the _removeOracleSignerActionKey in the removeOracleSignerAfterSignal function.

GMX Team: The recommendation was implemented in commit 3ef1161.

For stop-losses, the triggerPrice is used to represent the price of the asset instead of just using it as a trigger for execution. In traditional markets, the stop-loss price is not the guaranteed execution price especially in times of heavy volatility.

Users can open a long and place a SL ever so slightly below the current price. If they get stopped out then they will lose out on fees. However, with high leverage, the upside gain is immense with little risk. Such a high reward will come at the expense of the pool, hurting LPers and the market as a whole.

Ultimately, this allows sophisticated traders to have a superior strategy than that of traditional stop loss orders where they are treated like mere triggers – hurting the profitability of LPers.

Use the latest price (secondaryPrice) when executing a stop loss order rather than giving the user their exact triggerPrice.

GMX Team: Order pricing was switched to validate against primary price in commit 3243138.

The uiFee can be manipulated during the order/deposit/withdrawal execution to determine whether or not the action is executed and circumvent the validateRequestCancellation period.

Ultimately this allows malicious users to make a short-term risk-free trade as they can decide whether or not their action should be executed successfully with prices from a few blocks ago.

For example, during a withdrawal, a malicious user can re-enter the system during the execution of the first swap (WithdrawalUtils.sol: 388) into the ExchangeRouter.setUiFeeFactor function and change the uiFeeFactor for the uiFeeReceiver of the withdrawal executed.

The change in uiFeeFactor can make the difference between the subsequent swap satisfying the minOutputAmount — therefore deciding whether the withdrawal can go through.

Additionally, note that a similar effect can be achieved for any order/deposit/withdrawal by simply front-running the execution tx and changing the uiFee.

Notice that malicious uiFeeReceivers can manipulate the uiFeeFactor after a user submits their order/deposit/withdrawal. This way a uiFeeReceiver can promise a uiFee of .05%, but adjust it to be much higher right before the actual execution.

Do not allow the uiFeeFactor that is experienced in the execution to be changed after the order/deposit/withdrawal is created/updated.

GMX Team: Acknowledged.

A malicious user can manipulate the referral code associated with their account to decide whether or not a trade should be executed.

For example, the referral code could be switched to a higher discount just in time to allow a MarketIncrease execution tx to pass the order.minOutputAmount() validation and go through.

This allows a malicious trader to make a short-term risk-free trade as they can decide whether or not they should be executed successfully with prices from a few blocks ago.

Additionally, the customDiscountShare of a single discount code could be leveraged in the same way.

Notice that allowing the referral codes to be adjusted after orders are submitted also allows for referrer manipulations. This way referrers can promise a 2% discount but front-run order executions to adjust the customDiscountShare such that the trader receives no discount.

Store the referral discount on a per-order basis. Do not allow the referral discount to be adjusted in real-time by either the referral code being used or the customDiscountShare of a particular code.

GMX Team: Acknowledged.

The prices for a stop-loss are required to straddle the trigger price in setExactOrderPrice. In the case of a price gap where both the primary and secondary prices fall below/above the trigger price, the stop-loss will fail to execute. This will prevent a position’s profit from being secured or loss to be mitigated

For example, if the trigger price is $100 for a long SL but price gaps to $99 (primary) -> $98 (secondary) the SL will not be triggered and the user will still have exposure in the market.

Do not revert if both primary and secondary prices fall below/above the trigger price.

GMX Team: Validation is now performed only between primary and trigger price in commit 3243138.

Proof of concept: PoC

When a position is liquidated with getLiquidationValues the pnlAmountForPool is set to params.position.collateralAmount() - fees.funding.fundingFeeAmount.

However, this value does not account for the amount incremented for the claimable collateral with incrementClaimableCollateralAmount in the event that the price impact is capped.

This will result in a revert since the claimableCollateralAmount is included in the getExpectedMinTokenBalance. Therefore making a position unliquidatable when the price impact is capped.

Be sure to appropriately set aside the collateralCache.pnlDiffAmount in the cache.pnlToken when liquidations enter the getLiquidationValues function.

GMX Team: pnlAmountForPool delta is applied to the pool prior to the swap which includes the pnlDiffAmount in commit cd9f9c2.

Proof of concept: PoC

When calculating the PositionPricingUtils.getPriceImpactAmount the difference between the executionPrice and the latestPrice is used to derive the resulting priceImpactAmount for the positionImpactPool.

However, the executionPrice can be modified to be the acceptablePrice in the event that the acceptablePrice cannot be fulfilled by the initial max/min latestPrice. This will lead to a difference in the executionPrice and the latestPrice that is not necessarily from the priceImpactUsd amount.

Example

• Consider a LimitIncrease Long order
• priceImpactUsd is 0 for simplicity, although this applies when priceImpactUsd is nonzero
• The acceptablePrice is not fulfilled by the triggerPrice (max) so the acceptablePrice is used
• triggerPrice is used as the _latestPrice in getPriceImpactAmount
• However triggerPrice != acceptablePrice (where the acceptablePrice is my executionPrice)
• Therefore there is a nonzero priceDiff in getPriceImpactAmount, this is errantly credited as positive PI and taken out of the impact pool when there is no impact.

As a result, when the acceptablePrice is more favorable than the triggerPrice, the positionImpactPool is decreased even when the user caused a non-trivial imbalance in the OI and initially had negative priceImpactUsd.

This will influence the positionImpactPool to trend towards 0 as more orders that have an acceptablePrice that is more favorable than the triggerPrice are executed. Ultimately this stifles any amount of positive impact that can be offered to users to balance the OI, since the positive impact amount is capped to the balance of the positionImpactPool.

Consider removing the feature where the user may get their acceptable price if the first price + impact is not fulfillable and rather revert and have the order canceled/frozen if the acceptablePrice is not met.

Otherwise do not allow users to set an acceptablePrice that is more favorable than the triggerPrice.

GMX Team: The recommendation was implemented in commit 3243138.

Proof of concept: PoC

It is possible for user’s withdrawals to revert because the pending borrowing fees are attributed to the user’s withdrawal but have not yet been added to the poolAmount.

In cases where there is a significant amount of unpaid borrowing fees this can become a non-trivial issue for users attempting to withdraw.

Consider allowing a separate claiming process for borrowing fees, or implementing a pathway for regular position updates to pay the pending borrowing fees.

GMX Team: Acknowledged.

The getLatestPrice function is used to get prices for swaps, however, this will return the custom price for the token if any is set. In the case of a MarketIncrease long order, the min and max price for the customPrice are both the max of the primaryPrice. The inverse can be true using a MarketDecrease short order.

This way users can get more favorable execution while swapping for the index token during a Market order.

This invalidates the implemented protection where the inToken is supposedly valued at the minimum price and the outToken is valued at the max price:

cache.amountOut = cache.amountIn * cache.tokenInPrice.min / cache.tokenOutPrice.max;

Do not allow the max of the primaryPrice to be used as the price for the tokenIn during a swap.

GMX Team: The recommendation was implemented in commit 3243138.

The validateOracleBlockNumbers function for LimitSwaps does not allow oracle block numbers to be equal to the orderUpdatedAtBlock. This is in contradiction to the oracle block validation for increase and decrease orders.

Additionally, this unnecessarily requires that limit swaps be executed at a delayed block number, when the current block number may provide a more favorable execution for the trader.

Change the requirement from !minOracleBlockNumbers.areGreaterThan(orderUpdatedAtBlock) to !minOracleBlockNumbers.areGreaterThanOrEqualTo(orderUpdatedAtBlock).

GMX Team: The recommendation was implemented in commit c5fdc29.

A malicious actor can observe a user’s triggerPrice for their stop-loss (among other order types) approaching and shift price impact in the user’s market (or in the user’s virtual inventory) such that their minimum output becomes invalidated and the order gets canceled.

In some cases this could cause significant grief to users who would have otherwise exited the market. A malicious actor may stand to benefit from this by holding MarketTokens and griefing traders within that market.

Document this behavior clearly to users. Monitor such manipulations and disincentivize them accordingly by adjusting the price impact factors as necessary.

GMX Team: Acknowledged.

The poolAmountAdjustmentKey function in Keys.sol uses the POOL_AMOUNT key instead of the POOL_AMOUNT_ADJUSTMENT key.

There are luckily no catastrophic consequences as this is an int value and the poolAmountKey is a uint, however, it poses a significant risk to any future changes and would cause confusion/potential issues for those reading using the POOL_AMOUNT_ADJUSTMENT key.

Alter the poolAmountAdjustmentKey function to use the POOL_AMOUNT_ADJUSTMENT key.

GMX Team: The key was entirely removed.

When increasing a position, PositionUtils.validatePosition is called after incrementing the OI for the position increase. However, the validation will re-compute the price impact amount based on this updated OI.

The resulting cache.priceImpactUsd in isPositionLiquidatable would be inaccurate to the actual price impact experienced. Therefore some positions may be errantly prevented from being opened with this validation.

Validate the position based on the previous OI, therefore accurately representing the OI delta of the order.

GMX Team: Acknowleged.

The estimatedRemainingCollateralUsd is incremented by a token amount rather than a token amount multiplied by a price:

estimatedRemainingCollateralUsd += params.order.initialCollateralDeltaAmount().toInt256();

As a result, the main effect is estimatedRemainingCollateralUsd is much smaller than it should be and the position is more likely to get closed out in its entirety unexpectedly due to the MIN_COLLATERAL_USD check.

Multiply the params.order.initialCollateralDeltaAmount() by the price of the collateral token to receive a USD value before adding it with the estimatedRemainingCollateralUsd.

GMX Team: The recommendation was implemented in commit f8f2dd6.

When the positiveImpactAmount is calculated during a deposit, the _params.priceImpactUsd is divided by the tokenPrice.max to be converted into an outToken amount. However the token amount is converted back to a USD amount when incrementing the mintAmount, positiveImpactAmount.toUint256() * _params.tokenOutPrice.min.

This means that users are negatively impacted by the price spread because they receive less positive impact than they otherwise would have.

Multiply the positiveImpactAmount by the _params.tokenOutPrice.max so that users are not negatively impacted by the price spread.

GMX Team: The recommendation was implemented in commit 27a9164.

It is possible for a state to arise where liquidations are enabled but other order types are disabled. For example, increase orders can be disabled and a user is unable to add collateral to their position. Fees will accumulate until a user’s position is liquidatable which leads to loss of funds.

Consider disallowing liquidations when a user is unable to adjust their order due to a feature being disabled.

GMX Team: Acknowledged.

Even with the heartbeat validation, it may be prudent to validate that the Sequencer is active to prevent stale pricing.

This would avoid any scenarios where price movement triggers an update within the heartbeat duration but the new price is not reported to the L2, allowing traders to take advantage of stale pricing.

Validate whether the Sequencer is active or not. Furthermore, document keeper behavior in the case that the Sequencer is down.

GMX Team: Acknowledged.

In the Chain.sol file, uint256 constant public ARBITRUM_CHAIN_ID = 42161; is hardcoded.

From a comment in Oracle.sol, the codebase wishes to be impervious to a change in the chain ID:

// it might be possible for the block.chainid to change due to a fork or similar

However in the event that the Arbitrum chain ID changes, the currentBlockNumber and getBlockHash functions will not return the appropriate Arbitrum values, potentially causing drastic effects on the exchange.

Add a configurable chain ID for Arbitrum.

GMX Team: Acknowledged.

The PnL of the remaining position is no longer accounted for during the willPositionCollateralBeSufficient check.

In the case where the remaining position PnL is positive, this avoids errantly counting profit towards the remaining position’s collateral.

However, in the case where the remaining position PnL is negative, this check fails to consider that the remaining PnL could make the actual value backing the position significantly smaller than the minCollateralUsdForLeverage.

It may be prudent to consider the PnL of the remaining position, only when it is in a loss. This way the negative PnL, which would be subtracted from the collateral in the position, is taken into account.

Consider factoring the remaining position’s PnL into the willPositionCollateralBeSufficient check, only when the remaining PnL is negative and would be subtracted from the collateral in any future order.

GMX Team: Acknowledged.

In the event of a block re-org a malicious trader may see that price has moved against them and decide to get their order canceled rather than recorded.

Consider the following scenario:

• Bob sees the keeper execute his MarketIncrease in block A
• The block re-org occurs over a period of 1 minute
• Bob sees the re-org is happening and sees that the execution of his order has since lost money and gets his tx to cancel the order recorded in block B which will come before block A.
• Bob can make his tx cancel his MarketIncrease by having it send the tokens necessary for the MarketIncrease order elsewhere.
• Bob’s order is canceled rather than executed since it did not net him any profit.

Notice that there are likely many ways to exploit the two-step execution process during a re-org.

Beware of potential risks to the system during block re-orgs and communicate that risk with users. Consider implementing a mechanism to freeze all orders that were executed during a block re-org.

GMX Team: Acknowledged.

There are several instances where a user may gain control over the order execution tx before the dataStore has been properly updated.

Firstly, in the SwapOrderUtils.swap function, the SwapUtils.swap is executed before the swap order is removed from the dataStore. Since the swap can have shouldUnwrapNativeToken == true the order.receiver will get called upon receiving the output of the swap.

Since native token transfers forward 200,000 gas (from the test environment) the receiver will have ample gas to potentially exploit any system building on top of GMX V2 and relying on the swap order in the dataStore.

Similarly in the OrderUtils.cancelOrder function, the orderVault.transferOut is executed before the order is removed from the dataStore.

In the SwapOrderUtils.swap function, remove the order with OrderStoreUtils.remove before the SwapUtils.swap. And in the OrderUtils.cancelOrder function, remove the order with OrderStoreUtils.remove before the orderVault.transferOut.

This way third parties building on top of GMX V2 cannot be exploited by the outdated order state in these instances.

GMX Team: The recommendation was implemented in commit 27a9164.

Similarly to the minimum position size, it may be prudent to add a minimum order size for both the sizeDeltaUsd and initialCollateralDeltaAmount in the case where they are greater than 0 to avoid potential manipulation.

Consider introducing a minimum sizeDeltaUsd and a minimum initialCollateralDeltaAmount that take effect when either of each field is not 0.

Additionally, it may be prudent to introduce similar minimums for deposits and withdrawals.

GMX Team: Acknowledged.

The ExecuteOrderParams struct contains a positionKey variable that is never assigned nor referenced.

Remove the positionKey variable from the ExecuteOrderParams struct.

GMX Team: The recommendation was implemented.

In the _handleOrderError, _handleWithdrawalError and _handleDepositError functions, the revert reason is parsed before it is necessary. In many cases the function will return before the parsed string memory reason is used.

Move the ErrorUtils.getRevertMessage call after the ErrorUtils.revertWithCustomError case to save gas in the event of a revert.

GMX Team: The recommendation was implemented.

Throughout the codebase it is mentioned that the internal state changes of a market should be validated with validateMarketTokenBalance before handing over execution of the tx to a user.

However there are several places where the user may gain control of the tx before validateMarketTokenBalance has been called.

If a user has shouldUnwrapNativeToken == true, they will gain control over execution of the tx before the internal state changes of a market have been validated upon the swaps in withdrawals and orders.

Consider additionally validating the internal state changes of the market before potentially handing over control of the tx execution to an arbitrary address on swaps.

GMX Team: Acknowledged.

The InvalidFactor error is never used.

Remove the InvalidFactor error.

GMX Team: The recommendation was implemented.

The cache.longTokenAmount * prices.longTokenPrice.midPrice is re-computed when calculating price impact during a deposit but this value is already stored in the cache.longTokenUsd.

Similarly for cache.shortTokenUsd.

Use cache.longTokenUsd and cache.shortTokenUsd rather than re-computing these values.

GMX Team: The recommendation was implemented.

The NatSpec documentation for the GetPriceImpactUsdParams struct refers to a longToken and shortToken that are no longer there.

Update the NatSpec documentation for the GetPriceImpactUsdParams struct.

GMX Team: The recommendation was implemented.

Although the exchange does not intend to use ERC-777 tokens it should be emphasized that the system is vulnerable to them. Tokens with callbacks allow receivers to revert with an arbitrary revert string that can potentially cause a revert in the decoding process leading to a risk free trade opportunity.

Take care with the tokens able to be used on the exchange. Do not ever allow ERC-777 tokens to be used in the system.

GMX Team: Acknowledged.

The comment purports that the usdDelta offset is necessary to prevent overflow, however it prevents underflow.

Update the comment to mention underflow instead of overflow.

GMX Team: The recommendation was implemented.

If there exists a single market in the GMX ecosystem that fails the validateMarketTokenBalance check on a swap or for any other reason, it can be leveraged to perform a short term risk free trade.

A malicious user can include one of these markets in their swapPath and decide whether the order should be able to go through by “plugging the hole” in the market that would otherwise fail the validateMarketTokenBalance check.

Monitor markets closely to observe if any have entered such a state and be careful to not introduce any such markets with admin intervention.

Additionally, consider using the validateMarketTokenBalance check on the markets involved in a users order, deposit, or withdrawal upon creation.

GMX Team: Acknowledged.

The comment in the getLatestPrice function mentions that the acceptablePrice may be used as the customPrice but this is not true.

Do not mention the acceptablePrice in this comment.

GMX Team: The recommendation was implemented.

In the getNextTotalBorrowing function the prevPositionBorrowingFactor and nextPositionBorrowingFactor are applied without the use of applyFactor.

Favor the use of applyFactor for the prevPositionBorrowingFactor and nextPositionBorrowingFactor.

GMX Team: The recommendation was implemented.

The revertOracleBlockNumbersAreNotEqual function is never utilized and therefore the Errors.OracleBlockNumbersAreNotEqual error is never thrown.

Therefore the entire if statement checking for errorSelector == Errors.OracleBlockNumbersAreNotEqual.selector in the isOracleBlockNumberError function can be removed.

Remove the unused error and related logic.

GMX Team: The recommendation was implemented.

An order is validated to be non-empty twice during the createOrder function. The if case on line 130 can be removed as it is duplicated by the validateNonEmptyOrder call.

Remove the bespoke if case.

GMX Team: The recommendation was implemented.

There are two claimableFundingAmountKey functions, one with an account address parameter and one without. However the NatSpec for both of them references an account address parameter.

Remove the account parameter in the NatSpec for the claimableFundingAmountKey which does not have such a parameter.

GMX Team: The recommendation was implemented.

The Printer.sol file is errantly imported into the DataStore.sol file.

Remove the unnecessary import.

GMX Team: Acknowledged.

Throughout the codebase Chain.currentTimestamp is utilized however in the _getPriceFeedPrice function block.timestamp is used directly.

Use Chain.currentTimestamp rather than block.timestamp directly.

GMX Team: The recommendation was implemented.