L-16. Failure in emergencyClose results in funds being stuck

Submitted by FundsSafu.

Relevant GitHub Links

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

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

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

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXManager.sol#L123

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXReader.sol#L168

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

Summary

In order to activate the emergency withdrawals, the vault needs to reach the CLOSED status. If the vault fails to do so, users won't be able to withdraw their funds.

Vulnerability Details

Users have the ability to call emergencyWithdraw method in order to retrieve their funds when the vault is CLOSED link1. However to reach this status, the vault needs to be paused first by the owners and then the emergencyClose needs to be called link2.

However the emergencyClose has many possible points of failure since it does many external calls: 1- calls the lending contract to calculate debt and repay it link3 link4 link5 link 6

2- calls a swap router if a trade is needed link6 (can revert for example if the uniswap router does not provide enough tokenOut when given a fix amount of tokenIn).

If any of these calls fail, the vault status will remain at PAUSED.

Impact

Users cannot retrieve their funds after the pause unless all the external contracts done in emergencyClose work as expected and all the debt have been cleared.

Tools Used

Recommendations

Use a time limit after pausing the vault that allows anyone to CLOSE the vault without doing any external calls inside the method.

Example:

plain text
function emergencyClose(
GMXTypes.Store storage self,
uint256 deadline
) external {
GMXChecks.beforeEmergencyCloseChecks(self);

if(block.timestamp < self.pauseTimestamp + CLOSE_MAX_DURATION) {

// Repay all borrowed assets; 1e18 == 100% shareRatio to repay
GMXTypes.RepayParams memory _rp;
(
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
) = GMXManager.calcRepay(self, 1e18);

(
bool _swapNeeded,
address _tokenFrom,
address _tokenTo,
uint256 _tokenToAmt
) = GMXManager.calcSwapForRepay(self, _rp);

if (_swapNeeded) {
ISwap.SwapParams memory _sp;

_sp.tokenIn = _tokenFrom;
_sp.tokenOut = _tokenTo;
_sp.amountIn = IERC20(_tokenFrom).balanceOf(address(this));
_sp.amountOut = _tokenToAmt;
_sp.slippage = self.minSlippage;
_sp.deadline = deadline;

GMXManager.swapTokensForExactTokens(self, _sp);
}

GMXManager.repay(
self,
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
);

self.status = GMXTypes.Status.Closed;

emit EmergencyClose(
_rp.repayTokenAAmt,
_rp.repayTokenBAmt
);

} else {
self.status = GMXTypes.Status.Closed;

emit EmergencyClose(
0,
0
);
}

}