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

Malicious actors can stuck the state of the vault on `processDepositCancellation()`.

Rozzo

Summary

In the processDepositCancellation() 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, yet the state will remain as Deposit. This will necessitate constant intervention by the protocol owners to resolve the situation.

Vulnerability Details

No penalty is imposed on malicious actors who interact with this. Furthermore, the absence of the try/catch pattern, which the protocol employs in most similar functions, provides us with less information about the status and error, while maintaining the state in Deposit.

function processDepositCancellation(GMXTypes.Store storage self) external {
GMXChecks.beforeProcessDepositCancellationChecks(self);
​
// Repay borrowed assets
GMXManager.repay(
self, self.depositCache.borrowParams.borrowTokenAAmt, self.depositCache.borrowParams.borrowTokenBAmt
);
​
// Return user's deposited asset
// If native token is being withdrawn, we convert wrapped to native
if (self.depositCache.depositParams.token == address(self.WNT)) {
self.WNT.withdraw(self.WNT.balanceOf(address(this)));
(bool success,) = self.depositCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");
} else {
// Transfer requested withdraw asset to user
IERC20(self.depositCache.depositParams.token).safeTransfer(
self.depositCache.user, self.depositCache.depositParams.amt
);
}
self.status = GMXTypes.Status.Open;
emit DepositCancelled(self.depositCache.user);
}

Impact

The potential need for the protocol to take drastic measures due to a state being stuck, which can occur with little to no penalty, is a concern that should be considered to ensure the protocol's robustness. Additionally, since the plan is to monitor failed events/tx with a sentinel and retry them, the unchanged status will also prevent automatic keepers from fixing this bug.

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. At the very least, implement a try-catch block to change the status to Deposit_Fail so that keepers are notified.

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.