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

A malicious user can DOS the system leaving it in a perpetual status of "Deposit"

0xasen

Summary

A malicious user can basically DOS the whole system on demand and leave it perpetually in a "Deposit" status.

For that to happen, let's look at the user flow of depositing when addLiquidity to GMX fails and later we'll explain how we can force this.

The user flow would be:

  1. User calls deposit() on GMXVault, which sets Vault.Status = Deposit

  2. Borrowing of assets and transfer of tokens happens, then addLiquidity is called to GMX

  3. If addLiquidity fails then afterDepositCancellation on GMXCallback is called, which calls processDepositCancellation in GMXDeposit

This can be easily traced in the deposit sequence.

Vulnerability Details

Let's take a look at the processDepositCancellation in GMXDeposit:

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);
}

As you can see if the deposit token is native, then we're going to refund the user's native token and check for success via these lines:

(bool success, ) = self.depositCache.user.call{value: address(this).balance}("");
require(success, "Transfer failed.");

However, what a malicious user could do is deposit a native token through a smart contract that doesn't have a receive() function, for example, making the call always revert. In that case, the processDepositCancellation function can't be executed, and the status of the Vault can't be reset to Open, DOSing the system.

The user can maximize the chance of addLiquidity failing by specifying a very low slippage amount in his deposit params which would be slippage = minSlippage(of the vault). minSlippage here shouldn't be too high because otherwise, it'll be too restrictive for the users.

Given that slippage depends on liquidity, and liquidity depends on market conditions or it can also be influenced by flash loans, for example, I believe that the chance of a malicious user succeeding in his attempts to make addLiquidity fail is very high.

Also, he can try numerous times with the MINIMUM_VALUE deposit which is currently set as 9e16 in GMXChecks. 9e16 = 0.09ETH or AVAX, depending on which blockchain. 0.09 * 12 (price of AVAX) = ~1$

Impact

DOS of most of the functionality of the protocol. The status is set as "Deposit" when deposit() is called, and being unable to processDepositCancellation leaves the system in this status permanently.

Functions that are impacted:

Deposit will revert:

function beforeDepositChecks(
GMXTypes.Store storage self,
uint256 depositValue
) external view {
if (self.status != GMXTypes.Status.Open)
revert Errors.NotAllowedInCurrentVaultStatus();

Withdraw will revert:

function beforeWithdrawChecks(
GMXTypes.Store storage self
) external view {
if (self.status != GMXTypes.Status.Open)
revert Errors.NotAllowedInCurrentVaultStatus();

Rebalance will revert:

function beforeRebalanceChecks(
GMXTypes.Store storage self,
GMXTypes.RebalanceType rebalanceType
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Rebalance_Open
) revert Errors.NotAllowedInCurrentVaultStatus();

Compound will revert:

function beforeCompoundChecks(
GMXTypes.Store storage self
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Compound_Failed
) revert Errors.NotAllowedInCurrentVaultStatus();

Tools Used

Manual review

Recommendations

You can wrap the call in a try/catch like at other places in the code. Or, you could just credit his balance in an internal mapping and let him claim his assets from another function claimFailedDeposit.

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.