Liquidity Management

Description

Users can call PerpetualVault::deposit function when the Position is closed to directly deposit their collateral Tokens and receive shares but during a bull run or a similar scenario where the number of deposits is high there is a definite possibility that when keeper opens a position a users deposit transaction also goes in at the same time but just before keepers call i.e, The keeper's swapdata doesn't include the deposit of the user.

Now if any other user calls withdraw (considering that they have been holding their collateral way past the lock time) before the keepers run call then they can steal a good chunk of the other users collateral tokens.

The Deposits which require the keeper to call run again are essentially simulated as profits in the contract and everyone gets a share in them if they withdraw before the keeper calls run.

Impact

Users can lose a good portion of their collateral if they deposit when a position is being opened this can happen without any malicious intents as well

Proof of Concept

Attack Scenario:

1. Alice Deposits 2e10 collateral tokens

2. Keeper Opens a Position

3. Keeper Closes the position after a few days (Lock time of alice passed by this time)

4. keeper decides to open a position again

5. Alice deposits 2e10 collateral tokens are in the vault
   5.5 Bob calls deposit with 1e10 collateral tokens at same time as step 6

6. Keeper calls run to open a position (with 2e10 as swapdata as bob's deposit isn't present at the time of generating swapdata)

7. Position is opened with 2e10 collateral tokens swapped as Index and 1e10 collateral is in the vault

8. Now Alice calls withdraw and keeper calls withdraw with swapdata index token balance of vault i.e (since alice is the only depositor in long all Index Tokens beling to her)

9. Alice gets a withdrawal of 2.65e10 collateral tokens

10. Bob is left with 0.33 e10 collateral tokens

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 a oracle params )

Tools Used

Manual Review

Recommendations

A simple recommended mitigation for this scenario is to add a

statement in the PerpetualVault::run function

Declare a new variable uint256 PausedBlockNumber
and in PerpetualVault::setDepositPaused function to set PausedBlockNumber to the current L2 block number if used in arbitrum and L1 block number if used in avalanche

and adding another check in PerpetualVault::run to check if the current block number (for arbitrum this will be L2 block number , refer to the documentation link provided above) is greater than PausedBlockNumber

This will ensure that no deposits will be accepted before opening a position within the same to the time when the keepers run transaction is executed.