Liquidity Management

Vulnerability Details

In PerpetualVault a deposit or withdrawal flow can be cancelled via Keepers calling the cancelFlow function. The cancelFlow has the gmxLock modifier meaning that cancelFlow can be called anytime but not when gmx order request is on-going. Before _createIncrease/DecreasePosition and after it's executed fully.

But this could lead to situations where a flow is cancelled after a order is executed fully. And that might lead to some unintended results.

• During a deposit flow if the flow is cancelled after the execution of _createIncreasePosition it could result in a inflation in the value of shares if the vault has enough collateral tokens acquired form the last action. Since cancelling the flow in a situation like that would result in a refund of the user's collateral but it does not decrease the totalShares value. Meaning that although the user is now not a part of the vault the shares minted to them still remain in the totalShares. Resulting in inflation in the value of shares.

• During a withdrawal flow if the flow is cancelled after the execution of _createDecreasePosition, it could result in a situation where the position is not entirely closed and the vault now has the user's collateral that is to be returned plus any collateral amount resulting from the last action / settle action. But if the user calls withdraw again in this situation, the withdraw flow will execute resulting in another position of the withdrawer's amount being closed from the active position, and then the amount calculated in _handleReturn would result in a withdrawal amount a bit higher than during the previous withdrawal flow. Because the current balanceBeforeWithdrawal would not only include any collateral generated from last action and settle but also the collateral withdrawn during the previously cancelled flow.

Impact

During a deposit flow it could result in a inflation in share value and a deflation in share value during a withdrawal flow

PoC

During a deposit flow, the flow of action would be something like this:

We notice that even though the deposit was cancelled successfully the vault generates extra shares, this would result in inflation in share value during future withdrawals since we withdraw pro-rata to the shares owned shares / totalShares leaving unclaimable collateral becuase of the extra deposit amount that came in even though the flow was cancelled.

During Withdrawal flow :

Here we notice, if the withdrawal flow is cancelled after a position has been decreased, and then the withdrawer tries to withdraw again they receive more collateral for the same amount of tokens. Resulting in deflation of share value.

Tools Used

Manual Review
Foundry

Recommendations

The only recommendation I could think of is to allow keepers to only call cacelFlow before _createIncreasePosition / _createDecreasePosition is executed and not after that. This could be done by adding a flag variable such as bool positionExecuted that keeps track of the position execution during a single flow and allowing the keepers to enter cancelFlow only when positionExecuted is false

Note :- I consider this a valid issue because it does not stem from a scenario where the keeper loses functionality or behaves maliciously. The keeper is executing its role as intended. Instead, the root cause lies in a fundamental logical flaw in how cancelFlow operates. This design flaw can lead to unintended consequences, regardless of the keeper's trustability (hence why the low), making it a issue that needs to be addressed.