PerpetualVault Mitigation Review

GMX is moving to a balanceWasImproved model instead of a hasPositiveImpact approach for the position fees. This upcoming change is listed here: https://github.com/gmx-io/gmx-synthetics/pull/93

However the Gamma contracts assume that the hasPositiveImpact approach is still being used. As a result, for markets with no price impact configured the fee calculated by Gamma will not match the fee calculated by GMX.

Additionally, when an order flips the skew, moving the absolute skew closer to balanced the order may improve the skew but still receive negative impact due to the negative impact factor being larger than the positive impact factor. Orders which meet this criteria will have the fee overestimated for the user.

The balanceWasImproved value for the order is not easily accessible without comparing the pool state directly before and after an order’s execution. Therefore the best course of action may be to either ensure that no zero slippage GM markets are used with the Gamma system, or to acknowledge the imperfection in the fee calculations for these markets.

The setGovernanceFee function lacks an implementation to assign the related governanceFee value.

Implement the logic to assign the governanceFee value in the setGovernanceFee function.

In the _withdraw function the feeAmount is removed from the collateralDeltaAmount regardless of the sign and magnitude of the pnl.

If the pnl is positive and the swapPnlToCollateral token swap succeeds, the profit will be used to cover the fee amount in the decrease order execution. As a result the withdrawer would pay the position fee by having it deducted from the collateralDeltaAmount specified by Gamma and also by having it deducted by the pnl amount returned by GMX.

If the position pnl is positive then account for the fact that the positive pnl amount can be used to cover the feeAmount.

However be aware that the positive pnl amount cannot be used to cover the feeAmount if the current position is long and the swapPnlToCollateral swap fails. If this swap is assumed to always succeed in the system, then consider carefully monitoring for the failure of this swap, denoted by the SwapReverted event emitted by GMX, and taking manual action.

In the afterLiquidationExecution function the gmxLock variable is assigned to false, however it is possible that an order is still pending in GMX for the vault.

This may lead to unintended consequences such as un-cancellable orders through the cancelOrder keeper function or accidental order creation by the keeper when one is already pending in GMX.

Remove the gmxLock assignment in the afterLiquidationExecution.

In the _withdraw function, if a liquidation occurred and the curPositionKey has been assigned to zero then refundFee is assigned as true in the _handleReturn invocation.

As a result, a refund may be granted to the user when the settle action in GMX has already taken place for their withdrawal.

Consider the following scenario:

• User A initiates a withdrawal
• The settle decrease order is submitted and is executed in GMX
• The nextAction.selector is a withdrawal action
• Before the Gamma keeper executes the next action, the vault position is liquidated and the curPositionKey is set to 0.
• The Gamma keeper executes the next action and the _withdraw invocation invokes a _handleReturn with refundFee as true.
• The user is refunded for the majority of their executionFee, such that refund + executionFee paid to GMX for the settle action is greater than the executionFee initially paid by the user.

When the _withdraw function is invoked in runNextAction and the vault position has been liquidated consider not refunding the fee in this case, since the settle action will have already taken place.

When a liquidation occurs during the withdrawal flow, it is possible for the settlement action to get stuck and continue to fail and be retried in a cycle.

Consider the following order of events:

• A user initiates a withdrawal, the settlement action is created in GMX.
• The vault position is liquidated in GMX, the afterLiquidationExecution sets the nextAction to be NextActionSelector.WITHDRAW_ACTION to restart the withdrawal and allow the user to exit.
• The settlement action is executed after the liquidation and is therefore cancelled.
• In the afterOrderCancellation callback the nextAction is reset to NextActionSelector.SETTLE_ACTION, therefore overwriting the nextAction that was set by the liquidation callback.
• The settlement order is submitted again, and will again fail and continue the cycle, preventing the user from withdrawing.

Consider adding a check in the afterOrderCancellation callback such that if the order was a settlement order and the curPositionKey has been set to zero then the next action is set to NextActionSelector.WITHDRAW_ACTION so that the withdrawal flow can be reset and the user can receive their funds.

In the _cancelFlow function during the final step of a deposit flow the deposit object can have a nonzero shares amount associated which is not removed from the total upon cancellation.

As a result, if a deposit is cancelled at the last step before the FINALIZE action, the totalShares accounting will be perturbed.

Consider explicitly disallowing the _cancelFlow function from being called for deposits which have already been minted a nonzero amount of shares.

The tests and PositionInfo interface relies on an outdated Reader contract that was deployed almost a year ago. This Reader may become deprecated with future releases and increases risk of an interface mismatch bug.

Consider updating to the most recent Reader deployment seen here: https://github.com/gmx-io/gmx-synthetics/blob/updates/deployments/arbitrum/Reader.json

And ensure that the interfaces in that Reader deployment match the interfaces used by Gamma, namely the inclusion of a positionKey bytes32 variable at the start of the PositionInfo struct: https://github.com/gmx-io/gmx-synthetics/blob/cb47fb783b017cfa815a353f6f39d415f60646e8/contracts/reader/ReaderPositionUtils.sol#L20

The NatSpec across many functions in the Gamma codebase is outdated.

Consider updating the NatSpec documentation for each function in the Gamma PerpetualVault codebase.

In the runNextAction function the _swapOutIndexToken function is used to swap any remaining index tokens from the contract balance to the collateral token.

When submitting the runNextAction transaction to the mempool, the keeper may not supply any metadata[1] entry if it doesn’t observe any index token contract balance.

However a malicious actor could frontrun the runNextAction transaction and supply index tokens to the contract in order to cause the _swapOutIndexToken logic to occur and therefore revert when the metadata[1] entry is decoded, thus censoring the runNextAction invocation.

Be aware of this DoS vector, on Arbitrum this is not currently a concern, however for any Gamma vaults on Avalanche this is noteworthy. Consider ensuring the keeper always provides a metadata[1] entry to handle even malicious indexToken transfers.

In the _totalAmount function when the positionIsClosed value is false the positionData is always queried from GMX for the curPositionKey.

However there are cases such as liquidation where the curPositionKey is reset to 0, while positionIsClosed is intentionally not marked as true.

No cases have been identified where the curPositionKey is zero and positionIsClosed is false for a leveraged position during an invocation of the _totalAmount function. However out of an abundance of caution, consider handling the zero curPositionKey case to avoid any unintended consequences of future updates.

When the curPositionKey is 0 the _totalAmount function should not consult GMX for the position value with the getPositionInfo function.

The documentation for the fundEth function mentions that the GmxProxy attempts to avoid native ether from GMX. However the GmxProxy specifically wants to receive native Ether from GMX and does so by implementing the refundExecutionFee function.

Remove the misleading fundEth comment.

In the getPositionInfo, getNegativeFundingFeeAmount, and getPnl functions the sizeInTokens variable is assigned as the result of the getPositionSizeInUsd function.

Correct the sizeInTokens variables to be the result of the getPositionSizeInTokens function.

When computing the position fee during a withdrawal there is no consideration for discounts that may be granted to the vault position due to pro tiers or in the case that a nonzero referral code is used.

As a result withdrawers may be charged a slightly higher fee than what is actually levied against the vault position.

Be aware of this behavior in the event that the vault receives a pro tier discount or if a referral code is used. This inaccuracy goes in the favor of the vault so it is acceptable.

In the withdrawal flow for perpetual positions the refundFee value in the _nextAction.data is always assigned to false. As a result users will never receive refunds for the overestimated portion of the execution fee they paid up front.

Consider if this is the expected behavior, if it is not consider assigning the refundFee value to true.

The COMPOUND_ACTION NextActionSelector is unused in the codebase.

Consider removing the COMPOUND_ACTION from the NextActionSelector enum.

During the signal change flow it is possible for a switch from 1x long to 1x short to be left incomplete if a GMX swap is used to close the existing position and the GMX swap is cancelled.

When the GMX Market Swap action is cancelled the nextAction.selector is assigned as a swap action, overwriting the increase action. Therefore when the MarketSwap is retried it will be the final action in the flow and the signal change will complete without opening the new short position.

Be aware of this edge case when the MarketSwap action fails during a signal change. And consider only using dex swaps to avoid this failure when changing from long 1x to short 1x.

When a liquidation occurs directly after a user initiates a deposit the flowData is assigned as the result of the getPositionSizeInTokens for a curPositionKey of 0.

It is unlikely that a position may exist in GMX with a positionKey of 0, however in the event that such a position does exist the flowData would be errantly assigned to a value that is not accurate to the Gamma vault position.

Out of an abundance of caution in the getPositionSizeInTokens VaultReader function, return 0 if the positionKey provided is zero. Additionally, consider adopting the same behavior for other view functions which accept a position key as a parameter.

The rounding used in the getPriceImpactInCollateral function does not match the rounding performed by GMX when determining the sizeDeltaInTokens for a position. As a result the getPriceImpactInCollateral function will slightly miscalculate the price impact result.

The expectedSizeInTokensDelta is calculated as:

uint256 expectedSizeInTokensDelta = isLong ?
  sizeDeltaInUsd / prices.indexTokenPrice.max :
  sizeDeltaInUsd / prices.indexTokenPrice.min;

Meanwhile GMX calculates the sizeInTokensDelta as:

if (params.position.isLong()) {
    // round the number of tokens for long positions down
    cache.baseSizeDeltaInTokens = params.order.sizeDeltaUsd() / indexTokenPrice.max;
} else {
    // round the number of tokens for short positions up
    cache.baseSizeDeltaInTokens = Calc.roundUpDivision(params.order.sizeDeltaUsd(), indexTokenPrice.min);
}

Use round up division for shorts to match GMX’s calculation.

The priceImpactInCollateralTokens value is computed using the max or min indexTokenPrice and shortToken price depending on if the position isLong.

This may seem correlated to the way that the expectedSizeInTokensDelta is computed, however it is not. The expectedSizeInTokensDelta max and min price usage is simply necessary to compute the actual amount of price impact which was experienced in the GMX system.

Therefore the translation to a collateral token value of the price impact index token amount is arbitrarily assigned by the Gamma system. The calculation currently decides to use a maximum and minimum price interchangeably based upon isLong.

However, technically the most resilient method would be to use the price combination that rounds the amount of price impact experienced towards negative infinity. This follows the best practice of rounding in favor of the protocol over the favor of the user.

Consider implementing rounding in the favor of the protocol by assigning the minimum value of both calculations as the resulting priceImpactInCollateralTokens.

When a liquidation reduces the GMX position to zero, the curPositionKey is deleted and deposits are paused. However, since positionIsClosed remains false, users who withdraw are still charged an execution fee under the assumption that GMX calls like settle and withdrawGMX will be executed.

Since the curPositionKey is empty, these GMX calls are skipped. The _handleReturn function does issue a refund, but because no callback occurred, the refunded amount is incorrect. This causes users to be overcharged

Update the positionIsClosed flag after the GMX position is liquidated and the curPositionKey is cleared, or alternatively, deposit funds back into GMX immediately after liquidation so that future withdrawals trigger the expected callback and execution fee logic behaves correctly.

When a liquidation occurs, the vault processes it in afterLiquidationExecution and allows the keeper to create a new order using any remaining funds. However, if the vault is fully liquidated and no collateral is recovered, deposits become blocked.

This is due to a division-by-zero condition in the _mint function: totalAmountBefore becomes zero, while totalShares remains greater than zero.

As a result, the share calculation:

_shares = amount * totalShares / totalAmountBefore

Will revert due to a division by zero error, preventing any new deposits.

Add a case to handle the scenario where totalAmountBefore is zero but totalShares is non-zero. Depending on the desired behavior the depositor can receive the entire vault share or a diluted amount of it.

When an ADL occurs during the last step of a withdrawal action the entire withdrawal flow is re-started to account for any tokens which might have been sent to the PerpetualVault as a result of the ADL.

This however can mean that a withdrawal action can expend even more than the executionFee allocated for the settle and decrease actions.

Be aware of this behavior and be sure to top up the GmxProxy contract accordingly in the event that this occurs.

In the _handleReturn function the _transferToken function is only invoked when the withdrawal amount is nonzero. If a withdrawal amount is zero e.g. due to an insolvent liquidation of the vault, then the totalDepositAmount decrement inside of the _transferToken function will not be reached.

Consider refactoring the _transferToken function such that it handles the 0 amount case gracefully, decrementing the totalDepositAmount in all cases and early returning if the specified amount is 0.