sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

feelereth - the collateral checkpointing logic is vulnerable to miner/validator manipulation #87

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

feelereth

high

the collateral checkpointing logic is vulnerable to miner/validator manipulation

Summary

The collateral checkpointing logic in _checkpointCollateral() retroactively updates the collateral on the latest position based on fees/rewards incurred after that position snapshot.

Vulnerability Detail

The key issue is that _checkpointCollateral() retroactively modifies already committed state (the collateral level on the latest position). This allows a miner/validator to manipulate the state transition as follows:

  1. Miner includes a user transaction that incurrs a loss
  2. latestPosition snapshot occurs, saving the old collateral level
  3. Loss transaction processes, reducing the user's collateral
  4. Miner includes one or more transactions for the user that realize PnL gains
  5. Gains increase the user's context.local.collateral balance
  6. Miner includes liquidation transaction for the user
  7. _checkpointCollateral() executes as part of liquidation, modifying latestPosition.collateral to subtract out the gain from step 4
  8. This makes the user appear solvent again, preventing liquidation

Impact

This exploits the fact that miners/validators control transaction ordering and can manipulate state changes across transactions

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L371-L384

Tool used

Manual Review

Recommendation

• Remove the collateral adjustment logic completely • Only subtract fees/rewards up to the current block timestamp, not future pending positions

sherlock-admin commented 1 year ago

2 comment(s) were left on this issue during the judging contest.

141345 commented:

o

panprog commented:

invalid because scenario is not possible: user can't realize loss and profit in the same transaction: he's settled only once for each timestamp