MAX_BPS of FeeManager may not match Solady's ERC2981 _feeDenominator() dimension

Low/Info issue submitted by cducrest-brainbot

Summary

There is an implicit assumption that the fee denominator used internally in Solady's implementation of ERC2981 is 10_000 and will not change across updates. If this assumption is broken because Solady decides to use a different value and the dependencies are updated, the royaltyInfo() calculation will be broken.

Vulnerability Detail

To define royalty fees, FeeManager uses bps values in terms of x/10_000 as can be seen in the definition of MAX_ROYALTY_BPS, MIN_ROYALTY_BPS, MAX_BPS and their usage:

    /// @dev The maximum basis points (BPS) value. Equivalent to 100%.
    uint16 public constant MAX_BPS = 10_000;  // @audit this needs to match with Solady's ERC2981, nothing guarantees it

    /// @dev The maximum protocol fee in basis points (BPS). Equivalent to 33.33%.
    uint16 public constant MAX_PROTOCOL_FEE_BPS = 3333;

    /// @dev The maximum protocol fee in wei. Applies to both flat and percentage fees.
    uint64 public constant MAX_PROTOCOL_FEE = 0.1 ether;

    /// @dev The maximum royalty fee in basis points (BPS). Equivalent to 95%.
    uint16 public constant MAX_ROYALTY_BPS = 9500;

    /// @dev The minimum royalty fee in basis points (BPS). Equivalent to 2.5%.
    uint16 public constant MIN_ROYALTY_BPS = 250;

    ...

    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
        });
    }

Solady uses an internal value to calculate royalties, and also uses the denominator value of 10_000:

abstract contract ERC2981 {
    function _feeDenominator() internal pure virtual returns (uint96) {
        return 10000;
    }

    ...

    function royaltyInfo(uint256 tokenId, uint256 salePrice)
        public
        view
        virtual
        returns (address receiver, uint256 royaltyAmount)
    {
        uint256 feeDenominator = _feeDenominator();
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x00, tokenId)
            mstore(0x20, _ERC2981_MASTER_SLOT_SEED)
            let packed := sload(keccak256(0x00, 0x40))
            receiver := shr(96, packed)
            if iszero(receiver) {
                packed := sload(mload(0x20))
                receiver := shr(96, packed)
            }
            let x := salePrice
            let y := xor(packed, shl(96, receiver)) // `feeNumerator`.
            // Overflow check, equivalent to `require(y == 0 || x <= type(uint256).max / y)`.
            // Out-of-gas revert. Should not be triggered in practice, but included for safety.
            returndatacopy(returndatasize(), returndatasize(), mul(y, gt(x, div(not(0), y))))
            royaltyAmount := div(mul(x, y), feeDenominator)
        }
    }
}

The value used to calculate the output royalty is stored in Solady's ERC2981 by Edition's function setRoyaltyTarget() which calls Solady's internal _setTokenRoyalty():

    function setRoyaltyTarget(uint256 tokenId, address target)
        external
        onlyRoles(EDITION_MANAGER_ROLE)
    {
        _setTokenRoyalty(tokenId, target, works[tokenId].strategy.royaltyBps);
    }

In the above function, the feeNumerator value used is works[tokenId].strategy.royaltyBps. Edition does not enforce that the denominator used by Solady matches the expected denominator used in FeeManager.

Impact

If the denominator of Solady's ERC2981 is updated, the royalties will be incorrectly calculated, resulting in either a non-enforcement of ERC2981 or a loss of funds for royalties recipient or minters.

Code Snippet

https://github.com/Vectorized/solady/blob/a34977e56cc1437b7ac07e6356261d2b303da686/src/tokens/ERC2981.sol#L51-L53 https://github.com/Vectorized/solady/blob/a34977e56cc1437b7ac07e6356261d2b303da686/src/tokens/ERC2981.sol#L68-L92

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

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

Tool used

Manual Review

Recommendation

In Edition.sol define:

    function _feeDenominator() internal pure override returns (uint96) {
        return 10000;
    }

sherlock-audit / 2024-04-titles-judging