search

sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

WATCHPUG - `_accumulateFunding()` maker will get the wrong amount of funding fee. #139

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

WATCHPUG

high

_accumulateFunding() maker will get the wrong amount of funding fee.

Summary

Vulnerability Detail

The formula that calculates the amount of funding in Version#_accumulateFunding() on the maker side is incorrect. This leads to an incorrect distribution of funding between the minor and the maker's side.

// Redirect net portion of minor's side to maker
if (fromPosition.long.gt(fromPosition.short)) {
    fundingValues.fundingMaker = fundingValues.fundingShort.mul(Fixed6Lib.from(fromPosition.skew().abs()));
    fundingValues.fundingShort = fundingValues.fundingShort.sub(fundingValues.fundingMaker);
}
if (fromPosition.short.gt(fromPosition.long)) {
    fundingValues.fundingMaker = fundingValues.fundingLong.mul(Fixed6Lib.from(fromPosition.skew().abs()));
    fundingValues.fundingLong = fundingValues.fundingLong.sub(fundingValues.fundingMaker);
}

PoC

Given:

Then:

  1. skew(): 999/1000
  2. fundingMaker: 0.999 of the funding
  3. fundingShort: 0.001 of the funding

While the maker only matches for 1 of the major part and contributes to half of the total short side, it takes the entire funding.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/types/Version.sol#L207-L215

Tool used

Manual Review

Recommendation

The correct formula to calculate the amount of funding belonging to the maker side should be:

fundingMakerRatio = min(maker, major - minor) / min(major, minor + maker)
fundingMaker = fundingMakerRatio * fundingMinor
sherlock-admin commented 1 year ago

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

141345 commented:

m

panprog commented:

medium because incorrect result only starts appearing if abs(long-short) > maker and the larger the difference, the more incorrect the split of funding is. But this situation is exceptional case, most of the time abs(long-short) < maker due to efficiency and liquidity limits

arjun-io commented 12 months ago

We'd like to re-open this as it does appear to be a valid issue. Medium severity seems correct here

arjun-io commented 12 months ago

Fixed: https://github.com/equilibria-xyz/perennial-v2/pull/64

MLON33 commented 11 months ago

From WatchPug,

Fixed alongside with #180.