stopthecap - Unpaid funding fees from wrong calculation are going to be substracted from the pool

[sherlock-admin]

stopthecap

high

Unpaid funding fees from wrong calculation are going to be substracted from the pool

Summary

The vulnerability start at the function getNextFundingAmountPerSize:

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

Basically, GMX is handling incorrectly the way on how they calculate the FundingFees to be paid because they calculate the amount using the getOpenInterest from both the short and long tokens.

Vulnerability Detail

The detailed issue here is that GMX is incorrectly handling the calculation with the getOpenInterest (the open Interest) of the two tokens, short and long. They are dividing the cache.fundingUsd for the interest of the two tokens:

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

After dividing, you get the cache.fundingUsdForLongCollateral which is basically the amount that has to be paid or paidTo for any of both collaterals and positions.

That is a big problem in one case, when users need to pay those feed. When the user does have to pay the fundingFees. Because customers can only pay those fundingFees for their collateral token, not for both.

As said, this is not an issue when users do receive the fundingFees because you can claim both tokens, but it is an issue when paying the fees.

This will result in less fundingFees being paid by users.

Impact

2 Impacts:

1:
A missmatch in the pools accounting will occur over time which might have unexpected results in terms of insolvency's.

2:
High because users are not paying the fees that they should pay, so there is an economic damage. Specifically because the bug just described, does reduce the variable fundingAmountPerSizePortion :

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

which is what user pays for the collateral token. As said before, the amounts are being divided by the entire interest, therefore, just a small part only a portion of the FundingFees will be paid, meanwhile the entire amount can be claimed.

Code Snippet

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

Tool used

Manual Review

Recommendation

The way on how the funding fees are calculate for users that have to pay them has to change in a way that it is not calculated accounting all the open interest. The calculation of the funding fees for users that have to claim fundingFees, not necessarily has to change.

[0xffff11]

Escalate for 10 USDC

Issue is completely valid as a solo high. I see that my phrasing was not very clear which brought the issue to be excluded.

The problem here is that when calculating getNextFundingAmountPerSize both amount per size portion for long and short tokens are accounted https://github.com/sherlock-audit/2023-02-gmx/blob/b8f926738d1e2f4ec1173939caa51698f2c89631/gmx-synthetics/contracts/market/MarketUtils.sol#L953-L954

Those fundingAmounts for long and short tokens are divided by the entire open interest:

cache.fundingUsdForShortCollateral = cache.fundingUsd * cache.oi.shortOpenInterestWithShortCollateral / cache.oi.shortOpenInterest;

There are 2 scenarios for these:

1
User are paid those fees, in which they would be paid entirely, because they are able to clame both, long and short tokens

2
User has to pay those funding fees. Users are only able to pay the fundingAmountPerSizePortion for their collateral token instead of both tokens, which reduces drastically the amount of fundingAmountPerSizePortion that the user will actually pay.

To sum up. Not all the fees will be paid because you can only pay fundingAmountPerSizePortion for your collateral token, but actually all the fundingAmountPerSizePortion can be claimed.

To sum up the getNextFundingAmountPerSize needs to differenciate calculations from those 2 cases to actually work as intended.

As the issue enables paying less fees for portions because it is being used the entire open interest for calculation, there is an economic damage, therefore is a high

[sherlock-admin]

Escalate for 10 USDC

Issue is completely valid as a solo high. I see that my phrasing was not very clear which brought the issue to be excluded.

The problem here is that when calculating getNextFundingAmountPerSize both amount per size portion for long and short tokens are accounted https://github.com/sherlock-audit/2023-02-gmx/blob/b8f926738d1e2f4ec1173939caa51698f2c89631/gmx-synthetics/contracts/market/MarketUtils.sol#L953-L954

Those fundingAmounts for long and short tokens are divided by the entire open interest:

cache.fundingUsdForShortCollateral = cache.fundingUsd * cache.oi.shortOpenInterestWithShortCollateral / cache.oi.shortOpenInterest;

There are 2 scenarios for these:

1
User are paid those fees, in which they would be paid entirely, because they are able to clame both, long and short tokens

2
User has to pay those funding fees. Users are only able to pay the fundingAmountPerSizePortion for their collateral token instead of both tokens, which reduces drastically the amount of fundingAmountPerSizePortion that the user will actually pay.

To sum up. Not all the fees will be paid because you can only pay fundingAmountPerSizePortion for your collateral token, but actually all the fundingAmountPerSizePortion can be claimed.

To sum up the getNextFundingAmountPerSize needs to differenciate calculations from those 2 cases to actually work as intended.

As the issue enables paying less fees for portions because it is being used the entire open interest for calculation, there is an economic damage, therefore is a high

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'm having a hard time parsing what is being said, but it sounds like the submitter is worried that there won't be enough tokens to pay out funding fees until fees are deducted from actual positions. If that's what's being claimed, the way this is prevented is by having a reserved amount which has to be satisfied by depositors before any position increase is allowed. @xvi10 can you take a look and see if I'm missing something, or if the description of the issue is more clear to you?

[xvi10]

it seems to be a valid issue, i think the issue described is the same as MKTU-2 of https://github.com/GuardianAudits/Audits/blob/main/GMX/GMX_Synthetics_Audit_3.pdf

[IllIllI000]

I agree that this is the same issue described by MKTU-2, but I'm still not seeing what the problem is. getNextFundingAmountPerSize() updates the global information about funding for a market's collateral, not the funding fees for a specific position. I don't see any mathematical error in dividing by cache.oi.longOpenInterest since it equals cache.oi.longOpenInterestWithLongCollateral + cache.oi.longOpenInterestWithShortCollateral. The actual deduction of the funding fees is done separately in getFundingFees(), where fees are deducted from the collateral held by the position. For example, if user A is long, user B is short with short collat, and user C is short with long collat, and the shorts have to pay longs, A claims collateral that B and C will pay their tokens for. As I mentioned above, user A can't create an OI position until market LPs have added enough tokens to cover any potential gains/losses, and this is enforced by 'reserved amount' checks during increases, decreases, and swaps. @xvi10, am I misunderstanding something here? Do you have access to the POC provided in MKTU-2? Can you provide it, e.g. in a gist so I can see what the exploit is?

[xvi10]

the PoC:

import { expect } from "chai";

import { deployFixture } from "../../../utils/fixture";
import { expandDecimals, decimalToFloat } from "../../../utils/math";
import { getPoolAmount, getSwapImpactPoolAmount, getMarketTokenPrice } from "../../../utils/market";
import { handleDeposit, getDepositCount } from "../../../utils/deposit";
import { OrderType, getOrderCount, getOrderKeys, createOrder, executeOrder, handleOrder } from "../../../utils/order";
import { getPositionCount, getAccountPositionCount, getPositionKeys } from "../../../utils/position";
import { mine, time } from "@nomicfoundation/hardhat-network-helpers";
import * as keys from "../../../utils/keys";

describe("Guardian.MKTU-2", () => {
  const { provider } = ethers;

  let fixture;
  let user0, user1, user2, user3;
  let reader,
    dataStore,
    oracle,
    depositVault,
    ethUsdMarket,
    ethUsdSpotOnlyMarket,
    wnt,
    wbtc,
    usdc,
    attackContract,
    exchangeRouter,
    eventEmitter,
    ethEthMarket,
    solEthEthMarket,
    wbtcEthEthMarket;
  let executionFee;
  beforeEach(async () => {
    fixture = await deployFixture();

    ({ user0, user1, user2, user3 } = fixture.accounts);
    ({
      reader,
      dataStore,
      oracle,
      depositVault,
      ethUsdMarket,
      ethUsdSpotOnlyMarket,
      wnt,
      wbtc,
      usdc,
      attackContract,
      exchangeRouter,
      eventEmitter,
      ethEthMarket,
      solEthEthMarket,
      wbtcEthEthMarket,
    } = fixture.contracts);
    ({ executionFee } = fixture.props);

    await handleDeposit(fixture, {
      create: {
        market: ethUsdMarket,
        longTokenAmount: expandDecimals(1000, 18),
        shortTokenAmount: expandDecimals(1000 * 5000, 6),
      },
    });
    await handleDeposit(fixture, {
      create: {
        market: ethUsdSpotOnlyMarket,
        longTokenAmount: expandDecimals(10000000, 18),
        shortTokenAmount: expandDecimals(1000000 * 5000, 6),
      },
    });
  });

  it("CRITICAL: More claimable funding fees then paid", async () => {
    await dataStore.setUint(keys.fundingFactorKey(ethUsdMarket.marketToken), decimalToFloat(1, 7));
    await dataStore.setUint(keys.fundingExponentFactorKey(ethUsdMarket.marketToken), decimalToFloat(1));

    // User0 MarketIncrease long position with long collateral for $100K
    await handleOrder(fixture, {
      create: {
        account: user0,
        market: ethUsdMarket,
        initialCollateralToken: wnt,
        initialCollateralDeltaAmount: expandDecimals(10, 18), // $50,000 in 10 ETH
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(100 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketIncrease,
        isLong: true,
        shouldUnwrapNativeToken: false,
      },
    });

    // User1 MarketIncrease short position with short collateral for $100K
    await handleOrder(fixture, {
      create: {
        account: user1,
        market: ethUsdMarket,
        initialCollateralToken: usdc,
        initialCollateralDeltaAmount: expandDecimals(50 * 1000, 6), // $50,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(100 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketIncrease,
        isLong: false,
        shouldUnwrapNativeToken: false,
      },
    });

    // User2 MarketIncrease short position with long collateral for $100K
    await handleOrder(fixture, {
      create: {
        account: user2,
        market: ethUsdMarket,
        initialCollateralToken: wnt,
        initialCollateralDeltaAmount: expandDecimals(10, 18), // $50,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(100 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketIncrease,
        isLong: false,
        shouldUnwrapNativeToken: false,
      },
    });

    // User3 MarketIncrease long with short collateral for $100K
    await handleOrder(fixture, {
      create: {
        account: user3,
        market: ethUsdMarket,
        initialCollateralToken: usdc,
        initialCollateralDeltaAmount: expandDecimals(100 * 1000, 6), // $100,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(200 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketIncrease,
        isLong: true,
        shouldUnwrapNativeToken: false,
      },
    });

    // Check that everyone has a position open
    expect(await getAccountPositionCount(dataStore, user0.address)).eq(1);
    expect(await getAccountPositionCount(dataStore, user1.address)).eq(1);
    expect(await getAccountPositionCount(dataStore, user2.address)).eq(1);
    expect(await getAccountPositionCount(dataStore, user3.address)).eq(1);
    expect(await getPositionCount(dataStore)).eq(4);

    // 300 days later
    await time.increase(300 * 24 * 60 * 60);

    const prices = {
      indexTokenPrice: {
        min: expandDecimals(5000, 12),
        max: expandDecimals(5000, 12),
      },
      longTokenPrice: {
        min: expandDecimals(5000, 12),
        max: expandDecimals(5000, 12),
      },
      shortTokenPrice: {
        min: expandDecimals(1, 24),
        max: expandDecimals(1, 24),
      },
    };

    const positionKeys = await getPositionKeys(dataStore, 0, 10);
    const user0Position = await reader.getPositionInfo(dataStore.address, positionKeys[0], prices);
    const user3Position = await reader.getPositionInfo(dataStore.address, positionKeys[3], prices);

    // Total WNT FoundingFees paid by User0
    const totalWNTFeesPaidByUser0 = await user0Position.pendingFundingFees.fundingFeeAmount;

    // Total USDC FoundingFees paid by User3
    const totalUSDCFeesPaidByUser3 = await user3Position.pendingFundingFees.fundingFeeAmount;

    expect(await getOrderCount(dataStore)).to.eq(0);
    expect(await wnt.balanceOf(user0.address)).to.eq(0);
    expect(await wnt.balanceOf(user1.address)).to.eq(0);
    expect(await wnt.balanceOf(user2.address)).to.eq(0);
    expect(await wnt.balanceOf(user3.address)).to.eq(0);
    expect(await usdc.balanceOf(user0.address)).to.eq(0);
    expect(await usdc.balanceOf(user1.address)).to.eq(0);
    expect(await usdc.balanceOf(user2.address)).to.eq(0);
    expect(await usdc.balanceOf(user3.address)).to.eq(0);

    // User0 MarketDecrease for the whole position size
    await handleOrder(fixture, {
      create: {
        account: user0,
        market: ethUsdMarket,
        initialCollateralToken: wnt,
        initialCollateralDeltaAmount: expandDecimals(10, 18), // $50,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(100 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketDecrease,
        isLong: true,
        shouldUnwrapNativeToken: false,
      },
    });

    // User1 MarketDecrease for the whole position size
    await handleOrder(fixture, {
      create: {
        account: user1,
        market: ethUsdMarket,
        initialCollateralToken: usdc,
        initialCollateralDeltaAmount: expandDecimals(50 * 1000, 6), // $50,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(100 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketDecrease,
        isLong: false,
        shouldUnwrapNativeToken: false,
      },
    });

    // User2 MarketDecrease for the whole position size
    await handleOrder(fixture, {
      create: {
        account: user2,
        market: ethUsdMarket,
        initialCollateralToken: wnt,
        initialCollateralDeltaAmount: expandDecimals(10, 18), // $50,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(100 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketDecrease,
        isLong: false,
        shouldUnwrapNativeToken: false,
      },
    });

    // User3 MarketDecrease for the whole position size
    await handleOrder(fixture, {
      create: {
        account: user3,
        market: ethUsdMarket,
        initialCollateralToken: usdc,
        initialCollateralDeltaAmount: expandDecimals(100 * 1000, 6), // $100,000
        swapPath: [],
        sizeDeltaUsd: decimalToFloat(200 * 1000), // 2x Position
        acceptablePrice: expandDecimals(5000, 12),
        executionFee: expandDecimals(1, 15),
        minOutputAmount: 0,
        orderType: OrderType.MarketDecrease,
        isLong: true,
        shouldUnwrapNativeToken: false,
      },
    });

    // Check total wnt funding fees paid by User0
    expect(totalWNTFeesPaidByUser0).eq("3455997333333400000");

    // Check total usdc funding fees paid by User3
    expect(totalUSDCFeesPaidByUser3).eq("69120000000");

    // Check User1's claimable funding fees
    expect(
      await dataStore.getUint(keys.claimableFundingAmountKey(ethUsdMarket.marketToken, wnt.address, user1.address))
    ).to.eq("5183999266666700000");
    expect(
      await dataStore.getUint(keys.claimableFundingAmountKey(ethUsdMarket.marketToken, usdc.address, user1.address))
    ).to.eq("51840000000");

    // Check User2's claimable funding fees
    expect(
      await dataStore.getUint(keys.claimableFundingAmountKey(ethUsdMarket.marketToken, wnt.address, user2.address))
    ).to.eq("5183999266666700000");
    expect(
      await dataStore.getUint(keys.claimableFundingAmountKey(ethUsdMarket.marketToken, usdc.address, user2.address))
    ).to.eq("51840000000");

    // Check User0 has less balance than initially, e.g. User0 paid funding fees in WNT
    expect(await wnt.balanceOf(user0.address)).to.lt(expandDecimals(10, 18));

    // Check User3 has less balance than initially, e.g. User3 paid funding fees in USDC
    expect(await usdc.balanceOf(user3.address)).to.lt(expandDecimals(1000 * 1000, 6));

    // First we'll showcase that there is more WNT funding fee to be claimed the being paid, under that test we'll showcase that there is more USDC funding fee to be claimed then being paid

    // TEST more claimable WNT funding fees then paid
    const totalFeeAmountPaidWNT = BigInt(totalWNTFeesPaidByUser0);

    // Get total of claimable funding fees from User1 and User2
    const claimableWNTUser1 = await dataStore.getUint(
      keys.claimableFundingAmountKey(ethUsdMarket.marketToken, wnt.address, user1.address)
    );
    const claimableWNTUser2 = await dataStore.getUint(
      keys.claimableFundingAmountKey(ethUsdMarket.marketToken, wnt.address, user2.address)
    );
    const totalClaimableFeesWNT = BigInt(claimableWNTUser1) + BigInt(claimableWNTUser2);

    // When funding fees are paid by the long side, each token per size value is divided amongst the total long open interest, but not every long position is capable of paying out the fees for either collateral tokens

    // Check that the total amount of fees claimable is more the total fees paid
    expect(totalClaimableFeesWNT).to.gt(totalFeeAmountPaidWNT);

    // TEST more claimable USDC funding fees then paid
    const totalFeeAmountPaidUSDC = BigInt(totalUSDCFeesPaidByUser3);

    // Get total of claimable funding fees from User1 and User2
    const claimableUSDCUser1 = await dataStore.getUint(
      keys.claimableFundingAmountKey(ethUsdMarket.marketToken, usdc.address, user1.address)
    );
    const claimableUSDCUser2 = await dataStore.getUint(
      keys.claimableFundingAmountKey(ethUsdMarket.marketToken, usdc.address, user2.address)
    );
    const totalClaimableFeesUSDC = BigInt(claimableUSDCUser1) + BigInt(claimableUSDCUser2);

    // When funding fees are paid by the short side, each token per size value is divided amongst the total short open interest, but not every short position is capable of paying out the fees for either collateral tokens

    // Check that the total amount of fees claimable is more the the total fees paid
    expect(totalClaimableFeesUSDC).to.gt(totalFeeAmountPaidUSDC);

    // Check there is no open positions
    expect(await getAccountPositionCount(dataStore, user0.address)).eq(0);
    expect(await getAccountPositionCount(dataStore, user1.address)).eq(0);
    expect(await getAccountPositionCount(dataStore, user2.address)).eq(0);
    expect(await getAccountPositionCount(dataStore, user3.address)).eq(0);
    expect(await getPositionCount(dataStore)).eq(0);
  });
});```

[IllIllI000]

While the title of the issue, and the non-code parts of the summary are correct, the vulnerability detail and part of the impact are not correct. As I described above, there is no mathematical issue with the area of the code that the submitter links to, and the actual funding calculation is done elsewhere as I mention. However, there is still an adjustment needed in the per-size values, which is described in MKTU-2, but is not described in this submission. Essentially, while the full open interest division is correct, that amount is later converted to per-share units, which should have taken into account the collateral of the position, so that when it's multiplied later in getFundingFees(), we get the right amount. As is seen in the fix applied by the sponsor, the area of the code that the submitter points to remains unmodified, and the adjustment that MKTU-2 describes is done later. The patch I'm providing below shows that only the MKTU-2 adjustment is needed in order for the distribute short amounts to match the pay short amount (I commented out the long portion of the test, because including both makes the test run into contract size issues):

diff --git a/gmx-synthetics/contracts/market/MarketUtils.sol b/gmx-synthetics/contracts/market/MarketUtils.sol
index 7624b69..70399b2 100644
--- a/gmx-synthetics/contracts/market/MarketUtils.sol
+++ b/gmx-synthetics/contracts/market/MarketUtils.sol
@@ -960,12 +960,15 @@ library MarketUtils {
         // use Precision.FLOAT_PRECISION here because fundingUsdForLongCollateral or fundingUsdForShortCollateral divided by longTokenPrice
         // will give an amount in number of tokens which may be quite a small value and could become zero after being divided by longOpenInterest
         // the result will be the amount in number of tokens multiplied by Precision.FLOAT_PRECISION per 1 USD of size
-        cache.fps.fundingAmountPerSizePortion_LongCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.longOpenInterest);
+        //cache.fps.fundingAmountPerSizePortion_LongCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.longOpenInterest);
         cache.fps.fundingAmountPerSizePortion_LongCollateral_ShortPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.shortOpenInterest);
-        cache.fps.fundingAmountPerSizePortion_ShortCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max, cache.oi.longOpenInterest);
+        //cache.fps.fundingAmountPerSizePortion_ShortCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max, cache.oi.longOpenInterest);
         cache.fps.fundingAmountPerSizePortion_ShortCollateral_ShortPosition = getPerSizeValue(cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max, cache.oi.shortOpenInterest);
-
+	
         if (result.longsPayShorts) {
+	    cache.fps.fundingAmountPerSizePortion_LongCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.longOpenInterestWithLongCollateral);
+	    cache.fps.fundingAmountPerSizePortion_ShortCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max, cache.oi.longOpenInterestWithShortCollateral);
+
             // longs pay shorts
             result.fundingAmountPerSize_LongCollateral_LongPosition = Calc.boundedAdd(
                 result.fundingAmountPerSize_LongCollateral_LongPosition,
@@ -987,6 +990,9 @@ library MarketUtils {
                 cache.fps.fundingAmountPerSizePortion_ShortCollateral_ShortPosition.toInt256()
             );
         } else {
+	    //cache.fps.fundingAmountPerSizePortion_LongCollateral_ShortPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.shortOpenInterestWithLongCollateral);
+	    //cache.fps.fundingAmountPerSizePortion_ShortCollateral_ShortPosition = getPerSizeValue(cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max, cache.oi.shortOpenInterestWithShortCollateral);
+
             // shorts pay longs
             result.fundingAmountPerSize_LongCollateral_LongPosition = Calc.boundedSub(
                 result.fundingAmountPerSize_LongCollateral_LongPosition,
diff --git a/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol b/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol
index c274e48..d807bae 100644
--- a/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol
+++ b/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol
@@ -13,7 +13,7 @@ import "./PricingUtils.sol";
 
 import "../referral/IReferralStorage.sol";
 import "../referral/ReferralUtils.sol";
-
+import "hardhat/console.sol";
 // @title PositionPricingUtils
 // @dev Library for position pricing functions
 library PositionPricingUtils {
@@ -405,7 +405,7 @@ library PositionPricingUtils {
         address shortToken,
         int256 latestLongTokenFundingAmountPerSize,
         int256 latestShortTokenFundingAmountPerSize
-    ) internal pure returns (PositionFundingFees memory) {
+    ) internal view returns (PositionFundingFees memory) {
         PositionFundingFees memory fundingFees;
 
         fundingFees.latestLongTokenFundingAmountPerSize = latestLongTokenFundingAmountPerSize;
@@ -425,24 +425,36 @@ library PositionPricingUtils {
             position.shortTokenFundingAmountPerSize(),
             position.sizeInUsd()
         );
-
+console.log("sender: ", tx.origin);
+//console.log("  fundingAPS long");
+//console.logInt(latestLongTokenFundingAmountPerSize);
+console.log("  fundingAPS short");
+console.logInt(latestShortTokenFundingAmountPerSize);
+//console.log("  long: ");
+//console.logInt(longTokenFundingFeeAmount);
+console.log("  short:");
+console.logInt(shortTokenFundingFeeAmount);
         // if the position has negative funding fees, distribute it to allow it to be claimable
         if (longTokenFundingFeeAmount < 0) {
             fundingFees.claimableLongTokenAmount = (-longTokenFundingFeeAmount).toUint256();
+//console.log("  distribute long");
         }
 
         if (shortTokenFundingFeeAmount < 0) {
             fundingFees.claimableShortTokenAmount = (-shortTokenFundingFeeAmount).toUint256();
+console.log("  distribute short");
         }
 
         if (position.collateralToken() == longToken && longTokenFundingFeeAmount > 0) {
             fundingFees.fundingFeeAmount = longTokenFundingFeeAmount.toUint256();
+//console.log("  pay long");
         }
 
         if (position.collateralToken() == shortToken && shortTokenFundingFeeAmount > 0) {
             fundingFees.fundingFeeAmount = shortTokenFundingFeeAmount.toUint256();
+console.log("  pay short");
         }
-
+console.log("end\n");
         return fundingFees;
     }
 

output:

...
sender:  0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
  fundingAPS short
10368
  short:
1036800000
end

sender:  0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
  fundingAPS short
-10368
  short:
-1036800000
  distribute short
end

sender:  0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
  fundingAPS short
-10368
  short:
-1036800000
  distribute short
end

sender:  0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
  fundingAPS short
10368
  short:
2073600000
  pay short
end
...

-1036800000 + -1036800000 + 2073600000 = 0

If there had been another submission in the contest where I could have marked this as a duplicate, I would have done so, so I'm leaning towards this being a valid High. However, because the submission did not actually identify the right problem and didn't provide any test showing that they weren't just guessing that there was some sort of problem in this area, I'm not sure if this qualifies for a solo high, and will leave the decision to the Sherlock team

[IllIllI000]

Upon further inspection, I believe that there is another submission that is describing the MKTU-2 issue correctly, and that's #109. In the section they label with Calculate nextFundingAmountPerSize:, they properly include the per-size adjustment, and they properly trace propagation of the incomplete adjustment from updateFundingAmountPerSize() to the later call to getPositionFees(). They don't explicitly say that the issue is with the per-size adjustment, but the math they provide shows that the amount paid is not the same as the amount withdrawn. Again, this current issue doesn't properly describe the area of the problem, so I would say that it's either a duplicate of #109, or invalid

[xvi10]

i don't think it is the same issue as #109, the issue in 109 is due to rounding i believe

for this issue, the description is correct, i think the code referenced in "https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L963-L966" is to describe that the incorrect calculation affects these variables

the Code Snippet covers the code that should be updated

[IllIllI000]

@xvi10, just so I understand what you're saying about this current issue, do you agree that I've described the shortcomings of this finding correctly, but disagree that it should be invalid, or are you saying something else? With the fix to the area of the code that you changed, are you seeing some other issue not covered by the test?

Separately, for 109, can you elaborate on how/where it's due to rounding, and point to the change you've made in the code to address it?

[xvi10]

i think that the issue should be valid

the issue mentioned:

"High because users are not paying the fees that they should pay, so there is an economic damage. Specifically because the bug just described, does reduce the variable fundingAmountPerSizePortion :

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

the reporter mentions that the fundingAmountPerSizePortion values will be affected which is correct

for 109, the fix for the rounding issue: gmx-io/gmx-synthetics@8e5c776