Synthetics V2, Review 2

Proof of concept: PoC

The custom handling for the Keys.EMPTY_POSITION_ERROR_KEY allows users to create MarketDecrease orders that continue to revert and be retried until the user creates a position. The MarketDecrease order would then be executed at the prices of the block in which the decrease order was created.

This way a user can submit a long MarketDecrease for an empty position, and wait until the price of the index token decreases before submitting a MarketIncrease order and realizing risk-free profits from the exchange.

Do not revert and retry on Keys.EMPTY_POSITION_ERROR_KEY.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

In the beforeOrderExecution callback it is possible to cancel the order prior to processing which returns funds to the user and removes the order from the orderStore. However, the order will still execute and create a position with the initial collateral delta and USD size.

This results in a deficit in the orderStore balances which causes accounting issues across the entire exchange.

Do not allow order cancellation to occur during the execution of that order, possibly by moving the cancelOrder function to the orderHandler and allowing NonReentrant modifiers to resolve this issue. Furthermore, ensure consistency between storage and cached parameters.

GMX Team: A globalNonReentrant modifier was added to prevent this.

Proof of concept: PoC

The shouldUnwrapNativeToken flag can be exploited by users to create positions that cannot be decreased by liquidations or ADL orders.

For both liquidation orders and ADL orders the shouldUnwrapNativeToken flag is set to true, however the position can be created by a contract that is unable to receive the native token, causing the order execution to revert.

Refactor the shouldUnwrapNativeToken logic so that it cannot be used to determine whether or not transactions are able to succeed, and optionally set the shouldUnwrapNativeToken flag to false for liquidation and ADL orders.

GMX Team: Now if an address cannot accept the native asset, it is re-wrapped and the wrapped native asset is transferred to the address.

Proof of concept: PoC

In the process of cancelling or freezing an order, the executionFee is paid to the keeper and a native token refund is issued to the user. However, the address of the user can point to a contract that is unable to accept the native token, causing the cancellation or freezing to revert.

This way, the user may cause their order execution to revert and be retried until they wish their order to be executed – enabling risk free trades with knowledge of how the market moved after their order was created.

Consider refunding the user in WNT rather than the native token directly to avoid transaction manipulation.

GMX Team: Now if an address cannot accept the native asset, it is re-wrapped and the wrapped native asset is transferred to the address.

Proof of concept: PoC

The call to MarketUtils.applyDeltaToOpenInterestInTokens applies the positive sizeDeltaInTokens to the open interest in tokens while the position is being decreased by that amount of tokens rather than increased.

This incorrectly represents the accounting of the decrease order and perturbs all open interest, pnl and reserves accounting for that market.

Negate the sizeDeltaInTokens, as these tokens are being removed from the open interest.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The calculation of the priceImpactAmount is computed with the params.order.sizeDeltaUsd, however the price impact that the user actually experiences is based on the adjustedSizeDeltaUsd which can be significantly smaller than the params.order.sizeDeltaUsd.

This way the impact that is applied to the accounting for the pool and the impact that actually takes place can be significantly different and cause the market to start double counting funds in both the impact pool and the poolAmount.

Compute the priceImpactAmount using the adjustedSizeDeltaUsd.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

In the condition where the values.outputAmount is intended to be subtracted from the fee amount, the values.outputAmount is set to 0 before being subtracted from the fee amount, causing the fee to be taken from both the outputAmount and the user’s collateral.

In the case of large fees, this can lead to significant loss of assets for users interacting with the exchange, and can potentially cause unexpected accounting within a market.

Set the values.outputAmount to 0 after subtracting it from the fees.totalNetCostAmount.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The pnlAmountForPool is either the collateralToken if the user realized losses or the pnlToken if the user realized gains. However the applyDeltaToPoolAmount treats this amount as collateralToken no matter what, therefore perturbing the accounting of the pool by applying a pnlToken amount to a collateralToken amount.

Be sure to applyDeltaToPoolAmount for the correct token that is being removed from the pool.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

shouldValidateAscendingPrice errantly applies to StopLossDecrease orders as well, stipulating that for long position StopLossDecrease orders, the price must be increasing over the range to be executed and the inverse for short position StopLossDecrease orders.

These price range requirements are unexpected for StopLossDecrease orders and leads to them not acting as stop losses for positions, which would cause tremendous loss for traders.

Use a separate condition to validate the price range for StopLossDecrease orders.

GMX Team: StopLossDecrease orders are now validated with a separate condition.

Proof of concept: PoC

When swapping, the current MarketToken transfers the poolAmountOut to the next pool in the swap route, but this value does not include any positive price impact that the user might have accrued. This way the positive impact amount is left, unaccounted for, in the current MarketToken contract and the following MarketToken in the route “thinks” it has received the positive impact amount but it hasn’t.

This causes a deficit in the accounting for the MarketToken which was told that it received the positive impact amount, but never did.

Be sure to send the positive impact amount to the next MarketToken in the swap route.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

When swapping after decreasing an order, the first market in the swapPath is not required to be the market the position was in. This way a user can specify a market that accepts the result.outputToken and the result.outputAmount will be swapped from that market, but the user’s profits are left in the original market.

This breaks the accounting system in the market that was provided in place of the position’s market as the first market in the swapPath.

Transfer the funds to the first market in the swapPath, or require that the first market be the position’s market.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

When both increasing and decreasing a position, funding fees are incremented for a user when the fees.funding.longTokenFundingFeeAmount or fees.funding.shortTokenFundingFeeAmount are greater than 0.

However the funding fees are intended to be paid for by the user when they are positive and received by the user when they are negative. Currently, when funding fees are positive, the user both pays for and receives funding fees. But when they are negative, the funding fees are entirely ignored.

Refactor the incrementClaimableFundingAmount logic when both increasing and decreasing a position so that funding fees are paid out when they are negative.

GMX Team: The claimable funding fee logic was refactored.

Proof of concept: PoC

It is possible for user to create unliquidateable short positions with long collateral because when computing the openInterestWithPnL the Calc.sum operation underflows and reverts when the PnL is negative with magnitude greater than the open interest of the pool.

When a pool reaches this state, it is impossible to increase or decrease any positions within the pool, as the openInterestWithPnL is calculated during both actions.

Do not allow shorts to be able to lose more than the open interest in the pool or rectify these outstanding losses somehow.

GMX Team: The openInterestWithPnL function is no longer used.

Proof of concept: PoC

When creating an increase order, the collateral token gets sent to the market parameter on the order. This becomes problematic when a swapPath is provided, and the initialCollateralToken is not a token found in the order’s market.

Consider the following example:

1. Order with ETHUSD market, WBTC as initial collateral, and [BTCUSD, ETHUSD] swapPath.
2. WBTC gets transferred to the ETHUSD market rather than the BTCUSD market
3. BTCUSD market accounting is now off as the pool thinks there is more WBTC backing positions than there really is.

Transfer the collateral token to the first market in the swapPath if a swapPath is provided.

GMX Team: The recommendation was implemented.

shortCollateralFundingPerSizeForLongs is being both added to and subtracted in the same branch which not only misrepresents the true value of shortCollateralFundingPerSizeForLongs, but also leaves the longCollateralFundingPerSizeForShorts uninitialized. This negatively impacts the purpose of the funding fee which is to incentivize long and short balance in the pool.

Change line 655 to cache.fps.longCollateralFundingPerSizeForShorts -= cache.fps.fundingAmountPerSizeForLongCollateralForShorts.toInt256();

Change line 661 to cache.fps.longCollateralFundingPerSizeForShorts += cache.fps.fundingAmountPerSizeForLongCollateralForShorts.toInt256();

GMX Team: The recommendation was implemented.

Proof of concept: PoC

After the swap from collateral token to PnL token, values.outputAmount is incremented instead of replaced with the swapOutputAmount. This leads to the addition of two different tokens which can dramatically increase the values.outputAmount depending on the PnL token’s precision.

For example, if values.outputAmount represents $10,000 USDC, and then it swaps to WETH ($2000/ETH), swapOutputAmount will be 5 WETH which will get added to the $10,000 USDC. WETH, with 18 decimals of precision, will drastically inflate the user’s output. While this is unlikely to succeed due to the large difference in precision for USDC and WETH, tokens with closer precisions are highly susceptible to this bug.

Set values.outputAmount to 0 and values.pnlAmountForUser to swapOutputAmount

GMX Team: The swap from collateral token to PnL token logic was refactored.

Proof of concept: PoC1

If there is no open interest on a trader’s side and their collateral token, the funding amount per size stamped on their position is 0. This makes them immune from providing funding fees to the other side when necessary as the funding fee amount would simply be 0.

Furthermore, a separate trader whose claimable funding fees are incremented wouldn’t be receiving their tokens from the immune trader (as the funding fee to pay is always 0), but would transfer tokens directly from the market causing a disruption to the market’s accounting.

Consider refactoring funding fee logic so that if a newly created position is stamped with a 0 funding amount per size on their collateral token, the trader is able to pay a funding fee once there is open interest on the other side.

GMX Team: The funding fee logic was refactored.

Because the swapPath is simply the first element of the swapPathMarket, a malicious user could supply an arbitrary market which contains the collateral token. Such a market does not need to contain the PnL token, even though the purpose of the swap is to get the PnL token from collateral token. With the current logic, this can be used to inflate the values.outputAmount as per DPU-5.

If the logic was amended so that the cache.outputToken was the pnlToken, the amount of pnlToken to withdraw from the market can be inflated.

Add validation such that the market in the swapPathMarkets actually contains both the collateral token and PnL token, and that the token received from the swap is indeed the PnL token.

GMX Team: The swap collateral token to PnL token logic was refactored.

Proof of concept: PoC

When the sizeDeltaUsd of a LimitDecrease order exceeds the position sizeInUsd, the order lives on in the orderStore and the executionFee is set to 0. This can lead to a potential gas vamp attack on the keeper, where a user continually submits increase orders to create small positions to be decreased by a LimitDecrease order that remains in the orderStore.

Each execution of the LimitDecrease order can be arbitrarily expensive due to callbacks and the keeper would receive no remuneration for a potentially large amount of gas expenditure. An orchestrated attack like this could drain the keeper of its native tokens and shut down all execution on the exchange.

Require an executionFee for additional executions of LimitDecrease orders.

GMX Team: Partially filled orders are now removed from the order store.

Proof of concept: PoC

When the sizeDeltaUsd of a LimitDecrease order exceeds the position sizeInUsd, the order lives on in the orderStore but the sizeDeltaUsd of the order is set to the result.adjustedSizeDeltaUsd, which is the amount that the order was just able to decrease the position by, not the amount that the order has left to decrease.

Assign the sizeDeltaUsd of the order to be the order.sizeDeltaUsd() - result.adjustedSizeDeltaUsd.

GMX Team: Partially filled orders are now removed from the order store.

Proof of concept: PoC

When the execute order feature is blocked, all non-market orders will become frozen rather than being cancelled.

This could potentially spur on an infinite loop of execution for the frozen order keeper without any remuneration for gas costs.

Additionally, loops of continually failing frozen orders like this could occur from a number of errors caused during order execution.

Consider cancelling all orders or simply reverting when the execute order feature is blocked. Additionally take special care around the frozen order keeper logic to avoid costly infinite loops of frozen orders–and consider requiring an executionFee for additional executions of orders after they are frozen.

GMX Team: Orders are now cancelled when a feature is blocked.

There is a lack of general open interest caps per MarketToken aside from the reserves validation.

This enables users to open positions and increase the open interest right up to the reserve limit so that anyone attempting to withdraw from the MarketToken (or possibly swap with this MarketToken) will be unable to pass validateReserves. This way depositors funds can be held hostage – only being freed if others deposit (at which point someone may increase the open interest yet again and hold these new funds hostage as well).

Additionally, in the event of arbitrage attacks the exchange has no effective mechanism to limit the open interest per market to limit the attack size.

Implement open interest caps that limit the total open interest a MarketToken can have for either direction.

GMX Team: Open Interest caps were implemented.

Proof of concept: PoC

The price calculation in getExecutionPrice divides by the sizeDeltaUsd, therefore reverting when the sizeDeltaUsd is 0.

This results in users not being able to increase their collateral without increasing the size of their position. If a user were rushing to increase their collateral to avoid liquidation, the transaction would revert and the user would likely not be able to figure out why before their position becomes liquidated.

Refactor the getExecutionPrice logic to allow for a sizeDeltaUsd of 0.

GMX Team: The getExecutionPrice logic was refactored.

Proof of concept: PoC

When creating an order, there is no condition to handle the initialCollateralDeltaAmount for decrease orders, therefore the initialCollateralDeltaAmount is always 0 for decrease orders and it is impossible for users to decrease their collateral amount without fully closing their position.

Implement a condition for the initialCollateralDeltaAmount that allows users to decrease their collateral as expected.

GMX Team: The recommendation was implemented.

In the getMarkets function, the loop continues in the case of the NO_SWAP, SWAP_PNL_TOKEN_TO_COLLATERAL_TOKEN, and SWAP_COLLATERAL_TOKEN_TO_PNL_TOKEN addresses. This means that the index of these markets is left as an uninitialized Market.Props struct.

Currently, when decreasing a position and swapping the collateral token to the PnL token, the first market in the swapPathMarkets is used. However according to the logic in getMarkets this market will be uninitialized. Therefore a decrease order using the shouldSwapCollateralTokenToPnlToken or the shouldSwapPnlTokenToCollateralToken feature will always revert.

In the worst case, a user may make a StopLossDecrease order with either of these address variables in the swapPath expecting a swap to take place when their stop loss is hit. However the stop loss would revert upon execution, potentially leading to unintended loss of user funds, because their position is never closed.

Limit these address variables to only the first index of the swapPath, and use the second item in the swapPathMarkets to perform the corresponding swap.

GMX Team: The decrease order swap logic was refactored.

The exchange keeper and other permissioned addresses have the power to do nearly anything:

• Execute any order or liquidation at any arbitrary price
• Defer execution of any arbitrary deposit, withdrawal, order, or liquidation
• Select the order of execution for all incoming deposits, withdrawals, and orders
• Power to shut off any feature – including withdrawals and closing positions
• Change out the protocol-wide variables used in the dataStore

There are many assumptions made on the form of the price input from the keeper. Adding validation for the expected format and range of acceptable inputs would reduce the risk of high-cost mistakes, and assist in limiting the scope of the internal exploits available to the keeper.

The keeper must diligently execute orders, for example stop loss orders must be executed with the correct range of prices while staying within the MAX_ORACLE_PRICE_AGE limit.

Treat the keeper’s private key(s) with the utmost level of security and introduce as many safeguard checks as possible to limit the scope of the keepers potential attack vectors.

GMX Team: Acknowledged.

The sendWnt function can be called by anyone to collect any ether that may find itself in the ExchangeRouter contract.

Meanwhile, the FundReceiver contract that the ExchangeRouter extends stipulates that the controller is the only address that can recover these funds using the recoverNativeToken function.

If it is not desired that any address be able to rescue trapped ether, consider refactoring the sendWnt logic to be able to safely use the actual amount of ether the user provided. Otherwise, no changes are necessary.

GMX Team: The recommendation was implemented.

There is a potential DoS with createDeposit, createWithdrawal, and createOrder as a malicious address may send a miniscule amount of WNT to the deposit/withdrawal/order store such that another user’s WNT deposit no longer matches the executionFee in their deposit. Thus, the deposit execution reverts and is unable to be created.

Consider removing the strict equality for the executionFee or handling the accounting in the depositStore such that another address cannot increase the deposit amount for a user.

GMX Team: The strict equality has been replaced with a greater than or equal to check.

When creating a deposit, there is no validation that the longToken or the shortToken are valid for the supplied market. If WBTC is accidentally used as the long token in an ETH/USDC market, the user would lose their WBTC in the depositStore.

Furthermore, there is no validation that either the long token amount or short token amount is non-zero. This check should be added to prevent the keeper from executing trivial deposits.

Additionally, when creating an order, there is no validation that the initial collateral token is valid for the provided market, which can lead to invalid orders stored in the orderStore.

Implement the above mentioned validations.

GMX Team: The recommendation was implemented.

A trader is allowed to specify a different referral code each time an order is created. Only one affiliate is permitted per trader account, so when an order is created with a different affiliate, the affiliate associated with the trader’s account is updated.

This can lead to affiliates missing out on rewards if a trader decides to use another affiliate’s referral code even if they were the one to bring the trader onto the platform.

Consider whether this is desired behavior, if not refactor the referral logic to continue to reward referrers who first bring a trader to the platform.

GMX Team: This is the desired behavior.

A user does not have the capability to cancel their deposit or withdrawal like with an order. As a result, if the keeper for some reason does not execute their deposit/withdrawal, their funds are locked.

Allow users to cancel their deposits and withdrawals and recover their funds.

GMX Team: cancelDeposit and cancelWithdrawal functions were added.

A user can cancel an order on their own without the need of a keeper. Even if a user cancels an order on their own accord, the executionFee is paid to the keeper which doubles the amount of gas a user expends.

Do not pay the executionFee to the keeper if the user is simply cancelling the order.

GMX Team: The executionFee will still go to the user in this instance as their address is passed as the keeper.

In every catch (bytes memory _reason) case, the reason is parsed with string(abi.encode(_reason)) However, parsing the bytes reason like this results in unintelligible revert strings.

The first 4 bytes of the _reason bytes represent the selector for the error that caused the revert, and the rest represent the data that accompanies the error. There ought to be a way (perhaps off-chain) to map from these 4 bytes to the error type and decode the data.

Additionally, panic reverts will be caught in this case and should not be parsed.

Consider an alternative approach to parsing the bytes revert reasons, and know that it is possible by chance for the 4 byte selector for two separate errors to be the same if they are not defined in the same contract.

GMX Team: Reasons are no longer parsed for bytes reverts.

When a limit order is executed with an invalid increasing/decreasing price range, the order reverts with a bespoke revert string–which results in the order getting frozen. But this is unexpected as all other order price-related errors simply revert and will be retried. This might lead to a poor execution of limit order types or a complete lack of execution of limit orders.

Consider reverting in this case with a custom error that is handled similarly to the UNACCEPTABLE_PRICE_ERROR_KEY.

GMX Team: The recommendation was implemented.

When creating a decrease order or a swap order, there is no validation that the receiver address is not the zero address. In this case, when the order is executed and a swap takes place, the user’s funds will be left in the MarketToken, otherwise if no swap is executed during the decrease, the funds would be sent to the zero address.

Do not allow users to possibly lose funds this way and validate that the receiver is not the zero address for decrease and swap orders.

GMX Team: The recommendation was implemented.

Anyone can call notifyFeeReceived as there is a lack of access control, which leads to a depreciation of the authenticity of the event.

Implement access control concerning who may call notifyFeeReceived.

GMX Team: The recommendation was implemented.

When computing funding fees in the getNextFundingAmountPerSize function, the cache.oi.longOpenInterest == 0 || cache.oi.shortOpenInterest == 0 condition stipulates that when all of the open interest is stacked on one side no funding fees are charged to the users who have positions on the stacked side. This means there is a lack of disincentive for the users with stacked positions to switch sides.

In such a case, the funding fees could be redirected to the poolAmount for the benefit of LPers.

Exchanges like Binance have a minimum funding fee that is held at all times to properly disincentivize such imbalances.

Consider whether or not funding fees should be charged when there is no opposing side open interest. If funding fees should in fact be charged when only one side has all of the open interest, refactor the existing logic to charge funding fees and optionally send them to the pool or an arbitrary fee receiver.

GMX Team: The current behavior is desired.

Extra validation checks should be added on the result from the Chainlink price feed to ensure non-stale data. The price from the data feed influences the execution of orders and liquidations so it is imperative the data is up to date and correct.

Add the following require statements to validate the price feed:

• require(answeredInRound >= roundID, "Chainlink:: Stale price")
• require(timestamp > 0, "Chainlink:: Round not complete")

GMX Team: We were informed by the Chainlink team that these checks are now outdated

In the future, GMX hopes to enable permissionless MarketToken creation. However, a MarketToken can use an arbitrary malicious backing token that performs mischievous actions such as cancelling orders on the exchange during execution when the token is transferred.

Additionally, it is possible for contracts of the existing tokens on the exchange to be upgraded with new logic that is able to maliciously exploit the exchange.

Be wary of malicious tokens and ensure there is no way to re-enter into the application during token transfers.

GMX Team: Acknowledged.

There should be validation that the medianMaxPrice is greater than the medianMinPrice. Otherwise, many critical exchange values such as poolValue, pnlToPoolFactor, and many others could be adversely affected.

Implement the validation that medianMaxPrice is greater than the medianMinPrice.

GMX Team: The recommendation was implemented.

When cancelling an order, a bespoke “USER_INITIATED_CANCEL” string is used as the reason. This reason string might be better suited as a constant variable that can be reused when checking to see if the cancel was user generated.

Consider making the “USER_INITIATED_CANCEL” string a constant variable.

GMX Team: Acknowledged.

Currently, there is no zero address check for the _newAccount. Users may accidentally burn their code ownership if the zero address is provided.

Consider adding a zero address check for the _newAccount.

GMX Team: Acknowledged.

The codeOwners, referrerDiscountShares, and referrerTiers functions should be made singular as they all return a single value rather than multiple.

Additionally, the tiers function may be better named as tierValues as this more clearly represents what the function returns.

Consider renaming the codeOwners, referrerDiscountShares, referrerTiers, and tiers functions.

GMX Team: Acknowledged.

The comment “the account of the position to liquidation” should read “the account of the position to liquidate”.

Update the comment.

GMX Team: The recommendation was implemented.

In the comment, “current” is misspelled as “curent”.

Update the comment.

GMX Team: The recommendation was implemented.

In the comment, “the initial token sent it for the swap” should read “the initial token sent in for the swap”.

Update the comment.

GMX Team: The recommendation was implemented.

In the comment, “Library” is misspelled as “Libary”.

Update the comment.

GMX Team: Acknowledged.

In the comment, “for withdrawals a market” should read as “for withdrawals for a market”

Update the comment.

GMX Team: The recommendation was implemented.

There is no corresponding event emitted when the fundingAmountPerSize is set in updateFundingAmountPerSize.

Consider emitting an event when updating the fundingAmountPerSize.

GMX Team: The recommendation was implemented.

There is no corresponding event emitted when the cumulativeBorrowingFactor is set in updateCumulativeBorrowingFactor.

Consider emitting an event when updating the cumulativeBorrowingFactor.

GMX Team: The recommendation was implemented.

Since the claimFundingFees and claimAffiliateReward functions accept an arbitrary receiver address, they are somewhat prone to frontend code injection or related social engineering attacks.

Consider sending the rewards directly to the msg.sender for increased security. If the receiver address is kept, be sure to validate that it is not the zero address so users are not able to accidentally burn their fees.

GMX Team: Acknowledged.

At the beginning of the increasePosition function the account, market, collateralToken, and isLong are set on the position from the params. But the position must already have these values as it is created with them and keyed off of them.

Remove these lines, otherwise document why they are necessary.

GMX Team: The recommendation was implemented.

The ownership transfer process should have a push and pull step rather than executing in a single transaction with setGov.

This way the protocol may avoid catastrophic errors such as setting the wrong governance address.

Implement a two step transfer process for the gov role where the new gov address must explicitly accept its new role.

GMX Team: The recommendation was implemented.

Swap orders are required to be executed with a zero address market, this should be validated when first creating a swap order to save the keeper from executing orders that will possibly trivially fail.

Consider adding validation that the market address is 0 for all swap orders in the createOrder function.

GMX Team: Acknowledged.

The longTokenUsd and shortTokenUsd values are computed on line 190, but those same values are recomputed on line 203.

Instead of recomputing the longTokenUsd and shortTokenUsd, use those variables in place of the recomputation.

GMX Team: Acknowledged.

Every time the emitSwapFeesCollected function is called, the action bytes are recomputed. These common action bytes would be better suited as “action keys” in the Keys.sol contract.

Create keys for the action bytes.

GMX Team: Acknowledged.

In the ExecuteOrderParams struct, the positionKey is never set or used.

Remove the extraneous positionKey.

GMX Team: Acknowledged.

The estimateExecuteIncreaseOrderGasLimit, estimateExecuteDecreaseOrderGasLimit, and estimateExecuteSwapOrderGasLimit all have the same exact logic, the only difference being the order type gas limit key.

A common function that abstracts over the order type gas limit key could de-duplicate these three estimation functions.

Implement a single estimation function that can be used with any order type gas limit key.

GMX Team: Acknowledged.

The maxPnlFactorForWithdrawals variable is poorly named. It serves as a lower bound for the pnlToPoolFactor after an ADL has been executed, but the naming as a max and relation to withdrawals are not immediately obvious without additional documentation.

Rename the variable or provide more documentation expounding upon the naming.

GMX Team: maxPnlFactorForWithdrawals is now minPnlFactorAfterAdl.

The name of the uint256 emitted with the MaxPriceAgeExceeded error is blockNumber, however this uint256 now represents a timestamp rather than a block number.

Update the variable name appropriately.

GMX Team: The recommendation was implemented.

Throughout the ExchangeRouter contract, an account variable is used to store the msg.sender, but this variable is often only used once. Instead of storing this variable on the stack, reference msg.sender directly to save gas.

Remove the sub-optimal declarations of the account address variable.

GMX Team: Acknowledged.

The isLong boolean is checked twice with two ternary operators, but it would save gas to only check isLong once with an if condition.

Replace the two ternaries with a single if condition.

GMX Team: Acknowledged.

The openInterestParams variable is declared and only used once, rather than declaring it use the return value of getNextOpenInterest directly in the call to _getPriceImpactUsd

Remove the declaration of the openInterestParams variable.

GMX Team: Acknowledged.

The market can be checked if it is empty before transferring the collateral token to save gas upon failure.

Move validation to the top of the function.

GMX Team: Acknowledged.

The Chain.currentBlockNumber() and Chain.currentTimestamp() values can be stored outside the for-loop to save gas.

Implement the suggestion above.

GMX Team: Acknowledged.

Gas can be saved by not declaring the pnl variable before returning it.

Directly return the value.

GMX Team: Acknowledged.

The nextIndex, receiver, and _params variables can be declared outside the for-loop and re-assigned upon each iteration to save gas.

Implement the suggestion above.

GMX Team: Acknowledged.

The fees.totalNetCost includes the fees.positionFeeAmountForPool and fees.borrowingFeeAmount in it’s summation calculation, but these two numbers were already summed for the fees.feesForPool variable. The fees.feesForPool can be used to sum the totalNetCost and save some gas.

Implement the suggestion above.

GMX Team: The recommendation was implemented.

The signerIndex and signerIndexBit variables can be declared outside the for-loop and re-assigned upon each iteration to save gas.

Implement the suggestion above.

GMX Team: The recommendation was implemented.

The medianMinPrice and medianMaxPrice variables can be declared outside the for-loop and re-assigned upon each iteration to save gas.

Implement the suggestion above.

GMX Team: The recommendation was implemented.

The check require(priceFeedAddress != address(0), "Oracle: invalid price feed") could use a custom revert to save gas.

Implement the suggestion above.

GMX Team: Acknowledged.

The isMarketOrder variable is only used once. Rather than allocating stack variable space, the if condition can simply use OrderBaseUtils.isMarketOrder(order.orderType()) to reduce gas expenditure.

Implement the suggestion above.

GMX Team: Acknowledged.

The order is stored as a variable here, but it is often simply accessed through the params. Either undeclare it to save gas or use it to access every order attribute rather than repeatedly using the params.

Implement the suggestion above.

GMX Team: Acknowledged.

The ExecuteWithdrawalParams to execute a withdrawal can be created directly in the executeWithdrawal function call to save gas.

Implement the suggestion above.

GMX Team: Acknowledged.

The ExecuteDepositParams to execute a deposit can be created directly in the executeDeposit function call to save gas.

Implement the suggestion above.

GMX Team: Acknowledged.

The values.remainingCollateralAmount -= params.order.initialCollateralDeltaAmount().toInt256() calculation can occur on the line above where values.remainingCollateralAmount is declared.

Implement the suggestion above.

GMX Team: Acknowledged.

Instead of declaring the key variable, simply return the hash to save on gas expenditure.

Implement the suggestion above.

GMX Team: Acknowledged.

Instead of declaring the reason variable, pass the result directly to the event.

Implement the suggestion above.

GMX Team: Acknowledged.

Instead of declaring the reason variable, pass the result directly to cancelWithdrawal .

Implement the suggestion above.

GMX Team: Acknowledged.

The token, priceFeed, _price, price, precision, stablePrice, and priceProps variables can be declared outside the for-loop and re-assigned upon each iteration to save gas.

Implement the suggestion above.

GMX Team: Acknowledged.

The address, numbers, flags, and order variables can be created directly when calling the orderStore.set function to save gas.

Considering the trade-off in readability, Implement the suggestion above.

GMX Team: Acknowledged.

There is no in-code requirement that the keeper must updateAdlState to turn ADL mode off. This requires users to trust that the keeper will turn ADL off when necessary, but this could be avoided by adding a time range to ADL mode or validating the maxPnlFactor upon ADL execution.

Consider adding a time range to ADL mode or validating the maxPnlFactor upon ADL execution.

GMX Team: Acknowledged.

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

• MarketUtils.sol::1272
• Oracle.sol::242
• Oracle.sol::287
• Oracle.sol::424
• Oracle.sol::454
• Oracle.sol::472
• Oracle.sol::523
• OracleUtils.sol::121
• Reader.sol::59
• ExchangeRouter.sol::273
• ExchangeRouter.sol::302
• SwapUtils.sol::92
• Timelock.sol::44
• Timelock.sol::57
• Array.sol::37
• Array.sol::54
• PayableMulticall.sol::20

Implement the suggestion above.

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:

• MarketUtils.sol::1272
• Oracle.sol::242
• Oracle.sol::424
• Oracle.sol::454
• Oracle.sol::472
• Oracle.sol::523
• Reader.sol::59
• ExchangeRouter.sol::273
• ExchangeRouter.sol::302
• SwapUtils.sol::92
• Timelock.sol::44
• Timelock.sol::57
• PayableMulticall.sol::20

Implement the suggestion above.

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:

• DepositUtils.sol::215
• DepositUtils.sol::233
• DepositUtils.sol::295
• DepositUtils.sol::304
• Oracle.sol::551
• DecreaseOrderUtils.sol::74
• OrderUtils.sol::182
• DecreasePositionUtils.sol::302
• DecreasePositionUtils.sol::345
• DecreasePositionUtils.sol::386
• DecreasePositionUtils.sol::552
• IncreasePositionUtils.sol::243
• IncreasePositionUtils.sol::292
• TokenUtils.sol::186
• WithdrawalUtils.sol::233
• WithdrawalUtils.sol::255

Implement the suggestion above.

GMX Team: Acknowledged.