Incorrect Execution Fee Refund address on Failed Deposits or withdrawals in Strategy Vaults

Severity

Medium Risk

Relevant GitHub Links

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

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

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

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

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

Summary

The Strategy Vaults within the protocol use a two-step process for handling deposits/withdrawals via GMXv2. A createDeposit() transaction is followed by a callback function (afterDepositExecution() or afterDepositCancellation()) based on the transaction's success. In the event of a failed deposit due to vault health checks, the execution fee refund is mistakenly sent to the depositor instead of the keeper who triggers the deposit failure process.

Vulnerability Details

The protocol handles the deposit through the deposit function, which uses several parameters including an execution fee that refunds any excess fees.

function deposit(GMXTypes.DepositParams memory dp) external payable nonReentrant {
        GMXDeposit.deposit(_store, dp, false);
    }

struct DepositParams {
    // Address of token depositing; can be tokenA, tokenB or lpToken
    address token;
    // Amount of token to deposit in token decimals
    uint256 amt;
    // Minimum amount of shares to receive in 1e18
    uint256 minSharesAmt;
    // Slippage tolerance for adding liquidity; e.g. 3 = 0.03%
    uint256 slippage;
    // Execution fee sent to GMX for adding liquidity
    uint256 executionFee;
  }

The refund is intended for the message sender (msg.sender), which in the initial deposit case, is the depositor. This is established in the GMXDeposit.deposit function, where self.refundee is assigned to msg.sender.

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

        self.refundee = payable(msg.sender);

		...

		_dc.depositKey = GMXManager.addLiquidity(self, _alp);

        self.depositCache = _dc;

        emit DepositCreated(_dc.user, _dc.depositParams.token, _dc.depositParams.amt);
    }

If the deposit passes the GMX checks, the afterDepositExecution callback is triggered, leading to vault.processDeposit() to check the vault's health. A failure here updates the status to GMXTypes.Status.Deposit_Failed. The reversal process is then handled by the processDepositFailure function, which can only be called by keepers. They pay for the transaction's gas costs, including the execution fee.

function processDepositFailure(uint256 slippage, uint256 executionFee) external payable onlyKeeper {
        GMXDeposit.processDepositFailure(_store, slippage, executionFee);
    }

In GMXDeposit.processDepositFailure, self.refundee is not updated, resulting in any excess execution fees being incorrectly sent to the initial depositor, although the keeper paid for it.

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

        GMXTypes.RemoveLiquidityParams memory _rlp;

        // If current 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);
        } else {
            // Remove only the newly added LP amount
            _rlp.lpAmt = GMXReader.lpAmt(self) - self.depositCache.healthParams.lpAmtBefore;

            // If delta strategy is Long, remove all in tokenB to make it more
            // efficent to repay tokenB debt as Long strategy only borrows tokenB
            if (self.delta == GMXTypes.Delta.Long) {
                address[] memory _tokenASwapPath = new address[](1);
                _tokenASwapPath[0] = address(self.lpToken);
                _rlp.tokenASwapPath = _tokenASwapPath;

                (_rlp.minTokenAAmt, _rlp.minTokenBAmt) = GMXManager.calcMinTokensSlippageAmt(
                    self, _rlp.lpAmt, address(self.tokenB), address(self.tokenB), slippage
                );
            } else {
                (_rlp.minTokenAAmt, _rlp.minTokenBAmt) = GMXManager.calcMinTokensSlippageAmt(
                    self, _rlp.lpAmt, address(self.tokenA), address(self.tokenB), slippage
                );
            }

            _rlp.executionFee = executionFee;

            // Remove liqudity
            self.depositCache.withdrawKey = GMXManager.removeLiquidity(self, _rlp);
        }

The same issue occurs in the processWithdrawFailure function where the excess fees will be sent to the initial user who called withdraw instead of the keeper.

Impact

This flaw causes a loss of funds for the keepers, negatively impacting the vaults. Users also inadvertently receive extra fees that are rightfully owed to the keepers

Tools Used

manual analysis

Recommendations

The processDepositFailure and processWithdrawFailure functions must be modified to update self.refundee to the current executor of the function, which, in the case of deposit or withdraw failure, is the keeper.

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

        GMXTypes.RemoveLiquidityParams memory _rlp;

		self.refundee = payable(msg.sender);

		...
        }

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

	self.refundee = payable(msg.sender);

	...
  }