incorrect handling of compound cancelation lead vault to stuck at compound_failed status

Severity

High Risk

Relevant GitHub Links

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

Summary

the compound function allows the keeper to swap a token for TokenA or TokenB and add it as liquidity to GMX. However, if the deposit get cancelled, the contract enters a compound_failed status. leading to a deadlock and preventing further protocol interactions.

Vulnerability Details

-The compound function is invoked by the keeper to swap a token held by the contract (e.g., from an airdrop as sponsor said) for TokenA or TokenB. Initially, it exchanges this token for either tokenA or tokenB and sets the status to compound. Then, it adds the swapped token as liquidity to GMX by creating a deposit:

  function compound(GMXTypes.Store storage self, GMXTypes.CompoundParams memory cp) external {lt
      if (self.tokenA.balanceOf(address(self.trove)) > 0) {
          self.tokenA.safeTransferFrom(address(self.trove), address(this), self.tokenA.balanceOf(address(self.trove)));
      }
      if (self.tokenB.balanceOf(address(self.trove)) > 0) {
          self.tokenB.safeTransferFrom(address(self.trove), address(this), self.tokenB.balanceOf(address(self.trove)));
      }

>>      uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));

      // Only compound if tokenIn amount is more than 0
      if (_tokenInAmt > 0) {
          self.refundee = payable(msg.sender); // the msg.sender is the keeper.

          self.compoundCache.compoundParams = cp; // storage update.

          ISwap.SwapParams memory _sp;

          _sp.tokenIn = cp.tokenIn;
          _sp.tokenOut = cp.tokenOut;
          _sp.amountIn = _tokenInAmt;
          _sp.amountOut = 0; // amount out minimum calculated in Swap
          _sp.slippage = self.minSlippage; // minSlipage may result to a revert an cause the tokens stays in this contract.
          _sp.deadline = cp.deadline;

          GMXManager.swapExactTokensForTokens(self, _sp); // return value not checked.

          GMXTypes.AddLiquidityParams memory _alp;

          _alp.tokenAAmt = self.tokenA.balanceOf(address(this));
          _alp.tokenBAmt = self.tokenB.balanceOf(address(this));
          /// what this return in case zero balance?? zero
          self.compoundCache.depositValue = GMXReader.convertToUsdValue(
              self, address(self.tokenA), self.tokenA.balanceOf(address(this))
          ) + GMXReader.convertToUsdValue(self, address(self.tokenB), self.tokenB.balanceOf(address(this)));
          // revert if zero value, status not open or compound_failed , executionFee < minExecutionFee.
          GMXChecks.beforeCompoundChecks(self);

>>          self.status = GMXTypes.Status.Compound;

          _alp.minMarketTokenAmt =
              GMXManager.calcMinMarketSlippageAmt(self, self.compoundCache.depositValue, cp.slippage);

          _alp.executionFee = cp.executionFee;
>>          self.compoundCache.depositKey = GMXManager.addLiquidity(self, _alp);
      }

• In the event of a successful deposit, the contract will set the status to open again. However, if the deposit is cancelled, the callback will call processCompoundCancellation() function and the status will be set to compound_failed as shown in the following code:

  function processCompoundCancellation(GMXTypes.Store storage self) external {
        GMXChecks.beforeProcessCompoundCancellationChecks(self);
        self.status = GMXTypes.Status.Compound_Failed;

        emit CompoundCancelled();
    }

• The issue arises when the deposit is cancelled, and the status becomes compound_failed. In this scenario, only the compound function can be called again and only by the keeper, but the tokens have already been swapped for TokenA or TokenB (Because we successfully create a deposit in GMX that means the swap was successfull). Consequently, the amountIn will be zero, and in this case the compound logic will be skipped.

>>     uint256 _tokenInAmt = IERC20(cp.tokenIn).balanceOf(address(this));

        // Only compound if tokenIn amount is more than 0
>>      if (_tokenInAmt > 0) {
            //compound logic
            //....
        }

• As a result, the status will remain compound_failed, leading to a deadlock. If keeper continue to call this function, no progress will be made, only gas will be wasted. Furthermore, all interactions with the protocol are impossible since the status is compound_failed.

Impact

• strategy vault stuck at compond_failed status. prevent any interaction with the protocol
• keeper may waste a lot of gas trying to handle this situation .

Tools Used

manual review

Recommendations

• in the event of a deposit get cancelled when trying to compound. just add liquidity again without the swapping logic.