0xAmanda - Incorrect function call leads to stale borrowing fees

[sherlock-admin]

0xAmanda

high

Incorrect function call leads to stale borrowing fees

Summary

Due to an incorrect function call while getting the total borrow fees, the returned fees will be an inaccurate and stale amount. Which will have an impact on liquidity providers

Vulnerability Detail

As said the function getTotalBorrowingFees:

function getTotalBorrowingFees(DataStore dataStore, address market, address longToken, address shortToken, bool isLong) internal view returns (uint256) {
    uint256 openInterest = getOpenInterest(dataStore, market, longToken, shortToken, isLong);
    uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market, isLong);
    uint256 totalBorrowing = getTotalBorrowing(dataStore, market, isLong);
    return openInterest * cumulativeBorrowingFactor - totalBorrowing;
}

calculates the fess by calling getCumulativeBorrowingFactor(...):

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1890

which is the wrong function to call because it returns a stale borrowing factor. To get the actual borrowing factor and calculate correctly the borrowing fees, GMX should call the getNextCumulativeBorrowingFactor function:

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1826

Which makes the right calculation, taking into account the stale fees also:

    uint256 durationInSeconds = getSecondsSinceCumulativeBorrowingFactorUpdated(dataStore, market.marketToken, isLong);
    uint256 borrowingFactorPerSecond = getBorrowingFactorPerSecond(
        dataStore,
        market,
        prices,
        isLong
    );

    uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market.marketToken, isLong);

    uint256 delta = durationInSeconds * borrowingFactorPerSecond;
    uint256 nextCumulativeBorrowingFactor = cumulativeBorrowingFactor + delta;
    return (nextCumulativeBorrowingFactor, delta);

Impact

Ass fee calculation will not be accurate, liquidity providers will be have a less-worth token because pending fees are not accounted in the pool's value

Code Snippet

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1888

Tool used

Manual Review

Recommendation

In order to mitigate the issue, call the function getNextCumulativeBorrowingFactor instead of the function getCumulativeBorrowingFactor() for a correct accounting and not getting stale fees

[IllIllI000]

@xvi10 can you confirm that this is invalid?

[xvi10]

sorry thought i marked this, it is a valid issue

[IllIllI000]

@xvi10 can you elaborate on the conditions where this is a valid issue? Don’t most code paths call updateFundingAndBorrowingState() prior to fetching fees, so doesn’t that mean the value won’t be stale? I flagged the case where it’s missing in https://github.com/sherlock-audit/2023-02-gmx-judging/issues/158 . Are you saying that this is a separate issue, or that these should be combined, or something else?

[xvi10]

it affects the valuation of the market tokens for deposits / withdrawals

[IllIllI000]

@hrishibhat agree with sponsor – valid High

[0xffff11]

Escalate for 10 USDC

It is a valid solo high. Sponsor confirmed the issue and severity.

Re-explanation for better understanding:

The function:

 function getTotalBorrowingFees(DataStore dataStore, address market, address longToken, address shortToken, bool isLong) 
internal view returns (uint256) {
 uint256 openInterest = getOpenInterest(dataStore, market, longToken, shortToken, isLong);
  uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market, isLong);
 uint256 totalBorrowing = getTotalBorrowing(dataStore, market, isLong);
  return openInterest * cumulativeBorrowingFactor - totalBorrowing;
 }

is made to return the borrowingFees that are pending at that moment. The issue is that inside the getTotalBorrowingFees function, they are calling the wrong function to get the cumulativeBorrowingFactor.

They are calling the function cumulativeBorrowingFactor instead of getNextCumulativeBorrowingFactor. , that is the one that should be called to get the updated borrowing fees. Calling the cumulativeBorrowingFactor returns the outdated fees.

These outdate in fees can cause a missmatch in pricing specially while withdrawing, as sponsor confirmed.

To sum up: Issue is completely valid, sponsor confirmed and enough issue has been given. Please make it a valid solo high, thanks IIIIIII!

[sherlock-admin]

Escalate for 10 USDC

It is a valid solo high. Sponsor confirmed the issue and severity.

Re-explanation for better understanding:

The function:

 function getTotalBorrowingFees(DataStore dataStore, address market, address longToken, address shortToken, bool isLong) 
internal view returns (uint256) {
 uint256 openInterest = getOpenInterest(dataStore, market, longToken, shortToken, isLong);
  uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market, isLong);
 uint256 totalBorrowing = getTotalBorrowing(dataStore, market, isLong);
  return openInterest * cumulativeBorrowingFactor - totalBorrowing;
 }

is made to return the borrowingFees that are pending at that moment. The issue is that inside the getTotalBorrowingFees function, they are calling the wrong function to get the cumulativeBorrowingFactor.

They are calling the function cumulativeBorrowingFactor instead of getNextCumulativeBorrowingFactor. , that is the one that should be called to get the updated borrowing fees. Calling the cumulativeBorrowingFactor returns the outdated fees.

These outdate in fees can cause a missmatch in pricing specially while withdrawing, as sponsor confirmed.

To sum up: Issue is completely valid, sponsor confirmed and enough issue has been given. Please make it a valid solo high, thanks IIIIIII!

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

I messed up the tagging - Valid solo High

[hrishibhat]

Escalation accepted

Considering this issue a valid high

[sherlock-admin]

Escalation accepted

Considering this issue a valid high

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[xvi10]

Fix in https://github.com/gmx-io/gmx-synthetics/pull/89/files#diff-f764a51a3af5d3117eeeff219951425f3ecc8cd15b742bb044afc7c73b03f4aa