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

*strategy vault* can stuck at `Withdraw` status. if the balance of the user change in withdraw process

ElHaj

Summary

  • if the user balance of shares svToken decreased in the middle of withdraw process, the callback call from GMX will revert after Successful withdraw. and the stutas of the contract will stuck at Withdraw. with the withdrawed tokens.

Vulnerability Details

  • After a successful execution of the withdraw request in GMX , it will call the callback function afterWithdrawalExecution() the function then calls processWithdraw() in the GMXVault which delegate call to the processWithdraw() function in GMXWithdraw library.

function processWithdraw(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessWithdrawChecks(self);
try GMXProcessWithdraw.processWithdraw(self) {//@audit doesn't check the current share of the user..
// 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);
(bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));
​
// @audit-issue : if this revert the status will stuck at withdraw, and the token will stuck in the contract.
self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
​
self.status = GMXTypes.Status.Open;
​
emit WithdrawCompleted(
self.withdrawCache.user,
self.withdrawCache.withdrawParams.token,
self.withdrawCache.tokensToUser
);
} catch (bytes memory reason) {
self.status = GMXTypes.Status.Withdraw_Failed;
emit WithdrawFailed(reason);
}
}

  • as you can see the function first call processWithdraw() in GMXProcessWithdraw library with try. if this call executed Successfully it's then execute the code inside the try block.notice that in the try call , it does not check the current shares balance of the user.

  • The contract assumes that inside the try block, it shouldn't revert, since it doesn't have any Mechanism to handle the revert in this case and it will stuck in withdraw status. but that's not the case. the transaction can revert when try to burn shares from the user if the user balance not enough.

//.......
​
self.vault.burn(self.withdrawCache.user, self.withdrawCache.withdrawParams.shareAmt);
​
//.......

  • the contract only check the balance of the user in the first transaction to create a requestWithdraw. but because of the Two transaction mechanism of GMX , the user can send thier shares to another address ,or burn them or whatever... ,after the first tx which create the withdraw request. and in this case the callback will revert inside the try block.

  • NOTICE that GMX Executewithdraw transaction doesn't revert in case the callback call fail (revert), because it calls it inside try, catch blocks. if the callback revert, it get catched and only emmit an event, but the withdraw considered Successful and the withdrawed tokens already get sent to the receiver (GMXVault in our case).

  • when the contract in withdraw status only processWithdraw function can be called. even if the keeper call it it will get revert each time Cause insufficient user balance to burn

Impact

  • the contract will stuck at withdraw status. prevent any intrection with the protocol. (consider the need for rebalancing)

Tools Used

manual Review

Recommendations

  • consider to check the balance of shares of the user inside the try call, in processWithdraw from GMXProcessWithdraw library. if not enough then revert. so the status Successfully updated to Withdraw_Failed and the keeper can properly handle it.

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.

ElHaj Submitter
almost 2 years ago
hans Auditor
almost 2 years ago
ElHaj Submitter
almost 2 years ago
hans Lead Judge almost 2 years 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.