Front-Run Attacks Due Slippage Mishandling Lead to Total Losses For Depositors

Severity

High Risk

Relevant GitHub Links

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

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

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

Summary

• Depositor funds are at severe risk due to front-run attacks and slippage mishandling. An emergencyClose() can lead to complete investment loss, The same issue observed in other functions (processDepositFailureLiquidityWithdrawal and processWithdraw) but with less impact.

Vulnerability Details

• The emergencyClose() function repays all debt to the lendingVault contract, then sets the status to closed. Subsequently, users who deposited into the strategy contract can withdraw their deposits.

• If the contract balance of one token is insufficient to cover its debt, the contract swaps the other token for the necessary amount using swapTokensForExactTokens.

 function emergencyClose(GMXTypes.Store storage self, uint256 deadline) external {
        // some code ..
        //...
        // 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;
            // @audit-issue : this can be front-run to return
>>           GMXManager.swapTokensForExactTokens(self, _sp);
        }
       // more code ....
       //.....
    }

• Notice that the _sp struct includes the _sp.slippage parameter. Here, _sp.amountIn represents the entire contract balance of tokenFrom, while _sp.amountOut represent only the necessary amount to settle the debt of tokenTo. This function is vulnerable to front-running if the contract's tokenFrom balance exceeds the required amount to swap for amountOut of tokenTo. In such cases, all remaining tokens from _tokenFrom can be stolen in a front-run attack, as the _sp.slippage parameter has no effect in this scenario and we can see that here :

1. the contract first call GMXManager.swapTokensForExactTokens(self, _sp)

 function swapTokensForExactTokens(GMXTypes.Store storage self, ISwap.SwapParams memory sp)
     external
     returns (uint256)
 {
     if (sp.amountIn > 0) {
         return GMXWorker.swapTokensForExactTokens(self, sp);
     } else {
         return 0;
     }
 }

2. then call GMXWorker.swapTokensForExactTokens(self, sp);

 function swapTokensForExactTokens(GMXTypes.Store storage self, ISwap.SwapParams memory sp)
     external
     returns (uint256)
 {
     IERC20(sp.tokenIn).approve(address(self.swapRouter), sp.amountIn);

     return self.swapRouter.swapTokensForExactTokens(sp);
 }

3. then call self.swapRouter.swapTokensForExactTokens(sp)

  function swapTokensForExactTokens(ISwap.SwapParams memory sp) external returns (uint256) {
     IERC20(sp.tokenIn).safeTransferFrom(msg.sender, address(this), sp.amountIn);

     IERC20(sp.tokenIn).approve(address(router), sp.amountIn);

     ISwapRouter.ExactOutputSingleParams memory _eosp = ISwapRouter.ExactOutputSingleParams({
         tokenIn: sp.tokenIn,
         tokenOut: sp.tokenOut,
         fee: fees[sp.tokenIn][sp.tokenOut],
         recipient: address(this),
         deadline: sp.deadline,
         amountOut: sp.amountOut,
         amountInMaximum: sp.amountIn,
         sqrtPriceLimitX96: 0
     });

     uint256 _amountIn = router.exactOutputSingle(_eosp);

     // Return sender back any unused tokenIn
     IERC20(sp.tokenIn).safeTransfer(msg.sender, IERC20(sp.tokenIn).balanceOf(address(this)));
     IERC20(sp.tokenOut).safeTransfer(msg.sender, IERC20(sp.tokenOut).balanceOf(address(this)));

     return _amountIn;
 }

• notice that the function swapTokensForExactTokens calls exactOutputSingle from uniswap router and the _sp.slipage have no effect .

• the vault faces a significant risk, losing all remaining assets, which primarily consist of depositors' funds. This vulnerability is exacerbated by the Vault contract withdrawing liquidity from GMX without performing any swaps during the emergencyPause action, leaving withdrawed liquidity in terms of both tokens and making swaps in emergencyClose will be almost always necessary.

• The same vulnerability also exists in the processDepositFailureLiquidityWithdrawal and processWithdraw functions. However, the impact in the emergencyClose() function is significantly more severe compared to these cases. but the vulnerability is the same.

POC

consider the following sinario :
assume that The vault is a long vault has a debt of 10000 tokenA.

1. When the contract paused, it withdrawed 5000 tokenA and 10000 tokenB. so The contract still needs 5000 tokenA to cover all the debt.

2. The contract attempts to swap a maximum of tokenB to obtain the required 5000 tokenA. Typically, this swap requires 6000 tokenB to yield 5000 tokenA. After the swap, there are 4000 tokenB left, representing users deposits. These deposits can only be redeemed after the debt is settled.

3. The amountIn parameter is set to 10000 tokenB. A malicious actor exploit this by front-running the swap, inflating the price. The contract ends up swapping 10000 tokenB for 5000 tokenA and repays the debt.

4. After the swap and debt repayment, the vault's balance becomes zero, leaving no funds to cover user deposits.

NOTICE Depositors' funds in this case are always will be in terms of tokenFrom (or tokenB in our example), ensuring consistent losses.
- In a Neutral strategy, the attacker must keep enough tokenFromto the contract to pay tokenFrom debt to prevent transaction from revert and only take all the depositors funds.

Impact

• in emergencyClose() all depositors loses all thier deposits .
• in processDepositFailureLiquidityWithdrawal one user lose all his deposit (the failed deposit).
• in processWithdraw user have the ability to set the minTokenOut he will get but still possible that the user lose his funds if he misspass the right amount.

Tools Used

vs code
manual review

Recommendations

• after the swap. make sure that amountIn swapped for amountOut Get swapped for a reasonable price(in a specific buffer) using chainlink Oracle and if not revert.