Yield Contracts - Mevvy

The HighestAPYUnbondedStrategy contract implements a system where deposits are routed to protocols with the highest yield while respecting TVL (Total Value Locked) limits for each protocol. However, when a user deposits funds with useDefaultProtocol set to true, via the deposit() function, the contract calls the internal _deposit() function, which directly deposits to the default protocol without checking if this deposit would exceed the protocol's TVL limit:

function _deposit(uint256 amount, address protocol) internal virtual {
  require(_isProtocolRegistered[protocol], "Protocol not registered");
  require(amount > 0, "Invalid deposit amount");

  address asset = _getUnderlyingAsset();
  IProtocolBaseAdapter(protocol).supplyTo(asset, amount, address(this));
  emit SupplyToProtocol(protocol, amount);
}

In contrast, when useDefaultProtocol is false, the contract calls _depositToProtocols() which does properly check and respect TVL limits.

This inconsistency creates a mechanism to bypass the TVL limits entirely by simply setting useDefaultProtocol to true. This could lead to excessive concentration of funds in a single protocol, undermining the risk management purpose of TVL limits and potentially resulting in suboptimal yield distribution.

Modify the _deposit function to include TVL limit checks similar to those in _depositToProtocols, or modify the deposit function to calculate the maximum allowable deposit to the default protocol and handle any excess amount.

RISK ACCEPTED: The Mevvy team made a business decision to accept the risk of this finding and not alter the contracts, stating:

Business decision is to allow the default protocol to exceed TVL limits if users decide to deposit do it directly. In the next reconcile anything over its limit will be re-adjusted.

The _depositToProtocols() function in HighestAPYUnbondedStrategyBase contains a logical flaw that allows TVL limits to be completely bypassed on the first deposit. When the contract handles its first deposit (when totalAssets - amount == 0), it sets maxAllowed = amount without checking if this amount exceeds the protocol's TVL limit.

if (totalAssets - amount == 0) {
  // First ever deposit (no previous assets)
  maxAllowed = amount;
} else {
  // For all other cases, respect TVL limits
  maxAllowed = (totalAssets * protocolLimit) / MAX_BPS;
  if (protocolBalance < maxAllowed) {
    maxAllowed = maxAllowed - protocolBalance;
  } else {
    maxAllowed = 0;
  }
}

This creates a risk where an initial large deposit would be allocated entirely to a single protocol, regardless of the TVL limit set for that protocol. This undermines the risk management purpose of TVL limits and could concentrate too much capital in one protocol.

Modify the first deposit logic to respect TVL limits by calculating the maximum allowed amount based on the protocol's limit, even for first deposits.

SOLVED: The Mevvy team solved this finding in commit 2a481b8 by following the mentioned recommendation.

The withdrawFees() function in the HighestAPYUnbondedStrategy contract does not call _processYieldFees() before withdrawing accumulated fees, unlike other state-changing functions such as deposit(), withdraw(), and rebalance(). This inconsistency creates a significant vulnerability, where yield generated since the last user interaction is not properly accounted for in fee calculations.

function withdrawFees(uint256 amount, address recipient) external onlyRoleOrAdmin(ADMIN_ROLE) {
  require(amount <= _accumulatedFees, "Insufficient fees");

  _accumulatedFees -= amount;

  uint256 available = UNDERLYING.balanceOf(address(this));
  if (available < amount) {
    // Need to withdraw from protocols
    _withdrawOptimally(amount);

    // Double check we have enough after withdrawal
    require(UNDERLYING.balanceOf(address(this)) >= amount, "Withdrawal failed");
  }

  UNDERLYING.transfer(recipient, amount);

  _lastTotalAssets = getTotalAssets() + _accumulatedFees;
}

When yield is generated in the protocol, it should be recognized by calling _processYieldFees(), which calculates the fee portion of that yield and adds it to _accumulatedFees. Without this call, any yield generated between the last user interaction and an admin's fee withdrawal remains unprocessed, and the protocol permanently loses the fees it should have collected on this yield.

The function also updates _lastTotalAssets at the end, which resets the baseline for future yield calculations. This means that even if a subsequent operation calls _processYieldFees(), the system has already "forgotten" about the unprocessed yield, making the fee loss permanent and unrecoverable.

In the following scenario, lost fees are demonstrated:

    it("withdrawFees() permanently loses unprocessed yield fees", async function () {
      console.log("\n==== VULNERABILITY: Missing _processYieldFees() in withdrawFees() ====");

      // Setup - create initial deposit with fee percentage
      const FEE_PERCENTAGE = 2000; // 20% fee
      await strategy.setYieldFee(FEE_PERCENTAGE);

      // Initial deposit
      const INITIAL_DEPOSIT = ethers.parseUnits("1000", 6);
      await strategy.connect(user1).deposit(INITIAL_DEPOSIT, false);

      console.log("\n--- INITIAL STATE ---");
      console.log(`Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC`);
      console.log(`Accumulated Fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC`);
      console.log(`Internal _lastTotalAssets: (not directly visible but initialized to initial deposit)`);

      // First yield event
      console.log("\n--- FIRST YIELD EVENT (100 USDC) ---");
      const YIELD_AMOUNT = ethers.parseUnits("100", 6);
      await mockAavePool.simulateYield(strategyAddress, YIELD_AMOUNT);

      console.log(`After yield simulation:`);
      console.log(`Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC`);
      console.log(
        `Accumulated Fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC (unchanged - yield not processed yet)`
      );

      // Process first yield via rebalance
      console.log("\n--- PROCESSING FIRST YIELD VIA REBALANCE ---");
      await strategy.rebalance(LARGE_REBALANCE_AMOUNT);

      // Calculate expected fee from first yield
      const firstYieldFee = (YIELD_AMOUNT * BigInt(FEE_PERCENTAGE)) / 10000n;

      console.log(`After processing:`);
      console.log(`Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC`);
      console.log(
        `Accumulated Fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC (20% of first yield)`
      );
      console.log(`Expected fee: ${ethers.formatUnits(firstYieldFee, 6)} USDC`);
      console.log(
        `Internal _lastTotalAssets: updated to ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} + ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} = ${ethers.formatUnits((await strategy.getTotalAssets()) + (await strategy.getAccumulatedFees()), 6)}`
      );

      // Record balances before second yield
      const balanceBeforeSecondYield = await strategy.getTotalAssets();
      const feesBeforeSecondYield = await strategy.getAccumulatedFees();

      // Simulate second yield event without processing
      console.log("\n--- SECOND YIELD EVENT (100 USDC) ---");
      const SECOND_YIELD = ethers.parseUnits("100", 6);
      await mockAavePool.simulateYield(strategyAddress, SECOND_YIELD);

      console.log(`After yield simulation:`);
      console.log(
        `Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC (increased by second yield)`
      );
      console.log(
        `Accumulated Fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC (unchanged - second yield not processed yet)`
      );

      // Calculate what second yield fee should be
      const secondYieldFee = (SECOND_YIELD * BigInt(FEE_PERCENTAGE)) / 10000n;
      console.log(`Expected second yield fee (if processed): ${ethers.formatUnits(secondYieldFee, 6)} USDC`);

      // Admin withdraws all currently accumulated fees
      console.log("\n--- ADMIN WITHDRAWS ACCUMULATED FEES ---");
      console.log(`Before withdrawal:`);
      console.log(`Owner USDC balance: ${ethers.formatUnits(await mockUSDC.balanceOf(owner.address), 6)} USDC`);
      console.log(`Protocol accumulated fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC`);

      await strategy.connect(owner).withdrawFees(firstYieldFee, owner.address);

      console.log(`After withdrawal:`);
      console.log(
        `Owner USDC balance: ${ethers.formatUnits(await mockUSDC.balanceOf(owner.address), 6)} USDC (increased by fee amount)`
      );
      console.log(
        `Protocol accumulated fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC (should be zero)`
      );
      console.log(
        `Internal _lastTotalAssets: VULNERABILITY - updated to current total (${ethers.formatUnits(await strategy.getTotalAssets(), 6)}) without processing second yield`
      );

      // The key vulnerability explanation
      console.log("\n--- VULNERABILITY EXPLAINED ---");
      console.log(`1. Second yield of ${ethers.formatUnits(SECOND_YIELD, 6)} USDC was generated`);
      console.log(`2. Fees should have been ${ethers.formatUnits(secondYieldFee, 6)} USDC (20% of second yield)`);
      console.log(`3. withdrawFees() updated _lastTotalAssets WITHOUT processing the yield`);
      console.log(`4. This makes the system "forget" about the second yield`);
      console.log(`5. The ${ethers.formatUnits(secondYieldFee, 6)} USDC in fees are PERMANENTLY LOST`);

      // Let's process yields again via rebalance to prove fees are lost
      console.log("\n--- TRYING TO RECOVER LOST FEES VIA REBALANCE ---");
      await strategy.rebalance(LARGE_REBALANCE_AMOUNT);

      console.log(`After rebalance:`);
      console.log(`Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC (unchanged)`);
      console.log(
        `Accumulated Fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC (still zero - fees permanently lost)`
      );

      // PROOF: Add third yield and process it
      console.log("\n--- THIRD YIELD EVENT (100 USDC) ---");
      const THIRD_YIELD = ethers.parseUnits("100", 6);
      await mockAavePool.simulateYield(strategyAddress, THIRD_YIELD);

      console.log(`Before processing third yield:`);
      console.log(`Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC`);

      await strategy.rebalance(LARGE_REBALANCE_AMOUNT);

      const thirdYieldFee = (THIRD_YIELD * BigInt(FEE_PERCENTAGE)) / 10000n;
      console.log(`After processing third yield:`);
      console.log(`Total Assets: ${ethers.formatUnits(await strategy.getTotalAssets(), 6)} USDC`);
      console.log(
        `Accumulated Fees: ${ethers.formatUnits(await strategy.getAccumulatedFees(), 6)} USDC (only includes third yield's fees)`
      );
      console.log(`Expected third yield fee: ${ethers.formatUnits(thirdYieldFee, 6)} USDC`);

      console.log("\n--- SUMMARY OF LOST REVENUE ---");
      console.log(`Total protocol revenue lost: ${ethers.formatUnits(secondYieldFee, 6)} USDC`);
      console.log(`This revenue is permanently unrecoverable due to missing _processYieldFees() in withdrawFees()`);

      // Verify with assertions
      expect(await strategy.getAccumulatedFees()).to.equal(thirdYieldFee);
    });

Modify the withdrawFees() function to call the _processYieldFees() function at the beginning to ensure all accrued yield is properly accounted for.

SOLVED: The Mevvy team solved this finding in commit 1e14a85 by following the mentioned recommendation.

When a user deposits funds via the deposit() function, the contract mints strategy tokens representing their share of the total assets. However, if all protocols have reached their TVL limits, the _depositToProtocols() function will silently leave unallocated funds in the contract balance without generating yield or notifying the user.

The issue occurs in the following sequence:

1. User deposits funds and receives strategy tokens.

2. The function attempts to allocate funds to protocols via _depositToProtocols() .

3. When calling _getBestEligibleProtocol(), if all protocols are at their TVL limits, it returns address(0) .

4. The function simply returns with if (bestProtocol == address(0)) return; .

5. If allocation is partial, the recursive rebalancing will stop when no eligible protocols remain.

This creates a situation where users believe their entire deposit is generating yield, when in reality a significant portion may be sitting idle in the contract.

Consider one or more of the following recommendations to explicitly handle cases where deposits cannot be fully allocated:

1. Return unallocated funds to the user and mint strategy tokens only for the successfully allocated amount.

2. Emit an event when funds cannot be fully allocated to inform users and monitoring systems.

3. Consider returning a success ratio from the deposit function to indicate what percentage was successfully deployed.

4. Add a function to check the percentage of idle funds in the strategy.

5. Implement a "waiting list" mechanism that automatically deploys idle funds when TVL capacity becomes available.

6. Allow users to optionally reject deposits that cannot be fully allocated.

SOLVED: The Mevvy team solved this finding in commit aaa4675 by following the mentioned recommendation of emitting an event when funds cannot be fully allocated to inform users and monitoring systems.

The _depositToProtocols() function in the HighestAPYUnbondedStrategyBase contract contains a special case meant to handle first deposits. However, the detection mechanism is flawed, as it only checks if totalAssets - amount == 0. This condition bypasses TVL limits by setting maxAllowed = amount, allowing deposits to a single protocol without restrictions.

if (totalAssets - amount == 0) {
  // First ever deposit (no previous assets)
  maxAllowed = amount;
} else {
  // For all other cases, respect TVL limits
  maxAllowed = (totalAssets * protocolLimit) / MAX_BPS;
  if (protocolBalance < maxAllowed) {
    maxAllowed = maxAllowed - protocolBalance;
  } else {
    maxAllowed = 0;
  }
}

This special case is triggered not only during the first-ever deposit but also after all funds have been withdrawn from the strategy. As a result, a deposit made after a complete withdrawal will ignore the configured TVL limits, potentially concentrating all funds in a single protocol regardless of the risk parameters set by the protocol administrators.

Apply TVL limits consistently across all deposit scenarios, including after full withdrawals. Instead of bypassing TVL limits when totalAssets - amount == 0, the protocol should either:

1. Implement a dedicated variable to track the true first-deposit scenario rather than relying on asset calculations, or

2. Simply apply the same TVL limit calculation logic uniformly for all deposits, ensuring that risk parameters are respected regardless of the strategy's deposit/withdrawal history.

This ensures that risk diversification policies established by the protocol administrators remain in effect throughout the lifecycle of the strategy, preventing unexpected concentration of funds in a single protocol.

SOLVED: The Mevvy team solved this finding in commit 2a481b8 by following the mentioned recommendation.

Several contracts in scope inherit the OwnableUpgradeable contract implementation from OpenZeppelin's library, which is used to restrict access to certain functions to the contract owner. The OwnableUpgradeable pattern allows the contract owner to transfer ownership to another address using the transferOwnership() function. However, the transferOwnership() function does not include a two-step process to transfer ownership.

Regarding this, it is crucial that the address to which ownership is transferred is verified to be active and willing to assume ownership responsibilities. Otherwise, the contract could be locked in a situation where it is no longer possible to make administrative changes to it.

Additionally, the renounceOwnership() function allows renouncing to the owner permission. Renouncing ownership before transferring it would result in the contract having no owner, eliminating the ability to call privileged functions.

Consider using OpenZeppelin's Ownable2StepUpgradeable contract over the OwnableUpgradeable implementation. Ownable2StepUpgradeable provides a two-step ownership transfer process, which adds an extra layer of security to prevent accidental ownership transfers.

Additionally, it is recommended that the owner cannot call the renounceOwnership() function to avoid losing ownership of the contract.

SOLVED: The Mevvy team solved this finding in commits 3c69f48 and 31aa264 by following the mentioned recommendations.

Throughout the files in scope, there are several instances where direct ERC20 operations are performed without using safety wrappers. This functionality assumes that all ERC20 token transfers comply with the IERC20 standard, which is not always the case. For example, USDT's transfer and transferFrom functions do not return a boolean as required by the standard. Other problematic implementations include tokens that revert on zero-amount transfers and fee-on-transfer tokens that result in the received amount being less than the expected amount.

If non-standard ERC20 tokens are used with these contracts, the operations may fail unexpectedly or silently, potentially breaking core protocol functionality and endangering user funds.

Replace all direct ERC20 token operations with OpenZeppelin's SafeERC20 library. This library provides wrapper functions that properly handle the edge cases of non-standard ERC20 implementations.

SOLVED: The Mevvy team solved this finding in commit 1b2c320 by following the mentioned recommendation.

When removing a protocol using removeProtocol(), the function fails to check if the protocol being removed is currently set as the defaultProtocol.

  function removeProtocol(address protocol) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _removeProtocol(protocol);
    UNDERLYING.approve(protocol, 0);
    address cToken = IProtocolBaseAdapter(protocol).cTokens(address(UNDERLYING));
    if (cToken != address(0)) {
      IERC20(cToken).approve(protocol, 0);
    }
    _updateIndex();
  }

If an admin removes the current default protocol, the defaultProtocol variable will continue pointing to an invalid protocol address. This creates an inconsistent state where the protocol is officially unregistered (with _isProtocolRegistered[protocol] = false), but it's still referenced as the default protocol. When users later call deposit(amount, true), the transaction will revert with "Protocol not registered" because the _deposit() function tries to use the now-unregistered protocol.

The removeProtocol() function should check if the protocol being removed is currently set as the default protocol and reset it to address(0) if so.

SOLVED: The Mevvy team solved this finding in commit bdcb598 by following the mentioned recommendation.

Several contracts in scope follow the upgradeable pattern, inheriting from the Initializable module from OpenZeppelin. In order to prevent leaving the contracts uninitialized, OpenZeppelin's documentation recommends adding the _disableInitializers function in the constructor to automatically lock the contracts when they are deployed. However, the contract is missing this function call.

This omission can lead to potential security vulnerabilities, as an uninitialized implementation contract can be taken over by an attacker, which may impact the proxy.

Add a constructor and call the _disableinitializers() method within the contracts to prevent the implementation from being initialized. For example:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
  _disableInitializers();
}

For more reference, see OpenZeppelin's recommendation from their documentation, or this discussion from their forum.

SOLVED: The Mevvy team solved this finding in commit 68d8ece by following the mentioned recommendation.

In the HighestAPYUnbondedStrategy contract, the blockCompound() function contains incorrect logic that doesn't match its intended purpose. The function is meant to revoke permissions for a Compound adapter to interact with a Comet contract. However, it's currently calling IComet(cToken).allow(compoundAdapterAddress, true) which grants permissions instead of revoking them. This is the same behavior as the whitelistCompound() function, making blockCompound() ineffective.

This issue could lead to security vulnerabilities since administrators would believe they've revoked access for a Compound adapter when in fact they've granted or maintained that access. In scenarios where access needs to be urgently revoked due to a security concern, this bug would prevent that from happening, potentially leading to loss of funds.

Change the boolean parameter in the blockCompound() function from true to false to ensure the function properly revokes permissions.

SOLVED: The Mevvy team solved this finding in commit 6b1910b by following the mentioned recommendation.

The contracts in scope, despite having admin controls and upgradeability features, lack a circuit breaker (pause) mechanism that would allow administrators to temporarily suspend critical operations in emergency situations.

The absence of a pause mechanism could lead to possible financial losses if a bug or exploit is discovered and cannot be immediately contained.

Consider integrating OpenZeppelin's PausableUpgradeable contract to add pausing/unpausing functionality for critical functions, restricting these abilities to admin roles.

This would significantly improve the contract's ability to handle emergency situations while maintaining transparency and control.

SOLVED: The Mevvy team solved this finding in commit 132ee34 by following the mentioned recommendation.

The withdrawFees() function updates _lastTotalAssets but does not call _updateIndex(), unlike other functions that modify the protocol's asset state.

function withdrawFees(uint256 amount, address recipient) external onlyRoleOrAdmin(ADMIN_ROLE) {
  require(amount <= _accumulatedFees, "Insufficient fees");

  _accumulatedFees -= amount;

  uint256 available = UNDERLYING.balanceOf(address(this));
  if (available < amount) {
    // Need to withdraw from protocols
    _withdrawOptimally(amount);

    // Double check we have enough after withdrawal
    require(UNDERLYING.balanceOf(address(this)) >= amount, "Withdrawal failed");
  }

  UNDERLYING.transfer(recipient, amount);

  _lastTotalAssets = getTotalAssets() + _accumulatedFees;
}

Without this update, the index would be out of sync with the actual asset backing, potentially leading to incorrect token valuations after fee withdrawals.

To ensure consistency and accurate token valuation, call the _updateIndex() function at the end of the withdrawFees() function. This will align the behavior with other asset-modifying functions and ensure the liquidity index accurately reflects the protocol's state after fees are withdrawn.

SOLVED: The Mevvy team solved this finding in commit 0a4a256 by following the mentioned recommendation.

Throughout the code in scope, there are several instances of unoptimized for loop declarations that incur higher gas costs than necessary. These inefficiencies typically manifest as array length calculations in each iteration, unnecessary storage variable reads or updates, lack of unchecked increments for counters that cannot overflow, amongst others.

Such patterns significantly increase gas consumption during execution, particularly for functions that process large arrays or are called frequently. This results in higher transaction costs for users interacting with the protocol and, in extreme cases, could lead to functions reaching the block gas limit, effectively causing denial of service when the protocol handles larger datasets.

Optimize the for loop declarations to reduce gas costs. Best practices include:

• The non-redundant initialization of the iterator with a default value (declaring simply i is equivalent to i = 0 but more gas efficient),

• The use of the pre-increment operator (inside an unchecked block if using Solidity >=0.8.0 and <= 0.8.21 :unchecked {++i}, or simply ++i if compiling with Solidity >=0.8.22).

Additionally, when reading from storage variables, it is recommended to reduce gas costs significantly by caching the array to read locally and iterate over it to avoid reading from storage on every iteration. Moreover, if there are several loops in the same function, the i variable can be re-used, to be able to set the value from non-zero to zero and reduce gas costs without additional variable declaration. For example:

uint256[] memory arrayInMemory = arrayInStorage;
uint256 i;

for (uint256 i; i < arrayInMemory.length ; ++i) {
  // code logic
}

delete i;
uint256[] memory arrayInMemory2 = arrayInStorage2;

for (; i < arrayInMemory2.length ; ++i) {
  // code logic
}

SOLVED: The Mevvy team solved this finding in commit da8e0b5 by following the mentioned recommendation.

The withdrawFees function in HighestAPYUnbondedStrategy does not implement the nonReentrant modifier that is consistently applied to other external functions in the contract. While this function is protected by the onlyRoleOrAdmin(ADMIN_ROLE) modifier and can only be called by trusted administrators, it represents an inconsistency in the contract's security pattern.

The function performs external calls within its execution flow, including potential interactions with protocol adapters through _withdrawOptimally and transferring tokens to an external recipient. Although the admin-only restriction significantly reduces the risk, maintaining consistent security patterns across similar functions is a best practice in secure contract development.

For code consistency and to align with the defensive programming approach used throughout the rest of the contract, consider adding the nonReentrant modifier to the withdrawFees() function.

SOLVED: The Mevvy team solved this finding in commit 8a259ee by following the mentioned recommendation.

Throughout the contracts in scope, there are several instances where administrative functions change contract state by modifying core state variables without them being reflected in event emissions.

The absence of events may hamper effective state tracking in off-chain monitoring systems.

Emit events for all state changes that occur as a result of administrative functions to facilitate off-chain monitoring of the system.

SOLVED: The Mevvy team solved this finding in commit af549c9 by following the mentioned recommendation.

Several function calls throughout the contracts in scope ignore the return values of the functions they call.

Ignoring return values is a common anti-pattern that may obscure important error conditions, making it difficult to detect failures, debug issues, or ensure operations completed as expected. This can ultimately compromise the reliability and security of the system.

Ensure that the return values of external calls are handled appropriately throughout the contracts.

ACKNOWLEDGED: The Mevvy team made a business decision to acknowledge this finding and not alter the contracts stating:

The particular return values are left out intentionally, as they don't provide any use.

The withdrawFees() function in the strategy contract uses an inconsistent pattern when withdrawing assets from protocols compared to the withdraw function. When the contract doesn't have enough funds to cover a fee withdrawal, it calls _withdrawOptimally(amount) with the full withdrawal amount, while the withdraw function correctly calls _withdrawOptimally(amount - available) with only the needed difference.

Although the _withdrawOptimally() function internally recalculates the needed amount, passing the full amount is inefficient and inconsistent with the pattern used elsewhere in the codebase.

Update the withdrawFees() function to match the pattern used in the withdraw function by only passing the needed difference to _withdrawOptimally.

SOLVED: The Mevvy team solved this finding in commit cd86b8e by following the mentioned recommendation.

Some functions throughout the contracts in scope lack input validation.

Instances of this issue include:

• HighestAPYUnbondedStrategyBase._setProtocolTvlLimit()

• HighestAPYUnbondedStrategyBase._setYieldFee()

Add appropriate input validation to ensure parameters fall within reasonable ranges:

1. For _setProtocolTvlLimit(), validate that limitBps is greater than zero.

2. For _setYieldFee(), add an check to assert than the newFee falls within a valid threshold.

   
   

SOLVED: The Mevvy team solved this finding in commit dac7468 by following the mentioned recommendation.

Several functions of the contracts in scope have multiple modifiers, with one of them being nonReentrant which prevents reentrancy behavior on the functions. Ideally, the nonReentrant modifier should be the first one to prevent even the execution of other modifiers in case of reentrancy behavior.

In Solidity, if a function has multiple modifiers, they are executed in the order specified. If checks or logic of modifiers depend on other modifiers, this has to be considered in their ordering.

While there is currently no obvious vulnerability with nonReentrant not being the first modifier, placing it first ensures that all other modifiers are executed only if the call is non-reentrant. This is a safer practice and can prevent potential issues in future updates or unforeseen scenarios.

Switch modifier order to consistently place the nonReentrant modifier as the first one to run so that all other modifiers are executed only if the call is non-reentrant.

SOLVED: The Mevvy team solved this finding in commit 6714e00 by following the mentioned recommendation.

Some functions throughout the contracts in scope are currently defined with the public visibility modifier, even though the functions are not called from within the contract, resulting in higher gas costs than necessary.

Instances of this issue include:

• HighestAPYUnbondedStrategy.deposit()

Modify the public functions not used within the contracts with the external visibility modifier.

SOLVED: The Mevvy team solved this finding in commit 9719419 by following the mentioned recommendation.

Throughout the files in scope, there are several instances where revert strings are used over custom errors.

In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors. For example:

error ConditionNotMet();

if (!condition) revert ConditionNotMet();

or starting from Solidity 0.8.27 :

require(condition, ConditionNotMet());

For more references, see here and here.

SOLVED: The Mevvy team solved this finding in commit 96b722d by following the mentioned recommendation.

The project contains several unnamed mappings despite using a Solidity version that supports named mappings. Named mappings improve code readability and self-documentation by explicitly stating their purpose.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit. For example:

mapping(address myAddress => bool myBool) public myMapping;

SOLVED: The Mevvy team solved this finding in commit 729370c by following the mentioned recommendation.

Throughout the contracts in scope, there are several instances where magic numbers are used. Magic numbers are direct numerical or string values used in the code without any explanation or context. This practice can lead to code maintainability issues, potential errors, and difficulty in understanding the code's logic.

To improve the code’s readability and facilitate refactoring, consider defining a constant for every magic number, giving it a clear and self-explanatory name.

Alternatively, consider adding an inline comment explaining how the magic numbers are calculated or why they are chosen for complex values.

SOLVED: The Mevvy team solved this finding in commit e0fb94f by following the mentioned recommendation.

The contracts in scope use upgradeable patterns but they are currently missing storage gaps or namespaced storage pattern to manage its storage layout.

The usage of a storage gap was encouraged in OpenZeppelin Upgradeable Contracts up to version 4, where storage gaps (like the uint256[50] private __gap; array) are used to reserve storage slots for future variables, helping to prevent storage collisions during upgrades.

However, starting from OpenZeppelin Contracts version 5, the recommended best practice has shifted towards using Namespaced storage. Namespaced storage provides better isolation of storage variables, significantly reducing the risk of storage collisions, especially in complex inheritance structures common with upgradeable contracts.

Refactor the Staking contract to use Namespaced storage patterns, as recommended in OpenZeppelin Contracts v5.

Encapsulate the contract's storage variables within a struct and associate it with a unique storage slot, ensuring isolation from inherited contracts' storage.

SOLVED: The Mevvy team solved this finding in commit 6a77b17 by following the mentioned recommendation.

Several files throughout the scope uses floating pragma versions ^0.8.20 which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.20, and less than 0.9.0.

However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Additionally, from Solidity versions 0.8.20 through 0.8.24, the default target EVM version is set to Shanghai, which results in the generation of bytecode that includes PUSH0 opcodes. Starting with version 0.8.25, the default EVM version shifts to Cancun, introducing new opcodes for transient storage, TSTORE and TLOAD.

In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.

Lock the pragma version to the same version used during development and testing.

Make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above if deploying to chains that may not support newly introduced opcodes. Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

SOLVED: The Mevvy team solved this finding in commit 596c649 by following the mentioned recommendation.

The HighestAPYUnbondedStrategyBase contract contains several inefficiencies that increase gas consumption and reduce code maintainability. These issues include redundant variable initializations, repetitive validation checks that could be converted to modifiers, and an inefficient recursive implementation.

Specifically, there are multiple instances where variables are explicitly initialized to their default values (such as address nextBestProtocol = address(0), uint256 bestAPY = 0, and bool isExcluded = false), which consumes unnecessary gas since Solidity automatically initializes variables to these default values.

Additionally, many functions repeat the same validation check (require(_isProtocolRegistered[protocol], "Protocol not registered")), which creates code duplication and increases deployment costs.

Finally, the _recursiveRebalance function uses a recursive approach that increases gas costs and could potentially cause stack depth issues if many protocols are used.

1. Remove redundant initializations of variables to their default values.

   
   

2. Consider creating a modifier for repeated protocol validation.

   
   

3. Consider refactoring the _recursiveRebalance() function to use an iterative loop approach instead of recursion to reduce gas costs and avoid potential stack depth issues.

These optimizations would reduce gas costs during deployment and execution while improving code maintainability and readability.

SOLVED: The Mevvy team solved this finding in commit d9f6974 by following the mentioned recommendation.