search

sherlock-audit / 2023-02-gmx-judging

17 stars 11 forks source link

joestakey - Incorrect funding amount due to precision loss in `getNextFundingAmountPerSize()` for markets with low open interest #227

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

joestakey

medium

Incorrect funding amount due to precision loss in getNextFundingAmountPerSize() for markets with low open interest

Summary

Rounding happens in getNextFundingAmountPerSize() due to division before mulitplication, leading to a loss of precision impacting the funding amount.

Vulnerability Detail

cache.fundingUsd is computed by first dividing cache.sizeOfLargerSide by Precision.FLOAT_PRECISION == 1e30. The issue is that cache.sizeOfLargerSide also has 30 decimals, meaning that if the open interests are low enough, the rounding will be significant. For instance: cache.oi.longOpenInterest = 1.99999e30 -> cache.sizeOfLargerSide / Precision.FLOAT_PRECISION = 1.

File: contracts/market/MarketUtils.sol
941:     cache.sizeOfLargerSide = cache.oi.longOpenInterest > cache.oi.shortOpenInterest ? cache.oi.longOpenInterest : cache.oi.shortOpenInterest;
942:         result.fundingFactorPerSecond = getFundingFactorPerSecond(
943:             dataStore,
944:             market.marketToken,
945:             cache.diffUsd,
946:             cache.totalOpenInterest
947:         );
948:         cache.fundingUsd = (cache.sizeOfLargerSide / Precision.FLOAT_PRECISION) * cache.durationInSeconds * result.fundingFactorPerSecond;//@audit precision loss

Impact

Precision loss results in a lower funding amount, which means the position fees computed in PositionPricingUtils.getPositionFees() will be incorrect.

Code Snippet

https://github.com/gmx-io/gmx-synthetics/blob/7be3ef2d119d9e84473e1a49f346bcdc06fd57a3/contracts/market/MarketUtils.sol#L941-L948

Tool used

Manual Review

Recommendation

Refactor so that division happens after multiplication. Overflow could technically occur for very large open interests, but this is expected as per the comments in getTotalBorrowing().

+948:         cache.fundingUsd = cache.sizeOfLargerSide * cache.durationInSeconds * result.fundingFactorPerSecond  / Precision.FLOAT_PRECISION;
-948:         cache.fundingUsd = (cache.sizeOfLargerSide / Precision.FLOAT_PRECISION) * cache.durationInSeconds * result.fundingFactorPerSecond;

Duplicate of #113