Liquidity Management

Summary:

FIX -> By allowing raw uint256 subtractions on large priceImpact, the vault’s GMX callback can revert—contradicting the requirement that “it must never revert.” This can freeze the vault in a partial deposit flow, causing a denial of service and locking user funds. A robust fix involves safe checks or try/catch logic so that the callback can gracefully handle edge cases without bricking the protocol.

RootCause & Where It Happens

In afterOrderExecution(), when an increase position on GMX finishes, the code calculates how much a depositor’s deposit should “increase” by factoring in the position fee and price impact:

The problem:

1. If priceImpact is large (for example, if the deposit is small but the price impact is big),

2. Then amount - feeAmount - uint256(priceImpact) - 1 can underflow (become negative) in Solidity’s uint256 math.

Solidity will revert on an underflow. Because this is inside afterOrderExecution(), which GMX calls externally, that entire callback reverts. GMX will keep trying to finalize this order or expect the callback to succeed; meanwhile, your protocol gets stuck in a partial deposit flow. No user or keeper can fix it on-chain, because each time GMX triggers the callback, it fails again.

Why This Freezes the Vault

• The vault is locked in a deposit flow. GMX is “waiting” for a successful callback to finalize the deposit.

• Each repeated attempt to finalize calls afterOrderExecution() again, which triggers the same arithmetic, which triggers the same underflow → revert.

• There is no code path (like a try/catch) to absorb or handle big price impacts gracefully. The doc says “This callback function must never revert,” but it can and will if the arithmetic fails.

• The vault is effectively bricked for that position, because the deposit flow never completes, and the contract’s state machines (flow) do not proceed.

Severity

Potentially high severity. It creates a Denial of Service scenario for the entire deposit flow—and possibly the entire vault if that flow cannot be canceled or completed. Funds remain locked until the vault owner or some external fix (like an upgrade) intervenes.

Attack/Exploit Scenario

• Even if no one is malicious, a deposit with small amount and high real-time price impact can accidentally cause this revert.

• A malicious user or a volatile price environment could deliberately trigger large price impacts to freeze the vault’s deposit flow. Once stuck, normal users cannot proceed with deposits/withdrawals.

Proof by Example

1. User initiates a deposit with a small amount.

2. The market is extremely volatile, or the GMX price feed calculates an unusually high priceImpact.

3. priceImpact > (amount - feeAmount - 1) → leads to an underflow in the callback.

4. Callback reverts → GMX sees a revert → keeps retrying or flags the order as unfulfilled → your vault remains stuck in FLOW.DEPOSIT with _gmxLock = true until forcibly canceled (which may or may not be possible depending on the scenario).

Foundry PoC

Recommended Remediation

1. Never do raw sub/add with untrusted priceImpact in the callback without safe checks:

   • Use checked arithmetic or require statements like:

     if (priceImpact > 0) {

     require(uint256(priceImpact) <= amount - feeAmount - 1, "Excessive priceImpact");

     increased = amount - feeAmount - uint256(priceImpact) - 1;

     } else {

     ...

     }

     ​

     If the condition fails, handle it gracefully rather than letting the entire function revert. For example, treat it as if the deposit is fully consumed by fees/impact (i.e. increased = 0) or abort the deposit flow with a separate state update that doesn’t revert the callback.

2. Consider a try-catch around any calls or math that might revert. The contract states that _afterOrderExecution must never revert. So either:

   • Implement logic that sets increased = 0 if it’s going negative, or

   • Mark the deposit as “failed” but do not revert.