Liquidity Management

Summary

The contract code uses a global counter to assign unique identifiers to deposits. Each deposit is assigned a unique ID by incrementing the counter and storing the deposit information at that index. Later, when processing an order execution within the _mint function, the code references the deposit using the global counter.

This design implies that the refund mechanism directly accesses the deposit data corresponding to the current counter value. The Issue arises from the fact that if another deposit is made between the initial deposit and the subsequent execution of its associated order, the global counter would have incremented. This means that the deposit information being referenced depositInfo[counter] might now point to a newer deposit, not the one intended for processing.

Vulnerability Details

In the _mint function, when refunding the fee, it uses depositInfo[counter].executionFee. But counter is the current deposit ID. However, during the minting, which happens in afterOrderExecution for a deposit, the counter may have already been incremented again if another deposit was made. The code in _mint uses the correct depositId (counter is the current ID, but the deposit being processed is the one with the ID stored in counter at deposit time). In deposit function:

So when a deposit is made, counter is incremented, and the deposit is stored with the new counter value. Then, during afterOrderExecution, for flow == DEPOSIT, it uses depositInfo[counter]. If another deposit has been made since then, the counter would have been incremented again. So in this case, depositInfo[counter] would refer to the new deposit, not the one being processed.

Although the documentation emphasizes that the protocol processes one action at a time, the code itself does not include explicit on-chain enforcement measures to guarantee that deposits and executions do not interleave.

Impact

When the contract references the deposit data using an outdated or incorrect counter value, the wrong execution fee could be refunded, leading to financial discrepancies and loss of funds for the affected user.

POC

Heres a Scenario;

1. User A deposits, counter becomes 1. depositInfo[1] is created.

2. The keeper processes the deposit and triggers an increase position.

3. While that's pending, User B deposits, counter becomes 2. depositInfo[2] is created.

4. When the first deposit's afterOrderExecution is called, flow is DEPOSIT. The code uses counter (which is 2) to access depositInfo[2], but that's User B's deposit. This would cause User B's deposit to be minted with User A's amount, leading to incorrect share allocation and possible loss of funds.

Tools Used

Manual

Recommendations

Track the specific depositId in flowData during the deposit process. Modify the PerpetualVault::deposit to set flowData = counter after incrementing, then use flowData in PerpetualVault::afterOrderExecution, It ensures that the correct depositId is tracked during the deposit flow: