Yield in trove is lost when closing a strategy vault

Severity

High Risk

Relevant GitHub Links

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

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

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXCompound.sol#L40-L53

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXChecks.sol#L341-L344

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#L111-L156

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXTrove.sol#L19-L41

Summary

The funds in the trove contract are not claimed during the emergency close flow and can not be claimed in a normal way during this situation, because of a status change. Therefore, all the acquired yield will be lost.

Vulnerability Details

When users deposit, or withdraw tokens, all acquired yield from GMX is sent to the trove contract:

function deposit(
  GMXTypes.Store storage self,
  GMXTypes.DepositParams memory dp,
  bool isNative
) external {
  // Sweep any tokenA/B in vault to the temporary trove for future compouding and to prevent
  // it from being considered as part of depositor's assets
  if (self.tokenA.balanceOf(address(this)) > 0) {
    self.tokenA.safeTransfer(self.trove, self.tokenA.balanceOf(address(this)));
  }
  if (self.tokenB.balanceOf(address(this)) > 0) {
    self.tokenB.safeTransfer(self.trove, self.tokenB.balanceOf(address(this)));
  }
	...
}

function withdraw(
  GMXTypes.Store storage self,
  GMXTypes.WithdrawParams memory wp
) external {
  // Sweep any tokenA/B in vault to the temporary trove for future compouding and to prevent
  // it from being considered as part of withdrawers assets
  if (self.tokenA.balanceOf(address(this)) > 0) {
    self.tokenA.safeTransfer(self.trove, self.tokenA.balanceOf(address(this)));
  }
  if (self.tokenB.balanceOf(address(this)) > 0) {
    self.tokenB.safeTransfer(self.trove, self.tokenB.balanceOf(address(this)));
  }
	...
}

The only way in the system to claim these yield is the compound function, which calls the beforeCompoundChecks function:

function compound(
  GMXTypes.Store storage self,
  GMXTypes.CompoundParams memory cp
) external {
  // Transfer any tokenA/B from trove to vault
  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))
    );
  }
	...
	GMXChecks.beforeCompoundChecks(self);
	...
}

This function reverts if the current status of the system is not Open or Compound_Failed:

function beforeCompoundChecks(
  GMXTypes.Store storage self
) external view {
  if (
    self.status != GMXTypes.Status.Open &&
    self.status != GMXTypes.Status.Compound_Failed
  ) revert Errors.NotAllowedInCurrentVaultStatus();
	...
}

As the emergency close flow updates this status to Paused and later to Closed, calling compound will revert:

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

function emergencyClose(
  GMXTypes.Store storage self,
  uint256 deadline
) external {
  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
  );
}

And as we can see during these process the funds inside the trove contract are never claimed and as the strategy vault is the only address that can claim the funds of the trove, all funds are gone.

contract GMXTrove {

  /* ==================== STATE VARIABLES ==================== */

  // Address of the vault this trove handler is for
  IGMXVault public vault;

  /* ====================== CONSTRUCTOR ====================== */

  /**
    * @notice Initialize trove contract with associated vault address
    * @param _vault Address of vault
  */
  constructor (address _vault) {
    vault = IGMXVault(_vault);

    GMXTypes.Store memory _store = vault.store();

    // Set token approvals for this trove's vault contract
    _store.tokenA.approve(address(vault), type(uint256).max);
    _store.tokenB.approve(address(vault), type(uint256).max);
  }
}

Impact

If a strategy vault is closed, all funds in the trove are lost.

Tools Used

Manual Review

Recommendations

Transfer the funds inside the trove into the vault during the emergency close process.