GMX Synthetics

OrderHandler::cancelOrder, which is an external function, is not protected by a reentrancy guard. Moreover, OrderUtils::cancelOrder (which performs the actual operation) transfers funds to the user (orderStore.transferOut) before updating its state. As a consequence, a malicious adversary could re-enter in cancelOrder, and execute an arbitrary number of transfers, effectively draining the contract’s full balance in the corresponding token.

function cancelOrder(
    DataStore dataStore,
    EventEmitter eventEmitter,
    OrderStore orderStore,
    bytes32 key,
    address keeper,
    uint256 startingGas
) internal {
    Order.Props memory order = orderStore.get(key);
    validateNonEmptyOrder(order);
    if (isIncreaseOrder(order.orderType()) || isSwapOrder(order.orderType())) {
   	 if (order.initialCollateralDeltaAmount() > 0) {
   		 orderStore.transferOut(
   			 EthUtils.weth(dataStore),
   			 order.initialCollateralToken(),
   			 order.initialCollateralDeltaAmount(),
   			 order.account(),
   			 order.shouldConvertETH()
   		 );
   	 }
    }
    // Dedaub: state changed after the transfer, also idempotent
    orderStore.remove(key, order.account());

Note that the main re-entrancy method, namely the receive hook of an ETH transfer, is in fact protected by using payable(receiver).transfer which limits the gas available to the adversary’s receive hook. Nevertheless, an ERC20 token transfer is an external contract call and should be assumed to potentially pass the execution to the adversary. For instance, an ERC777 token (which is ERC20 compatible) would implement transfer hooks that could easily be used to perform a reentrancy attack.

Note also that the state update (orderStore.remove(key, order.account())) is idempotent, so it can be executed multiple times during a reentrancy attack without causing an error.

To protect against reentrancy we recommend

• Adding reentrancy guards, and
• Execute all state updates before external contract calls (such as transfers).

Note that (2) by itself is sufficient, so reentrancy guards could be avoided if gas is an issue. In such a case, however, comments should be added to the code to clearly state that updates should be executed before external calls, to avoid a vulnerability being reintroduced in future restructuring of the code.

For all order types that involve ETH, a user can set the option shouldConvertETH =true to indicate that he wishes to receive ETH instead of WETH. Although convenient, this gives an adversary the opportunity to execute “conditional orders” in a very easy way. The adversary can simply use a smart contract as the receiver of the order, and set a receive function as follows:

contract Adversary {
    bool allow_execution = false;
    receive() {
        require(allow_execution);
    }
}

Then, in the time period between the order creation and its execution, the adversary can decide whether he wishes the order to succeed or not, and set the allow_execution variable accordingly. If unset, the receive function will revert and the protocol will cancel the order.

The possibility of conditional executions could be exploited in a variety of different scenarios, a concrete example is given at the end of this item.

Note that the use of payable(receiver).transfer (in Bank::_transferOutEth) does not protect against this attack. The 2300 gas sent by transfer are enough for a simple check like the one above. Note also that, although the case of ETH is the simplest to exploit, any tokens that use hooks to allow the receiver to reject transfers (eg ERC777) would enable the same attack. Note also that, if needed, the time period between creation and execution could be increased by simultaneously submitting a large number of orders for tiny amounts (see L2 below).

One way to protect against conditional execution is to employ some manual procedure for recovering the funds in case of a failed execution (for instance, keeping the funds in an escrow account), instead of simply canceling the order. Since a failed execution should not happen under normal conditions, this would not affect the protocol’s normal operation.

The adversary wants to take advantage of the volatility of ETH at a particular moment, but without any trading risk. Assume that the current price of ETH is 1000 USD, he proceeds as follows:

• He creates a market swap order A to buy ETH at the current price. In this order, he sets shouldConvertETH = true and the receive function above that conditionally allows the execution.
• He also creates a limit order B to sell ETH at 1010 USD.

He then monitors the price of ETH before the order’s execution:

• If ETH goes down, he does nothing. allow_execution is false so order A will fail, and order B will also fail since the price target is not met.
• If ETH goes up, he sets allow_execution = true, which leads to both orders succeeding for a profit of 10 USD / ETH.

StrictBank::recordTokenIn computes the number of received tokens by comparing the contract's current balance with the balance at the previous execution. However, this approach could lead to incorrect results in the case of ERC20 tokens with non-standard behavior, for instance:

• Tokens in which balances automatically increase (without any transfers) to include interest.
• Tokens that allow interacting from multiple contract addresses

To be more robust with respect to such types of tokens, we recommend comparing the balance before and after the current incoming transfer, and not between different transactions.

The protocol is susceptible to an attack involving opening a delta neutral position using Sybil accounts, with maximum leverage, on a volatile asset.

Scenario: \

1. Attacker controls Alice and Bob \

2. Alice opens a large long position with maximum leverage \

3. Bob opens a large short position with maximum leverage

4. Market moves

5. Alice liquidates their underwater position, causing bad debt

6. Bob closes their other position, profiting on the slippage

The reason why this attack is possible is that using the current design, it takes multiple blocks for a liquidator to react and by that time their order is executed it is possible that one of the positions is underwater. Secondly, when liquidating, Alice does not suffer a bad price from the slippage incurred in the liquidation but Bob benefits from the slippage, when closing their position just after. Another factor that contributes towards this attack is that the liquidation penalty is linear, while the price impact advantage is higher-order, making the attack increasingly profitable the larger the positions.

To deter this, the protocol could support (i) partial liquidations for large positions and therefore force the positions to be closed gradually, making the attack non-viable, and, (ii) slippage open-interest calculations used to determine the price of the liquidation.

OrderStore does not contain a receive function so it cannot receive ETH. However, this is needed by Bank::_transferOutEth, which withdraws ETH before sending it to the receiver.

function _transferOutEth(address token, uint256 amount, address receiver) internal {
	require(receiver != address(this), "Bank: invalid receiver");

	IWETH(token).withdraw(amount);
	payable(receiver).transfer(amount);

	_afterTransferOut(token);
}

WIthout a receive function any transaction with shouldConvertETH = true would fail.

Since there are no lower bounds for the size of a position, someone could in principle create a large number of tiny orders. Such a strategy would cost the adversary only the gas fees and the total amount of the requested positions, which can be as small as he wishes. In this 2-step procedure used in this version of the protocol, such a behavior could potentially create a problem, because the order keepers would have to execute a huge number of orders.

We suggest setting a minimum size for positions and swaps.

The comment in PositionUtils::isPositionLiquidatable indicates that price impact is not used when computing whether a position is liquidatable. However, the price impact is in fact used in the code:

// price impact is not factored into the liquidation calculation
// if the user is able to close the position gradually, the impact
// may not be as much as closing the position in one transaction
function isPositionLiquidatable(
  	DataStore dataStore,
   	Position.Props memory position,
   	Market.Props memory market,
    	MarketUtils.MarketPrices memory prices
) internal view returns (bool) {
      ...

    	int256 priceImpactUsd = PositionPricingUtils.getPriceImpactUsd(...)
      …
    	int256 remainingCollateralUsd = collateralUsd.toInt256() + positionPnlUsd + priceImpactUsd + fees.totalNetCostAmount;

On the other hand, when the liquidation is executed, the price impact is not used. The comment in DecreasePositionUtils::processCollateral indicates that this is intentional:

 // the outputAmount does not factor in price impact
 // for example, if the market is ETH / USD and if a user uses USDC to long ETH
 // if the position is closed in profit or loss, USDC would be sent out from or
 // added to the pool without a price impact
 // this may unbalance the pool and the user could earn the positive price impact
 // through a subsequent action to rebalance the pool
 // price impact can be factored in if this is not desirable

If this inconsistency is intentional, it should be properly documented in the comments.

Note that, when deciding on a liquidation strategy, you should have in mind the possibility of cascading liquidations, namely the possibility that executing a liquidation causes other positions to become liquidatable.

Due to the two step execution system, any operation requires an order keeper to execute it. Trust is needed in that order keepers will timely execute all pending orders. If the order keeper does not submit execution transactions, all operations will cease to function, including closing positions and withdrawing funds.

It would be beneficial to implement fallback mechanisms that guarantee that users can at least withdraw their funds in case order keepers cease to function for any reason. For instance, the protocol could allow users to execute orders by providing oracle prices themselves, but only if an order is stale (a certain time has passed since its creation).

There is nothing in the current system that prevents an order keeper from front-running or reordering transactions, for instance to exploit changes in the price impact. The protocol could include mechanisms that limit this possibility: for instance, the order keeper could be forced to execute orders in the same order they were created.

Price impact is calculated as

The values of exponent (e) and impact factor (f) are set by the protocol for each market. If the impact factor is simply a percentage, then the price impact will have units USD^(price impact exponent). But this seems erroneous, since price impact is treated as a USD amount which is finally added to the amount requested by the user. A problem arises in case that these two quantities are selected independently of each other but also of the pool's deposits and status.

For example, consider a pool with tokens A and B of total USD value x and y respectively. Consider that x < y. Then the imbalance equals d = y - x. If a user swaps A tokens of worth d/2, then prior to the price impact he will get B tokens of the same value. The new deposits of the pool will now be x'=y'=(x+y)/2 and the pool will become balanced. The price impact for this transaction is f*d^e, which could be ( if the parameters are not chosen carefully) larger than d/2, which is the requested swap amount. Also, this fact could lead to a pool which is even more imbalanced than the previous state.

We suggest that (total_deposits)^(e-1)*f always be less than 1 to avoid the above mentioned undesirable behavior.

Price keepers are responsible for submitting prices for most of the protocol's tokens. The submitted price should be the price retrieved from exchange markets at the time of each order's creation (the median of all these prices is finally used). However, for some tokens Chainlink oracles are used. In this case, the price at the time of the order execution is used.

ExchangeRouter::createLiquidation can be called only by liquidation keepers. However, there is nothing preventing a user from calling createOrder with orderType=Liquidation, effectively creating a liquidation order for their own position. Although this is not necessarily an issue, it is unclear whether this functionality is intentional or not.

The price impact calculation is a function with an exponential factor of the absolute differences between open long and short interests before and after a trade. This works well for the average market, but on a large market with large open interests, it is not more efficient to open large positions. Consider that in other AMM designs, it is possible to open large positions with minimal price impact if the market is large (e.g., Uniswap ETH-USDC).

The code can be compiled with Solidity 0.8.0 or higher. For deployment, we recommend no floating pragmas, but a specific version, to be confident about the baseline guarantees offered by the compiler. Version 0.8.0, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.