Yield.xyz: Allocatorvaultv3_2.sol

Description

The harvest function is restricted to the HARVESTER_ROLE, but _harvest is called internally in deposit, mint, withdraw, and redeem. Anyone can trigger a harvest by performing a dust amount deposit or withdrawal (e.g., 1 wei, assuming no minimum limits in the strategy), bypassing the restriction at minimal cost. This makes the access control ineffective, as harvests can still be forced without role permission.

Recommendation

Make harvest public by removing the onlyRole(HARVESTER_ROLE) modifier or add minimum deposit/withdraw limits in the vault for this check to be meaningful.

Description

In the redeem function, _underlying is calculated using strategy.convertToAssets(assets), which provides a theoretical conversion rate. However, per ERC4626 standard, convertToAssets may differ from the actual redemption amount due to slippage or other conditions. The function calls strategy.redeem(assets, receiver, address(this)) but ignores its return value (the actual underlying redeemed), returning and emitting the pre calculated _underlying instead.

We know that pre calculation is required because support for withdraw with cooldown is required. When using the return value from actual redeem is not feasible, strategy.previewRedeem(assets) should be used for accurate return value.

Proof of Concept

Update test/allocator-vaults/utils/Strategy.sol to have withdrawal fees. This could be the reason previewRedeem and convertToAssets might not return the same amount.

// SPDX-License-Identifier: MITpragma solidity ^0.8.25;
import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";import "forge-std/Test.sol";import { MockToken } from "test/allocator-vaults/utils/Token.sol";
contract MockStrategy is Test, ERC4626 {    MockToken token;
    uint256 public fees = 10;//10% withdrawal fee
    constructor(        IERC20 _underlying,        string memory name,        string memory symbol    )        ERC4626(_underlying)        ERC20(name, symbol)    {        token = MockToken(address(_underlying));    }
    function previewRedeem(uint256 shares) public view override returns (uint256) {        uint256 assets = super.previewRedeem(shares);        uint256 fee = (assets * fees) / 100;        return assets - fee;    }
    function previewWithdraw(uint256 assets) public view override returns (uint256) {        uint256 fee = (assets * fees) / 100;        assets = assets - fee;        return super.previewWithdraw(assets);    }
    function _withdraw(        address caller,        address receiver,        address owner,        uint256 assets,        uint256 shares    ) internal override {        uint256 fee = (assets * fees) / 100;        assets = assets - fee;        super._withdraw(caller, receiver, owner, assets, shares);    }
    function generateYield(uint256 yieldAmount) external {        token.mint(address(this), yieldAmount);    }
    function generateLoss(uint256 lossAmount) external {        token.burn(address(this), lossAmount);    }
    function mintForUser(address user, uint256 amount) external {        _mint(user, amount);    }}

Update the test_redeem test in test/allocator-vaults/unit/Baseline.t.sol. Notice that the actual underlying amount received and the return amount are not equal:

function test_redeem() public {        vm.startPrank(finneas);        vm.expectRevert("Exceeded max redeem");        vaultProxy.redeem(100, finneas, finneas);        vm.stopPrank();
        vm.startPrank(owner);        underlying.mint(finneas, 1000);        vm.stopPrank();
        vm.startPrank(finneas);        underlying.approve(address(vaultProxy), 1000);        vaultProxy.deposit(100, finneas);
        uint256 finneasBalance = vaultProxy.balanceOf(finneas);
        uint256 finneasBalanceBefore = underlying.balanceOf(finneas);
        vm.expectEmit(true, true, false, true, address(vaultProxy));        emit Withdraw(finneas, finneas, finneas, 99, 99 * VIRTUAL_SHARES);        uint256 assetsReturned = vaultProxy.redeem(finneasBalance, finneas, finneas);        vm.stopPrank();
        uint256 finneasBalanceAfter = underlying.balanceOf(finneas);        uint256 actualUnderlyingReceived = finneasBalanceAfter - finneasBalanceBefore;
        //Notice that the actual underlying received is not equal to assetsReturned due to previewRedeem not being used.         assertNotEq(assetsReturned, actualUnderlyingReceived);    }

Recommendation

Capture and use the actual return value from strategy.redeem when possible. If not, use previewRedeem instead:

...
uint256 _underlying;if (config.hasCooldown) {    IERC20(address(strategy)).safeTransfer(receiver, assets);    _underlying = strategy.previewRedeem(assets);  // Approximation for cooldown mode} else {    _underlying = strategy.redeem(assets, receiver, address(this));}
...
emit Withdraw(msg.sender, receiver, owner, _underlying, shares);return _underlying;

Description

asset for this ERC4626 vault (AllocatorVault) being the strategy contract breaks the ERC4626 in most places. In ERC4626 asset is referred to as the underlying token managed by the vault. In this case, asset is the strategy

For example, Here's the requirement for deposit function:

MUST support EIP-20 approve / transferFrom on asset as a deposit flow

This is not true for this vault. The deposit function doesn't pull the asset (strategy tokens) from the user.

Mints shares Vault shares to receiver by depositing exactly assets of underlying tokens

Deposit doesn't deposit exactly assets of underlying tokens because the underlying token for the this vault are strategy tokens.

Similar issues affect mint, withdraw, redeem, and previews. Only convertToShares, convertToAssets, and totalAssets align correctly with the current setup

Recommendation

Change asset to the true underlying token via initialization swap, and adjust related functions for accurate conversions (e.g., update totalAssets to reflect underlying equivalents via strategy conversions):

function initialize(    address _underlying,    IERC4626 _strategy,    string memory vaultTokenName,    string memory vaultTokenSymbol) public initializer {-   __ERC4626_init(IERC20(address(_strategy)));+   __ERC4626_init(IERC20(_underlying));   ...    require(_underlying == _strategy.asset(), "Underlying token mismatch");-   require(address(_strategy) == asset(), "Strategy token mismatch");+   require(_underlying == asset(), "Underlying token mismatch");   ...}

If you fix this and change the underlying asset for the vault, you would need to change the public facing convertToShares, convertToAssets and totalAssets to return amounts in underlying assets instead of strategy shares.

Description

In computeHarvestFee, currentTotalUnderlying is computed via strategy.convertToAssets(totalAssets()), which per ERC4626 excludes fees charged against assets. This overestimates gain by including unrealized fees (e.g., protocol fees on interest in a lending strategy) that users won't receive, leading to inflated performance fees.

previewRedeem should be used, as it includes withdrawal fees and simulates ideal redemption without limits.

convertToAssets definition from ERC4626

The amount of assets that the Vault would exchange for the amount of shares provided, in an ideal scenario where all the conditions are met.

MUST NOT be inclusive of any fees that are charged against assets in the Vault

previewRedeem definition from ERC4626

MUST NOT account for redemption limits like those returned from maxRedeem and should always act as though the redemption would be accepted, regardless if the user has enough shares, etc. MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees.

Recommendation

Replace convertToAssets with previewRedeem for accurate redeemable assets:

uint256 currentTotalUnderlying = strategy.previewRedeem(totalAssets());

Update totalUnderlyingStamp assignments (e.g., in deposit, redeem) similarly for consistency.

Description

The maxDeposit and maxMint functions do not account for the paused state, returning non zero values even when deposits are disabled.

From ERC4626 standard for both maxDeposit and maxMint:

MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0

Recommendation

Override maxDeposit and maxWithdraw check for paused state:

function maxDeposit(address receiver) public view override returns (uint256) {    if (paused) {        return 0;    }    return super.maxDeposit(receiver);}
function maxMint(address receiver) public view override returns (uint256) {    if (paused) {        return 0;    }    return super.maxMint(receiver);}

Description

In addToWhitelist, require(target != address(strategy), "Cannot whitelist strategy"); prevents whitelisting the strategy but allows the underlying asset. If the underlying is whitelisted with transferFrom selector, admins could exploit via claimAdditionalRewards by calling underlying.transferFrom from the vault context, potentially draining the hanging approvals to the vault. While admins control vault funds, they shouldn't be able to pull from user wallets in any condition.

Recommendation

Add check in addToWhitelist:

require(target != underlying, "Cannot whitelist underlying");

Description

In previewRedeem, the underlying amount is computed via strategy.convertToAssets(strategyShares), which per ERC4626 excludes fees charged against assets. This overestimates redeemable assets if the strategy has withdrawal fees, leading to inaccurate simulations. previewRedeem should be used, as it includes fees and simulates actual redemption.

Recommendation

Use previewRedeem:

function previewRedeem(uint256 shares) public view override returns (uint256) {        uint256 newTotalShares = previewHarvest();        if (newTotalShares == 0) {            return 0;        }        uint256 strategyShares = shares * (totalAssets() + VIRTUAL_ASSETS) / (newTotalShares + VIRTUAL_SHARES);-       return strategy.convertToAssets(strategyShares);+       return strategy.previewRedeem(strategyShares);     }

Description

The AllocatorVaultV3_2 contract implements an asymmetric pause mechanism where the notPaused modifier only restricts the deposit and mint functions, but explicitly allows withdraw and redeem operations to continue unrestricted when paused is set to true. While the design intention is to prevent new capital from entering the vault during uncertain periods while allowing users to exit, this approach creates several security and operational risks. During a pause scenario triggered by suspected vulnerabilities, oracle manipulation, or strategy contract issues, allowing withdrawals can still expose the vault to exploitation through redemption-based attacks. Additionally, mass withdrawals during a paused state could deplete vault liquidity, preventing administrators from implementing proper remediation measures or emergency asset recovery procedures before users drain available funds.

Recommendation

Implement a more granular pause mechanism that provides administrators with flexible control over vault operations during emergency scenarios. Consider introducing separate pause flags such as depositPaused and withdrawPaused that can be independently toggled, allowing the DEFAULT_ADMIN_ROLE to pause withdrawals when necessary to prevent exploitation during suspected attacks or strategy failures, while maintaining the option to allow exits during less critical situations like temporary operational pauses.

Description

The AllocatorVaultV3_2 contract defines SECONDS_IN_YEAR as a constant value of 365 * 24 * 60 * 60, which equals 31,536,000 seconds and assumes every year contains exactly 365 days. This calculation ignores the existence of leap years that occur approximately every four years and contain 366 days. The computeHarvestFee function utilizes this constant when calculating both management fees based on time elapsed and performance fees annualized over the year constant.

Recommendation

Consider using a more precise annual time representation of 365.25 days to account for the average occurrence of leap years.

Description

The AllocatorVaultV3_2 contract implements a cooldown mechanism in the withdraw and redeem functions that fundamentally violates ERC-4626 standard compliance when config.hasCooldown is enabled. In the withdraw function, when the cooldown is active, the function transfers strategy shares directly to the receiver via IERC20(address(strategy)).safeTransfer(receiver, assets), rather than the underlying tokens as specified by the ERC-4626 standard. Similarly, the redeem function exhibits the same behavior, transferring strategy shares instead of underlying tokens when cooldown is enabled. The ERC-4626 standard explicitly requires that withdraw must return the exact amount of underlying asset tokens specified in the _underlying parameter, and redeem must convert shares to the underlying asset. By returning strategy shares (which are themselves ERC-4626 vault tokens representing a claim on the underlying), the contract breaks this fundamental expectation. Users calling withdraw(1000, receiver, owner) expecting 1000 underlying tokens will instead receive an equivalent value in strategy shares, which they must then separately redeem from the strategy contract. This creates a multi-step withdrawal process that violates the standard's interface guarantees.

Recommendation

Consider implementing a compliant cooldown mechanism that maintains ERC-4626 standard adherence while still achieving the desired time-delay security feature. One approach is to implement a two-phase withdrawal system where users first call a requestWithdrawal function that burns their vault shares and records a pending withdrawal request with a timestamp, then after the cooldown period expires, users call a separate claimWithdrawal function that completes the withdrawal by transferring underlying tokens from the strategy. Alternatively, maintain separate compliant withdraw and redeem functions that always return underlying tokens, and introduce new non-standard functions like withdrawWithCooldown and redeemWithCooldown that explicitly return strategy shares, allowing integrators to choose the appropriate function based on their requirements.

Description

The vault's decimals match the strategy's decimals, ignoring VIRTUAL_SHARES (10^6), which provides precision to avoid rounding errors. This leads to inflated share minting; e.g., depositing 0.01 EURC mints ~10,000 shares (as seen in this transaction). The decimals should be underlying asset decimals + 6 for accuracy.

This is not an accounting error, but just a UX issue.

Recommendation

Override _decimalsOffset() from ERC4626Upgradeable to return 6, ensuring decimals() computes correctly (underlying decimals + 6). No storage changes needed for upgrade:

function _decimalsOffset() internal view virtual override returns (uint8) {    return 6;}

Alternatively, override decimals() directly:

function decimals() public view virtual override returns (uint8) {    return IERC20Metadata(underlying).decimals() + 6;}

Description

In addToWhitelist, require(functionSelector != bytes4(0), "Invalid function selector"); blocks whitelisting selectors equal to 0x00000000. While the chances of it happening by chance is low, some projects might deliberately mine function selector to be bytes4(0) for important functions to save gas.

Here's a list of real function selectors with bytes4(0) function selectors.

Recommendation

Remove the unnecessary restriction line to allow valid zero selectors.

Description

The _spendAllowance function is overridden but replicates the exact logic from ERC20Upgradeable (inherited via ERC4626Upgradeable), adding unnecessary code duplication without functional changes. This increases maintenance overhead.

Recommendation

Remove the entire _spendAllowance override to rely on the parent implementation.

Description

In initialize, require(address(_strategy) == asset(), "Strategy token mismatch"); checks an internal invariant post __ERC4626_init. This should always hold unless an internal error occurs (not from external inputs like params or interactions). Per Solidity best practices, use assert for invariants (causing panic) and require for input validation (reverting with error).

Recommendation

Replace require with assert:

assert(address(_strategy) == asset());

Description

The AllocatorVaultV3_2 contract contains duplicated share calculation logic between the deposit function and previewDeposit function, which creates unnecessary gas overhead and increases the risk of implementation inconsistencies.

Recommendation

Extract the core deposit share calculation logic into a dedicated internal function such as _calculateDepositShares(uint256 assets, uint256 totalAssetsBefore, bool applyFee) that can be reused by both deposit and previewDeposit functions.

Description

The AllocatorVaultV3_2 contract implements an inefficient pattern for managing harvest-related state updates across multiple functions. The _harvest internal function updates totalUnderlyingStamp and lastStamp after minting fee shares, but every calling function (deposit, mint, withdraw, and redeem) subsequently performs identical storage writes to these same variables after executing their core logic. This results in double storage writes within a single transaction, where _harvest sets these stamps first, and then the calling function immediately overwrites them with potentially the same or only marginally different values. This pattern does create unnecessary gas overhead through redundant SSTORE operations and increasing the cognitive burden on developers who must remember to manually update these stamps in every function that calls _harvest.

Recommendation

Consider refactoring the harvest mechanism into a modifier pattern that encapsulates the pre-harvest fee calculation and post-operation state updates, eliminating the need for manual stamp management in each calling function.

Description

In deposit and mint, the contract approves the exact deposit amount to the strategy via IERC20(underlying).forceApprove(address(strategy), _underlying) before each strategy.deposit call. This repeated approval incurs unnecessary gas costs.

The vault is not supposed to hold any underlying assets anyway. The balance returned to zero after each action. Making infinite approval is Safe

Recommendation

Add infinite approval in initialize:

IERC20(underlying).forceApprove(address(strategy), type(uint256).max);

Remove approvals from deposit and mint

Description

In initialize, strategy = _strategy; stores the strategy address separately. However, ERC4626Upgradeable already stores it internally via __ERC4626_init(IERC20(address(_strategy))), accessible via asset(). This duplicates storage, increasing contract size and gas costs during initialization without any benefit.

Recommendation

Remove the strategy variable declaration and all assignments. Replace all strategy usages with IERC4626(asset())