search

sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

feelereth - Basing fees on the current pending position magnitude instead of the amount being closed can lead to incorrect fee calculations #38

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

feelereth

high

Basing fees on the current pending position magnitude instead of the amount being closed can lead to incorrect fee calculations

Summary

This code does have an issue in how it calculates fees based on the current pending position magnitude rather than the amount being closed.

Vulnerability Detail

This code does have an issue in how it calculates fees based on the current pending position magnitude rather than the amount being closed. The key parts are:

  1. In _liquidate, it calls _liquidationFee to get the liquidation fee.
  2. _liquidationFee calls _latest to get the latest pending position and price.
  3. _latest loops through pending positions, updating latestPosition and closableAmount.
  4. It returns latestPosition.magnitude() for closableAmount.
  5. _liquidationFee uses closableAmount as the amount when calculating the liquidation fee. The problem is that closableAmount is the full magnitude of the latest pending position, not the amount being closed. If the position was previously larger but is now smaller, the fee will be larger than it should be. For example: • Current position is 100 DSU • New pending position is 50 DSU • Only 50 DSU is being closed • But closableAmount will be 100 • So fee is calculated on 100 instead of actual 50 being closed

Impact

Users could pay excess fees if their position reduces in size.

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L229 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L344-L345 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L349-L350

Tool used

Manual Review

Recommendation

_latest should track the actual delta of DSU being closed, not just the latest magnitude. This accurately tracks the amount being closed rather than just using the latest magnitude.

sherlock-admin commented 11 months ago

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

panprog commented:

invalid because _latest returns both latestPosition and closableAmount, and closableAmount is not latestPosition.magnitude

n33k commented:

invalid, not convincing without PoC

polarzero commented:

Medium. Exactly the same as above.