GMX V2 Cauldron

In the sendValueInCollateral function the amount provided as a parameter is a collateralShare amount (GM shares in the degen box), that amount is then converted to a shortToken amount via an exchange rate.

The amountShortToken is then sent to the degenBox, however the amount (GM shares) is what is deposited.

Therefore a GM token Shares amount will be treated as a short token amount, which may have severely different valuations depending on the exchange rate. Additionally this can often cause the liquidation to revert if the GM token is a lower value than the short token.

In the sendValueInCollateral function, line 197:

degenBox.deposit(IERC20(shortToken), address(degenBox), recipient, amount, 0);

Should be replaced with:

degenBox.deposit(IERC20(shortToken), address(degenBox), recipient, amountShortToken, 0);

Abracadabra Team: Remediated with commit: bda0214.

During liquidation, if a user has an open order, the order is cancelled in order to retrieve the underlying tokens. However orders cannot be cancelled while within the REQUEST_EXPIRATION_BLOCK_AGE.

Therefore a malicious user can front-run liquidations and create an order to prevent liquidations. The REQUEST_EXPIRATION_BLOCK_AGE is currently configured to 1200 blocks on Arbitrum. In practice, the time to execute an order in GMX should be ~2 seconds, however when the deposit or withdrawal feature is disabled on GMX the keeper will not execute or cancel orders.

Therefore a malicious user can gain a ~5 minute grace period where they cannot be liquidated due to the REQUEST_EXPIRATION_BLOCK_AGE when the deposit or withdrawal feature is disabled as the order will not be executed or cancelled by the keeper.

Do not allow the creation of orders when an account is liquidatable.

Additionally, consider validating that the deposit and withdrawal feature are enabled before users are able to use the ACTION_CREATE_ORDER.

Abracadabra Team: An insolvency check upon order creation was added in commit debb03.

Validation that deposit and withdrawal execution is not disabled was added in commit 9f2aa9.

When validating whether a borrow position is solvent, the COLLATERIZATION_RATE is used to verify that the amount borrowed does not exceed the maximum percentage borrowable with the current collateral.

However, the COLLATERIZATION_RATE is not applied to amountToAdd, which is the amount of GM tokens expected to be obtained from a pending deposit.

For example, assume MIM and GM have a 1-1 price and the COLLATERIZATION_RATE is 75%. 10 GM tokens would allow borrowing 10 MIM rather than 7.5.

As a result, the amount of MIM able to be borrowed is calculated to be larger than allowed, and the position is deemed solvent.

Value the amountToAdd at the COLLATERIZATION_RATE.

Abracadabra Team: Resolved in commit 2c2d6c0.

In the _get function the price of the long token provided is the price of the index token, however not all GM markets have the long token as the index token.

For example, the DOGE/USD GM market has Ether as its long token. In this case the price of ETH is provided as the price of DOGE which will be dramatically inaccurate.

Treat the long token separately from the index token, as these two are not guaranteed to be the same.

Abracadabra Team: With the planned supported GM markets this oracle configuration is expected, however if we choose to support the DOGE or other similar gm markets in the future we will make a change.

Insolvent liquidations cannot occur if the Order contract does not have enough to cover the outstanding amount.

It is unlikely that positions are able to become insolvent as this would require unexpected immediate price action or inaction from liquidators, however it would be prudent to be able to handle such a case.

Consider implementing logic such that insolvent positions may be liquidated successfully.

Abracadabra Team: We can do partial liquidations to avoid this.

In the event where a homogenous market or any GMX market had WETH as a short token, it would be possible for the user to withdraw their backing tokens through the refundWETH function, without going through the expected ACTION_WITHDRAW_FROM_ORDER cook action, therefore avoiding the solvency check and allowing for a user to cause their position to go insolvent.

Such a market is unlikely to exist, however it should be explicitly stated that markets with Ether as the short token are incompatible with the system.

Abracadabra Team: Fixed with commit e265a7.

In the closeOrder function the blacklistedCallees mapping entry is set to false for the user’s order, however the user’s order was just set to the zero address and so therefore the previous order address entry was not correctly set to false.

Update the blacklistedCallees mapping before zeroing out the user’s order.

Abracadabra Team: Fixed with commit 623f7d.

If a user were to ever close their Order but still have some funds remaining in the Order, anyone would be able to ACTION_CALL the withdrawFromOrder function from the Cauldron and take those funds. This can occur if a user were to call withdrawFromOrder without their entire amount and set closeOrder = True.

Consider forcing a user to retrieve their entire shortToken and WETH balance prior to closing an order with the withdrawFromOrder function.

Otherwise, clearly document that users should not leave their funds in an Order after closing it, as it becomes unblacklisted and accessible for all.

Abracadabra Team: Fixed with commit c1fdaf.

In the _get function, the latestAnswer function is used to read the latest price from the Chainlink indexAggregator and shortAggregator.

However the latestAnswer function is deprecated and should be replaced by a latestRoundData call with heartbeat validation as well as a sequencer uptime check for Arbitrum.

Use the latestRoundData function to fetch the latest price from Chainlink and implement the necessary heartbeat and sequencer uptime validations. Blockchain Oracles for Connected Smart Contracts | Chainlink Documentation Chainlink Data Feeds Documentation | Chainlink Documentation

Abracadabra Team: Acknowledged.

In the GmOracleWithAggregator contract the reported price uses the MAX_PNL_FACTOR_FOR_TRADERS PNL_TYPE to read the market token price from GMX. However this PnL type is less constrictive on the trader PnL than the MAX_PNL_FACTOR_FOR_DEPOSITS.

Therefore when the market PnL is in between these two factors like so: MAX_PNL_FACTOR_FOR_TRADERS < PnL in market < MAX_PNL_FACTOR_FOR_DEPOSITS

Then the resulting price of the market token will be higher when measured using the more constrictive PnL factor for traders. Therefore the user’s collateral will be valued higher using the PnL factor for traders than compared to the PnL factor for deposits.

Out of an abundance of caution, it may be preferable to use the less constrictive max PnL for deposits, so that the collateral is not optimistically valued by capping the PnL to a lower amount. This is somewhat arbitrary as GM tokens cannot be redeemed as long as the pnl to pool ratio exceeds the MAX_PNL_FACTOR_FOR_WITHDRAWALS, which is the lowest of these PnL factors.

Consider using the less constrictive MAX_PNL_FACTOR_FOR_DEPOSITS to read the price of the GM token.

Abracadabra Team: Acknowledged.

In the orderValueInCollateral function the minOut and minOutLong values are considered regardless of if the order is pending or if it has reached a terminal status.

A deposit may have been cancelled or a withdrawal may have been executed, and therefore the Order contract has a distinct amount of short tokens, however the collateral value is still based on the minimum output that was configured for the order.

This directly discounts a user’s collateral as they will often receive more than the configured minimum amount.

If the order has reached a terminal status, return the balance of short tokens converted to market tokens as the orderValueInCollateral.

Abracadabra Team: We want to incentivize users to close orders if they are completed and wanted to limit complexity.

There are several characteristics of GM tokens that make them less than ideal as collateral.

Firstly, liquidations may be somewhat unwieldy for the liquidator as there is currently no market to swap GM tokens and unwrapping the GM tokens cannot occur in a single liquidation transaction.

Additionally, if the pnlToPoolFactor in the GMX system is above the MAX_PNL_FACTOR_FOR_WITHDRAWALS GM tokens cannot be redeemed for their backing longTokens or shortTokens.

No changes may be necessary, simply be aware of this unwieldiness for liquidators, ensure that liquidators are still properly incentivized and consider this when determining the COLLATERIZATION_RATE for GM tokens.

Abracadabra Team: Acknowledged.

In the liquidate function, when additional value is necessary to cover the borrowed amount and the Order.sendValueInCollateral function is called, the provided amountMarketToken is computed from collateralShare - userCollateralShare[user]. However this is a share amount rather than an elastic market token amount.

Currently there is no strategy for GM tokens in the DegenBox, however if there were to be a strategy that increased the elastic supply of GM tokens relative to the base shares, then the share value provided to the Order.sendValueInCollateral would be inaccurate.

Consider converting the outstanding collateralShare amount to a market token amount with the DegenBox.toAmount function before providing this amount as the amountMarketToken for the sendValueInCollateral function.

Abracadabra Team: Fixed with commit 431592.

In the cook function the ACTION_UPDATE_EXCHANGE_RATE action includes a minRate and maxRate to bound the allowed updated rate.

The rate is validated to be > minRate as well as > maxRate if a maxRate is set. However this misconstrues the meaning of a maxRate as the rate should be validated to be < maxRate.

Validate that the rate is < maxRate when the maxRate ≠ 0.

Abracadabra Team: Fixed with commit 55c602.

The init function may not be called again if the cauldron address has been assigned a non-zero value, however there is no restriction that the _cauldron parameter is not the zero address.

Within the context of the GmxV2CauldronOrderAgent this is not an issue as the _cauldron address provided to the init function is always the msg.sender which cannot be the zero address.

However it may be prudent to add validation that the _cauldron parameter value is not the zero address to prevent vulnerabilities if the GmxV2CauldronRouterOrder contract were to be used in a different context in the future.

Consider adding validation such that the _cauldron address cannot be the zero address in the init function.

Abracadabra Team: Fixed with commit b03d57.

In the GmvV2CauldronOrderAgent contract, the CALLBACK_GAS_LIMIT is hardcoded to 1_000_000. However in some cases the GMX team may reduce the maximum allowed gas limit such that 1_000_000 exceeds the maximum cap.

In this case it may be better to allow a reconfiguration of the CALLBACK_GAS_LIMIT than to deploy a new GmxV2CauldronRouterOrder implementation.

Consider making the CALLBACK_GAS_LIMIT a configurable value by a trusted address. Otherwise be aware that the GMX team could reduce the maximum callback gas limit to below 1_000_000 which would prevent any deposits or withdrawals from being created.

Abracadabra Team: Fixed with commit 965af4.

In the init function, there are no requirements that the assigned INTEREST_PER_SECOND, COLLATERIZATION_RATE, LIQUIDATION_MULTIPLIER, and BORROW_OPENING_FEE are within a reasonable range.

Consider adding validation in the future to restrict the possible values for these critical values.

Abracadabra Team: This is a design choice.

With the ACTION_WITHDRAW_FROM_ORDER action it is possible to close an order in the Abracadabra system while the order in GMX is still pending.

If the order is already closed in the Abracadabra system, then during the afterDepositExecution or afterWithdrawalCancellation callbacks, the call to the closeOrder function will revert and prevent any logic executing on the Abracadabra side, yet the order will still be executed.

The ACTION_WITHDRAW_FROM_ORDER action must leave the position in a solvent state in Abracadabra so there is no imminent risk to the Abracadabra system. However this is an unusual edge case and can result in users losing funds, therefore it may make sense to explicitly disallow it.

Consider disallowing the ACTION_WITHDRAW_FROM_ORDER action when an order is active.

Abracadabra Team: This is an edge case we expect only to be possible to custom frontends or direct contract interaction.