stopthecap - Unpaid funding fees from wrong calculation are going to be substracted from the pool
Opened this issue · 20 comments
stopthecap
high
Unpaid funding fees from wrong calculation are going to be substracted from the pool
Summary
The vulnerability start at the function getNextFundingAmountPerSize:
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:
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 :
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
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.
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
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
getNextFundingAmountPerSizeboth 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-L954Those 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 tokens2
User has to pay those funding fees. Users are only able to pay thefundingAmountPerSizePortionfor their collateral token instead of both tokens, which reduces drastically the amount offundingAmountPerSizePortionthat the user will actually pay.To sum up. Not all the fees will be paid because you can only pay
fundingAmountPerSizePortionfor your collateral token, but actually all thefundingAmountPerSizePortioncan be claimed.To sum up the
getNextFundingAmountPerSizeneeds 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.
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?
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
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?
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);
});
});```
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
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
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
@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?
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 :
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
They do mention fundingAmountPerSizePortion, but the adjustment is in each fundingAmountPerSizePortion_*Collateral_*Position, not in the raw fundingAmountPerSizePortion. They do mention a problem that ended up being shown to be valid, and I believe that by the Sherlock rules, it doesn't matter that it may not have described every aspect, and it should count as a High
For 109 can you point to where in the issue they refer to a rounding issue, or what step of the math shows it?
for 109, they don't mention it as a rounding issue, but i believe the difference in amount paid vs amount claimed can be fixed by fixing the rounding
@xvi10 Why is it not the same issue as the one described here? Wouldn't their tracing of the various calls made in this issue point to the same issue? What in their description made it seem like a rounding issue instead? Their submission doesn't mention units of precision anywhere
this issue only affects the case where positions are opened with different types of collateral, because the calculation:
cache.fps.fundingAmountPerSizePortion_LongCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForLongCollateral / prices.longTokenPrice.max, cache.oi.longOpenInterest);
cache.fps.fundingAmountPerSizePortion_ShortCollateral_LongPosition = getPerSizeValue(cache.fundingUsdForShortCollateral / prices.shortTokenPrice.max, cache.oi.longOpenInterest);
would be affected, since it should be
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);
e.g. if all positions used the long token as collateral, then cache.oi.longOpenInterest == cache.oi.longOpenInterestWithLongCollateral and fundingUsdForShortCollateral would be zero
in the example given in #109, all positions use the same collateral so it is not the same issue
as mentioned, 109 does not describe the issue as a rounding issue, just that it possible for the amount claimable to be larger than the amount paid or pending to be paid, fixing the rounding would prevent this from occurring
I agree that this issue requires that collaterals be different, and 109 does not describe that
@xvi10 I do not see how the fix provided for #109 makes one or the other of the inequalities at the end of the submission always negative, which I believe would be required for that issue to be resolved, but I'll take another look. Please let me know if you've already figured it out
would it be possible to get clarification from the reporter on this?
Escalation accepted
Based on the above discussions. Considering this issue a valid high
Escalation accepted
Based on the above discussions. Considering this issue a valid high
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.