Mellow Finance

Core

6.1 Inconsistent Price Conversion Logic Leads to Incorrect Oracle Prices

The OracleHelper contract is responsible for calculating the prices of assets held within a vault, which are then reported to an oracle. The price of each asset is defined relative to a "base asset" through the priceD18 parameter, as explained in the comments of the AssetPrice struct. According to these comments, to convert an amount from a specific asset to the base asset, one should multiply by priceD18.

The getPricesD18(...) function performs two key calculations using priceD18 that are logically inconsistent with each other.

First, when calculating the totalRedeemDemand (the total value of withdrawal demands in terms of the base asset), the code divides the demand_ by assetPrice.priceD18. This contradicts the logic described in the comments.

// ...
// @audit This comment explains that priceD18 is a multiplier.
// @audit - If priceD18 = 2e18, it means that 1 asset = 2 base assets
/**
 * Price of the asset expressed via the base asset.
// ...
 */

// ...
$.unprocessedShares += shares_;
if (assetPrice.priceD18 == 0) {
    $.totalRedeemDemand += demand_;
} else {
    // @audit-issue The demand is divided by the price, not multiplied.
    // @audit-issue This should be Math.mulDiv(demand_, assetPrice.priceD18, 1 ether).
    $.totalRedeemDemand += Math.mulDiv(demand_, 1 ether, assetPrice.priceD18);
}
// ...

Later in the same function, when calculating the final prices of the other assets, the logic correctly multiplies the base asset's price by assetPrices[i].priceD18. This second calculation is consistent with the comments.

// ...
for (uint256 i = 0; i < assetPrices.length; i++) {
    if (i != $.baseAssetIndex) {
        // @audit This calculation correctly uses priceD18 as a multiplier.
        pricesD18[i] = Math.mulDiv(
            pricesD18[$.baseAssetIndex], assetPrices[i].priceD18, 1 ether
        );
    }
// ...

The incorrect calculation of totalRedeemDemand will lead to a miscalculation of the base asset's price. Since all other asset prices are derived from the base asset price, the entire set of prices returned by the function will be incorrect. This can severely compromise the protocol's economic security, leading to incorrect vault valuations, unfair distribution of funds upon withdrawal, and potential financial exploits.

Recommendation

Review the intended formula for converting asset amounts to their base asset equivalents. Assuming the comments and the final price calculation logic are correct, the calculation for totalRedeemDemand should be changed from division to multiplication.

6.2 Off-chain price calculation can result in an infinite loop

The OracleHelper contract includes the getPricesD18(...) function, which is designed for off-chain execution by keepers to calculate a vault's share price. This calculation is iterative, using a while(true) loop to find a stable price that correctly accounts for performance and protocol fees. The loop is expected to terminate once the calculated price converges.

The performance fee calculation, handled by the FeeManager contract, is conditional. It is applied only when the share price surpasses its previous high water mark, which corresponds to when priceD18 is less than minPriceD18. This conditional logic can create a scenario where the calculated price oscillates between two values, preventing convergence and causing the loop to run indefinitely.

This issue arises when an iteration calculates a price ( P1) low enough to trigger the performance fee. In the next iteration, the inclusion of this fee results in a higher calculated price ( P2). If P2 is now above the high-water mark, the performance fee is no longer applied in the subsequent iteration, causing the calculated price to revert to P1. This cycle of price oscillation between P1 and P2 means the loop's exit condition is never met.

The core of the issue lies in the while loop in the OracleHelper contract.

// src/oracles/OracleHelper.sol

// ...
        uint256 baseAssetPriceD18 = vault.oracle().getReport($.baseAsset).priceD18;
// @audit The loop lacks a circuit breaker and can run indefinitely under certain conditions.
        while (true) {
            uint256 feeShares = totalShares > recipientShares
                ? feeManager.calculateFee(
                    address(vault), $.baseAsset, baseAssetPriceD18, totalShares - recipientShares
                )
                : 0;
            uint256 newBaseAssetPriceD18 = Math.mulDiv(
                totalShares + $.unprocessedShares + feeShares,
                1 ether,
                totalAssets - $.totalRedeemDemand
            );
// @audit-issue This equality check may never be true if the price oscillates.
            if (newBaseAssetPriceD18 == baseAssetPriceD18) {
                break;
            }
            baseAssetPriceD18 = newBaseAssetPriceD18;
        }
// ...

The conditional logic in the FeeManager contract.

// src/managers/FeeManager.sol
function calculateFee(address vault, address asset, uint256 priceD18, uint256 totalShares)
    public
    view
    returns (uint256 shares)
{
// ...
        if (asset == $.baseAsset[vault]) {
            uint256 minPriceD18_ = $.minPriceD18[vault];
            // @audit Performance fees are only added if the price is below the high-water mark.
            if (priceD18 < minPriceD18_ && minPriceD18_ != 0) {
                shares = Math.mulDiv(
                    minPriceD18_ - priceD18,
                    $.performanceFeeD6 * totalShares,
                    priceD18 * 1e6
                );
            }
// ...
        }
}

An infinite loop in the off-chain price calculation script will cause it to fail, preventing keepers from submitting updated price reports. This leads to stale prices on-chain, which can halt critical vault operations or be exploited by attackers, resulting in a Denial of Service for the price update mechanism.

Recommendation

Consider re-architecting the iterative price calculation to guarantee convergence.

6.3 Incorrect Natspec for CUSTOM_VERIFIER verification data

The getVerificationResult(...) function in the Verifier contract handles the authorization of delegated function calls based on different verification types. One such type is CUSTOM_VERIFIER, which allows for verification logic to be delegated to an external contract.

The natspec documentation for the VerificationPayload struct provides instructions on how to encode the verificationData for each verification type. For CUSTOM_VERIFIER, it incorrectly suggests using abi.encodePacked(address customVerifier, customVerifierSpecificData).

/// @notice Struct containing all inputs required to verify a delegated function call.
struct VerificationPayload {
    /// @dev The method used to verify the delegated call.
    VerificationType verificationType;
    /// @dev Encoded payload to be verified, varies by verification type:
    /// - MERKLE_COMPACT: abi.encode(who, where, selector)
    /// - MERKLE_EXTENDED: abi.encode(who, where, value, callData)
    // @audit-issue Natspec suggests abi.encodePacked, which is incompatible with the implementation.
    /// - CUSTOM_VERIFIER: abi.encodePacked(address customVerifier, customVerifierSpecificData)
    bytes verificationData;
    /// @dev Merkle proof used to validate the `verificationType` and `verificationData` for MERKLE_COMPACT,
    /// MERKLE_EXTENDED, and CUSTOM_VERIFIER types.
    bytes32[] proof;
}

The issue arises in the getVerificationResult(...) function, where the verifier address is extracted from verificationData using an assembly block with the calldataload instruction. This instruction reads a full 32-byte word.

    function getVerificationResult(...) public view returns (bool) {
// ...
        } else if (payload.verificationType == VerificationType.CUSTOM_VERIFIER) {
            address verifier;
            assembly {
                // @audit This instruction reads a full 32-byte word from calldata.
                verifier := calldataload(verificationData.offset)
            }
            return ICustomVerifier(verifier).verifyCall(who, where, value, data, verificationData[0x20:]);
// ...
    }

If a developer follows the natspec and uses abi.encodePacked, the 20-byte verifier address will be tightly packed with customVerifierSpecificData. The calldataload instruction would then read the address plus the first 12 bytes of the custom data, resulting in a corrupted address and causing the verification to always fail. The correct encoding should pad the address to 32 bytes, for example, by using abi.encode(...).

This discrepancy between the documentation and the implementation can mislead developers, causing them to create invalid payloads and breaking the CUSTOM_VERIFIER functionality for anyone who relies on the natspec.

Recommendation

Consider updating the natspec comment for verificationData in the VerificationPayload struct. Instead of abi.encodePacked(...), it should specify abi.encode(...) or another encoding method that ensures the customVerifier address is padded to a full 32-byte word to align with the implementation.

6.4 Incorrect natspec for ClaimableRequestExists error

The cancelDepositRequest(...) function in the DepositQueue contract allows a user to cancel their pending deposit request. The function checks if the request has become claimable by comparing the request's timestamp with the latest price checkpoint timestamp. If the request is claimable, the function reverts with the ClaimableRequestExists error.

// src/queues/DepositQueue.sol

function cancelDepositRequest() external nonReentrant {
    // ...
    (bool exists, uint32 timestamp,) = $.prices.latestCheckpoint();
    // @audit The function reverts if a user tries to cancel a request that is already claimable.
    if (exists && timestamp >= request._key) {
        revert ClaimableRequestExists();
    }
    // ...
}

However, the natspec documentation for the ClaimableRequestExists error in the IDepositQueue interface is incorrect. It states that the error is thrown when a user tries to deposit again, which is not the case.

// src/interfaces/queues/IDepositQueue.sol

// @audit-issue The natspec incorrectly states the error is for depositing again.
/// @notice Thrown when a user tries to deposit again while a claimable request exists.
error ClaimableRequestExists();

This discrepancy can mislead developers, auditors, and integrators about the actual behavior of the cancelDepositRequest(...) function, potentially causing confusion when interacting with the contract.

Recommendation

Consider updating the natspec for the ClaimableRequestExists error to accurately reflect its usage. The comment should clarify that the error is thrown when a user attempts to cancel a deposit request that has already become claimable.

6.5 Use of abi.encodePacked with dynamic types could lead to storage slot collisions

The SlotLibrary contract provides a helper function, getSlot(...), to compute deterministic storage slots for various modules based on a contractName, an instance name, and a version. This mechanism is used by contracts such as VerifierModule and Verifier to derive their unique storage locations.

The implementation of getSlot(...) uses abi.encodePacked(...) to concatenate multiple dynamic string arguments. This function is known to be unsafe when used with multiple dynamic types, as it can lead to hash collisions. For example, the following two different sets of inputs would produce the same output:

• abi.encodePacked("VerifierModule", "example", 1);
• abi.encodePacked("Verifier", "Moduleexample", 1);

This is because abi.encodePacked directly concatenates the byte representations of the strings without any padding or delimiters.

function getSlot(string memory contractName, string memory name, uint256 version)
    internal
    pure
    returns (bytes32)
{
    // @audit-issue Use of abi.encodePacked with multiple dynamic types can lead to collisions.
    return keccak256(
        abi.encode(
            uint256(
                keccak256(
                    abi.encodePacked(
                        "mellow.flexible-vaults.storage.",
                        contractName,
                        name,
                        version
                    )
                )
            ) - 1
        )
    ) & ~bytes32(uint256(0xff));
}

This vulnerability could allow two different modules, for instance, a VerifierModule with name set to "example" and a Verifier with name set to "Moduleexample", to be assigned the same storage slot if they share the same version. This would cause the storage of one module to overwrite the other, leading to state corruption and unpredictable behavior.

Recommendation

Consider revisiting the slot generation logic in light of the potential for hash collisions with abi.encodePacked. While the risk may be low, it is important to be aware of this behavior. Adding explanatory comments to the function could also serve to inform future developers of this subtlety.

Resolution

Acknowledged, not applicable. This issue is mitigated by the current assumptions: contractName always corresponds to the actual contract name, and name is static, so storage collisions are impossible.