Synthetics V2, Review 1

Proof of concept: PoC

In validateOracleBlockNumbersForPosition, both conditionals on lines 295 and 306 handle the LimitIncrease order type but with different oracle block requirements.

During execution, the first if statement is entered, therefore a LimitIncrease order is forced to use prices where the oracleBlockNumbers are equal to the orderUpdatedAtBlock. This contradicts the behavior outlined in the Order.sol documentation where:

“LimitSwap and LimitIncrease orders can always be executed if the right prices are reached due to this, validating that the prices presented are for blocks after the order's updatedAtBlock should be sufficient to prevent gaming of the pricing”.

Because LimitIncrease orders can only be executed with prices from their updatedAtBlock they are rendered nearly useless.

Remove LimitIncrease from the first conditional, as it is currently duplicated.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

A MarketIncrease order can mimic a limit order by specifying a minOutputAmount to fabricate a limit price. However, MarketIncrease orders will always be executed at the current market price, rather than limit and stop loss orders which always execute at the acceptablePrice. This behavior allows traders to obtain an unfair advantage as they can now have a market order acting as a traditional limit order (execution price <= acceptablePrice) rather than simply using a limit order (execution price == acceptablePrice). Combined with a stop loss, risk-free trades could be made.

Consider the following scenario:

• The current price of ETH is $2,000
• User A creates a stop-loss order for ETH at $1,800
• User A creates a market order for ETH with a minOutputAmount such that the market order is only executable when ETH is at or below the price of $1,800.
• The price of ETH drops to $1,700
• The market increase order is then executed at $1,700 while the stop-loss order is executed at the guaranteed $1,800
• User A is able to profit $100/ETH risk free.

Consider cancelling the MarketIncrease order if the minOutputAmount is not met. Alternatively, standardize the behavior of limit and stop loss orders so that they behave similarly to a MarketIncrease order with a minOutputAmount, taking the current price when it is <= the acceptablePrice rather than taking the acceptablePrice. However, protocol gaming would have to be taken into account.

GMX Team: MarketIncrease orders are now cancelled when the minOutputAmount is not met.

Proof of concept: PoC1

The try/catch block in each handler does not catch any errors related to a Panic exception or custom revert. This enables a number of bugs and exploits on the exchange.

Most notably, It is possible to create a LimitIncrease order that exceeds the max reserved usd, therefore reverting on execution but remaining in the order store, then when conditions are favorable the user can deposit enough reserves so their order gets executed.

Consider the following scenario:

• The current price of ETH is $2,000
• User A creates a LimitIncrease long order for ETH at $2,000 in block 100 with usd size larger than allowable
• User A’s order reverts due to insufficient reserves but remains in the order store
• 20 blocks later ether is now $2,100
• User A deposits enough reserves to execute their LimitIncrease
• The LimitIncrease long order now executes with the prices from block 101
• User A closes their position making risk free profit

Utilize catch (bytes memory lowLevelData) to catch Panic exceptions and custom reverts.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

Decrease orders that are partially filled are not removed from the orderStore and continue to exist in perpetuity. However the amount the phantom decrease position order is left with is equivalent to the amount your position was just decreased by not the amount that is leftover from the original decrease position order.

Additionally, because the order is not touched upon a partial decrease order, it is possible for a user to continually submit increase orders and realize immediate and risk-free profits.

Consider the following scenario:

• The current price of ETH is $2,000
• User A creates a MarketIncrease long order for ETH at $2000 with $1,000,000 sizeInUSD
• User A’s position is opened with a size of $1,000,000, and increases to a size of $1,050,000 when ETH reaches a price of $2,100
• User A sends a MarketDecrease order with sizeDeltaUSD $1,050,001 when ETH is $2,100
• User A’s position is closed but the decrease order remains in the store for $1,050,000
• The phantom MarketDecrease is not touched, therefore it must be executed at $2,100
• User A submits a MarketIncrease long order when ETH is back at $2,000 for $1,049,999
• The newly created position is closed by the MarketDecrease order at $2,100 for immediate profit
• The MarketDecrease order continues to exist in the orderStore, and as long as the price of ETH is below the decrease order price, the user can keep submitting large longs and realizing profit

Do not allow partially fulfilled orders to remain in the orderStore, or fix the partial fulfillment logic and address the block number at which these partially fulfilled orders can be executed at.

GMX Team: Partially filled orders are now touched upon execution.

Proof of concept: PoC

Bank.sol has no receive/fallback function defined to be able to accept ether, therefore it is impossible for the Bank to withdraw ether from the WETH contract and by extension it is impossible to transfer ether out of the Bank.sol contract.

Because of this:

• Swap orders with hasCollateralInEth == true lead to complete loss of funds for users.
• Orders, deposits, and withdrawals using hasCollateralInEth == true are unable to be canceled. This means users are unable to cancel hasCollateralInEth orders and the keeper is unable to cancel failing orders, deposits, and withdrawals leading to phantom entries in each corresponding store.
• StopLoss orders with no swapPath and hasCollateralInEth == true cannot be executed as they attempt to pay the user in ether from the bank.
• Additionally, users are unable to use the hasCollateralInEth flag to receive ether when withdrawing or exercising a decrease order.

Implement a receive function in the Bank.sol contract.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

For decrease orders with a defined swapPath the inputAmount for the swap is the initialCollateralDeltaAmount, which cannot be set for decrease order types and defaults to 0.

Therefore, if a user supplies a swapPath on a decrease order, they will experience a complete loss of funds. Zero tokens would be swapped and sent back to the user, yet their position would still decrease/close.

Provide the correct inputAmount for the swap that corresponds to how much the user’s position was decreased by.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

The pool value interprets traders profiting as an increase of value for the pool, and traders losing as a decrease of value of the pool. This is exactly the inverse of what should be happening. When traders lose and their loss is realized, their collateral is taken into the pool and the value of the pool increases.

Use return Calc.sum(value, -pnl).

GMX Team: The recommendation was implemented.

The liquidation handler has no reference to the orderStore, and does not assign one in the params to processLiquidation. Therefore, liquidations revert when attempting to access the orderstore on the ExecuteOrderParams params. Therefore it is impossible to perform liquidations on any position.

Include the orderStore in the ExecuteOrderParams in the liquidatePosition function.

GMX Team: The recommendation was implemented.

In processLiquidation, the marketDecrease order that is created for liquidation does not get touched, therefore the updatedAtBlock is always 0.

Therefore it is impossible to execute a liquidation with the correct block number for oracle prices and by extension impossible to liquidate any position.

Furthermore, the oracleBlockNumber requirements for a MarketDecrease order stipulate that the prices come from the block in which the order was updated at. This would demand that the oracle provide finalized prices for the block in which the liquidation transaction is to be executed, which is impossible as the block has not yet been confirmed.

Resolve the contradiction in block numbers for liquidations and MarketDecrease orders. Additionally, make sure to call touch() on the order when creating it in the processLiquidation function.

GMX Team: The recommendation was implemented.

Proof of concept: PoC

If a user creates a long position with the short token as collateral, they may only withdraw the long token or short token if there is enough short token liquidity in the pool. This can lead to circumstances where users are unable to withdraw from their positions even though they are technically backed.

This is because positions are keyed based on the initialCollateralToken and decrease orders must specify the same initialCollateralToken so there must exist enough short token liquidity to pay out the position for a decrease order.

This is strange as long positions are backed by long tokens, thus the user should be able to withdraw the long token upon decrease without relying on adequate short token liquidity to perform a swap.

Allow users to withdraw the long token in this case, or prevent such a scenario from happening in the first place in the validateReserves function.

GMX Team: Users profits are now paid in their backing PnL token.

Proof of concept: PoC

An order’s executionFee is validated when the keeper is already attempting to execute the order. A malicious user can send a large number of orders that are unable to be executed and have an executionFee of 0.

This will waste the keeper’s gas at no cost to the user each time execution is attempted, potentially delaying the execution of other deposits, withdrawals, and orders on the exchange.

Validate the executionFee upon order creation, similarly to deposits.

GMX Team: The recommendation was implemented.

The FeeReceiver contract is responsible for receiving fees for the entire protocol, but provides no functions to retrieve erc20 tokens nor ether. Therefore any fees sent to the FeeReceiver contract are lost.

Add a withdraw function with proper access controls to withdraw and manage fees.

GMX Team: The recommendation was implemented.

Documentation in README.md suggests that spot prices from multiple exchanges will be used to determine prices for execution. Such a price collection scheme potentially allows for economically viable price manipulation/arbitrage attacks. Attackers may be able to manipulate prices to game orders into guaranteed profits, or cause mass liquidations.

Adopt a TWAP with multiple price readings to make such attacks economically unviable. Otherwise, be prepared to use failsafes such as open interest caps to limit these attacks.

Optionally, order book depth/liquidity for each exchange should be considered in the calculation to further limit the scope of manipulation attacks.

GMX Team: Acknowledged

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 block numbers while staying within the MAX_ORACLE_BLOCK_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

There is a potential DoS with the setPrices function, as the lengths of the tokens, signers, compactedOracleBlockNumbers, and compactedPrices arrays are unbounded. The for loop through these arrays in addition to memory expansion costs may result in gas fees exceeding the block gas limit, especially with long swap routes that require many token prices.

Carefully consider the max signer bounds to limit gas usage. Furthermore, consider bounds on the number of tokens, as all signers are iterated over for each token.

GMX Team: The sizes of these arrays in practice should not come close to the block gas limit.

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: Acknowledged.

There is a potential DoS with createDeposit, as a malicious address may send a miniscule amount of WETH to the deposit store such that another user’s WETH 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: Acknowledged.

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: Acknowledged

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: Acknowledged.

It is possible to have a decrease order in the orderStore when no corresponding position is present because orders are not cancelled upon failure with the EMPTY_POSITION_ERROR_KEY.

Consider the following scenario:

• User A creates a position
• User A sends a LimitDecreaseOrder and a MarketDecreaseOrder in the same block
• LimitDecreaseOrder gets executed first
• MarketDecreaseOrder is now floating on an empty position and it never gets canceled
• User A opens a new position with an increase order of the same market, collateral token, and directionality
• The market decrease order unexpectedly affects User A’s new position as it was waiting to be executed in the orderStore

Cancel the order upon failed execution in the case of EMPTY_POSITION_ERROR_KEY.

GMX Team: Acknowledged.

According to documentation, the price for a token is only retrieved once a block is finalized.

However, _setPrices does not revert when the block number of the prices provided by the oracle equals the current block number: if (cache.oracleBlockNumber > block.number) { revert … }.

This allows the keeper to provide prices for a block number that is equivalent to the current block and waste gas when it would likely continue on to revert when validating signatures.

Use >= instead of >.

GMX Team: Acknowledged

In the processCollateral function, the collateralDeltaAmount is initially set to params.order.initialCollateralDeltaAmount().toInt256(), which cannot be set for decrease orders. Therefore, there is no way to decrease collateral from a position without closing the entire position.

Allow for initialCollateralDeltaAmount to be set for decrease orders so users are able to decrease their collateral without closing their position.

GMX Team: The recommendation was implemented.

There is potential for a position with $0 of remainingCollateral to avoid liquidation. This is due to the fact that the first conditional can avoid being triggered if the MIN_COLLATERAL_USD was not set. If so, the second conditional would revert due to division by 0. Thus, a position that is liquidatable cannot be liquidated.

Add zero checks for such a scenario.

if (remainingCollateralUsd == 0) return true;

GMX Team: The recommendation was implemented.

When creating a new market, nothing prevents the long token from being set to the same address as the short token.

In such a case, deposits would double count the funds you are sending to the pool. Since the creation of markets will eventually be unpermissioned, this validation is paramount.

Validate that the long token is not equal to the short token.

GMX Team: The recommendation was implemented.

On line 58, “current” is misspelled as “curent“.

Replace “curent” with “current” in the comment.

GMX Team: Acknowledged.

In the _setPrices function, nothing prevents the signatures array from being a different length than the signers array. If there are less signatures than signers, that would lead to an out-of-bounds error and the index price would not be able to get set.

In this case it would save gas to add a length validation check before commencing potentially expensive price computations.

Check that the length of the signatures array is a multiple of the length of the signers array.

GMX Team: The net result of the recommendation is still a reverted transaction, however optimizing for the revert case rather than the expected use case.

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: Acknowledged.

In the setGov function there are no zero address checks on the new gov address.

Add zero address checks as an errantly set gov address would be catastrophic for the protocol.

GMX Team: Acknowledged.

The createDeposit, createWithdrawal, and createOrder functions compose the user’s interface for the exchange but perform no zero address checks on the addresses they accept.

Add zero address checks to avoid any unexpected behavior when users interact with these functions. Document where zero addresses are expected, and enforce when they are not.

GMX Team: The recommendation was implemented.

Throughout the contracts many contract level variables can be declared immutable. Common variables such as the dataStore, depositHandler, withdrawalHandler, orderHandler, depositStore, withdrawalStore, and orderStore among others should be declared immutable in multiple contracts.

Implement the above recommended immutability modifiers.

GMX Team: The recommendation was implemented.

In the setExactOrderPrice function, the revert statement on line 264 can be replaced with a revertUnsupportedOrderType function call.

Implement the above function call.

GMX Team: The recommendation was implemented.

The _executeDeposit function is called from within the context of a try/catch, therefore it can be declared external.

Declare the function external.

GMX Team: The recommendation was implemented.

The _executeOrder function is called from within the context of a try/catch, therefore it can be declared external.

Declare the function external.

GMX Team: The recommendation was implemented.

The market, nextIndex, receiver, and _params variables can be declared outside of the for loop to save gas.

Declare these variables outside the for loop.

GMX Team: Acknowledged.

hasCollateralInEth is used in many different ways throughout the codebase, it should have a more adaptable/explicit name.

Rename hasCollateralInEth or assign specific names to use-cases such as deposits and withdrawals.

GMX Team: hasCollateralInEth is now shouldUnwrapNativeToken.

The processCollateral function accepts a position parameter, but the same position is already available on the params parameter of type DecreasePositionParams.

Remove the position parameter as it is available on the DecreasePositionParams.

GMX Team: Acknowledged.

It should be noted that when withdrawing marketTokens, a user will not always be able to redeem the corresponding amounts of underlying assets that they provided. This is because other depositors may withdraw whichever long or short collateral token they wish, as long as their withdrawal matches the marketToken value they received for depositing into the market.

Example:

• The ETH/USD market is empty
• User A deposits $500 worth of ETH and $500 worth of USDC
• User B deposits $500 worth of ETH and $500 worth of USDC
• User A withdraws $1000 worth of USDC
• User B attempts to withdraw $500 worth of ETH and $500 worth of USDC, but the tx reverts because there is no USDC left in the pool

Ensure the withdrawal behavior is clearly documented and communicated to users.

GMX Team: This will be clearly documented for users.

When an order is executed and reverts with an error that is not retried, it gets canceled silently. There are no events emitted nor is there a return value for the executeOrder function to communicate whether an order was canceled or not.

Implement a return value or event that indicates whether or not an order was canceled.

GMX Team: An event is now emitted upon order cancellation.

In Bank.sol the require(receiver != address(this), "Bank: invalid receiver") check can be deduplicated into a modifier that also validates that the receiver is a non-zero address.

Additionally, these checks can be made in transferOut and validate that the amount is greater than zero.

Implement the above refactors.

GMX Team: Acknowledged.

Throughout the contracts there are missing events for important actions such as order execution, position size modifications, liquidations, deposits, and withdrawals.

Implement events throughout the contracts for improved monitoring, readability, and transparency.

GMX Team: Events have been added throughout the contracts.

The function contains is declared as public but is never called from within the contract.

Modify the visibility from public to external.

GMX Team: The recommendation was implemented.

The following parameters are marked as memory when they could be declared calldata:

• PositionStore::18
• DepositHandler::73,102
• OrderHandler::58,73,155
• OrderUtils::73
• DepositUtils::61
• DepositStore::17
• WithdrawalUtils::74
• WithdrawalStore::17
• WithdrawalHandler::84,112
• OracleModule::15
• Oracle::82
• LiquidationHandler::51

If it is desired to reduce gas for these functions, potentially at the expense of deployment costs, change the suggested memory modifiers from memory to calldata.

GMX Team: Acknowledged.

Throughout the contracts uint variables are initialized to the default value (0):

• MarketUtils.sol::619
• Oracle.sol::100,136,226,252,285
• OracleUtils.sol::45
• SwapUtils.sol::44
• Timelock.sol::38,44
• Array.sol::33,41,51

Remove the unnecessary assignments.

GMX Team: Acknowledged.

Throughout the contracts array lengths should be computed outside of for loops:

• MarketUtils.sol::619
• Oracle.sol::22,100,252,285
• SwapUtils.sol::44
• Timelock.sol::38,44
• Array.sol::33,41,51

Compute array lengths outside of for loops.

GMX Team: Acknowledged.

Throughout the contract, revert strings can be shortened to 32 bytes to decrease gas costs:

• WithdrawalHandler.sol::46
• IncreaseOrderUtils.sol::48
• OrderUtils.sol::264
• DecreasePositionUtils.sol::141
• WithdrawalUtils.sol::76,106,204

Shorten revert strings to 32 bytes.

GMX Team: Acknowledged.

Throughout the contract, switching from division/multiplication to right-shift/left-shift can save gas:

• PricingUtils.sol::64
• Array.sol::62,65

Switch uses of division to right-shift and uses of multiplication to left-shift.

GMX Team: Acknowledged.

In the call to SwapPricingUtils.GetSwapPricingParams, the values stored in longTokenUsd and shortTokenUsd on lines 113 and 114 are recalculated on lines 126 and 127.

Use the previously computed values instead of recalculating.

GMX Team: Acknowledged.

Declaring the price variable on line 208 is unnecessary, as the calculated value is only used once.

Remove the price variable declaration and use the result of the calculation directly.

GMX Team: Acknowledged.

Validation for the market can occur before any transfer logic in order to save gas.

Move the MarketUtils.validateNonEmptyMarket call to the first line in processOrder.

GMX Team: The recommendation was implemented.

Throughout the contracts, several require statements can be converted to custom reverts. The following files contain statements that can be replaced:

• OrderUtils.sol::90
• WithdrawalUtils::76,204
• DepositHandler::44
• OrderHandler::53,130
• withdrawalHandler::46
• RoleModule.sol::31,36,41,46,51

Replace require statements with custom reverts.

GMX Team: The recommendation was implemented.

MarketToken.burn will revert on its own if the user has insufficient market tokens.

Remove the validation check if a custom revert is not necessary.

GMX Team: Acknowledged.

There is no need to set the position’s collateralAmount to 0, as the position is immediately removed on line 99.

Remove the line setting the position’s collateralAmount.

GMX Team: Acknowledged.

Throughout the contracts numerous for loops have upper bounds with no risk of overflow for the index. Therefore an unchecked block can be used in the loop to increment the index and save gas.

Implement the above suggestion.

GMX Team: Acknowledged.

Throughout the codebase there are numerous library functions that can be declared internal to save gas.

Declare library functions that do not need to be called in an external context as internal.

GMX Team: Acknowledged.