Minting of tokens for affected work/collection might be broken

Summary

The minting of tokens for affected work/collection will be broken, which is a core functionality of the protocol.

The root cause is that when computing the mint free, the assets to be collected as fees will always be hardcoded to Native ETH, which ignores the strategy's asset.

As a result, the minting of works that have strategy's asset configured to ERC20 tokens will always revert because the protocol will always expect the Native ETH fee to be sent to the function, while the users expect the fee to be pulled from their wallets as ERC20 tokens.

Vulnerability Detail

The following is an extract from the contest's README that states that only native ETH is supported across the protocol.

If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of weird tokens you want to integrate?

The current version supports native ETH

When someone creates a new work in the Edition, the validateStrategy function at Line 121 will be executed to ensure that the work's strategy conforms with the protocol requirements (e.g., Only Native ETH is supported)

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L121

File: Edition.sol
095:     /// @notice Create a new work in the Edition.
096:     /// @param creator_ The creator of the work.
097:     /// @param maxSupply_ The maximum number of mintable tokens for the work.
098:     /// @param opensAt_ The timestamp after which the work is mintable.
099:     /// @param closesAt_ The timestamp after which the work is no longer mintable.
100:     /// @param attributions_ The attributions for the work.
101:     /// @param strategy_ The fee strategy for the work.
102:     /// @param metadata_ The metadata for the work.
103:     function publish(
104:         address creator_,
105:         uint256 maxSupply_,
106:         uint64 opensAt_,
107:         uint64 closesAt_,
108:         Node[] calldata attributions_,
109:         Strategy calldata strategy_,
110:         Metadata calldata metadata_
111:     ) external override onlyRoles(EDITION_MANAGER_ROLE) returns (uint256 tokenId) {
112:         tokenId = ++totalWorks;
113: 
114:         _metadata[tokenId] = metadata_;
115:         works[tokenId] = Work({
116:             creator: creator_,
117:             totalSupply: 0,
118:             maxSupply: maxSupply_,
119:             opensAt: opensAt_,
120:             closesAt: closesAt_,
121:             strategy: FEE_MANAGER.validateStrategy(strategy_)
122:         });

While reviewing the validateStrategy function, it was found that the protocol allows the assets to be set to other ERC20 assets apart from the native ETH based on the logic in Line 337 below. As a result, it is possible for some users to create a new work with the assets set to ERC20 tokens, resulting in fees to be collected in the defined ERC20 tokens (e.g., USDC)

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L337

File: FeeManager.sol
322:     function validateStrategy(Strategy calldata strategy_)
323:         external
324:         pure
325:         returns (Strategy memory strategy)
326:     {
..SNIP..
336:         strategy = Strategy({
337:             asset: strategy_.asset == address(0) ? ETH_ADDRESS : strategy_.asset,

However, the issue is that when computing the mint free, the assets to be collected as fees will always be hardcoded to Native ETH as per Line 256 below, which ignores the strategy's asset. As a result, the minting of works that have strategy's asset configured to ERC20 tokens will always revert because the protocol will always expect the Native ETH fee to be sent to the function, while the users expect the fee to be pulled from their wallets as ERC20 tokens.

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L256

File: FeeManager.sol
243:     /// @notice Calculates the mint fee for a given {Strategy} based on the quantity of tokens being minted.
244:     /// @param strategy_ The {Strategy} for which to calculate the mint fee.
245:     /// @param quantity_ The quantity of tokens being minted.
246:     /// @return fee The {Fee} for minting the tokens.
247:     /// @dev The mint fee is calculated as the sum of:
248:     ///      - The {Strategy.mintFee}.
249:     ///      - The {protocolFlatFee}.
250:     function getMintFee(Strategy memory strategy_, uint256 quantity_)
251:         public
252:         view
253:         returns (Fee memory fee)
254:     {
255:         // Return the total fee (creator's mint fee + protocol flat fee)
256:         return Fee({asset: ETH_ADDRESS, amount: quantity_ * (strategy_.mintFee + protocolFlatFee)});
257:     }

Impact

The minting of tokens for affected work/collection will be broken, which is a core functionality of the protocol.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L121

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L337

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L256

Tool used

Manual Review

Recommendation

If the protocol requirement is to only support Native ETH as the fee asset, the code should not allow users to define ERC20 tokens as the fee assets during work creation to avoid potential issues.

Consider making the following changes to ensure that only Native ETH is accepted.

    function validateStrategy(Strategy calldata strategy_)
        external
        pure
        returns (Strategy memory strategy)
    {
                ..SNIP..
+               if (strategy_.asset != address(0)) revert ERROR_OnlyNativeETHAccepted();
        strategy = Strategy({
            asset: strategy_.asset == address(0) ? ETH_ADDRESS : strategy_.asset,
            mintFee: strategy_.mintFee,
            revshareBps: revshareBps,
            royaltyBps: royaltyBps
        });
    }

sherlock-audit / 2024-04-titles-judging

xiaoming90 - Minting of tokens for affected work/collection might be broken #276