Synthetics V2, Review 3

Proof of concept: PoC

When using the getPoolValue function for markets with identical long and short backing tokens, the cache.longTokenAmount and cache.shortTokenAmount represent the same token amount.

However both of these token amounts are counted in the value of the pool. Therefore the deposited pool value is doubled.

Additionally, several validations e.g. validateReserve, validateMaxPnl, and validatePoolAmount among others are immediately invalidated as they errantly count the same exact token amount for both the short and long side. This way the single backing token pool is effectively double counted as backing both long positions and short positions.

Similarly open interest in these markets is double counted in the getNextFundingAmountPerSize function, producing a completely invalid calculation for funding fees.

Reconsider if markets with the same backing longToken and shortToken should be possible. If they should, then handle their accounting/validation separately.

GMX Team: A pool divisor has been added to account for these markets.

Proof of concept: PoC

When creating an order there is no validation that the swapPath is under a certain max length. This allows malicious users to create risk-free trades on the exchange.

Notably, among other ways, a trader may submit a MarketIncrease order with a swapPath that puts the order just over the block gas limit when combined with a callback that consumes nearly the entire maxCallbackGasLimit. When a trader wishes the trade to be executed using outdated prices, they can toggle the callback contract to only consume a very small amount of gas, enabling the order to be executed and recorded in a block.

Add validation on the max length for the swapPath of orders to protect the exchange from the entire class of swapPath gas manipulation attacks.

GMX Team: A maximum swapPath length has been implemented.

Proof of concept: PoC

In the getNextOpenInterestParams function the nextLongOpenInterest and nextShortOpenInterest variables are not initialized from the default values and only one is set in either of the params.isLong cases.

This drastically misrepresents the open interest balance of the market and yields nonsensical price impact calculations.

Initialize each of the nextLongOpenInterest and nextShortOpenInterest values to the current open interest on each side.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

In the getPoolValue function, the position impact pool value is added to the value of the pool. However the position impact pool is comprised of a notional value of synthetic index tokens, therefore it should not be added to the pool value.

When a trader is negatively impacted, two things happen:

1. indexTokens are taken from the trader and allocated to the position impact pool.
2. The trader immediately experiences a loss on their PnL.

This results in the pool accounting for this negative impact amount twice. Once for the indexTokens that are allocated to the position impact pool. And a second time for the negative PnL that the trader has just experienced.

This double counting invalidates the market’s accounting system and does not allow depositors to withdraw all of their MarketTokens.

Do not account the position impact pool as a part of the net poolValue.

GMX Team: The value of the impact pool is now subtracted from the poolValue.

Proof of concept: PoC

In the getNextFundingAmountPerSize function, the fundingAmountPerSizePortion values are calculated by dividing the corresponding fundingUsd by the open interest of both longs and shorts. This is done to get a value that is to be received/paid for each collateral type for each position direction.

This logic makes sense in the case where users are being paid the funding fees since they are able to claim both the long and short tokens. However users are only able to pay the fundingAmountPerSizePortion value for their respective collateral token.

This errantly reduces the amount the fundingAmountPerSizePortion that is paid for each corresponding collateral token as the amounts are divided by the entire open interest of the side that is paying funding fees. As a result, only a portion of the funding fees are being paid while the entire amount can be claimed, and a deficit in the pool accounting is created which will force markets into insolvency over time.

Implement separate logic for the side that is paying the funding fees where the funding fees are not spread across the entirety of the open interest for that side, but rather the open interest of that side that is able to pay the particular token through their collateral.

E.g.

cache.fps.fundingAmountPerSizePortion_LongCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.longOpenInterestWithLongCollateral);

GMX Team: The funding fees are now divided amongst traders with the appropriate collateral. 23

Proof of concept: PoC

When users attempt to claim their collateral, the adjustedClaimableAmount is asserted to be < the claimedAmount, otherwise the tx reverts. However if a user has not claimed any of their collateral the claimed amount will be 0 and therefore the adjustedClaimableAmount cannot be strictly less than the claimedAmount.

Therefore users are unable to claim their collateral.

Modify the if statement to revert in the case where adjustedClaimableAmount < claimedAmount.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

Addresses that are blacklisted for popular ERC20 tokens such as USDC can be leveraged to exploit the exchange in a number of ways.

These addresses cannot be liquidated in any case where they would be transferred back a leftover collateral amount in a token which they are blacklisted for.

Among other ways, blacklisted addresses can execute risk-free trades using MarketIncrease orders in the following way:

1. Force the collateral swap to fail via low liquidity in a niche market.
2. The order cannot be cancelled since the cancellation would attempt to send the token that the user is blacklisted for.
3. Therefore the order will remain in the dataStore until the liquidity is added.
4. Deposit liquidity into the low liquidity market so the MarketIncrease can go through when the attacker wants it to, using out of date prices for a risk-free trade.

Be extremely cautious when adding markets with tokens that include a blacklist. Consider implementing checks to see if users are blacklisted and denying them service to the relevant markets.

GMX Team: If a user is found to be blacklisted when transferring to them, the tokens are now sent to a holding address.

Proof of concept: PoC

In the case where the remainingCollateral for a position is negative during a liquidation, the getLiquidationValues function is called. The values.pnlTokenForPool is used in the returned values. However the values.pnlAmountForPool computed on line 347 is strictly a collateral token amount.

A position may be in profit and still be liquidated due to the minCollateralUsdForLeverage combined with a depreciation in the user’s collateral token price.

Therefore the pnlTokenForPool may be different from the collateral token, in which case a shortToken amount could be applied as a longToken amount or vice-versa. This will drastically perturb the poolAmount for the pnlTokenForPool in such a way that could leave the market insolvent or simply cause extreme loss for the market depositors.

Adjust the getLiquidationValues function so that it accounts for the cases where traders are being liquidated in profit.

GMX Team: The getLiquidationValues function was refactored.

Proof of concept: PoC

In each callback function the bytes memory reasonBytes returned from the third party callback contract in the event of an error is loaded into memory in the catch case. reasonBytes can be potentially very large and therefore extremely gas intensive when it is copied into memory.

Furthermore, the catch block continues to perform computation with reasonBytes, first parsing the error message with ErrorUtils.getRevertMessage and also emitting an event that contains reasonBytes.

An attacker may simply implement a callback contract that reverts with an extremely large reasonBytes so that the execution tx would require more gas than the block gas limit. The attacker can then toggle the callback contract to no longer revert when they want their order to be executed successfully, enabling a risk-free trade.

In another attack, a trader could observe the keeper’s execution tx in the mempool and front-run it to toggle the callback contract to revert with a large reasonBytes. This would cause the keeper’s execution tx to consume an unforeseen amount of gas and likely run out of gas and fail. The attacker could leverage this in a similar manner to create a risk-free trade opportunity.

Do not load the third party callback contract’s error reasonBytes into memory.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

Removing all of the collateral from a position will result in the EmptyPosition error, which will be retried for LimitDecrease orders.

An attacker can leverage this by creating a LimitDecrease order that initially only reduces their position.sizeInUsd by half, but reduces their collateral to 0. The LimitDecrease will continue to result in the EmptyPosition error until the attacker creates a MarketDecrease order that reduces the size of their position by half. Now when the original LimitDecrease is executed, it will close the position and no longer revert with the EmptyPosition error.

A malicious trader can leverage this to make a risk-free trade with their LimitDecrease.

Revert with the InsufficientCollateral error, which is not retried, in the case where values.remainingCollateralAmount is 0.

GMX Team: Limit orders no longer revert on the EmptyPosition error.

Proof of concept: PoC

In the getPoolValue function, the cache.totalBorrowingFees utilizes 60 decimals of precision, therefore Precision.applyFactor(cache.totalBorrowingFees, cache.borrowingFeeReceiverFactor) will have 60 decimals of precision.

Whenever an LP wants to withdraw after borrowing fees have been accumulated, the call to marketTokenAmountToUsd in WithdrawalUtils.sol would overflow since the pool value is multiplied by the market token amount which has 18 decimals of precision.

Furthermore, the amount of market tokens depositors receive will be drastically reduced as the pool value is inflated.

Do not use 60 decimals of precision for the cache.totalBorrowingFees or account for this additional precision when applying the factor.

GMX Team: The precision handling was refactored.

Proof of concept: PoC

The estimateExecuteWithdrawalGasLimit does not account for either the longTokenSwapPath or the shortTokenSwapPath, therefore the keeper will not be remunerated for users using the swap feature on withdrawals.

This will lead to the protocol keeper being unexpectedly drained of the native token, potentially stopping execution on the exchange for a period of time. The gas draining can occur due to regular exchange usage or can be easily leveraged by an attacker to maliciously drain the keeper.

Add gas estimation logic for the longTokenSwapPath and shortTokenSwapPath in the estimateExecuteWithdrawalGasLimit function, similar to the estimation for deposits.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

In increasePosition, the sizeDeltaInTokens is rounded down for long positions. An attacker can provide a sizeDeltaUsd that is 1 wei less than their triggerPrice and have their sizeDeltaInTokens rounded to 0. In this case a LimitIncrease would revert with the EmptyPosition error.

The attacker can make a LimitIncrease long for a niche market with low open interest where it is easy to manipulate the price impact by controlling the open interest. The attacker can then manipulate the open interest such that their LimitIncrease long is positively price impacted, meaning their executionPrice is decreased. A reduction in the executionPrice would cause the sizeDeltaInTokens to no longer be rounded to 0, and the order to no longer revert with an EmptyPosition error.

The attacker can leverage this with a large initialCollateralDeltaAmount and a swapPath that allows them to take advantage of outdated prices for a risk-free trade.

Do not allow users to create position orders with any sizeDeltaUsd less than a particular value such as 1e30 e.g. $1.

GMX Team: Limit orders no longer revert on the EmptyPosition error, additionally a minimum position size was implemented.

Proof of concept: PoC

The _validateRange function does not perform any validation on the value, but rather reverts for specific keys such as the Keys.SWAP_FEE_FACTOR and Keys.POSITION_FEE_FACTOR.

This restricts the protocol from setting these crucial values for the exchange.

Update the _validateRange function to perform validation on the value being passed rather than simply reverting for certain keys.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

A trader is able to update their referralCode at any time by creating a new order through the ExchangeRouter.

A malicious trader can submit a LimitIncrease where the fees.totalNetCostAmount will be their exact initialCollateralDeltaAmount, yielding an EmptyPosition error on execution.

The malicious trader can then allow the LimitIncrease to be executed once they see that prices have moved in their favor by updating their referralCode so that their fees.totalNetCostAmount is discounted and now the order will no longer revert with the EmptyPosition error.

This attack can also be executed if the tier or traderDiscountFactor is updated on a trader’s current referralCode as well.

Do not allow a trader’s totalRebateFactor or traderDiscountFactor to be updated in any way for existing orders.

GMX Team: Limit orders no longer revert on the EmptyPosition error.

Proof of concept: PoC

LimitIncrease, LimitDecrease, and StopLossDecrease orders depend on the positionIncreasedAtBlock for the price ranges that they can be executed with.

A malicious trader may take advantage of past prices by closing their position to reset the positionIncreasedAtBlock to 0.

Consider the following attack:

1. A trader makes two LimitIncrease orders with the same triggerPrice.
2. The first one is successfully executed and the positionIncreasedAtBlock is set to 100.
3. The second one is not executed since the valid descending price range is from before the positionIncreasedAtBlock.
4. The trader waits and observes that price moves in their favor, and then closes their position with a MarketDecrease order. Therefore resetting the positionIncreasedAtBlock to 0.
5. Now the second LimitIncrease is able to be executed with the out of date price range, enabling a risk-free trade.

Track when the position was closed and add this block number to the validation for LimitIncrease, LimitDecrease, and StopLossDecrease orders so that they cannot be used to abuse past prices.

GMX Team: The positionIncreasedAtBlock validation was removed for LimitIncrease orders, this gaming should not apply to decrease orders.

Proof of concept: PoC

The pool value is incremented by Precision.applyFactor(cache.totalBorrowingFees, cache.borrowingFeeReceiverFactor) which is the portion of borrowing fees going to the feeReceiver.

Because this amount is paid to the feeReceiver rather than the pool, it should not be counted as part of the pool value. This misrepresents the pool’s accounting and incorrectly values the pool for deposits and withdrawals.

Include the portion of pending borrowing fees which will go into the pool rather than the portion allocated for the feeReceiver.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

When the per-size values are computed, for tokens such as USDC with only 6 decimals of precision and large open interest values, there can be rounding error due to the division that occurs in toFactor.

For example:

1. $10,000,000 of open interest will have a total number of 38 digits.
2. 1 USDC will have a total number of 37 digits after multiplying by FLOAT_PRECISION.
3. Therefore when calculating the per-size values, precision loss on the order of <10 USDC can occur due to truncation.

Additionally, when calculating the per-size values, the resulting values are often not exact multiples of each other.

For example:

1. Open Interest is $10,000 short and $20,000 long both with the long token as collateral.
2. Assume cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max yields 57600399999999999.
3. 57600399999999999 / 10,000 = 5760039999999 will be the short funding factor magnitude.
4. 57600399999999999 / 20,000 = 2880019999999 will be the long funding factor.
5. However 2880019999999 / 5760039999999 = ~0.49999999999991 != 0.5.

This can result in less funding fees being paid than collected. Such a deficit will eat into the balance of the market and potentially prevent LPers from withdrawing their entire deposits or prevent traders from claiming their funding fees.

Avoid this precision loss for low precision tokens so that the same amount of funding fees are paid as collected. Address the precision lost when the resulting per-size values are not exact multiples.

Proof of concept: PoC

As shown in the following calculations, the price impact experienced by a trader differs from the amount taken out of/into the impact pool which over time will lead to meaningful accounting inconsistencies in the pool.

Consider using int256 priceImpactUsd = size.toInt256() * priceDiff / executionPrice.toInt256() instead of int256 priceImpactUsd = size.toInt256() * priceDiff / _latestPrice.toInt256() on line 182.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The isPositionLiquidatable validation is checked before PositionUtils.updateFundingAndBorrowingState(params, cache.prices) is performed.

Therefore the borrowing/funding fees are potentially significantly outdated when being accounted for.

This leads to the liquidation keeper not being able to liquidate positions that would be liquidateable when accounting for borrowing/funding fees and ultimately exposes the market to bad debt once the fees are updated.

Update the funding and borrowing state before checking whether the position is liquidatable.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The startingGas variable is declared inside of the executeOrder function. As a result, it will be the amount of gas left after the setting of prices, which is particularly gas intensive.

When calculating how much gas was used in GasUtils.payExecutionFee, keepers won't be remunerated for this expenditure which will run a significant deficit over time.

Refactor the way gas expenditure is tracked so that the gas used for the withOraclePrices modifier is included in the keeper’s remuneration.

GMX Team: This gas expenditure should be accounted for in the base fee.

Proof of concept: PoC

In several cases the getAdjustedLongAndShortTokenAmounts function reverts or returns a nonsensical result:

1. poolLongTokenAmount and poolShortTokenAmount access the pool amount for the same token as longToken and shortToken are the same. As a result, the pool amounts are equal and the else case will always be entered.
2. On line 387, when uint256 diff = poolLongTokenAmount - poolShortTokenAmount is performed, the larger value is always subtracted from the smaller value causing underflow.
3. On line 396, when uint256 diff = poolShortTokenAmount - poolLongTokenAmount is performed, the larger or equal value is being subtracted from the smaller value which will underflow in most cases.
4. On line 400, adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount does not set the adjustedLongTokenAmount but rather computes the result of subtraction.

Refactor the getAdjustedLongAndShortTokenAmounts function to address the above problems.

GMX Team: The getAdjustedLongAndShortTokenAmounts function was removed.

Proof of concept: PoC

The getTotalBorrowingFees function, which returns the pending borrowing fees, is out of date as it uses an outdated cumulativeBorrowingFactor rather than getting the updated factor with getNextCumulativeBorrowingFactor.

As a result, LPs who withdraw will have their market tokens worth less than they should be as the pending fees aren’t reflected in the pool value.

Additionally, this introduces the opportunity for arbitrages that take advantage of the stepwise increase in borrowingFees by forcing an update with a trivial MarketIncrease order.

Use getNextCumulativeBorrowingFactor instead of getCumulativeBorrowingFactor to get the latest pending borrowing fees.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The PnL of a position is added to the current collateral when calculating the remainingCollateralUsd. This is logically sound when a trader’s PnL is negative, as their losses will be subtracted from their collateral upon closing their position.

However, when a position is in profit, there is no effect on the position’s collateral since the profits come from the pool. In the case where positions are in profit, adding the profit to the remainingCollateralUsd misrepresents how much collateral value actually remains.

Furthermore, if a position is profitable, this profit can be used as collateral to continue to increase the position size. This allows trader’s to open positions with far greater leverage than the minCollateralFactor.

Do not count profit as a part of the remaining collateral of a position.

GMX Team: This is the desired behavior.

An attacker with enough size can manipulate the reference exchanges to influence the median price and take advantage of the price movements.

Carefully monitor the protocol and adjust parameters such as OI caps accordingly. Furthermore, use enough reference exchanges so the median is less likely to be affected by price outliers.

GMX Team: Acknowledged.

Proof of concept: PoC

In the event that an order is frozen it can no longer be simulated to check for validity since the msg.sender will not be a frozen order keeper.

Allow the simulation to bypass the _validateFrozenOrderKeeper authentication check.

GMX Team: Acknowledged.

In the withdrawAndSendNativeToken function, .call is used to send ether to the receiver address. Although bytes memory data is commented out, it will still be loaded into memory.

A malicious receiver may load unexpectedly large return data into memory and potentially cause the keeper to expend more gas than expected.

The size of the returned data that the receiver is able to generate is however constricted by the gasLimit, but this form of manipulation may still pose a risk to the system.

Utilize a low level call to avoid loading the returned data into memory:

assembly { success := call(gasLimit, receiver, amount, 0, 0, 0, 0) }

GMX Team: The recommendation was implemented.

Proof of concept: PoC

A malicious trader may be able to execute a profitable short-term risk-free trade by creating a limit order, observing the price it will be executed at and optionally front-running the execution to update or cancel the order.

This way the order is cancelled/frozen if price doesn’t move in the a direction that benefits the trader in the blocks between where the execution price is from and the current execution block.

Ensure order fees are sufficient to invalidate short term risk-free trades. Otherwise, do not allow users to decide whether or not their order is executed by cancelling or updating the order right before execution.

GMX Team: Acknowledged.

block.number is used to setLatestAdlBlock rather than Chain.currentBlockNumber(). This could yield unexpected behavior as the arbSys.arbBlockNumber() may differ from the block.number.

Utilize the Chain.currentBlockNumber() when setting the latest ADL block.

GMX Team: The recommendation was implemented.

The validatePoolAmount validation check can be circumvented for the _params.tokenOut if a user receives a positiveImpactAmount which comes in the form of tokenOut that pushes the pool amount for tokenOut over the maxPoolAmount.

Validate that the _params.tokenOut pool amount is still within the valid range by using validatePoolAmount in the case where the user is positively impacted.

GMX Team: The recommendation was implemented.

Order execution takes ~2,000,000 gas units for a vanilla execution without a swapPath or callbackContract. On certain chains, such as Avalanche C-chain, the block gas limit can be close to how much gas it takes for order executions. This makes the protocol susceptible to a block stuffing attack.

For example, this transaction on the Avalanche Fuji testnet took over 4,000,000 gas units which is greater than 50% of the gas limit of 8,000,000. An attacker may choose to stuff blocks to delay the execution of their order until the current price moves favorably.

Consider gas optimization strategies alongside measures to remove stale execution tx’s from the mempool to prevent such manipulations.

GMX Team: Acknowledged.

For markets where the shortToken is the same as the longToken, the validateReserve function call will only validate reserves for longs. This is because cache.tokenOut == _params.market.longToken will always be true.

For markets where the shortToken is the same as the longToken, be sure to validate the reserves for both longs and shorts.

GMX Team: These markets should never be used in the swap function, an explicit validation for this was implemented.

In the validateExecutionFee function, the current tx.gasprice is used to estimate the gas price in the block of the execution. However the current tx.gasprice can be significantly different from the tx.gasprice actually experienced in the block of execution.

This allows the keeper to expend more gas than the executionFee in the case where the tx.gasprice is greater in the block of execution.

Be wary of the potential gasprice difference and set the ESTIMATED_GAS_FEE_MULTIPLIER_FACTOR accordingly.

GMX Team: Acknowledged.

When the swapProfitToCollateralToken swap is performed, the pnlAmountForPool has not yet been decremented from the poolAmount. Therefore the price impact calculation as well as subsequent validation checks during swapProfitToCollateralToken are based on the pnlAmountForPool not being withdrawn from the poolAmount, but yet still being swapped.

This leads to inaccurate price impact being applied during the swap as well as validation that is hinged upon a temporary invalid state of the pool accounting.

In the case where the swapProfitToCollateralToken swap is performed, account for the user’s profit tokens first being removed from the pool before they are used to swap in the pool.

GMX Team: A pool adjustment key was introduced to account for the intermediate balance update.

A malicious user can front-run the execution of their withdrawal and send their market tokens to another address so that the withdrawal execution fails.

The attacker can observe if price moved in their favor between the block where the prices are provided from and the block where their withdrawal execution is happening and decide if they would like their withdrawal to succeed or fail.

An attacker can leverage this using the swaps at the end of a withdrawal to capitalize on outdated prices for any assets in the longTokenSwapPath or shortTokenSwapPath.

Consider transferring the user’s market tokens to a WithdrawalVault upon the withdrawal creation, similar to deposits and orders. Otherwise ensure that the withdrawal fees invalidate any possible risk-free trade that could be made.

GMX Team: The recommendation was implemented.

For markets where the pnlToken can be the same as the indexToken, there are cases where the indexToken and pnlToken are valued differently and users benefit from this difference at the market’s expense.

Consider a StopLossDecrease order for a long in profit, the pnlToken is the same as the indexToken.

The user's pnl calculation can value the indexToken at the acceptablePrice, say $5495. However the resulting values.positionPnlUsd is converted to the pnlToken at the secondaryPrice, say $5490.

This yields a delta of tokens that was not originally factored into the PnL for the market, so the market experiences slightly more loss than expected and the user gains slightly more than expected.

For cases where the pnlToken is the same as the indexToken, consider using the executionPrice to denominate the values.pnlAmountForPool.

GMX Team: The execution price logic was refactored.

Proof of concept: PoC

A trader is allowed to decrease their position such that the collateral is below the minimum collateral because shouldValidateMinCollateralUsd is false. However, shouldValidateMinCollateralUsd is set to true for liquidation orders.

Therefore, a trader’s decrease order can go through and their position can be liquidated right after by a liquidation keeper. An attacker may leverage this to drain the keeper of its gas by creating trivial positions and decreasing them to invalidate the minimum collateral so that they are subsequently liquidated.

Always validate the minimum collateral amount when decreasing or increasing a position.

GMX Team: Acknowledged.

In the getPriceImpactUsd function the priceImpactUsdForVirtualInventory is asserted to be <= the thresholdPriceImpactUsd, otherwise the normal priceImpactUsd is used.

This however allows the priceImpactUsdForVirtualInventory to negatively impact users without bound. This way malicious actors in other markets are able to grief users using the same priceImpactUsdForVirtualInventory.

Modify the priceImpactUsdForVirtualInventory > thresholdPriceImpactUsd comparison to priceImpactUsdForVirtualInventory < thresholdPriceImpactUsd.

GMX Team: The misleading check was removed.

The fees.totalNetCostAmount includes both the fees.feeReceiverAmount and the fees.borrowingFeeAmount. The borrowingFeeAmount is comprised of both the borrowing fees for the pool and for the feeReceiver.

The fees.feeReceiverAmount also includes the borrowing fees for the feeReceiver, therefore the borrowingFeeAmountForFeeReceiver amount is accounted for twice in the fees.totalNetCostAmount.

Only account for the borrowingFeeAmountForFeeReceiver once in the fees.totalNetCostAmount.

GMX Team: The recommendation was implemented.

When computing the cache.fundingUsd, a USD amount with 30 decimals of precision is divided by 1e30: cache.sizeOfLargerSide / Precision.FLOAT_PRECISION.

This results in precision loss on the order of magnitude of tens of cents for the distribution of funding fees.

Consider if this magnitude of precision loss is acceptable and adjust the calculation if it isn’t.

GMX Team: The fundingUsd calculation was adjusted.

When calculating the price impact USD value for a swap, the thresholdPriceImpactUsd is based on the params.usdDeltaForTokenA.abs() and params.usdDeltaForTokenB.abs(). However these values will always have the same magnitude during a swap, therefore the user’s swap USD value will be double counted.

If the configured thresholdImpactFactorForVirtualInventory is intended to be 70% of the user’s swap USD value, it will instead account for 140% of the user’s swap USD value.

Only base the thresholdPriceImpactUsd on a single token side of usdDelta for swaps.

GMX Team: The recommendation was implemented.

In the case where a user’s collateral is deemed to be insufficient after removing the initialCollateralDeltaAmount, the initialCollateralDeltaAmount is set to 0.

However if the estimatedRemainingCollateralUsd, which was based upon the initialCollateralDeltaAmount being removed from the position’s collateral, is smaller than the MIN_COLLATERAL_USD the position will still be closed.

This is unexpected behavior as the initialCollateralDeltaAmount has been set to 0 so the position’s collateral will no longer be less than the MIN_COLLATERAL_USD.

Do not close the user’s position in the case where the initialCollateralDeltaAmount is set to 0.

GMX Team: The initialCollateralDeltaAmount is now added back to the estimatedRemainingCollateralUsd if it is set to 0.

If increase and decrease position functionalities are disabled, funding fees will continue to accumulate as time goes by. Traders will be charged unexpected fees when trading resumes.

Consider pausing funding fee accumulation when trading is disabled.

GMX Team: Acknowledged.

The latest ADL block is updated each time updateAdlState is called even if the ADL state was not enabled or changed. This may lead to a keeper continuously updating ADL block and preventing much needed ADL decrease orders from going through.

Consider using if (shouldEnableAdl) setLatestAdlBlock(dataStore, market, isLong, Chain.currentBlockNumber()).

Otherwise, if current functionality is intended, add more documentation surrounding ADL block updates.

GMX Team: The current functionality is intended.

If a limit order is in the dataStore and the trading features are disabled, then the possibility of a risk-free trade arises.

Right before a feature is re-enabled, if prices have moved against the trader, the trader may cancel or update their limit order. Otherwise, the order can execute with outdated prices.

Do not revert on the FeatureUtils.DisabledFeature error, but rather freeze or cancel these limit orders.

GMX Team: Acknowledged.

When values.remainingCollateralAmount is less than or equal to collateralCache.adjustedPriceImpactDiffAmount, values.remainingCollateralAmount is set to 0 and the collateralCache.adjustedPriceImpactDiffAmount is placed in the holding area.

The position is stamped with a collateral amount of 0 in DecreasePositionUtils.sol on line 195. Once validatePosition is entered, validateNonEmptyPosition will be called which will revert due to the 0 collateral amount with the EmptyPosition error.

Overall, the position’s remainingCollateral is updated only to subsequently revert when validatePosition is called. Furthermore, this may open the market to scenarios where a user can influence when their order is executed and create a risk-free trade as the order stays in the store with the same updatedAtBlock.

Do not set the remainingCollateralAmount to 0 only to have the order revert later on with an EmptyPosition error. Instead consider leaving some remainingCollateral or reverting with a separate error so that the order is cancelled or frozen.

GMX Team: Acknowledged.

A liquidity provider is unable to utilize the minOutputAmount functionality without performing a swap. This may lead LPs to get less output than intended and/or confusion with the existing minLongTokenAmount and minShortTokenAmount usage in swap.

Consider adding the functionality to specify the minimum output amounts without swapping.

GMX Team: Acknowledged.

The following places lack slippage protection and may lead to loss of assets for a user:

1. DecreasePositionCollateralUtils.sol Line 392 and Line 434 use 0 as the minOutputAmount which may unexpectedly reduce the profit for a trader.
2. ExecuteDepositUtils.sol Line 432 which may reduce the amount of market tokens the user obtains although risk may be limited by specifying minMarketTokens.

Consider adding the functionality to specify the minimum output amounts and/or further document this behavior.

GMX Team: The recommendation was implemented.

handleExcessExecutionFee does not allow users to claim the excess executionFee that they may have sent. Currently the excess is simply sent to a holding address with no accounting of which user is in excess or by how much and there is no way to claim the excess fee.

Either make a way for users to claim these tokens or ensure it is well documented and explicit that these tokens will be lost for the user.

GMX Team: The excess execution fee logic was removed.

Keys.MAX_PNL_FACTOR is used when checking if the PnL factor for ADL is exceeded instead of Keys.MAX_PNL_FACTOR_FOR_ADL as in AdlHandler.sol line 123. This can lead to ADL being enabled and not going through, or ADL being consistently disabled due to misconfiguration between the two factors.

Use the same factor key for the same validation. If the difference is intended for finer and more precise protocol control, document such behavior.

GMX Team: The recommendation was implemented.

When performing a swap, Keys.MAX_PNL_FACTOR_FOR_WITHDRAWALS is used to check whether the current pnlToPoolFactor exceeds the the maximum PnL factor for withdrawals, which is the strictest (lowest) maximum pnlToPoolFactor.

When performing a swap, the tokenIn is deposited and tokenOut is withdrawn. By treating both the “deposit” and “withdrawal” with the same withdrawal pnlToPoolFactor threshold, it can potentially prevent swaps that will improve the current pnlToPoolFactor on a particular side.

Consider validating tokenIn against the MAX_PNL_FACTOR_FOR_DEPOSITS and tokenOut against MAX_PNL_FACTOR_FOR_WITHDRAWALS.

GMX Team: The recommendation was implemented.

When passing the startingGas to the this._executeOrder external call, it is assumed that all of the startingGas is available for the execution of the external call. However, due to the 63/64 rule, only 63/64 of the startingGas will be available in the subsequent call to this._executeOrder.

Therefore when the executionFee is paid in the external call to this._executeOrder, the gas used will be overestimated, and the user will be errantly charged for a false 1/64 expenditure.

Account for the 63/64 rule when estimating the gas consumption.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

With a range between the MAX_PNL_FACTOR_FOR_WITHDRAWALS and the MAX_PNL_FACTOR_FOR_ADL, if the profit in the pool exceeds the pnlToPoolFactor for withdrawals but is not high enough to trigger ADL, withdrawals for LPs will be bricked until users deposit more tokens into the pool and a trader decides to close their profitable position.

Ensure that this risk is well communicated. Additionally consider setting the MAX_PNL_FACTOR_FOR_WITHDRAWALS close to the MAX_PNL_FACTOR_FOR_ADL to limit this scenario.

GMX Team: Acknowledged.

Proof of concept: PoC

Deposits are restricted if the MAX_PNL_FACTOR_FOR_DEPOSITS is exceeded since the market token price is below the “allowed” amount. However, market token price can continue to decrease after ADL, and deposits can once again resume.

Do not count on this minimum bound for market token price and be sure to document that the price can keep dropping below the asserted “allowed” amount.

GMX Team: Documentation was added.

The boundedSub function does not correctly prevent underflow.

For example, boundedSub(type(int256).min, 1) causes an underflow and reverts because the condition check if (a < 0 && b <= type(int256).min - a) is incorrect.

This poses inherent risk for the future use of boundedSub in this codebase and others that may adopt it.

Replace with if (a < 0 && b >= a - type(int256).min) or if (a < 0 && -b <= type(int256).min - a).

GMX Team: The recommendation was implemented.

Proof of concept: PoC

When increasing or decreasing a position, fees are calculated based on the size of the order and then taken out of the collateral.

However, when validating the position with validatePosition and checking isPositionLiquidatable, fees are calculated and applied a second time. This further reduces how much collateral the position has during this validation. This can prevent increasing or decreasing a position as the isPositionLiquidatable check would fail unexpectedly.

Check if the position is liquidatable prior to fees being paid and taken out of the collateral.

GMX Team: The recommendation was implemented.

When updating the cumulativeBorrowingFactor, the fees are maximized in the protocol’s favor as can be seen in getBorrowingFactorPerSecond. The poolUsd uses the min price and the reservedUsd uses the max price.

However, in getPositionFees, the amount of collateral tokens is calculated from the borrowing fees using the min price. Then, that amount will be multiplied by the max price of the collateral token. Therefore the resulting borrowing fee is maximized twice. This means the borrowing fees will be larger than expected, and the position more likely to be liquidated.

An explicative example:

• A collateral token has a min-max range of $1-$2.
• The borrowingFee is $100 which is already maximized.
• The resulting fees.borrowingFeeAmount is $100 / $1 = 100 collateral tokens.
• The fees.totalNetCostUsd calculation multiplies 100 * $2 = $200 in borrowing fees.

Therefore, what was originally a $100 fee turns into a $200 fee.

Use the direct value returned from the getBorrowingFactorPerSecond function for the fees.totalNetCostUsd.

GMX Team: The maximization logic was refactored.

The minCollateralFactor is derived from the current open interest such that the remaining collateral of a position is positively correlated with open interest.

E.g. as there is more open interest in a market, more remaining collateral is required to be considered sufficient. This prevents smaller positions from being opened in markets with high open interest.

Additionally, users who originally opened positions in markets before the open interest had grown may find themselves unable to decrease their position collateral without closing their entire position.

Reconsider if these externalities are desired and if so make sure they are well documented.

GMX Team: This behavior is desired.

params.position.borrowingFactor() and params.position.sizeInUsd() are swapped as MarketUtils.updateTotalBorrowing takes the prevPositionSizeInUsd followed by the prevPositionBorrowingFactor. The end result is the same because multiplication is commutative, but poses a risk for future changes.

Flip the parameters so they are correctly ordered.

GMX Team: The recommendation was implemented.

When making a withdrawal, the _getOutputAmounts function multiplies two float precision USD amounts. This multiplication can result in overflow when both USD amounts are on the order of hundreds of millions. E.g. $600,000,000 * $200,000,000 will overflow and revert.

Be aware of this overflow risk, and consider altering the arithmetic if values of this size are expected.

GMX Team: Acknowledged.

Affiliates still receive their fees.referral.affiliateRewardAmount upon liquidation because the handleReferral function is always called in the decreasePosition function.

Consider whether this is desired behavior. If not, do not call the handleReferral in the case of liquidations.

GMX Team: This is the desired behavior.

cache.prices.indexTokenPrice.midPrice() is used to estimate the position’s PnL for cache.estimatedPositionPnlUsd.

However, it may make more sense to use prices.indexTokenPrice.pickPriceForPnl as in isPositionLiquidatable to get the worst-case scenario price for estimation purposes.

Consider switching cache.prices.indexTokenPrice.midPrice() to prices.indexTokenPrice.pickPriceForPnl(isLong, false).

GMX Team: The recommendation was implemented.

In the incrementClaimableFundingAmount function, the claimable funding fees are incremented for the receiver rather than the position owner. This could be unexpected and may lead to loss of funds in the event that the receiver is a contract.

Consider if the position.account() should receive the claimable funding fees rather than the params.order.receiver().

GMX Team: The recommendation was implemented.

A trader may perform a weak version of a sandwich attack on large LimitIncrease orders to benefit from the priceImpact in the following way:

• A malicious trader observes that the triggerPrice is approaching for a LimitIncrease order.
• The malicious trader creates a MarketIncrease order that will balance the open interest in the pool to receive positive price impact.
• The LimitIncrease order is executed and gets negatively price impacted because it unbalances the pool.
• The malicious trader creates a MarketDecrease order that will rebalance the pool and receive positive impact.

There are levers in place to limit the scope of these sandwich attacks such as the two-step execution system and the acceptablePrice, but nonetheless the risk of such an attack should be well documented.

GMX Team: Acknowledged.

Proof of concept: PoC

Malicious traders can stop LimitIncrease orders from getting filled if the order has a swapPath.

A malicious trader can observe that the triggerPrice is approaching for the LimitIncrease and then create their own MarketSwap that removes the necessary liquidity to execute the swapPath of the LimitIncrease order.

Be aware and document that user’s LimitIncrease orders can be fail due to their swapPath.

GMX Team: Acknowledged.

In the case where the swap is negatively impacted, the cache.amountIn includes the negativeImpactAmount. This misrepresents the amountInAfterFees in the emitSwapInfo function.

Do not include negative impact in the amountInAfterFees that is provided to the emitSwapInfo function.

GMX Team: The recommendation was implemented.

In the event that a user’s position is liquidated with negative collateral, an empty PositionFees object is returned from the getLiquidationValues function. This means that the claimableLongTokenAmount and claimableShortTokenAmount are reset to 0.

In this case any funding fees that the trader had accumulated are lost.

Consider if this is the expected behavior. If not, maintain the existing claimableLongTokenAmount and claimableShortTokenAmount in the new PositionFees object returned from getLiquidationValues.

GMX Team: The recommendation was implemented.

The action key signalSetPriceFeed key should be setPriceFeed to match up with the other key patterns.

Additionally, when performing the _validateAndClearAction, the label when validating & clearing should be setPriceFeedAfterSignal on line 209 as well.

Update the keys as recommended.

GMX Team: The recommendation was implemented.

The roundUpDivision function rounds down instead of up when a is negative.

Add documentation that this function rounds up purely the magnitude of the integer a.

GMX Team: The function was renamed to roundUpMagnitudeDivision.

When markets are disabled, limit orders are not cancelled but frozen. This opens the opportunity for the frozen order keeper to possibly execute orders with outdated prices when a market is re-enabled. The use of outdated prices could lead to risk-free trade opportunities.

Consider adding cancellation logic for limit orders that revert due to disabled markets.

GMX Team: Acknowledged.

The blockNumber variable represents a timestamp value rather than a block number value.

Rename variable to something more fitting.

GMX Team: The recommendation was implemented.

It may be prudent to add features that can be disabled for things like the DecreasePositionSwapTypes.

Consider adding additional features that can be disabled.

GMX Team: Acknowledged.

It is possible to execute a swap with 0 tokenIn and a non-zero swapPath which is just a waste of resources.

Consider adding tokenIn > 0 as a validation.

GMX Team: Acknowledged.

It is possible for the max oracle block numbers to be unsorted unlike the min oracle block numbers. This is because the only requirement for the max block numbers is that they are at least as large as the min oracle block numbers.

For example, the keeper may pass:

min block #’s: [5,5] max block #’s [6, 5]

Validate that the max oracle block numbers are ascending.

GMX Team: Acknowledged.

Calculating the result will be safe as long as value is <= 10^47. However, the result may not fit in a int256 leading to a revert SafeCast: value doesn't fit in an int256

For example, the inputs 57923633301321315440238040719798086540604777067 and 1 yield a SafeCast revert.

Document such behavior.

GMX Team: Acknowledged.

In the event that the oracle provides multiple prices for a single token during the execution of a market order or a liquidation, the customPrice assigned in setExactOrderPrice (primaryPrice) would differ from the price retrieved and used from getMarketPricesForPosition (secondaryPrice).

The price used to validate liquidation (secondaryPrice) and the price used to execute the liquidation (primaryPrice) would be different.

Consider using the secondaryPrice for market orders in the event that a range is errantly provided.

GMX Team: Acknowledged.

Popular wrapped network tokens like WAVAX and WFTM which the synthetics exchange will likely interact with use .transfer to withdraw native tokens to the caller.

The Bank contract’s receive function does not currently consume more than 2300 gas, but gas consumption for opcodes like SLOAD which are being used in the receive function are subject to change over time which may cause the receive function to revert when called with a .transfer.

(See Berlin Hardfork)

In such a scenario the exchange would be unable to withdraw it’s WNT balance into NT and the receive failure could open up opportunities for risk free trading exploits.

Be wary of this possibility and have a plan in the event that gas costs are updated.

GMX Team: Acknowledged.

The fees.totalNetCostAmount is misrepresented in emitPositionFeesCollected since the fee is reduced by the profit amount in the processCollateral function.

Account for the fees being reduced by profits in the event.

GMX Team: The recommendation was implemented.

It is checked that the oracleTimestamp was set not too far in the past but there is no check to ensure that the oracleTimestamp is not set in the future.

Add a check that validates that the oracleTimestamp is not in the future.

GMX Team: Acknowledged.

The position’s collateral still may not be considered sufficient even after the initialCollateralDeltaAmount is set to 0.

Consider whether the order should still be allowed to execute in the case where the position’s collateral is still not sufficient even when the initialCollateralDeltaAmount is 0. If not, revert with an error.

GMX Team: Acknowledged.

The validateEnabledMarket function can be reused once the market is obtained from the address instead of duplicating code.

Consider implementing the above suggestion.

GMX Team: The recommendation was implemented.

The fees.feeAmountForPool can be reused in the fees.totalNetCostAmount calculation.

Consider implementing the above suggestion.

GMX Team: The recommendation was implemented.

The priceImpactUsd > 0 check can include 0 to save gas from the else case.

Update the conditional to priceImpactUsd >= 0.

GMX Team: Acknowledged.

Library functions throughout could be made internal to save gas.

Make as many library functions internal as possible while staying within contract bytecode deployment limits.

GMX Team: Acknowledged.

The initialCollateralDeltaAmount variable is left unused and the attribute is instead repeatedly accessed directly from the order.

Use the cached initialCollateralDeltaAmount or remove it.

GMX Team: The recommendation was implemented.

The sizeDeltaUsd calculation for shorts can be replaced with the getSizeDeltaInTokens function.

Consider implementing the above suggestion.

GMX Team: The recommendation was implemented.

Throughout the codebase the index for many for-loops is initialized to 0 which is the default uint value. Avoid the unnecessary initialization and allow the default values to be implicitly assigned to these uint variables:

• FeeHandler.sol::37
• MarketUtils.sol::1859
• Oracle.sol::235
• Oracle.sol::287
• Oracle.sol::440
• Oracle.sol::476
• Oracle.sol::494
• Oracle.sol::562
• OracleModule.sol::61
• OracleModule.sol::67
• OracleUtils.sol::195
• ExchangeRouter.sol::283
• ExchangeRouter.sol::309

Do not assign these default values.

GMX Team: Acknowledged.

Throughout the codebase the index for many for-loops is initialized to 0 which is the default uint value. Avoid the unnecessary initialization and allow the default values to be implicitly assigned to these uint variables:

• ExchangeRouter.sol::343
• SwapUtils.sol::123
• Array.sol::56
• Array.sol::73
• Array.sol::90
• Array.sol::107
• Array.sol::124
• BasicMulticall.sol::17
• PayableMulticall.sol::21

Do not assign these default values.

GMX Team: Acknowledged.

Throughout the codebase there are many for-loops that compute the length of an array upon each iteration. Caching the length of these arrays will decrease gas costs throughout the application:

• FeeHandler.sol::37
• MarketUtils.sol::1859
• Oracle.sol::235
• Oracle.sol::287
• Oracle.sol::440
• Oracle.sol::476
• Oracle.sol::494
• Oracle.sol::562
• OracleModule.sol::61
• OracleModule.sol::67
• OracleUtils.sol::195

Cache the length of arrays before iterating through them.

GMX Team: Acknowledged.

Throughout the codebase there are many for-loops that compute the length of an array upon each iteration. Caching the length of these arrays will decrease gas costs throughout the application:

• ExchangeRouter.sol::283
• ExchangeRouter.sol::309
• ExchangeRouter.sol::343
• SwapUtils.sol::123
• Array.sol::56
• Array.sol::73
• Array.sol::90
• Array.sol::107
• Array.sol::124
• BasicMulticall.sol::17
• PayableMulticall.sol::21

Cache the length of arrays before iterating through them.

GMX Team: Acknowledged.

Throughout the codebase uint variables are checked to be > 0 however != 0 is a more efficient comparison to check if uint values are positive. Switching to != 0 will decrease gas costs throughout the application:

• ExecuteDepositUtils.sol::163
• ExecuteDepositUtils.sol::192
• ExecuteDepositUtils.sol::208
• GasUtils.sol::95
• Oracle.sol::592
• BaseOrderUtils.sol::352
• BaseOrderUtils.sol::369
• DecreaseOrderUtils.sol::57
• DecreasePositionCollateralUtils.sol::174
• DecreasePositionUtils.sol::96
• DecreasePositionUtils.sol::150
• IncreasePositionUtils.sol::151
• IncreasePositionUtils.sol::177
• PositionUtils.sol::471
• PositionUtils.sol::482
• PositionUtils.sol::535
• Precision.sol::61

Use != 0 to check if uint values are positive.

GMX Team: Acknowledged. 109

The NatSpec documentation for the SwapParams is outdated.

Update the NatSpec to reflect the current contents of SwapParams.

GMX Team: The recommendation was implemented.

The decreasePositionSwapType is missing from the NatSpec documentation for the CreateOrderParams struct.

Update the NatSpec documentation for the CreateOrderParams struct.

GMX Team: The recommendation was implemented.

The timeKey parameter is missing from the NatSpec documentation for the claimCollateral function.

Update the NatSpec documentation for the claimCollateral function.

GMX Team: The recommendation was implemented.

The validatePosition function does not include shouldValidateMinCollateralUsd as an @param in it’s NatSpec documentation.

Update the NatSpec for validatePosition.

GMX Team: The recommendation was implemented.

The NatSpec documentation for the ExecuteOrderParams is outdated.

Update the NatSpec to reflect the current contents of ExecuteOrderParams.

GMX Team: The recommendation was implemented.

The NatSpec documentation for the UpdatePositionParams is outdated.

Update the NatSpec to reflect the current contents of UpdatePositionParams.

GMX Team: The recommendation was implemented.