Protcol fees not properly constrained

Low/Info issue submitted by cducrest-brainbot

Summary

Due to rounding in FeeManager._buildSharesAndTargets() attributions could receive a revenue share lower than MIN_ROYALTY_BPS.

Incorrect limitation on mint and collection referrer RevshareBps could result in a revshare higher than MAX_ROYALTY_BPS.

Vulnerability Detail

The function to validate a strategy enforces that revshareBps is within [MIN_ROYALTY_BPS, MAX_ROYALTY_BPS]:

    function validateStrategy(Strategy calldata strategy_)
        external
        pure
        returns (Strategy memory strategy)
    {
        // Clamp the revshare to the range of [MIN_ROYALTY_BPS...MAX_ROYALTY_BPS]
        uint16 revshareBps = strategy_.revshareBps > MAX_ROYALTY_BPS
            ? MAX_ROYALTY_BPS
            : strategy_.revshareBps < MIN_ROYALTY_BPS ? MIN_ROYALTY_BPS : strategy_.revshareBps;

        // Clamp the royalty to the range of [0...MAX_ROYALTY_BPS]
        uint16 royaltyBps =
            strategy_.royaltyBps > MAX_ROYALTY_BPS ? MAX_ROYALTY_BPS : strategy_.royaltyBps;

        strategy = Strategy({
            asset: strategy_.asset == address(0) ? ETH_ADDRESS : strategy_.asset,
            mintFee: strategy_.mintFee,
            revshareBps: revshareBps,
            royaltyBps: royaltyBps
        });
    }

This value of strategy.revshareBps is used in createRoute() to set up the distribution of fees to attributions:

    function createRoute(
        IEdition edition_,
        uint256 tokenId_,
        Target[] calldata attributions_,
        address referrer_
    ) external onlyOwnerOrRoles(ADMIN_ROLE) returns (Target memory receiver) {
        Target memory creator = edition_.node(tokenId_).creator;

        if (attributions_.length == 0) {
            // No attributions, pay the creator directly
            receiver = creator;
        } else {
            // Distribute the fee among the creator and attributions
            (address[] memory targets, uint256[] memory revshares) = _buildSharesAndTargets(
                creator, attributions_, edition_.feeStrategy(tokenId_).revshareBps
            );

            // Create the split. The protocol retains "ownership" to enable future use cases.
            receiver = Target({
                target: splitFactory.createSplit(
                    SplitV2Lib.Split({
                        recipients: targets,
                        allocations: revshares,
                        totalAllocation: 1e6,
                        distributionIncentive: 0
                    }),
                    address(this),
                    creator.target
                    ),
                chainId: creator.chainId
            });
        }

        _feeReceivers[getRouteId(edition_, tokenId_)] = receiver;
        referrers[edition_] = referrer_;
    }

The function _buildSharesAndTargets() rounds down the revenue share distributed to each attributions:

    function _buildSharesAndTargets(
        Target memory creator,
        Target[] memory attributions,
        uint32 revshareBps
    ) internal pure returns (address[] memory targets, uint256[] memory shares) {
        uint32 attributionShares = uint32(attributions.length);
        uint32 attributionRevShare = revshareBps * 100 / attributionShares;
        uint32 creatorShare = 1e6 - (attributionRevShare * attributionShares);  // @audit rounding => we could have lower attributionRevShare than min amount if 101 attributions for example

        // Build the targets and shares arrays using this layout:
        // - targets: [creator, ...attributions]
        // - shares: [creatorShare, ...attributionShares]
        targets = new address[](attributionShares + 1);
        shares = new uint256[](attributionShares + 1);

        targets[0] = creator.target;
        shares[0] = creatorShare;

        for (uint8 i = 0; i < attributionShares; i++) {
            targets[i + 1] = attributions[i].target;
            shares[i + 1] = attributionRevShare;
        }
    }

The total revshare distributed to attributions is floor(strategy.revshareBps * 100 / attributions.length) * attributions.length. If the minimum value for strategy.revshareBps = 250 is used and attributions.length = 101 then the total revshare is floor(250 * 100 / 101) * 100 = 247 which is below the MIN_ROYALTY_BPS = 250 value enforced in validateStrategy().

Similarly the function setProtocolFees() incorrectly constrains the mint and referrer revshare:

    /// @notice Updates the protocol fees which are collected for various actions.
    /// @param protocolCreationFee_ The new protocol creation fee. This fee is collected when a new {Edition} is created. Cannot exceed {MAX_PROTOCOL_FEE}.
    /// @param protocolFlatFee_ The new protocol flat fee. This fee is collected on all mint transactions. Cannot exceed {MAX_PROTOCOL_FEE}.
    /// @param protocolFeeShareBps_ The new protocol fee share in basis points. Cannot exceed {MAX_PROTOCOL_FEE_BPS}.
    /// @param mintReferrerRevshareBps_ The new mint referrer revenue share in basis points. This plus the collection referrer share cannot exceed {MAX_ROYALTY_BPS}.
    /// @param collectionReferrerRevshareBps_ The new collection referrer revenue share in basis points. This plus the mint referrer share cannot exceed {MAX_ROYALTY_BPS}.
    /// @dev This function can only be called by the owner or an admin.
    function setProtocolFees(
        uint64 protocolCreationFee_,
        uint64 protocolFlatFee_,
        uint16 protocolFeeShareBps_,
        uint16 mintReferrerRevshareBps_,
        uint16 collectionReferrerRevshareBps_
    ) external onlyOwnerOrRoles(ADMIN_ROLE) {
        if (
            protocolCreationFee_ > MAX_PROTOCOL_FEE || protocolFlatFee_ > MAX_PROTOCOL_FEE
                || protocolFeeShareBps_ > MAX_PROTOCOL_FEE_BPS
                || (mintReferrerRevshareBps_ + collectionReferrerRevshareBps_) > MAX_BPS  // @audit should be MAX_ROYALTY_BPS
        ) {
            revert InvalidFee();
        }
        protocolCreationFee = protocolCreationFee_;
        protocolFlatFee = protocolFlatFee_;
        protocolFeeshareBps = protocolFeeShareBps_;
        mintReferrerRevshareBps = mintReferrerRevshareBps_;
        collectionReferrerRevshareBps = collectionReferrerRevshareBps_;
    }

The comments state that mintReferrerRevshareBps_ plus collectionReferrerRevshareBps_ cannot exceed MAX_ROYALTY_BPS = 9_500 while the code enforces that it cannot exceed MAX_BPS = 10_000.

Impact

Fee values may be lower than expected lowest enforced amounts for attributions, and higher than highest expected amount for mint referrer or creation referrer fees.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Use MAX_ROYALTY_BPS instead of MAX_BPS in setProtocolFees() and enforce total revshare limits in _buildSharesAndTargets().

sherlock-audit / 2024-04-titles-judging