search

sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

feelereth - The current liquidation logic in _liquidate subtracts the full closable amount uniformly from each position side (long, short, maker). This means it could liquidate more from one side than necessary if only one side is insolvent. #37

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 11 months ago

feelereth

high

The current liquidation logic in _liquidate subtracts the full closable amount uniformly from each position side (long, short, maker). This means it could liquidate more from one side than necessary if only one side is insolvent.

Summary

It subtracts the full closable amount from each position side uniformly. However, longs and shorts may not be liquidated evenly. Can liquidate more from one side if that's the insolvent part.

Vulnerability Detail

  1. It calculates the full closable amount based on the account's latest position: Link 1
  2. Then it subtracts this uniform closable amount from each position side: This subtracts the full closable amount from the maker, long, and short positions: Link 2

This means if an account was long 10 and short 5, with a closable amount of 7 due to insolvency, it would liquidate 3 from the long side and 4 from the short side. But the account is solvent on the short side.

In Essence

It's possible that only the long or only the short position is actually insolvent. In that case, it would be better to only liquidate the insolvent side rather than evenly across both.

For example, say an account has:

Long: 100 DSU Short: 50 DSU Closable: 60 DSU And the short position is insolvent. Currently, it would liquidate:

Long: 100 - 60 = 40 DSU Short: 50 - 60 = 0 DSU When actually only the short should be liquidated:

Long: 100 DSU Short: 50 - 60 = 0 DSU This could lead to over-liquidation.

Impact

This can lead to over-liquidation.

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L231-L235 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L341

Tool used

Manual Review

Recommendation

_liquidate should calculate the insolvent amounts separately for longs and shorts based on the current price, and close each side proportional to its insolvency amount

sherlock-admin commented 11 months ago

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

panprog commented:

invalid because market positions are enforced to be single-sided (so only one of market/long/short can be non-0)

n33k commented:

invalid, not convincing without PoC

polarzero commented:

Medium. Downgrading to medium; however, even though it should not cause any loss of funds or malfunction, it appears to be a significan flaw in the system as this part represents one of the main aspects of the protocol.