emergencyPause
does not check the state before running && can cause loss of funds for usersMedium Risk
https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXEmergency.sol#L47
https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXEmergency.sol#L63
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: