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

Emergency overrides any Status and user loses funds

auditism

Summary

Emergency Pauses will override the current status, and 'forget' about the overridden status once it resumes or shutdowns. This is a problem because actions are a two step process in this protocol.

Vulnerability Details

When the emergency status is triggered, any transaction status will be overwritten, after Paused is lifted, the Open status is set again.
This is problematic because in this protocol any kind of transaction is actually not atomic ,they happen in two transactions, this is why this status concept was initially introduced to the protocol. However the emergency related functions completely disrupt this concept and introduce danger to the protocol.
If a user was in the middle of a deposit or withdraw, and some kind of emergency was to happen the user would essentially lose his funds as the process would be interrupted but not reverted.

POC

Here below is a test that shows a concrete example where a user gets interrupted mid action and his funds are essentially eaten by the protocol. His money is lost and the protocol enjoys more lp tokens.

// 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 User1DepositEmergency is GMXMockVaultSetup, GMXTestHelper, TestUtils {
​
function test_processDepositAndEmergency() external {
//normal deposit
vm.startPrank(owner);
_createAndExecuteDeposit(
address(WETH),
address(USDC),
address(WETH),
2e18,
0,
SLIPPAGE,
EXECUTION_FEE
);
//user start depositing process
​
vm.startPrank(user1);
console.log(IERC20(WETH).balanceOf(address(user1)));
​
uint256 lpAmtBefore = vault.lpAmt();
console2.log("lpAmtBefore", lpAmtBefore);
​
​
​
_createDeposit(address(WETH), 1e18, 0, SLIPPAGE, EXECUTION_FEE);
​
console.log(IERC20(WETH).balanceOf(address(user1)));
assertEq(uint256(vault.store().status), 1, "vault status not set to deposit");
//emergency arises straight after the deposit, process deposit didn't have time to occur
vm.startPrank(owner);
​
vault.emergencyPause();
mockExchangeRouter.executeWithdrawal(
address(WETH),
address(USDC),
address(vault),
address(callback)
);
​
vault.emergencyResume();
mockExchangeRouter.executeDeposit(
address(WETH),
address(USDC),
address(vault),
address(callback)
);
​
assertEq(uint256(vault.store().status), 0, "vault status not to Open");
​
//resume deposit of user1
​
expectRevert("NotAllowedInCurrentVaultStatus()");
mockExchangeRouter.executeDeposit(
address(WETH),
address(USDC),
address(vault),
address(callback)
);
​
​
uint256 lpAmtAfter = vault.lpAmt();
console2.log("lpAmtAfter", lpAmtAfter);
​
​
}
​
}

Impact

The impact is pretty severe, as a user would mourn his lost(stolen) founds. The necessary checks to finalize any transactions previously initialized won't be accessible anymore since the protocol resume to status Open, or Closed if the protocol doesn't resume to normal state at all.

Tools Used

Manual review

Recommendations

In order to solve this issue it would be best to introduce a previousStatus enum variable that the protocol could refer to so that transactions could resume to this previous status, if processEmergencyResume was to be triggered.
In the event of a market shut down with the emergencyClose, it would be best if we let the protocol finish the transaction, it means that in the checks, the Closed status should be accepted to proceed for further action. For example for the withdraw function it should look like this :

function beforeProcessWithdrawCancellationChecks(
GMXTypes.Store storage self
) external view {
if (self.status != GMXTypes.Status.Withdraw && self.status != GMXTypes.Status.Close)
revert Errors.NotAllowedInCurrentVaultStatus();
}

The same fix should be applied any further action related to user withdraws and deposits.

Updates

Lead Judging Commences

hans Lead Judge over 1 year 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.