chaduke - MarketUtils.getFundingAmountPerSizeDelta() has a rounding logical error.

[sherlock-admin]

chaduke

medium

MarketUtils.getFundingAmountPerSizeDelta() has a rounding logical error.

Summary

MarketUtils.getFundingAmountPerSizeDelta() has a rounding logical error. The main problem is the divisor always use a roundupDivision regardless of the input roundUp rounding mode. Actually the correct use should be: the divisor should use the opposite of roundup to achieve the same logic of rounding.

Vulnerability Detail

MarketUtils.getFundingAmountPerSizeDelta() is used to calculate the FundingAmountPerSizeDelta with a roundup input mode parameter.

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1126-L1136

This function is used for example by the IncreaseLimit order via flow OrderHandler.executeOrder() -> _executeOrder() -> OrderUtils.executeOrder() -> processOrder() -> IncreaseOrderUtils.processOrder() -> IncreasePositionUtils.increasePosition() -> PositionUtils.updateFundingAndBorrowingState() -> MarketUtils.updateFundingAmoutPerSize() -> getFundingAmountPerSizeDelta().

However, the main problem is the divisor always use a roundupDivision regardless of the input roundUp rounding mode. Actually the correct use should be: the divisor should use the opposite of roundup to achieve the same logic of rounding.

My POC code confirms my finding: given fundingAmount = 2e15, openInterest = 1e15+1, and roundup = true, the correct answer should be: 1999999999999998000000000000001999999999999999. However, the implementation returns the wrong solution of : 1000000000000000000000000000000000000000000000. The reason is that the divisor uses a roundup and gets a divisor of 2, as a result, the final result is actually rounded down rather than rounding up!

function testGetFundingAmountPerSizeDelta() public{
        uint result = MarketUtils.getFundingAmountPerSizeDelta(2e15, 1e15+1, true);
        console2.log("result: %d", result);
        uint256 correctResult = 2e15 * 1e15 * 1e30 + 1e15; // this is a real round up
        correctResult = correctResult/(1e15+1);
        console2.log("correctResult: %d", correctResult);
        assertTrue(result  == 1e15 * 1e30);
    }

Impact

MarketUtils.getFundingAmountPerSizeDelta() has a rounding logical error, sometimes, when roundup = true, the result, instead of rounding up, it becomes a rounding down!

Code Snippet

Tool used

VScode

Manual Review

Recommendation

Change the rounding mode of the divisor to the opposite of the input roundup mode. Or, the solution can be just as follows:

function getFundingAmountPerSizeDelta(
        uint256 fundingAmount,
        uint256 openInterest,
        bool roundUp
    ) internal pure returns (uint256) {
        if (fundingAmount == 0 || openInterest == 0) { return 0; }
     
       

        // how many units in openInterest
-        uint256 divisor = Calc.roundUpDivision(openInterest, Precision.FLOAT_PRECISION_SQRT);

-        return Precision.toFactor(fundingAmount, divisor, roundUp);
+       return Precision.toFactor(fundingAmount*Precision.FLOAT_PRECISION_SQRT, openInterest, roundUp
    }

[xvi10]

would classify this as a low since the impact should be very small

[IllIllI000]

a fix was done for this one, so I don't think this can be low: gmx-io/gmx-synthetics@f8cdb4d

[xvi10]

we will fix valid low issues if it can improve the accuracy of calculations, even if the issue may not lead to a significant difference

was referencing this for the criteria, "Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future."

the rounding issues need to lead to a material impact to be considered for a medium?

[IllIllI000]

won't this lead to funding miscalculations for every second, for every user, and become larger as the funding amounts grow?

[xvi10]

that's true, have updated the issue to confirmed

[IllIllI000]

@xvi10 Also, if the precision loss is enough to make you want to add a fix, I would say it should be medium - let me know if you intend to fix any of the other precision loss submissions

[xvi10]

not sure i agree with that since it doesn't seem to match the criteria in https://docs.sherlock.xyz/audits/judging/judging

but will notify and let Sherlock decide

the MarketUtils.getFundingFeeAmount was also updated, not due to precision but to avoid the risk of overflow, it was mentioned as a precision issue in:
#193

[hrishibhat]

Considering this issue as valid medium based on the following comment which the sponsor agrees too.

won't this lead to funding miscalculations for every second, for every user, and become larger as the funding amounts grow?

[xvi10]

fixed in gmx-io/gmx-synthetics@f8cdb4d