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

Flow state is not cleared for vault with active position if dex swap is used for deposits blocking user interaction until resolved by keeper.

den_dokka

Summary

The vault keeps track of the current flow via the flow state variable. Due to the PerpetualVault:_noneFlow:L1258-1262 function, deposit, withdraw, and run functions cannot be called while flow is not NONE (0).
The flow is set to none after a current flow is finalized, and this can happen in different ways depending on the vault and which flow is the current one. For example, a PerpetualVault:deposit call will run _finalize if the positionIsClosed variable is true, meaning the vault has no active position. PerpetualVault:_finalize:L1208-1218 will reset the flow for a deposit like this:

delete swapProgressData;
delete flowData;
delete flow;

However if the vault uses DEX swaps only for a deposit, the flow will not be reset to none. And there is no next action to run for the keeper, resulting in the _noneFlow function limiting the actions that can be performed on the vault by users and keepers, until the keeper can clear the flow via setting the state manually using either of the functions: PerpetualVault:cancelFlow:L1220-1242 or PerpetualVault:setVaultState:L681-697

Vulnerability Details

The flow is cleared in these functions:

  • PerpetualVault:afterOrderCancellation:L592-623 which is a callback function for gmx positions so not relevant for deposits using dex only.

  • PerpetualVault:_handleReturn:L1129-1156 which is only called in withdrawal cases (flow == FLOW.WITHDRAW) so not relevant either.

  • PerpetualVault:_finalize:L1208-1218 which are only called from deposit if there is no current position. Or runNextAction if the next action is FINALIZE, and the afterOrderExecution which again is a gmx callback function.

  • PerpetualVault:_updateState:L1186-1206 which is called from afterOrderExecution function which again is a gmx callback. It is also done in the _runSwap function but only if the flow is not withdraw or deposit.

//PerpetualVault:_runSwap:L999
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;
}
...

The _runSwap function which is referenced above, will be called (in the case of a deposit in a vault with an active position) by the runNextAction function. The withdraw flow will be cleared as it calls, _handleReturn, but the deposit flow will just call _mint which does not clear the flow. As an example we can see how in the PerpetualVault:deposit:L215-243 function, in the case there is no active position, and we can complete the deposit without any next action, the call to _mint is followed by calling _finalize to clear the flow.

//PerpetualVault:deposit:L233-237
if (positionIsClosed) {
MarketPrices memory prices;
@> _mint(counter, amount, false, prices);
@> _finalize(hex"");
} else {

As the _runSwap is a result of the keeper calling the runNextAction and the execution ends with _mint without any clearing of flow state/finalizing, or setting any nextAction for keeper to call again to finalize, the vault will be stuck in the deposit flow with no next action for keeper to run.

POC

Add this in test/PerpetualVault.t.sol

function test_Deposit_Into_Paraswap_Does_Not_Finalize() external {
uint256 collateralTokensUsed = 1e10;
address keeper = PerpetualVault(vault).keeper();
​
//Bob deposit
address bob = makeAddr("bob");
payable(bob).transfer(1 ether);
depositFixture(bob, collateralTokensUsed);
​
// open 1x long position using paraswap
MarketPrices memory prices = mockData.getMarketPrices();
bytes memory paraSwapData = mockData.getParaSwapData(vault);
bytes[] memory swapData = new bytes[](1);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
​
//alice deposit
address alice = makeAddr("alice");
payable(alice).transfer(1 ether);
depositFixture(alice, collateralTokensUsed);
​
//keeper runs swap
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
​
//No next action
(PerpetualVault.NextActionSelector selector, ) = PerpetualVault(vault)
.nextAction();
assertEq(uint8(selector), 0);
​
//flow should be None
uint8 flow = uint8(PerpetualVault(vault).flow());
assertEq(flow, 0);
}
​

Impact

The likelihood of this is high as every deposit into a 1x vault where the keeper will use only DEX swaps (ParaSwap) will result in a flow not being reset.
The impact is medium as this needs to be manually resolved by either calling the cancelFlow function to reset it, but this will cancel the deposit and the depositor will need to try again, resulting in the same outcome if the keeper does not decide to use GMX in addition. The other way to handle this is to call setVaultState, but doing this every time this happens can be error-prone as so much of the state for the vault is set using this single call. Also, taking into account the time it can take the keeper to notice this, this issue will cause a significant disruption to the functioning of the vaults where only DEX swaps are used frequently.

Tools Used

Manual review

Recommendations

Call finalize after mint in the _runSwap function to finalize the flow.

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);
+ _finalize(hex"");
}...
Updates

Lead Judging Commences

n0kto Lead Judge 9 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.

Give us feedback!