GMX V1 BOUNTY(1)

Stale cumulativeFundingRate is being used when fetching validateLiquidation externally which allows unhealthy positions to appear healthy

This issue is part of a different set of vulnerabilities that 0xKato and I (0xWeiss) found during the review of Lexer Markets V2, which was forking GMX V1.

If you are a protocol owner or someone integrating with GMX V1, GMX V2, or LEXER Markets, reach out to my DMs on Twitter: 0xWeiss or email: mweiss.eth@gmail.com . We will make sure your code is as secure as possible.

REPORT:
The issue was present in the Vault contract.

Any system relying on the external validateLiquidation function to determine if their positions or users' positions are liquidatable can receive an incorrect response:

function validateLiquidation(address _account, address _collateralToken, address _indexToken, bool _isLong, bool _raise) public view override returns (uint256, uint256) {
        Position memory position = getPosition(_account, _collateralToken, _indexToken, _isLong);
        ISVault _vault = vault;
        (bool hasProfit, uint256 delta) = _vault.getDelta(_indexToken, position.size, position.averagePrice, _isLong, position.lastIncreasedTime, position.reserveAmount);
        uint256 marginFees = getFundingFee(_account, _collateralToken, _indexToken, _isLong, position.size, position.entryFundingRate);
        marginFees = marginFees.add(getPositionFee(_account, _collateralToken, _indexToken, _isLong, position.size));

        if (!hasProfit && position.collateral < delta) {
            if (_raise) { revert("Vault: losses exceed collateral"); }
            return (1, marginFees);
        }

        uint256 remainingCollateral = position.collateral;
        if (!hasProfit) {
            remainingCollateral = position.collateral.sub(delta);
        }

        if (remainingCollateral < marginFees) {
            if (_raise) { revert("Vault: fees exceed collateral"); }
            // cap the fees to the remainingCollateral
            return (1, remainingCollateral);
        }

        if (remainingCollateral < marginFees.add(_vault.liquidationFeeUsd())) {
            if (_raise) { revert("Vault: liquidation fees exceed collateral"); }
            return (1, marginFees);
        }

        if (remainingCollateral.mul(_vault.maxLeverage()) < position.size.mul(BASIS_POINTS_DIVISOR)) {
            if (_raise) { revert("Vault: maxLeverage exceeded"); }
            return (2, marginFees);
        }

        if (hasProfit && delta >= _vault.tokenToUsdMin(_collateralToken, position.reserveAmount)) {
            if (_raise) { revert("Vault: maxProfit hit"); }
            return (3, marginFees);
        }
        return (0, marginFees);
    }

This is because `validateLiquidation()` does not update the CumulativeFundingRate by calling `updateCumulativeFundingRate()`.

 function updateCumulativeFundingRate(address _indexToken) public {

        if (lastFundingTimes[_indexToken] == 0) {
            lastFundingTimes[_indexToken] =
                (block.timestamp / (fundingInterval)) *
                (fundingInterval);
            return;
        }

        if (lastFundingTimes[_indexToken] + (fundingInterval) > block.timestamp) {
            return;
        }

        uint256 longFunding = longFundingRates[_indexToken];
        uint256 shortFunding = shortFundingRates[_indexToken];
        uint256 intervals = (block.timestamp - (lastFundingTimes[_indexToken])) / (fundingInterval);

        longCumulativeFundingRates[_indexToken] =
            longCumulativeFundingRates[_indexToken] +
            longFunding *
            (intervals);
        shortCumulativeFundingRates[_indexToken] =
            shortCumulativeFundingRates[_indexToken] +
            shortFunding *
            (intervals);
        lastFundingTimes[_indexToken] = (block.timestamp / (fundingInterval)) * (fundingInterval);

        emit UpdateFundingRate(
            _indexToken,
            longCumulativeFundingRates[_indexToken],
            shortCumulativeFundingRates[_indexToken]
        );
    }

If a significant amount of time has passed since the last call of `updateCumulativeFundingRate()`, validateLiquidation()` can return a false state. As demonstrated in the PoC, the user’s position may appear healthy when calling `validateLiquidation()`, but when `updateCumulativeFundingRate()` is called, the position becomes liquidatable.

This can be highly misleading for users or developers building on top of GMX who rely on the public view function to check the state of users’ positions. If GMX or any integration were to use the function to actually liquidate positions, the impact would be critical, as unhealthy positions would not be liquidated on time.

PoC:

it.only("validateLiquidation can return incorrect data", async () => {
    await shortsTracker.setIsGlobalShortDataReady(true)
    await glpManager.setShortsTrackerAveragePriceWeight(10000)
    expect(await glpManager.shortsTrackerAveragePriceWeight()).to.be.equal(10000)

    await positionManager.connect(user1).increasePosition([dai.address], btc.address, expandDecimals(50000, 18), 0, toUsd(100000), false, toNormalizedPrice(60000))

    await btcPriceFeed.setLatestAnswer(toChainlinkPrice(48000))
    await positionManager.connect(user0).increasePosition([dai.address], btc.address, expandDecimals(10000, 18), 0, toUsd(100000), false, toNormalizedPrice(48000))

    await increaseTime(provider, 86400 * 365)
    await btcPriceFeed.setLatestAnswer(toChainlinkPrice(49000))

    let [liquidationStateBefore, marginFeeBefore] = await vault.validateLiquidation(user0.address, dai.address, btc.address, false, false)
    expect(liquidationStateBefore).to.eq(BigNumber.from("0"));

    expect(marginFeeBefore).to.eq(BigNumber.from("1000000000000000000000000000000000"));

    await vault.updateCumulativeFundingRate(dai.address, btc.address)

    let [liquidationStateAfter, marginFeeAfter] = await vault.validateLiquidation(user0.address, dai.address, btc.address, false, false)
    expect(marginFeeAfter).to.eq(BigNumber.from("7816666666666666666666666666666667"));
    expect(liquidationStateAfter, "liquidation state").to.be.eq(1)


    const [liquidationState, marginFee] = await getLiquidationState(user0.address, dai.address, btc.address, false, false)
    expect(liquidationState, "liquidation state").to.be.eq(1)

    await positionManager.setLiquidator(user1.address, true)
    await positionManager.connect(user1).liquidatePosition(user0.address, dai.address, btc.address, false, user1.address)
 })