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

Malicious actors can cause grief to the protocol by forcing `self.status` to `GMXTypes.Status.Withdraw_Failed`.

Rozzo

Summary

In the processWithdraw() function, if self.depositCache.depositParams.token == address(self.WNT), there is a call to the user's address. If a malicious actor sets up a contract for the receiver that reverts, the execution will fail, and the catch will execute setting the state to Withdraw_Failed.

Vulnerability Details

No penalty is imposed on malicious actors who interact with this.

function processWithdraw(
GMXTypes.Store storage self
) external {
GMXChecks.beforeProcessWithdrawChecks(self);
​
// We transfer the core logic of this function to GMXProcessWithdraw.processWithdraw()
// to allow try/catch here to catch for any issues such as any token swaps failing or
// debt repayment failing, or any checks in afterWithdrawChecks() failing.
// If there are any issues, a WithdrawFailed event will be emitted and processWithdrawFailure()
// should be triggered to refund assets accordingly and reset the vault status to Open again.
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);
(bool success, ) = self.withdrawCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed."); // same problm as in deposit but we have a try cathc at least
} else {
// Transfer requested withdraw asset to user
IERC20(self.withdrawCache.withdrawParams.token).safeTransfer(
self.withdrawCache.user,
self.withdrawCache.tokensToUser
);
}
​
// Transfer any remaining tokenA/B that was unused (due to slippage) to user as well
self.tokenA.safeTransfer(self.withdrawCache.user, self.tokenA.balanceOf(address(this)));
self.tokenB.safeTransfer(self.withdrawCache.user, self.tokenB.balanceOf(address(this)));
​
// Burn user shares
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);
}
}

Impact

The ease with which a bad actor can set the status to failed, forcing keepers to retry the call or fix the issue, is too great.

Tools Used

Manual Review

Recommendations

Add penalties for malicious actors as a punitive measure or consider an alternative method for claiming tokens, such as a two-step process that involves transferring them first to the vault.

Updates

Lead Judging Commences

hans Lead Judge over 1 year 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.