Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

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

SupaRoutis

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.

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();
}

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

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.

Updates

Lead Judging Commences

hans Auditor
almost 2 years ago
hans Auditor
almost 2 years ago
hans Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Not caching the previous state on emergencyPause

Impact: High Likelihood: Low/Medium Deposit or withdraw that were in progress will be ignored and cause fund loss. Because emergencyPause is only callable by keepers, Medium is the proper severity.

cosine Auditor
almost 2 years ago
hans Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Not caching the previous state on emergencyPause

Impact: High Likelihood: Low/Medium Deposit or withdraw that were in progress will be ignored and cause fund loss. Because emergencyPause is only callable by keepers, Medium is the proper severity.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.