Yield Contracts - Mevvy


Prepared by:

Halborn Logo

HALBORN

Last Updated 04/01/2025

Date of Engagement: March 26th, 2025 - April 1st, 2025

Summary

100% of all REPORTED Findings have been addressed

All findings

27

Critical

0

High

1

Medium

3

Low

7

Informational

16


1. Introduction

Mevvy engaged Halborn to conduct a security assessment on their smart contracts beginning on March 17th, 2025 and ending on March 21st, 2025. The security assessment was scoped to the smart contracts provided to Halborn. Commit hashes and further details can be found in the Scope section of this report.


The Mevvy codebase in scope consists of a strategy implementation that automatically routes funds to highest yielding protocols, including Aave, Compound and Morpho.

2. Assessment Summary

Halborn was provided 5 days for the engagement and assigned 1 full-time security engineer to review the security of the smart contracts in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.


The purpose of the assessment is to:

    • Identify potential security issues within the smart contracts.

    • Ensure that smart contract functionality operates as intended.


In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, which were mostly addressed by the Mevvy team. The main ones are the following:

    • The _withdrawOptimally() function should be updated to correctly handle its input parameter as the remaining amount needed from protocols, without recalculating or double-subtracting the contract's balance.

    • Modify the _deposit() function to include TVL limit checks similar to those in _depositToProtocols().

    • 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.


3. Test Approach and Methodology

Halborn performed a combination of manual review of the code and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of smart contracts and can quickly identify items that do not follow security best practices.

The following phases and associated tools were used throughout the term of the assessment:

    • Research into architecture, purpose and use of the platform.

    • Smart contract manual code review and walkthrough to identify any logic issue.

    • Thorough assessment of safety and usage of critical Solidity variables and functions in scope that could led to arithmetic related vulnerabilities.

    • Local testing with custom scripts (Foundry).

    • Fork testing against main networks (Foundry).

    • Static analysis of security for scoped contract, and imported functions (Slither).


4. Static Analysis Report

4.1 Description

Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their abis and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.


The security team assessed all findings identified by the Slither software, however, findings with related to external dependencies are not included in the below results for the sake of report readability.

4.2 Output

The findings obtained as a result of the Slither scan were reviewed, and many were not included in the report because they were determined as false positives.











5. RISK METHODOLOGY

Every vulnerability and issue observed by Halborn is ranked based on two sets of Metrics and a Severity Coefficient. This system is inspired by the industry standard Common Vulnerability Scoring System.
The two Metric sets are: Exploitability and Impact. Exploitability captures the ease and technical means by which vulnerabilities can be exploited and Impact describes the consequences of a successful exploit.
The Severity Coefficients is designed to further refine the accuracy of the ranking with two factors: Reversibility and Scope. These capture the impact of the vulnerability on the environment as well as the number of users and smart contracts affected.
The final score is a value between 0-10 rounded up to 1 decimal place and 10 corresponding to the highest security risk. This provides an objective and accurate rating of the severity of security vulnerabilities in smart contracts.
The system is designed to assist in identifying and prioritizing vulnerabilities based on their level of risk to address the most critical issues in a timely manner.

5.1 EXPLOITABILITY

Attack Origin (AO):
Captures whether the attack requires compromising a specific account.
Attack Cost (AC):
Captures the cost of exploiting the vulnerability incurred by the attacker relative to sending a single transaction on the relevant blockchain. Includes but is not limited to financial and computational cost.
Attack Complexity (AX):
Describes the conditions beyond the attacker’s control that must exist in order to exploit the vulnerability. Includes but is not limited to macro situation, available third-party liquidity and regulatory challenges.
Metrics:
EXPLOITABILITY METRIC (mem_e)METRIC VALUENUMERICAL VALUE
Attack Origin (AO)Arbitrary (AO:A)
Specific (AO:S)
1
0.2
Attack Cost (AC)Low (AC:L)
Medium (AC:M)
High (AC:H)
1
0.67
0.33
Attack Complexity (AX)Low (AX:L)
Medium (AX:M)
High (AX:H)
1
0.67
0.33
Exploitability EE is calculated using the following formula:

E=meE = \prod m_e

5.2 IMPACT

Confidentiality (C):
Measures the impact to the confidentiality of the information resources managed by the contract due to a successfully exploited vulnerability. Confidentiality refers to limiting access to authorized users only.
Integrity (I):
Measures the impact to integrity of a successfully exploited vulnerability. Integrity refers to the trustworthiness and veracity of data stored and/or processed on-chain. Integrity impact directly affecting Deposit or Yield records is excluded.
Availability (A):
Measures the impact to the availability of the impacted component resulting from a successfully exploited vulnerability. This metric refers to smart contract features and functionality, not state. Availability impact directly affecting Deposit or Yield is excluded.
Deposit (D):
Measures the impact to the deposits made to the contract by either users or owners.
Yield (Y):
Measures the impact to the yield generated by the contract for either users or owners.
Metrics:
IMPACT METRIC (mIm_I)METRIC VALUENUMERICAL VALUE
Confidentiality (C)None (I:N)
Low (I:L)
Medium (I:M)
High (I:H)
Critical (I:C)
0
0.25
0.5
0.75
1
Integrity (I)None (I:N)
Low (I:L)
Medium (I:M)
High (I:H)
Critical (I:C)
0
0.25
0.5
0.75
1
Availability (A)None (A:N)
Low (A:L)
Medium (A:M)
High (A:H)
Critical (A:C)
0
0.25
0.5
0.75
1
Deposit (D)None (D:N)
Low (D:L)
Medium (D:M)
High (D:H)
Critical (D:C)
0
0.25
0.5
0.75
1
Yield (Y)None (Y:N)
Low (Y:L)
Medium (Y:M)
High (Y:H)
Critical (Y:C)
0
0.25
0.5
0.75
1
Impact II is calculated using the following formula:

I=max(mI)+mImax(mI)4I = max(m_I) + \frac{\sum{m_I} - max(m_I)}{4}

5.3 SEVERITY COEFFICIENT

Reversibility (R):
Describes the share of the exploited vulnerability effects that can be reversed. For upgradeable contracts, assume the contract private key is available.
Scope (S):
Captures whether a vulnerability in one vulnerable contract impacts resources in other contracts.
Metrics:
SEVERITY COEFFICIENT (CC)COEFFICIENT VALUENUMERICAL VALUE
Reversibility (rr)None (R:N)
Partial (R:P)
Full (R:F)
1
0.5
0.25
Scope (ss)Changed (S:C)
Unchanged (S:U)
1.25
1
Severity Coefficient CC is obtained by the following product:

C=rsC = rs

The Vulnerability Severity Score SS is obtained by:

S=min(10,EIC10)S = min(10, EIC * 10)

The score is rounded up to 1 decimal places.
SeverityScore Value Range
Critical9 - 10
High7 - 8.9
Medium4.5 - 6.9
Low2 - 4.4
Informational0 - 1.9

6. SCOPE

Files and Repository
(a) Repository: mevvy-yield-contracts
(b) Assessed Commit ID: 50762c5
(c) Items in scope:
  • contracts/core/libraries/WadRayMath.sol
  • contracts/protocols/aave/src/AaveAdapter.sol
  • contracts/protocols/compound/src/CompoundAdapter.sol
↓ Expand ↓
Out-of-Scope: Mocks and interfaces., Third party dependencies and economic attacks.
Remediation Commit ID:
Out-of-Scope: New features/implementations after the remediation commit IDs.

7. Assessment Summary & Findings Overview

Critical

0

High

1

Medium

3

Low

7

Informational

16

Security analysisRisk levelRemediation Date
Impossibility to withdraw assets across protocolsHighSolved - 03/21/2025
Missing TVL limit check in default protocol depositsMediumRisk Accepted - 03/26/2025
TVL limit bypass on first depositMediumSolved - 03/25/2025
Revenue loss due to missing fee processing in withdrawFees()MediumSolved - 03/23/2025
Unallocated funds without notification when protocols reach TVL limitsLowSolved - 03/25/2025
TVL limit bypass after complete withdrawalLowSolved - 03/25/2025
Single step ownership transfer processLowSolved - 03/26/2025
Use of non-compliant tokens may break contract functionalityLowSolved - 03/26/2025
Default protocol removal handling issueLowSolved - 03/26/2025
Missing _disableInitializers() function callLowSolved - 03/23/2025
Permission revocation failure in blockCompound functionLowSolved - 03/23/2025
Missing pausing/unpausing mechanismInformationalSolved - 03/26/2025
Missing index update in withdrawFees functionInformationalSolved - 03/23/2025
Unoptimized for loopsInformationalSolved - 03/28/2025
Inconsistent reentrancy protection pattern in admin functionsInformationalSolved - 03/27/2025
Missing eventsInformationalSolved - 03/28/2025
Unhandled return valuesInformationalAcknowledged - 03/26/2025
Inconsistent withdrawal pattern in withdrawFees() functionInformationalSolved - 03/21/2025
Missing input validationInformationalSolved - 03/27/2025
Ordering for nonReentrant modifierInformationalSolved - 03/25/2025
Public functions can be marked externalInformationalSolved - 03/27/2025
Use of revert strings instead of custom errorsInformationalSolved - 03/28/2025
Lack of named mappingsInformationalSolved - 03/27/2025
Use of magic numbersInformationalSolved - 03/27/2025
Lack of upgradeable storage management patternInformationalSolved - 03/27/2025
Floating pragmaInformationalSolved - 03/28/2025
Unnecessary gas consumption due to suboptimal coding patternsInformationalSolved - 03/28/2025

8. Findings & Tech Details

8.1 Impossibility to withdraw assets across protocols

//

High

Description

The withdrawal mechanism in the withdraw() and _withdrawOptimally() functions contains a logical flaw that causes withdrawals to fail under specific conditions. When a user attempts to withdraw an amount greater than the contract's direct balance but less than the total assets managed by the strategy, the withdrawal fails due to incorrect handling of the "needed" amount in _withdrawOptimally().


function _withdrawOptimally(uint256 amount) internal virtual {
  uint256 available = IERC20(_getUnderlyingAsset()).balanceOf(address(this));
  if (available >= amount) return;

  uint256 needed = amount - available;

  // Get protocols sorted by APY (ascending - lowest first)
  address[] memory sortedProtocols = _getSortedProtocolsByAPY(_getUnderlyingAsset(), true);

  // Withdraw from protocols in ascending APY order (lowest yield first)
  for (uint i = 0; i < sortedProtocols.length && needed > 0; i++) {
    address protocol = sortedProtocols[i];
    uint256 protocolBalance = getProtocolBalance(protocol, _getUnderlyingAsset());

    if (protocolBalance > 0) {
      uint256 withdrawAmount = needed > protocolBalance ? protocolBalance : needed;
      IProtocolBaseAdapter(protocol).withdrawTo(_getUnderlyingAsset(), withdrawAmount, address(this));
      emit WithdrawFromProtocol(protocol, withdrawAmount);
      needed -= withdrawAmount;
    }
  }
}

The issue arises because _withdrawOptimally() treats its input parameter (amount) as the total withdrawal amount rather than the already-calculated remaining amount needed after subtracting the contract's balance.

In withdraw():

if (available < amount) {
  _withdrawOptimally(amount - available);
}

This results in a double-subtraction of the contract's balance, leading to insufficient withdrawals from protocols. Consequently, the withdraw() function fails to transfer the requested amount to the user, rendering the withdrawal mechanism unusable in such scenarios, directly affecting users deposits.

Proof of Concept

In the following test case, the issue is demonstrated:


    it("Should demonstrate withdrawOptimally logic bug", async function () {
      // Setup: Split initial deposit between two protocols
      // Set different APYs for clear protocol selection
      await mockCompoundPool.setSupplyRate(400); // 4% (lower)
      await mockAavePool.setLiquidityRate(ethers.parseUnits("0.06", 27)); // 6% (higher)

      // Set equal TVL limits
      await strategy.setProtocolTvlLimit(await compoundAdapter.getAddress(), 5000); // 50%
      await strategy.setProtocolTvlLimit(await aaveAdapter.getAddress(), 5000); // 50%

      // Deposit 10,000 USDC
      await strategy.connect(user1).deposit(DEPOSIT_AMOUNT * 10n, false); // 10,000 USDC
      await strategy.rebalance(LARGE_REBALANCE_AMOUNT); // Ensure the funds are split

      // Verify initial state - totalAssets should equal sum of protocol balances
      let initialBalances = {
        compound: await strategy.getProtocolBalance(await compoundAdapter.getAddress(), await mockUSDC.getAddress()),
        aave: await strategy.getProtocolBalance(await aaveAdapter.getAddress(), await mockUSDC.getAddress()),
        morpho: await strategy.getProtocolBalance(await morphoAdapter.getAddress(), await mockUSDC.getAddress()),
        total: await strategy.getTotalAssets(),
        contract: await mockUSDC.balanceOf(strategyAddress)
      };

      console.log("\n===== WITHDRAW OPTIMALLY BUG DEMONSTRATION =====");
      console.log(`Total assets: ${ethers.formatUnits(initialBalances.total, 6)} USDC`);
      console.log(`Compound balance: ${ethers.formatUnits(initialBalances.compound, 6)} USDC`);
      console.log(`Aave balance: ${ethers.formatUnits(initialBalances.aave, 6)} USDC`);
      console.log(`Contract USDC balance: ${ethers.formatUnits(initialBalances.contract, 6)} USDC`);

      // Initial state verification - sum of protocol balances should equal total assets
      expect(initialBalances.total).to.equal(
        initialBalances.compound + initialBalances.aave + initialBalances.morpho + initialBalances.contract,
        "Initial total assets should match sum of protocol balances + contract balance"
      );

      // Scenario 1: Add a small amount directly to the contract
      const SMALL_BALANCE = ethers.parseUnits("100", 6); // 100 USDC
      await mockUSDC.transfer(strategyAddress, SMALL_BALANCE);

      // Update contract balance
      let contractBalance = await mockUSDC.balanceOf(strategyAddress);
      console.log(`\nAfter adding ${ethers.formatUnits(SMALL_BALANCE, 6)} USDC to contract balance`);
      console.log(`Contract USDC balance: ${ethers.formatUnits(contractBalance, 6)} USDC`);

      // Now try to withdraw an amount larger than the contract balance
      const withdrawAmount = ethers.parseUnits("20000", 6);

      console.log(`\nAttempting to withdraw: ${ethers.formatUnits(withdrawAmount, 6)} USDC`);
      console.log("(This should fail due to the withdrawOptimally logic bug)");

      // This should fail due to the bug in _withdrawOptimally
      await expect(strategy.connect(user1).withdraw(withdrawAmount)).to.be.revertedWithCustomError(
        mockUSDC,
        "ERC20InsufficientBalance"
      );

      //without espeting error:
      // await strategy.connect(user1).withdraw(withdrawAmount);

      // Explain the bug
      console.log("\n===== WHY THIS FAILS =====");
      console.log(
        `1. withdraw() function calculates needed = withdrawAmount - contractBalance = ${ethers.formatUnits(withdrawAmount, 6)} - ${ethers.formatUnits(contractBalance, 6)} = ${ethers.formatUnits(withdrawAmount - contractBalance, 6)} USDC`
      );
      console.log(`2. withdraw() calls _withdrawOptimally(${ethers.formatUnits(withdrawAmount - contractBalance, 6)})`);
      console.log(
        `3. _withdrawOptimally() rechecks contract balance, finds ${ethers.formatUnits(contractBalance, 6)} USDC`
      );
      console.log(
        `4. It incorrectly checks "if (available >= amount)" which becomes "if (${ethers.formatUnits(contractBalance, 6)} >= ${ethers.formatUnits(withdrawAmount - contractBalance, 6)})"`
      );
      console.log("5. Since this is false, it should continue withdrawing from protocols");
      console.log(
        `6. But it calculates needed = ${ethers.formatUnits(withdrawAmount - contractBalance, 6)} - ${ethers.formatUnits(contractBalance, 6)} = ${ethers.formatUnits(withdrawAmount - contractBalance - contractBalance, 6)}`
      );
      console.log(
        "7. The key issue: _withdrawOptimally() is incorrectly treating its input parameter as the total withdrawal amount"
      );
      console.log("8. But the parameter is already the REMAINING amount needed after subtracting contract balance");
      console.log("9. This causes no withdrawals from protocols, so when withdraw() tries to transfer, it fails");

      // Verify final state - contract balance should still only be the added amount
      let finalContractBalance = await mockUSDC.balanceOf(strategyAddress);
      console.log(`\nFinal contract USDC balance: ${ethers.formatUnits(finalContractBalance, 6)} USDC`);

      // Final assertion
      expect(finalContractBalance).to.equal(SMALL_BALANCE, "Contract balance should be unchanged");

      // Demonstrate that total assets still includes all protocol balances + contract balance
      let finalTotalAssets = await strategy.getTotalAssets();
      let finalProtocolBalances = {
        compound: await strategy.getProtocolBalance(await compoundAdapter.getAddress(), await mockUSDC.getAddress()),
        aave: await strategy.getProtocolBalance(await aaveAdapter.getAddress(), await mockUSDC.getAddress()),
        morpho: await strategy.getProtocolBalance(await morphoAdapter.getAddress(), await mockUSDC.getAddress())
      };

      console.log(`Final total assets: ${ethers.formatUnits(finalTotalAssets, 6)} USDC`);

      // This assertion is correct because we need to include the contract's direct balance
      expect(finalTotalAssets).to.equal(
        finalProtocolBalances.compound +
          finalProtocolBalances.aave +
          finalProtocolBalances.morpho +
          finalContractBalance,
        "Final total assets should match sum of protocol balances + contract balance"
      );
    });



BVSS
Recommendation

The _withdrawOptimally() function should be updated to correctly handle its input parameter as the remaining amount needed from protocols, without recalculating or double-subtracting the contract's balance.


Treat the amount parameter as the exact amount to withdraw from protocols, as it is pre-calculated in the withdraw() function.


By making this adjustment, _withdrawOptimally() will correctly withdraw the required amount from protocols, ensuring that the withdraw() function can successfully transfer the requested amount to the user. This fix will restore the intended functionality of the withdrawal mechanism and ensure that users can withdraw funds as expected.

Remediation Comment

SOLVED: The Mevvy team solved this finding in commit cd86b8e by following the mentioned recommendation, implementing a fix which directly uses the input parameter as the needed amount, eliminating the double subtraction problem.

Remediation Hash
References

8.2 Missing TVL limit check in default protocol deposits

//

Medium

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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.

References

8.3 TVL limit bypass on first deposit

//

Medium

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.4 Revenue loss due to missing fee processing in withdrawFees()

//

Medium

Description

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.


Proof of Concept

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



BVSS
Recommendation

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


Remediation Comment

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

Remediation Hash
References

8.5 Unallocated funds without notification when protocols reach TVL limits

//

Low

Description

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.

BVSS
Recommendation

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.


Remediation Comment

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.

Remediation Hash
References

8.6 TVL limit bypass after complete withdrawal

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.7 Single step ownership transfer process

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.8 Use of non-compliant tokens may break contract functionality

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.9 Default protocol removal handling issue

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.10 Missing _disableInitializers() function call

//

Low

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.11 Permission revocation failure in blockCompound function

//

Low

Description

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.

BVSS
Recommendation

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

Remediation Comment

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

Remediation Hash
References

8.12 Missing pausing/unpausing mechanism

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.13 Missing index update in withdrawFees function

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.14 Unoptimized for loops

//

Informational

Description

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.

BVSS
Recommendation

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
}

Remediation Comment

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

Remediation Hash
References

8.15 Inconsistent reentrancy protection pattern in admin functions

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.16 Missing events

//

Informational

Description

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.

BVSS
Recommendation

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

Remediation Comment

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

Remediation Hash
References

8.17 Unhandled return values

//

Informational

Description

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.

BVSS
Recommendation

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

Remediation Comment

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.


References

8.18 Inconsistent withdrawal pattern in withdrawFees() function

//

Informational

Description

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.

BVSS
Recommendation

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

Remediation Comment

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

Remediation Hash
References

8.19 Missing input validation

//

Informational

Description

Some functions throughout the contracts in scope lack input validation.


Instances of this issue include:

  • HighestAPYUnbondedStrategyBase._setProtocolTvlLimit()

  • HighestAPYUnbondedStrategyBase._setYieldFee()


BVSS
Recommendation

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.



Remediation Comment

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

Remediation Hash
References

8.20 Ordering for nonReentrant modifier

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.21 Public functions can be marked external

//

Informational

Description

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()


BVSS
Recommendation

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

Remediation Comment

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

Remediation Hash
References

8.22 Use of revert strings instead of custom errors

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.23 Lack of named mappings

//

Informational

Description

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.

BVSS
Recommendation

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;

Remediation Comment

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

Remediation Hash
References

8.24 Use of magic numbers

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.25 Lack of upgradeable storage management pattern

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.26 Floating pragma

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

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

Remediation Hash
References

8.27 Unnecessary gas consumption due to suboptimal coding patterns

//

Informational

Description

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.

BVSS
Recommendation
  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.

Remediation Comment

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

Remediation Hash
References

Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.

// Download the full report

Yield Contracts

* Use Google Chrome for best results

** Check "Background Graphics" in the print settings if needed

© Halborn 2025. All rights reserved.