Liquidity Management

Summary

PerpetualVaulthas logic to account for every different type of situation / deposit type, depending on the user action, the current state of the position, and the keeper inputs -> there are several different moving pieces and many different code paths that can be executed to fulfill a deposit.

However, when a user deposits when the strategy is currently open, 1x Long Leverage-> there is a situation where the state is not updated properly and left in a state that will not allow any further depositsor withdraws.

Vulnerability Details

Lets follow the exact path of a user depositing, when the protocol position is currently: openand 1x long leverage:

• User calls deposit -> flowis set to FLOW.DEPOSIT

  function deposit(uint256 amount) external nonReentrant payable {

  _noneFlow();

  if (depositPaused == true) {

  revert Error.Paused();

  }

  if (amount < minDepositAmount) {

  revert Error.InsufficientAmount();

  }

  if (totalDepositAmount + amount > maxDepositAmount) {

  revert Error.ExceedMaxDepositCap();

  }

  -> flow = FLOW.DEPOSIT;

  collateralToken.safeTransferFrom(msg.sender, address(this), amount);

  counter++;

  depositInfo[counter] = DepositInfo(amount, 0, msg.sender, 0, block.timestamp, address(0));

  totalDepositAmount += amount;

  EnumerableSet.add(userDeposits[msg.sender], counter);

  ​

  if (positionIsClosed) {

  MarketPrices memory prices;

  _mint(counter, amount, false, prices);

  _finalize(hex'');

  } 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);

becasue, NextActionSelector.INCREASE.ACTION-> the function runNextActionis called:

• Here, the function accurately deletes nextAction-> which sets NEXT_ACTION = NONE

• The last line of code of this function that is executed is _runSwap

• The important piece here is that the inputs are generated off-chain and the keeper calls this function and inputs those values. Of those values is metadata

• metadatacan include both swap parameters for dexSwapand gmxSwapor just 1 of them. Since it is 1x long leverage, there will only be a swap done

For this situation, the keeper only sends the dexSwapfor the metadata, meaning the entire swap will only take place on dexSwap-> which is the preferred method of the protocol. The inline documentation also reaffirms that there can be either both swap paths sent in or just 1 - the code itself also has logic to handle the situation when both gmxSwapand dexSwapare sent in, or just 1 of them.

_runSwapis executed, passing in ONLY dexSwapfor the metadata

As can be seen in the function above, swapProgressDatais used (and is also never deleted / reset)

The FINAL line of code that is executed in this function under this scenario, is _mint-> becasue the current flowis still FLOW.DEPOSIT

The inline documentation before calling _mintalso shows that it is expected that the global state will be updated (which it isnt)

** THE FINAL AND END OF THIS CODE PATH -> _mint

all of the inline documentation is original, i did not modify or edit it at all. As can be seen, this is the end of deposit flow

At the end of this, the user is accounted for their shares and the totalShares is updated to include the new shares, the user also gets refunded any excess fee (becasue they did not go through GMX for the swap)

BUT THAT IS IT, THATS THE END OF THE PATH. Global state variables left populated and never deleted include:

• flow-> it is still FLOW.DEPOSIT

• swapProgressDatais never deleted

Impact

Users will not be able to depositor withdrawbecasue flowis still FLOW.DEPOSIT

They wont be able to, becasue, at the beginning of the depositand withdrawfunctions, there is this check to ensure there is no active flow:

Also, the documentation in the contest page says that a key invariant-> is that swapProgressDatashould always be empty after an action is completed.

Tools Used

Manual Review

Recommendations

All of the other scenarios handle updating the state changes after completing an action by either calling handleReturnor updateState-> and updateStatehas logic that is specific to handle this scenario's code path , but it is never called!

nextActionwas deleted within the code path, therefore it is NextActionSelector.NO_ACTION-> which is exactly what this function handles. IF called, this will update the state PERFECTLY.

• delete flow

• delete swapProgressData

Which are the EXACT variables that need to be updated and are left populated.

Call updateStateafter the _mintand update the state properly.