incorrect handling for deposit failure leads to stuck at deposit_failed status .

Severity

High Risk

Relevant GitHub Links

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

Summary

When a deposit fails, the contract can become stuck in a deposit_failed status due to improper handling of debt repayment by swapping through the swapTokensForExactTokens() function.which leads to gas losses for keeper attempting to handle that and puts user deposits at risk.

Vulnerability Details

• In case of a user making a deposit to the strategy, it will create a deposit in GMX. After a successful deposit, GMX will call the callback function afterDepositExecution, and the callback function will call processDeposit.
• If the processDeposit() fails in the try call for any reason, the function will catch that and set the status to deposit_failed. An event will be emitted so the keeper can handle it.

  function processDeposit(GMXTypes.Store storage self) external {
       // some code ..
>>      try GMXProcessDeposit.processDeposit(self) {
           // ..more code
        } catch (bytes memory reason) {
>>            self.status = GMXTypes.Status.Deposit_Failed;

            emit DepositFailed(reason);
        }
    }

• The keeper calls the function processDepositFailure(). This function initiates a requestWithdraw to GMX to remove the liquidity added by the user deposit (+ the borrowed amount).
• After executing the removeLiquidity, the callback function afterWithdrawalExecution is triggered. and since the status is deposit_failed, it invokes the function processDepositFailureLiquidityWithdrawal.
• In processDepositFailureLiquidityWithdrawal, it first checks if a swap is necessary. If required, it swaps tokens to repay the debt.

  >>      (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 = block.timestamp;
 >>           GMXManager.swapTokensForExactTokens(self, _sp);
        }

• The problem arises if the swap revert if the tokenIn balance is insufficient to cover the _amountOut of _tokenOut, leading to a failed swap since the swap function is swapTokensForExactTokens. Consequently, the status remains deposit_failed and the callback revet.

  Note: The swap can fail for various reasons.

• In this scenario, the keeper can only invoke the processDepositFailure() function again. During the second call, it directly triggers processDepositFailureLiquidityWithdrawal since the lp tokens for the failed deposit has already been withdrawn.

 function processDepositFailure(GMXTypes.Store storage self, uint256 slippage, uint256 executionFee) external {
        GMXChecks.beforeProcessAfterDepositFailureChecks(self);

        GMXTypes.RemoveLiquidityParams memory _rlp;

        // If current gmx LP amount is somehow less or equal to amount before, we do not remove any liquidity
        if (GMXReader.lpAmt(self) <= self.depositCache.healthParams.lpAmtBefore) {
  >>          processDepositFailureLiquidityWithdrawal(self);
         //... more code
        }}

• The swap will always revert because the contract's balance of tokenIn will never be sufficient to cover the _amountOut of _tokenOut. Consequently, the status remains stuck at deposit_failed.

Impact

• The strategy remains stuck at the deposit_failed status, halting any further interactions with the protocol.
• Keepers lose gas for each call to processDepositFailure().
• Users may lose their deposits.

Tools Used

vs code manual review

Recommendations

• Utilize swapExactTokensForTokens and swap the remaining tokens from tokenIn after substracting debt need to be repaid of this token.for tokenOut.
• Implement safeguards to calculate the appropriate amount for swapping, avoiding potential reverting transactions. Here's an example of how to calculate the swap amount:

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