sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

feelereth - Doesn't validate account is actually undercollateralized before liquidating. Price could fluctuate between checks. #40

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

feelereth

high

Doesn't validate account is actually undercollateralized before liquidating. Price could fluctuate between checks.

Summary

Doesn't validate account is actually undercollateralized before liquidating. Price could fluctuate between checks. Should verify current collateralization.

Vulnerability Detail

The key lines are in the _liquidate function. It computes the liquidation fee and closable amount based on the latest price via _liquidationFee, but does not re-check the collateralization at the time of liquidation. This is problematic because: • The price could move between _liquidationFee and market.update • An account's collateralization ratio could go from under to overcollateralized • But its position would still get closed and charged a fee incorrectly

Impact

Improper liquidations if the price changes.

Code Snippet

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

Tool used

Manual Review

Recommendation

_liquidate should re-check collateralization at the time of liquidation. This verifies the account is still undercollateralized at the time of liquidation, preventing improper liquidations if the price changes.

sherlock-admin commented 1 year ago

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

panprog commented:

invalid because price can't change in the same transaction between these functions, and even if it could, market.update will simply revert

n33k commented:

invalid, not convincing without PoC

darkart commented:

Not clearly writen vulnerability

polarzero commented:

High. This could easily be manipulated repeatedly to drain the funds of the contract.