emergencyClose() may fail to repay any debt

Severity

High Risk

Relevant GitHub Links

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

Summary

• the emergencyClose() function may become ineffective, preventing the contract from repaying any outstanding debt, leading to potential financial losses.

Vulnerability Details

• When the contract is paused, all the liquidity from GMX is withdrawn (in term of tokenA and tokenB).
• The emergencyClose() function is called after the contract is paused due some reasons, possibly when the strategy incurs bad debts or when the contract gets hacked, High volatility, and so on...
• This function is responsible for repaying all the amounts of tokenA and tokenB borrowed from the lendingVault contract. It then sets the contract's status to closed. After that, users who hold svToken shares can withdraw the remaining assets from the contract.
• The issue with this function lies in its assumptions, which are not accurate. It assumes that the withdrawn amounts from GMX are always sufficient to cover the whole debt.

   function emergencyClose(GMXTypes.Store storage self, uint256 deadline) external {
        // Revert if the status is Paused.
        GMXChecks.beforeEmergencyCloseChecks(self);

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

• Please note that _rp.repayTokenAAmt and _rp.repayTokenBAmt represent the entire debt, and these values remain the same even if a swap is needed.
• The function checks if a swap is needed to cover its debt, and here's how it determines whether a swap is required:

  function calcSwapForRepay(GMXTypes.Store storage self, GMXTypes.RepayParams memory rp)
        external
        view
        returns (bool, address, address, uint256)
    {
        address _tokenFrom;
        address _tokenTo;
        uint256 _tokenToAmt;
        if (rp.repayTokenAAmt > self.tokenA.balanceOf(address(this))) {
            // If more tokenA is needed for repayment
            _tokenToAmt = rp.repayTokenAAmt - self.tokenA.balanceOf(address(this));
            _tokenFrom = address(self.tokenB);
            _tokenTo = address(self.tokenA);

            return (true, _tokenFrom, _tokenTo, _tokenToAmt);
        } else if (rp.repayTokenBAmt > self.tokenB.balanceOf(address(this))) {
            // If more tokenB is needed for repayment
            _tokenToAmt = rp.repayTokenBAmt - self.tokenB.balanceOf(address(this));
            _tokenFrom = address(self.tokenA);
            _tokenTo = address(self.tokenB);

            return (true, _tokenFrom, _tokenTo, _tokenToAmt);
        } else {
            // If there is enough to repay both tokens
            return (false, address(0), address(0), 0);
        }
    }

• In plain English, this function in this case assumes: if the contract's balance of one of the tokens (e.g., tokenA) is insufficient to cover tokenA debt, it means that the contract balance of the other token (tokenB) should be greater than the debt of tokenB, and the value of the remaining balance of tokenB after paying off the tokenB debt should be equal or greater than the required value to cover the debt of tokenA
  

The two main issues with this assumption are:

1. If the contract balance of tokenFrom is not enough to be swapped for _tokenToAmt of tokenTo, the swap will revert, causing the function to revert each time it is called when the balance of tokenFrom is insufficient.(in most cases in delta long strategy since it's only borrow one token), This is highly likely since emergency closures occur when something detrimental has happened, (such as bad debts).
2. The second issue arises when the balance of tokenFrom(EX: tokenA) becomes less than _rp.repayTokenAAmt after a swap. In this case, the repay call will revert when the lendingVault contract attempts to transferFrom the strategy contract for an amount greater than its balance. ex :
   • tokenA balance = 100, debtA = 80.
   • tokenB balance = 50 , debtB = 70.
   • after swap tokenA for 20 tokenB .
   • tokenA balance = 75 , debtA = 80 : in this case repay will keep revert .

• so if the contract accumulates bad debts(in value), the emergencyClose() function will always revert, preventing any debt repayment.
• Another critical factor to consider is the time between the pause action and the emergency close action. During periods of high volatility, the pause action temporarily halts the contract, but the prices of the two assets may continue to decline. The emergency close function can only be triggered by the owner, who operates a time-lock wallet. In the time between the pause and close actions, the prices may drop significantly and this condition will met since the swap is needed in almost all cases.

Impact

• emergencyClose() function will consistently fail to repay any debt.
• lenders may lose all their funds

Tools Used

vs code manual review

Recommendations

• the debt need to be repayed in the pause action. and in case of resume just re-borrow again.