Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Under A Case The Flow Is Not Finalized Breaking An Invariant

sakshamseth5

Summary

After the entire flow is complete it needs to be finalized i.e. flow , flowData and swappedData needs to be cleared out ( as mentioned in the invariant section in readme After all actions completed, nextAction, swapProgressData should be empty.) , but under a certain case this is violated and the flow is not emptied out.

Vulnerability Details

Consider the following ->

1.) There's an active perp vault position open on GMX and it is 1x Long.

2.) A user deposits by calling deposit() ->

[https://github.com/CodeHawks-Contests/2025-02-gamma/blob/main/contracts/PerpetualVault.sol#L215]

the flow is assigned as DEPOSIT at L226 ,

and since positionIsClosed is false (as mentioned there is a position open on GMX) next action would be INCREASE_POSITION ->

} else {
_payExecutionFee(counter, true);
// mint share token in the NextAction to involve off-chain price data and improve security
nextAction.selector = NextActionSelector.INCREASE_ACTION;
nextAction.data = abi.encode(beenLong);
}

3.) Keeper would call runNextAction() ->

and since next action is INCREASE_ACTION this branch would be executed (remember the position was 1x long)->

if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
​
if (_isLongOneLeverage(_isLong)) {
_runSwap(metadata, true, prices);
}

4.) Therefore , _runSwap() is invoked (L976) and assuming that metadata length == 1 (which was passed in runNextAction) we enter this branch ->

else {
if (metadata.length != 1) {
revert Error.InvalidData();
}
(PROTOCOL _protocol, bytes memory data) = abi.decode(metadata[0], (PROTOCOL, bytes));
if (_protocol == PROTOCOL.DEX) {
uint256 outputAmount = _doDexSwap(data, isCollateralToIndex);
// update global state
if (flow == FLOW.DEPOSIT) {
// last `depositId` equals with `counter` because another deposit is not allowed before previous deposit is completely processed
_mint(counter, outputAmount + swapProgressData.swapped, true, prices);
} else if (flow == FLOW.WITHDRAW) {
_handleReturn(outputAmount + swapProgressData.swapped, false, true);
} else {
// in the flow of SIGNAL_CHANGE, if `isCollateralToIndex` is true, it is opening position, or closing position
_updateState(!isCollateralToIndex, isCollateralToIndex);
}
return true;
}

5.) _doDexSwap() is performed , and since flow is DEPOSIT we would enter the if (flow == FLOW.DEPOSIT) { branch and perform _mint of the swapped amount and shares are assigned to the user , and then we return.

6.) But , we did not clear out the flow and the nextAction , meaning we should have called _finalize(hex'') after the mint operation to correctly finalize the flow , hence breaking the invariant. Now no further deposits/withdraw or run() functions can take place due to the _noneFlow() check.

There can be another case with the same issue , where a GMX swap requested after a swap on DEX which would update swappedData.swapped and assign swappedData.remaining but that order failed and on retry the remaining swap would be done by DEX and then similar issue as above where _mint would be called but swappedData would not be cleared meaning that for the next swap swappedData.swapped would be way higher (L1005) and the user would receive way higher shares.

Impact

We did not clear out the flow and the nextAction (it would still point to INCREASE_ACTION) , meaning we should have called _finalize(hex'') after the mint operation to correctly finalize the flow , hence breaking the invariant.

Tools Used

Manual analysis

Recommendations

We should have called _finalize(hex'') after the mint operation to correctly finalize the flow.

Updates

Lead Judging Commences

n0kto Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_deposit_1x_long_dex_positionIsOpened_DoS_Flow

Likelihood: Medium/High, - Leverage = 1x - beenLong = True - positionIsClosed = False - Metadata → 1 length and Dex Swap Impact: Medium/High, DoS on any new action before the admin uses setVaultState Since this seems to be the most probable path for a 1x PerpVault, this one deserves a High.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.