M-05. Emergency Closed Vault Can Be Paused Then Resume

Submitted by nmirchev8, kz0213871, Giorgio, nisedo, NeverGonnaGiveYulUp. Selected submission by: nisedo.

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXEmergency.sol#L47-L66

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXEmergency.sol#L72-L91

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXEmergency.sol#L111-L156

Vulnerability Details

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.

plain text
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.

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

plain text
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
);
}

Impact

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.

PoC

Add this to GMXEmergencyTest.t.sol and test with forge test --mt test_close_then_pause -vv:

plain text
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));
}

Recommendations

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.