Liquidity Management

Description

The natspec comments over PerpetualVault::afterLiquidationExecution

and the following line if code in PerpetualVault::_mint

prove that the PerpetualVault.sol is designed to be used even after liquidations occur but the contract itself doesn't handle liquidations properly which breaks several key invariants

1. When ever Liquidation occurs and a new user deposits , it is expected that all the existing shares are transferred to the new depositor

but it is not the case i.e in PerpetualVault::afterLiquidationExecution after Liquidation occurs the totalShares value is not reset , so if a new user deposits after the liquidation:

he receives ---> A = _shares = amount * totalShares / totalAmountBefore

now the totalshares is updated to ------> totalShares = totalShares + _shares ---> B + A

here B amount of shares are unaccounted for i.e they have a hold over the funds in the contract siphoning off the profits and breaking the key invariant Total Shares Consistency

1. After Liquidation occurs the depositInfo , userDeposits (Enumerable set mapping) are not reset preserving the depositInfo.amount,depositInfo.shares values of the users who got liquidated , Now when new users start depositing after liquidation and the new users start to gain huge profits these old users can call withdraw and take a share from their profits since their depositInfo is still preserved breaking the key invariant Depositor Share Value Preservation

Impact

Users can even withdraw after liquidation and as portion of totalshares is still preserved for the Liquidated users siphoning present positions profits

Proof of Concept

Attack Scenario:

1. User A deposits in 3x Leverage Vault for Max Profits

2. Market moves Against him and position gets liquidated

3. afterLiquidationExecution is called and deposits are paused

4. Vault deposits are opened again when the keeper is ready or when the market recovers

5. Since, the market is in bull run now many new users deposit in the 3x vault

6. The vault receives Huge profits

7. Now user A calls withdraw and gets a share of the new profits even though he got liquidated before

Note that in step 4 User A can deposit again gaining even greater profits and if his position gets liquidated again then he can still wait to recover those loses when profits appear in the vault.

It is also to be noted that totalShares and shares variables in the contract are only decreased in the _burn function preserving the userdata until they withdraw

Tools Used

Manual Review

Recommendations

A single user may deposit minimum amount to open a positionwith 3x leverage and get easily liquidated so even though the contract is upgradeable , it would be infeasible to change implementation for every liquidation especially when the position size is small

Consider a design change to handle liquidation in a better way i.e to declare a new secondCounter variable which iterates in a loop from 1 to depositId and clears depositInfo , userDeposits of every user upto present depositId after liquidation and set totalShares to 0 , make sure not to hit block gaslimits while iterating , making this a seperate function will be much better then after liquidation while deposits are paused the keeper can clear previous deposits and get ready for new deposits

Pseudo code for the above fix:

Note: A bool which acts as a flag can also be added to which is set to true in afterLiquidationExecution then required to be true to call the above function and set to false when secondCounter == counter ensuring that the above function can only be called during the liquidation and also ensuring that the contract is ready for new deposits.