Prepared by:
HALBORN
Last Updated 04/01/2025
Date of Engagement: March 26th, 2025 - April 1st, 2025
100% of all REPORTED Findings have been addressed
All findings
27
Critical
0
High
1
Medium
3
Low
7
Informational
16
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.
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.
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
).
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.
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.
EXPLOITABILITY METRIC () | METRIC VALUE | NUMERICAL 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 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL 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 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
0
High
1
Medium
3
Low
7
Informational
16
Security analysis | Risk level | Remediation Date |
---|---|---|
Impossibility to withdraw assets across protocols | High | Solved - 03/21/2025 |
Missing TVL limit check in default protocol deposits | Medium | Risk Accepted - 03/26/2025 |
TVL limit bypass on first deposit | Medium | Solved - 03/25/2025 |
Revenue loss due to missing fee processing in withdrawFees() | Medium | Solved - 03/23/2025 |
Unallocated funds without notification when protocols reach TVL limits | Low | Solved - 03/25/2025 |
TVL limit bypass after complete withdrawal | Low | Solved - 03/25/2025 |
Single step ownership transfer process | Low | Solved - 03/26/2025 |
Use of non-compliant tokens may break contract functionality | Low | Solved - 03/26/2025 |
Default protocol removal handling issue | Low | Solved - 03/26/2025 |
Missing _disableInitializers() function call | Low | Solved - 03/23/2025 |
Permission revocation failure in blockCompound function | Low | Solved - 03/23/2025 |
Missing pausing/unpausing mechanism | Informational | Solved - 03/26/2025 |
Missing index update in withdrawFees function | Informational | Solved - 03/23/2025 |
Unoptimized for loops | Informational | Solved - 03/28/2025 |
Inconsistent reentrancy protection pattern in admin functions | Informational | Solved - 03/27/2025 |
Missing events | Informational | Solved - 03/28/2025 |
Unhandled return values | Informational | Acknowledged - 03/26/2025 |
Inconsistent withdrawal pattern in withdrawFees() function | Informational | Solved - 03/21/2025 |
Missing input validation | Informational | Solved - 03/27/2025 |
Ordering for nonReentrant modifier | Informational | Solved - 03/25/2025 |
Public functions can be marked external | Informational | Solved - 03/27/2025 |
Use of revert strings instead of custom errors | Informational | Solved - 03/28/2025 |
Lack of named mappings | Informational | Solved - 03/27/2025 |
Use of magic numbers | Informational | Solved - 03/27/2025 |
Lack of upgradeable storage management pattern | Informational | Solved - 03/27/2025 |
Floating pragma | Informational | Solved - 03/28/2025 |
Unnecessary gas consumption due to suboptimal coding patterns | Informational | Solved - 03/28/2025 |
//
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.
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"
);
});
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.
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.
//
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:
User deposits funds and receives strategy tokens.
The function attempts to allocate funds to protocols via _depositToProtocols()
.
When calling _getBestEligibleProtocol()
, if all protocols are at their TVL limits, it returns address(0)
.
The function simply returns with if (bestProtocol == address(0)) return;
.
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:
Return unallocated funds to the user and mint strategy tokens only for the successfully allocated amount.
Emit an event when funds cannot be fully allocated to inform users and monitoring systems.
Consider returning a success ratio from the deposit function to indicate what percentage was successfully deployed.
Add a function to check the percentage of idle funds in the strategy.
Implement a "waiting list" mechanism that automatically deploys idle funds when TVL capacity becomes available.
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:
Implement a dedicated variable to track the true first-deposit scenario rather than relying on asset calculations, or
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:
For _setProtocolTvlLimit()
, validate that limitBps
is greater than zero.
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());
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.
Remove redundant initializations of variables to their default values.
Consider creating a modifier for repeated protocol validation.
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.
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