Liquidity Management

Description

The PerpetualVault::_mint function has the following piece of code

(Note the A and B conditions in the above snippet)

A can be reached only when the following conditions are met:

1. positionIsClosed == false

2. beenLong == true

3. totalShares != 0

4. leverage is 1x (1x Vault only for A)

5. Index token balance of Vault is 0 (note that this and condition 3 are possible at the same time, will be shown in the poc )

B can be reached when the Vault is liquidated i.e it requires only steps 3,5 from above and collateral token balance of Vault should be 0 instead of Index and possible in any leverage.

Now, whenever this line is reached the totalShares value is inflated by an exponent of 1e8 or 1e18
and subsequent shares are minted based on the new exponent value

Letz say, If the totalShares value is 1e18 (if a user deposits 1e10 collateral initially) , now if A is reached with 1e18 amount (index token has 18 decimals) from this line _shares = amount * totalShares / totalAmountBefore we can say that it becomes 1e36 or if B is reached it becomes 1e24, Significantly inflating the totalShares value , now subsequent shares minted for all users will also be of this exponent. Now if these lines are reached multiple times i.e COMPOUNDING liquidation funds,
Setting Vault State Manually (4 of 5 conditions above can be reached easily by any user) etc . Then the totalShares Value can reach 1e77 which is type(uint256).max locking all the funds permanently as almost all major operations of the protocol are dependent on the totalShares Variable.

reference to a similar issue

Impact

If totalShares inflates to 1e77 it can lock all the funds permanently, many operations start to fail even before that as shares values are also inflated and are multiplied with token amounts i.e collateral amount (1e6 decimals) and Index amount (1e18 decimals)

There is also a very low probability case where if the owner or keeper is not careful when opening or closing positions or changing the vault's state manually then totalShares can be inflated with user funds still in the vault which typically allows the depositor to hijack the vault (they cannot withdraw as keeper won't pass that call, no other user can withdraw as total shares are inflated and their individual share values are not, Deposits will be locked by the keeper, leaving the vault in a hijacked state)

Proof of Concept

Poc Scenario for reaching "A":

1. Alice Deposits in 1x vault (leverage is 1x)

2. Keeper opens a Position and lock time passes

3. Keeper Closes Position (beenLong is true)

4. keeper Opens Position Again but "bob2" also sends a deposit at the same time (positionIsClosed is true) so keepers swap data does'nt include bob2's deposit (highly possible during high deposit times and keeper may include the missed deposits in another transaction) (positionIsClosed is false)

5. Alice calls withdraw before the keeper can include bob2's deposit
   (index token balance is 0 , totalShares !=0 bob2's share still exists but not swapped to index token yet)

(at this point all the conditions are met and any deposit will inflate the totalshares value)
6. Alice calls deposit again Inflating the totalShares value permanently

In this scenario
a.If alice calls withdraw again she can steal bob2's funds
b.The process can be reached multiple times inflating the totalShare value to 1e72 (4 times increasing by 18 decimals each time) , at 1e60 some operations start failing , further deposits from 1e72 will easily inflate the value to 1e77 and the vault's funds will be locked permanently.
c. If the keeper opens or closes position without proper swaps or it the owner sets vault state improperly then the totalShares will be inflated with user funds in it and the user who last deposits will hijack the vault

Note: Paste the Test Below in PerpetualVault.t.sol, run it with -vv option to view the logs

1. The Test below requires the PerpetualVaultTest::depositFixture testing helper function to include the following line , for withdrawal and consecutive deposit executions

and also add import {console} from "forge-std/console.sol"; in the test file

1. In PerpetualVault::withdraw make the following change

we do this as we set lockTime to 0 before withdrawal to simulate locktimepassed and also to make a successful gmxExecute within the same block (ease of use instead of generating new oracle params )

Tools Used

Manual Review

Recommendations

The recommended Mitigation for this issue is to not set totalAmountBefore to 1 but set it with a proper decimal prescision i.e 1e6 in case B and 1e18 in case A this will ensure that the totalShares value is not Inflated