stent - Calc.boundedAdd used intitally but later regular subtraction used

[sherlock-admin]

stent

medium

Calc.boundedAdd used intitally but later regular subtraction used

Summary

In MarketDecrease order flow the funding amount per size calculation uses Calc.boundedAdd but further down the call path there is a calculation that does not use Calc.boundedSubtract but rather a regular -, which could cause overflow errors.

Vulnerability Detail

In MarketUtils.getNextFundingAmountPerSize the funding amount per size variables in the GetNextFundingAmountPerSizeResult variable are calculated using Calc.boundedAdd(a,b). If the inputs to the function are (type(int256).min+10,-12), for example, then the value returned will be type(int256).min i.e. it will not cause an arithmetic overflow, like it would if the values were just added normally with +.

Further down the call path of MarketDecrease in MarketUtils.getFundingFeeAmount the funding amount is negated, and negating type(int256).min results in an arithmetic overflow.

If the point of defending overflow of the funding amount per size variables was to allow execution to pass without failure then all calculations involving the funding amount per size variables should use Calc.boundedAdd or Calc.boundedSub.

Impact

MarketDecrease orders that would be expected to pass would fail, causing them to be cancelled.

Code Snippet

// contract & func: MarketUtils.getNextFundingAmountPerSize

            result.fundingAmountPerSize_LongCollateral_LongPosition = Calc.boundedAdd(
                result.fundingAmountPerSize_LongCollateral_LongPosition,
                cache.fps.fundingAmountPerSizeDelta_LongCollateral_LongPosition.toInt256()
            );

https://github.com/gmx-io/gmx-synthetics/blob/a2e331f6d0a3b59aaac5ead975b206840369a723/contracts/market/MarketUtils.sol#L1074

// contract: Calc

    function boundedAdd(int256 a, int256 b) internal pure returns (int256) {
        // if either a or b is zero or if the signs are different there should not be any overflows
        if (a == 0 || b == 0 || (a < 0 && b > 0) || (a > 0 && b < 0)) {
            return a + b;
        }

        // if adding `b` to `a` would result in a value less than the min int256 value
        // then return the min int256 value
        if (a < 0 && b <= type(int256).min - a) {
            return type(int256).min;
        }

        // if adding `b` to `a` would result in a value more than the max int256 value
        // then return the max int256 value
        if (a > 0 && b >= type(int256).max - a) {
            return type(int256).max;
        }

        return a + b;
    }

https://github.com/gmx-io/gmx-synthetics/blob/a2e331f6d0a3b59aaac5ead975b206840369a723/contracts/utils/Calc.sol#L90

// contract: MarketUtils

    function getFundingFeeAmount(
        int256 latestFundingAmountPerSize,
        int256 positionFundingAmountPerSize,
        uint256 positionSizeInUsd
    ) internal pure returns (int256) {
        int256 fundingDiffFactor = (latestFundingAmountPerSize - positionFundingAmountPerSize);

https://github.com/gmx-io/gmx-synthetics/blob/a2e331f6d0a3b59aaac5ead975b206840369a723/contracts/market/MarketUtils.sol#L1345

Tool used

Manual Review

Recommendation

Use Calc.boundedAdd & Calc.boundedSub on all calculations involving addition and subtraction with funding amount per size variables.

[IllIllI000]

will let sponsor review

[xvi10]

it seems the market could get bricked if reaching this state, despite attempts at avoiding that, the likelihood of this should be quite small

if the market reaches this state, the market may not function correctly, it may be preferable to update the funding fee factors to prevent this state instead of handling this in the contracts, to avoid the complexity of handling this case for this scope

the contracts could be updated in the future to properly handle this case