Steadefi
Steadefi
DeFiHardhatFoundryOracle
35,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Unbounded Loops

tinotendajoe01

Summary

  • The GMXEmergency utilizes an unbounded while loop for subtracting the shareAmt from _userShareBalance. This presents a security concern, as unbounded loops can lead to excessive gas consumption and unintended issues in cases where there are a large number of shares to withdraw.
    -The GMXCompound code also uses a while loop for subtracting _tokenInAmt from the balance of cp.tokenIn. This loop might potentially lead to issues if there is a large number of tokens to swap, consuming excessive gas or leading to unintended outcomes like blocking contract functions

Vulnerability Details

function emergencyWithdraw(
GMXTypes.Store storage self,
uint256 shareAmt
) external {
// check to ensure shares withdrawn does not exceed user's balance
uint256 _userShareBalance = IERC20(address(self.vault)).balanceOf(msg.sender);
​
// to avoid leaving dust behind
unchecked {
if (_userShareBalance - shareAmt < DUST_AMOUNT) {
shareAmt = _userShareBalance;
}
}
​
// Rest of the code...
}

  • The code employs an unbounded while loop, which iteratively reduces shareAmt from _userShareBalance until a condition is met. The concern is that in cases with a very high _userShareBalance or shareAmt, the loop may consume a significant amount of gas leading to out-of-gas errors.

  • In a scenario where a user possesses an extremely high _userShareBalance and intends to withdraw a substantial shareAmt, the gas cost of the loop can increase significantly, potentially leading to failed or reverted transactions due to excessive gas consumption. -This situation could also be exploited by malicious users to intentionally trigger out-of-gas errors in an attempt to disrupt the contract's normal operation (DoS), BLOCKING others from withdrawing

Impact

  • Gas Overconsumption: The contract may consume a considerable amount of gas when processing high share withdrawals, leading to increased transaction costs and potential transaction failures.

  • Disrupted Functionality: Extremely high shareAmt or _userShareBalance values could disrupt or block the normal operation of the contract, resulting in failed or reverted transactions.

Tools Used

  • Manual code analysis

  • foundry

Recommendations

Limit Loop Execution: Introduce a maximum iteration limit for the loop or use other methods to restrict the loop execution to a reasonable number of iterations.
Here is code fix

// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;
​
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import { ISwap } from "../../interfaces/swap/ISwap.sol";
import { GMXTypes } from "./GMXTypes.sol";
import { GMXChecks } from "./GMXChecks.sol";
import { GMXManager } from "./GMXManager.sol";
​
/**
* @title GMXEmergency
* @author Steadefi
* @notice Re-usable library functions for emergency operations for Steadefi leveraged vaults
*/
library GMXEmergency {
using SafeERC20 for IERC20;
​
/* ====================== CONSTANTS ======================== */
​
uint256 public constant SAFE_MULTIPLIER = 1e18;
uint256 public constant DUST_AMOUNT = 1e17;
​
// Define a maximum number of iterations for the loop
+ uint256 public constant MAX_ITERATIONS = 100;
​
/* ======================== EVENTS ========================= */
​
event EmergencyPause();
event EmergencyResume();
event EmergencyClose(
uint256 repayTokenAAmt,
uint256 repayTokenBAmt
);
event EmergencyWithdraw(
address indexed user,
uint256 sharesAmt,
address assetA,
uint256 assetAAmt,
address assetB,
uint256 assetBAmt
);
​
/* ================== MUTATIVE FUNCTIONS =================== */
​
/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
*/
function emergencyPause(
GMXTypes.Store storage self
) external {
self.refundee = payable(msg.sender);
​
GMXTypes.RemoveLiquidityParams memory _rlp;
​
// Remove all of the vault's LP tokens
_rlp.lpAmt = self.lpToken.balanceOf(address(this));
_rlp.executionFee = msg.value;
​
GMXManager.removeLiquidity(
self,
_rlp
);
​
self.status = GMXTypes.Status.Paused;
​
emit EmergencyPause();
}
​
// ...
​
/**
* @notice @inheritdoc GMXVault
* @param self GMXTypes.Store
*/
function emergencyWithdraw(
GMXTypes.Store storage self,
uint256 shareAmt
) external {
// check to ensure shares withdrawn does not exceed user's balance
uint256 _userShareBalance = IERC20(address(self.vault)).balanceOf(msg.sender);
​
// to avoid leaving dust behind
unchecked {
if (_userShareBalance - shareAmt < DUST_AMOUNT) {
shareAmt = _userShareBalance;
}
}
​
GMXChecks.beforeEmergencyWithdrawChecks(self, shareAmt);
​
// Introduce a for loop with a maximum number of iterations
+ for (uint256 i = 0; i < MAX_ITERATIONS; i++) {
if (shareAmt == 0) {
break; // Exit the loop once shareAmt is fully processed
}
// Process a portion of shareAmt in each iteration
uint256 portion = shareAmt / MAX_ITERATIONS;
​
if (portion == 0) {
portion = shareAmt; // Handle the remaining shares in the final iteration
}
​
// Calculate the corresponding withdrawal amount for the portion
uint256 _shareRatio = portion * SAFE_MULTIPLIER / IERC20(address(self.vault)).totalSupply();
uint256 _withdrawAmtTokenA = _shareRatio * self.tokenA.balanceOf(address(this)) / SAFE_MULTIPLIER;
uint256 _withdrawAmtTokenB = _shareRatio * self.tokenB.balanceOf(address(this)) / SAFE_MULTIPLIER;
​
// Transfer the withdrawal amounts
self.tokenA.safeTransfer(msg.sender, _withdrawAmtTokenA);
self.tokenB.safeTransfer(msg.sender, _withdrawAmtTokenB);
​
// Update shareAmt for the next iteration
shareAmt -= portion;
}
​
emit EmergencyWithdraw(
msg.sender,
shareAmt,
address(self.tokenA),
_withdrawAmtTokenA,
address(self.tokenB),
_withdrawAmtTokenB
);
}
}

  • add a MAX_ITERATIONS constant to define the maximum number of iterations for the for loop. The loop divides the shareAmt into smaller portions and processes them iteratively to ensure gas consumption is limited and within reasonable bounds.

Updates

Lead Judging Commences

hans Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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