M-02. `emergencyPause` does not check the state before running && can cause loss of funds for users

Submitted by Citris, Giorgio, nmirchev8, Cosine, Drynooo, 0xCiphky, SupaRoutis, NeverGonnaGiveYulUp. Selected submission by: SupaRoutis.

Relevant GitHub Links

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

Summary

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.

Vulnerability Details

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.

javascript
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).

javascript
function beforeProcessDepositChecks(
GMXTypes.Store storage self
) external view {
if (self.status != GMXTypes.Status.Deposit)
@>      revert Errors.NotAllowedInCurrentVaultStatus();
}

Impact

If the emergency pause is triggered at an inopportune time, it could:

Prevent the completion of in-progress transactions.

Lead to loss of funds if the transactions are not properly rolled back.

Erode user trust in the system due to potential for funds to be stuck without recourse.

POC :

You can copy this test in the file GMXEmergencyTest.t.sol then execute the test with the command forge test --mt

javascript
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);

}

Tools Used

Manual review

Recommendations

To mitigate this risk, the following recommendations should be implemented:

Introduce a state check mechanism that prevents emergencyPause from executing if there are pending critical operations that must be completed to ensure the integrity of in-progress transactions.

Implement a secure check that allows emergencyPause to queue behind critical operations, ensuring that any ongoing transaction can complete before the pause takes effect.