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

There is a scenario where flow does not get deleted, which leads to the protocol being stuck until owner manually sets the flow

0xl33

Description:

_noneFlow function checks if the flow is not none and if so, reverts. This function gets called at the start of these functions in PerpetualVault:

  • deposit

  • withdraw

  • run

  • claimCollateralRebates

This makes the flow very important, because if it is not deleted when it must be, it stops users and the keeper from interacting with the protocol.

Consider this scenario:

  1. A user calls PerpetualVault::deposit, where flow gets set to FLOW.DEPOSIT. At this time, position is open, so next action gets set to INCREASE_ACTION.

  2. The keeper calls PerpetualVault::runNextAction. At this time the position is 1x long, so _runSwap gets called:

/// ...
} else { // metadata.length == 1
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;
} else {
_doGmxSwap(data, isCollateralToIndex);
return false;
}
}
}

  1. Let's assume the _protocol variable, which was encoded in metadata (provided by keeper), is equal to PROTOCOL.DEX, which means that the swap occurs on ParaSwap. After this, _mint is called, because the current flow is still FLOW.DEPOSIT:

function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices) internal {
uint256 _shares;
if (totalShares == 0) {
_shares = depositInfo[depositId].amount * 1e8;
} else {
uint256 totalAmountBefore;
if (positionIsClosed == false && _isLongOneLeverage(beenLong)) {
totalAmountBefore = IERC20(indexToken).balanceOf(address(this)) - amount;
} else {
totalAmountBefore = _totalAmount(prices) - amount;
}
if (totalAmountBefore == 0) totalAmountBefore = 1;
_shares = amount * totalShares / totalAmountBefore;
}
​
depositInfo[depositId].shares = _shares;
totalShares = totalShares + _shares;
​
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[counter].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
​
emit Minted(depositId, depositInfo[depositId].owner, _shares, amount);
}

  1. Flow is not deleted anywhere in _mint function, which leaves it as FLOW.DEPOSIT and locks the main functions of the protocol until owner calls setVaultState function.

Additional note:

PerpetualVault::_cancelFlow cannot be called by the keeper to solve this situation, because safeTransfer will most likely revert, due to the contract not having enough collateral tokens, since they were swapped to index tokens, because current position is 1x long.

Impact:

Halted protocol until owner sets the flow. This can get ugly quick in a situation where nobody is monitoring the state of the system, because a signal could come to close the position or to switch position side, and keeper will not be able to do it, due to _noneFlow function causing reverts. Also during this time users will not be able to deposit or withdraw, so if the market is moving down rapidly, it will lead to losses that users cannot do anything about, and the keeper not being able to close the position or change side, until owner intervenes.

Likelihood:

This issue will occur commonly in vaults that have 1x leverage.

Proof of Concept:

  1. Add the test provided below to PerpetualVault.t.sol

  2. Run the test with this command: forge test --mt test_Flow_NotDeleted --via-ir --rpc-url <YOUR_RPC_URL_HERE> -vv

  3. Read the comments in the provided test to understand what's happening

Code
function test_Flow_NotDeleted() external {
// Alice deposits (position is closed)
​
address alice = makeAddr("alice");
deal(alice, 1 ether);
depositFixture(alice, 10e9);
​
// Setting up for opening 1x long
​
MarketPrices memory prices = mockData.getMarketPrices();
bytes memory paraSwapData = mockData.getParaSwapData(vault);
bytes[] memory swapData = new bytes[](1);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
​
// Keeper opens 1x long position using ParaSwap
​
address keeper = PerpetualVault(vault).keeper();
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
assertEq(uint8(PerpetualVault(vault).flow()), 0); // flow is 0
assertEq(PerpetualVault(vault).positionIsClosed(), false); // position is open
(PerpetualVault.NextActionSelector selector,) = PerpetualVault(vault).nextAction();
assertEq(uint8(selector), 0); // next action is 0
​
// Bob deposits
​
address bob = makeAddr("bob");
deal(bob, 1 ether);
depositFixture(bob, 10e9);
assertEq(uint8(PerpetualVault(vault).flow()), 1); // flow is `DEPOSIT`
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, swapData);
assertEq(uint8(PerpetualVault(vault).flow()), 1); // flow is still `DEPOSIT` !!!
assertEq(PerpetualVault(vault).positionIsClosed(), false); // position is open
(PerpetualVault.NextActionSelector selector2,) = PerpetualVault(vault).nextAction();
assertEq(uint8(selector2), 0); // next action is 0
​
// Setting up for withdrawal
​
uint256 executionFee = PerpetualVault(vault).getExecutionGasLimit(false);
uint256[] memory aliceDepositIds = PerpetualVault(vault).getUserDeposits(alice);
uint256 lockTime = 1;
PerpetualVault(vault).setLockTime(lockTime);
vm.warp(block.timestamp + lockTime + 1);
​
// Alice tries to withdraw, but can't, due to flow not being deleted
​
vm.prank(alice);
vm.expectRevert();
PerpetualVault(vault).withdraw{value: executionFee * tx.gasprice}(alice, aliceDepositIds[0]);
​
// Keeper tries to call `cancelFlow`, but it reverts, because collateral is swapped to index tokens
​
vm.prank(keeper);
vm.expectRevert();
PerpetualVault(vault).cancelFlow();
}

Recommended Mitigation:

Add a call to _finalize function with empty data after _mint, which will delete 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!