Earn V2 Core - Async Implementation - Blueprint Finance

Throughout the codebase, there are several instances where input values are assigned without proper validation. Failing to validate inputs before assigning them to state variables or using them in protocol logic can lead to unexpected system behavior, weakened safety guarantees, or even complete failure.

Instances of this issue include:

• In PositionAccountingLib.setMaxAccountingChangeThreshold(), the maxAccountingChangeThreshold_ parameter is assigned directly without checking that it is less than or equal to BASIS_POINTS (10,000).

• In PositionAccountingLib.setCooldownPeriod(), the cooldownPeriod_ parameter is not validated for a sensible minimum value (e.g., greater than zero), which could allow disabling cooldown protection.

• In ConcreteAsyncVaultImpl.toggleQueueActive(), there is no validation to prevent disabling the queue while there are pending requests, which could lead to user confusion or inconsistent withdrawal behavior.

• In management and performance fee setters (updateManagementFee(), updatePerformanceFee()), there are no hard-coded upper bounds on fee rates, allowing privileged roles to set excessive fees and dilute user shares.

• In PositionAccountingStorageLib.initialize(), the maxAccountingChangeThreshold_ parameter is assigned directly without checking that it is less than or equal to BASIS_POINTS (10,000).

• In PositionAccountingStorageLib.initialize(), the accountingValidityPeriod_ parameter is not validated to ensure it is greater than cooldownPeriod_, which can lead to immediate expiry or liveness issues.

• In TwoWayFeeSplitter.initialize(), the feeType parameter is not validated or restricted to a known set of values, which can lead to inconsistent or meaningless fee type labeling across deployments.

Add proper validation to ensure that input values are within expected ranges and that addresses are not the zero address.

PARTIALLY SOLVED: The Blueprint Finance team partially solved this finding in the specified commit by adding input validation to several of the aforementioned instances.

The setAccountingValidityPeriod() and setCooldownPeriod() functions in PositionAccountingLib enforce that accountingValidityPeriod must be strictly greater than cooldownPeriod. However, the contract does not enforce a minimum margin between these two values.

If accountingValidityPeriod is set only slightly greater than cooldownPeriod (e.g., by 1 second), there will be an extremely narrow window to perform accounting updates after the cooldown expires. This fragility can lead to missed updates due to block time variance or transaction delays, potentially causing the protocol to revert with AccountingValidityPeriodExpired() even when the cooldown has passed.

For example, if cooldownPeriod is set to 100 seconds and accountingValidityPeriod to 101 seconds, there is only a 1-second window to perform the next update after cooldown, which is impractical and unreliable.

Enforce a minimum margin between accountingValidityPeriod and cooldownPeriod in both setter functions to ensure a robust and predictable update window.

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

The MultisigStrategy.unpauseAndAdjustTotalAssets() function allows an address with the STRATEGY_ADMIN role to unpause the strategy and directly adjust the reported total assets by any amount, without passing through the accounting validation logic enforced by PositionAccountingLib.isValidAccountingChange(). This bypasses the intended controls on asset reporting and nonce/timestamp advancement.

Restrict unpauseAndAdjustTotalAssets() to only allow adjustments that pass the same validation as adjustTotalAssets(), or remove the function if not strictly necessary.

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

The ConcreteAsyncVaultImpl contract is designed to reserve assets during epoch processing to guarantee that all user withdrawal claims can be fulfilled. According to the technical documentation (ConcreteAsyncVaultImpl-doc.md, section 7.2), the calculation for reserving assets should use rounding up (Math.Rounding.Ceil) when converting shares to assets.

However, the actual implementation in the contract uses rounding down (Math.Rounding.Floor) when calculating the share price and, by extension, the reserved assets.

This discrepancy means that, for each epoch, a small amount of value may be left unreserved, causing users to receive slightly less than their fair share when claiming withdrawals. Over time, this dust can accumulate in the vault, diverging from the intended behavior described in the documentation.

Update the implementation to use Math.Rounding.Ceil when reserving assets for processed epochs, aligning the code with the documented specification.

Alternatively, if the current behavior is preferred, update the documentation to reflect the use of rounding down.

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

The ConcreteAsyncVaultImplStorageLib library defines the storage slot constant ConcreteAsyncVaultImplStorageLocation as:

/// @dev keccak256(abi.encode(uint256(keccak256("concrete.storage.ConcreteAsyncVaultImplStorage")) - 1)) & ~bytes32(uint256(0xff))
bytes32 private constant ConcreteAsyncVaultImplStorageLocation =
    0xd3b5f67b5a9bb5c5a5b5c5a5b5c5a5b5c5a5b5c5a5b5c5a5b5c5a5b5c5a5b500;

However, the correct value, as computed by the documented formula
keccak256(abi.encode(uint256(keccak256("concrete.storage.ConcreteAsyncVaultImplStorage")) - 1)) & ~bytes32(uint256(0xff))
using Foundry's Chisel tool is 0xada5b606f7944319310c49c0f9f30d6272793a991bd2b9c3db8049867746700:

While this does not currently break storage access if the incorrect slot is used consistently throughout the codebase, it may cause confusion for future maintainers or integrators who expect the slot to match the documented formula. This inconsistency could lead to integration issues or errors if other contracts or tools rely on the documented calculation.

Update the ConcreteAsyncVaultImplStorageLocation constant to match the documented formula.

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

The TwoWayFeeSplitter contract allows either the mainRecipient or secondaryRecipient to be set to the splitter contract’s own address (address(this)), or for both recipients to be set to the same address.

When this occurs, calling distributeFees() will transfer vault tokens to the splitter itself or to the same address twice, leaving the contract’s balance unchanged and/or inflating the feesDistributed metric. This can mislead off-chain systems or dashboards that rely on these metrics.

Add checks to prevent either recipient from being set to address(this) and to ensure that mainRecipient and secondaryRecipient are not identical.

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

AsyncVaultHelperLib.processEpoch() burns requestingShares and sets epochPricePerShare but leaves totalRequestedSharesPerEpoch[epochID] untouched. Post-processing logic never uses it, yet off-chain indexers may misinterpret the non-zero value as still-queued requests.

Set totalRequestedSharesPerEpoch[epochID] = 0 .

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

The TwoWayFeeSplitter contract inherits from ReentrancyGuardUpgradeable but does not call __ReentrancyGuard_init() in its initialize() function.

While this omission does not currently impact the contract's security or functionality, it deviates from OpenZeppelin's recommended upgradeable contract initialization pattern.

Add a call to __ReentrancyGuard_init() in the initialize() function.

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

In the ConcreteAsyncVaultImplStorage struct of the ConcreteAsyncVaultImplStorageLib library, there is a typo, where the word Unclaimed is misspelled as Unlcaimed. The same case can be found in the pastEpochsUnlcaimedAssets() function of the ConcreteAsyncVaultImpl contract.

While this typo does not affect the functionality of the code, it can make the codebase harder to read and understand.

It is recommended to fix all typos to improve the readability of the codebase.

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

The ConcreteAsyncVaultImpl._executeWithdraw() function contains two identical checks: require(shares > 0, ZeroShares());. One at the start and another inside the if ($.isQueueActive) block. Since the first check already reverts if shares == 0, the second is redundant and unreachable.

Remove the second require(shares > 0, ZeroShares()); inside the if ($.isQueueActive) block to simplify the code.

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