Earn V2 Core - Standard Implementation - Blueprint Finance

The deposit() and redeem() functions in ConcreteStandardVaultImpl do not enforce that the calculated shares (for deposit) or assets (for redeem) are greater than zero before proceeding. This means that, under certain edge-case conditions, such as when totalSupply is very low and cachedTotalAssets is very high (which can be caused by a strategy over reporting yield), a user may deposit assets and receive zero shares, or redeem shares and receive zero assets.

In both cases, the user loses value with no compensation.

Add validations to assert that the shares and assets are greater than 0 in deposit() and redeem() to prevent user loss in these scenarios.

SOLVED: The Blueprint Finance team solved this finding in the specified commit by following the mentioned recommendation.

The AllocateModule contract uses forceApprove(strategy, type(uint256).max) before each allocation. While this is reset to zero after the call, a malicious or compromised strategy could, in theory, exploit the approval window to transfer more assets than intended.

Consider using minimal approvals or validating strategy contracts more strictly. Alternatively, document the trust assumptions for strategy contracts.

RISK ACCEPTED: The Blueprint Finance team made a business decision to accept the risk of this finding and not alter the contracts.

The performanceFeeHighWaterMark variable in ConcreteStandardVaultImplStorageLib is defined but never read from or updated in the ConcreteStandardVaultImpl implementation. As a result, performance fees are charged on any net positive yield within a single accrual period, including gains that merely recover previous losses.

This means the vault can charge performance fees multiple times on the same economic gain if the vault value fluctuates, rather than only on new profits above the previous high.

Document the fee-on-recovery behavior clearly in user-facing documentation and disclosures, so users understand that performance fees may apply to recovered losses as well as new gains.

Alternatively, if the high-water mark will not be used, consider removing the unused variable to avoid confusion.

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

We consciously opted for this design after carefully considering the options and the industry practices and do not consider it a threat but a design choice. Our design charges directly on the yield (also minted, but net effect is share value appreciation) instead of inflating the shares (net effect is share value depreciation). Thus charging the fees wont drop the share value. We are in line with major defi protocols, who handle the situation similarly.

The AllocateModule contract, used via delegatecall in ConcreteStandardVaultImpl.allocate, updates the vault's internal .allocated value for each strategy based solely on the return value of IStrategyTemplate(strategy).allocateFunds() and deallocateFunds(). However, there is no check that the actual asset balance change matches the reported value. A malicious or buggy strategy could over report allocation or under report deallocation, leading to incorrect accounting, fee miscalculation, and potential user loss.

Additionally, when a strategy is toggled to Halted, the vault stops updating its .allocated value for that strategy. If the real value of the strategy decreases (e.g., due to a hack or loss) after being halted, the vault continues to use the old, higher value in its accounting. This allows users to withdraw or redeem at an inflated share price, extracting more than their fair share and pushing hidden losses onto remaining holders.

Update allocation logic to compare the vault's asset balance before and after the call, and use the actual delta for allocated updates.

Consider excluding halted strategies from total assets until reconciled.

ACKNOWLEDGED: The Blueprint Finance team made a business decision to acknowledge this finding and not alter the contracts.

The ConcreteStandardVaultImpl contract exhibits inconsistencies between preview and execution logic in two areas:

• Performance fee preview:
  The _previewAccrueYieldAndFees() function subtracts the management fee amount from total assets before calculating the performance fee, while the actual accrual path (accruePerformanceFee()) uses the full total assets value. This results in the previewed performance fee being slightly lower than the fee actually minted, potentially confusing users and integrators.

  
  
  

• Liquidity preview:
  The maxWithdraw and maxRedeem functions preview available liquidity by considering all active strategies, but actual withdrawals only use strategies listed in deallocationOrder. If deallocationOrder omits any active, liquid strategies, the previewed maximum withdrawable amount may be higher than what can actually be withdrawn, leading to failed transactions and user confusion.

Standardize the asset base used for performance fee calculations in both preview and accrual paths, and align the liquidity preview logic with the actual withdrawal execution logic to ensure consistency.

ACKNOWLEDGED: The Blueprint Finance team made a business decision to acknowledge this finding and not alter the contracts.

The ConcreteStandardVaultImpl contract supports pre-action hooks (such as preDeposit, preMint, preWithdraw, and preRedeem) via the HooksLibV1 library. These hooks are invoked before the vault calculates the number of shares or assets for a user action, but after the vault's asset balance is cached.

If a hook implementation transfers assets into or out of the vault during its execution, the cached asset value used for conversion will be stale. This could result in value extraction or dilution, as the effective price at which shares are minted or redeemed will be manipulated.

Recompute or recheck the vault's asset balance after executing pre-action hooks.

ACKNOWLEDGED: The Blueprint Finance team made a business decision to acknowledge this finding and not alter the contracts.

The deallocationOrder array in ConcreteStandardVaultImpl determines the order in which strategies are used to fulfill withdrawals. However, the protocol does not enforce that this array contains all and only the active strategies, nor does it prevent duplicates. Notably, when a strategy is removed, it is not automatically purged from deallocationOrder, leaving stale entries.

As a result, deallocationOrder can contain stale (removed) strategies, omit active ones, or include duplicates. This can cause withdrawals to fail even when sufficient funds exist, or waste gas on unnecessary or invalid withdrawal attempts.

Ensure that deallocationOrder always contains each active strategy exactly once, with no duplicates or stale entries.

Alternatively, automatically update deallocationOrder when strategies are added or removed, or validate its integrity before processing withdrawals.

ACKNOWLEDGED: The Blueprint Finance team made a business decision to acknowledge this finding and not alter the contracts.

The _withdraw() function in ConcreteStandardVaultImpl.sol currently transfers assets to the receiver before burning the user's shares, while the accompanying comment states that the transfer should occur after burning shares for safer ERC777 handling.

// Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
// shares are burned and after the assets are transferred, which is a valid state.

SafeERC20.safeTransfer(IERC20(asset()), receiver, assets);
CachedVaultStateLib.fetch().cachedTotalAssets -= assets;
_burn(owner, shares);

Although the function is protected by the nonReentrant modifier, which currently prevents reentrancy attacks, this mismatch between the comment and implementation could lead to confusion or introduce vulnerabilities if the function is refactored or the guard is removed in the future.

Additionally, in ConcreteFactory, the NatSpec documentation for the create and predictVaultAddress functions claims that if salt == 0, a deterministic salt will be computed from the deployer, version, and owner. In reality, the implementation simply forwards zero as the salt, and no computation occurs. This discrepancy may confuse users expecting automatic salt derivation.

Update comments and documentation to match the actual implementation, or update the code to match the documented behavior.

SOLVED: The Blueprint Finance team solved this finding in the specified commit by following the mentioned recommendation.

The setDeallocationOrder(address[] calldata order) function in StateSetterLib.sol uses a uint8 loop index.

function setDeallocationOrder(address[] calldata order) external {
    SVLib.ConcreteStandardVaultImplStorage storage $ = SVLib.fetch();

    delete $.deallocationOrder;

    uint256 orderLength = order.length;
    for (uint8 i = 0; i < orderLength; i++) {
        address strategy = order[i];
        require($.strategies.contains(strategy), IConcreteStandardVaultImpl.StrategyDoesNotExist());
        require(
            $.strategyData[strategy].status == IConcreteStandardVaultImpl.StrategyStatus.Active,
            IConcreteStandardVaultImpl.StrategyIsHalted()
        );

        $.deallocationOrder.push(strategy);
    }

    emit IConcreteStandardVaultImpl.DeallocationOrderUpdated();
}

If more than 255 strategies are passed, the function will revert due to overflow.

Change the loop index from uint8 to uint256 to match the array length type. Alternatively, limit the amount of strategies that can be added to be less than 256.

SOLVED: The Blueprint Finance team solved this finding in the specified commit by following the mentioned recommendation.

The contracts in scope currently use floating pragma version ^0.8.0 which means that the code can be compiled by any compiler version that is greater than these version, 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 (for example: pragma solidity 0.8.28;), and 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.

ACKNOWLEDGED: The Blueprint Finance team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the code, there are several instances of unused components that could be removed to improve code readability and maintainability.

Instances of this issue include:

• In ConcreteStandardVaultImpl:

  import {IConcreteFactory} from "../interface/IConcreteFactory.sol";

  
  

• In StateSetterLib:

  import {IStrategyTemplate} from "../interface/IStrategyTemplate.sol";

Remove the unused imports from the files.

SOLVED: The Blueprint Finance team solved this finding in the specified commit by following the mentioned recommendation.