TORCH

UserOwnedRoyaltySplitter is a contract deployed for each original minter of the Club NFT. Three royalty recipients are configured during deployment.

    function initialize(address recipient1, address recipient2, address recipient3) external {
       ...
        recipientOne = recipient1;
        recipientTwo = recipient2;
        recipientThree = recipient3;
    }

These recipients are respectively the owner of the original NFT, the royalty machine and the owner of the club NFT contract.

UserOwnedRoyaltySplitter(splitter).initialize(msg.sender, royaltyMachine, owner());

UserOwnedRoyaltySplitter.withdraw() is a permissionless function that sends a third of the available native token balance of the contract to each of the three recipients. The last recipient, the NFT contract owner, is forwarded the whole balance. This is done in case one or more of the previous recipients couldn't receive their funds - then the owner gets everything.

    function withdraw() external {
        uint256 oneThird = address(this).balance / 3;

        (bool success,) = recipientOne.call{value: oneThird}("");
        (success,) = recipientTwo.call{value: oneThird}("");

        // remainder gets sent to recipient 3. If either previous recipients cannot receive ether, then recipient 3 will receive an extra allotment.
        (success,) = recipientThree.call{value: address(this).balance}("");

        if (!success) revert TransferFailed();
    }

However, this mechanism introduces an exploitable reentrancy vector through which royalties can be stolen by the first recipient. The first recipient can call withdraw() and reenter as much time as needed in order to withdraw most, if not all, of the contract balance.

Then all of the calls to the second recipient from the upper call contexts will revert, but this doesn't matter since the success flag is ignored.

Only the success flag of the call to the third recipient is checked, but this call will always be successful due to it using the whole balance of the contract.

Looking back at BoredApeYachtClub._deployRoyaltySplitter(), one can see that the first recipient is the owner of the original NFT.

UserOwnedRoyaltySplitter(splitter).initialize(msg.sender, royaltyMachine, owner());

The owner of the original NFT can exploit this vector and steal the royalties of the machine and the Club owner.

Add a nonReentrant modifier to UserOwnedRoyaltySplitter()

TheSplit.claimYugaLabs() and TheSplit.claimMachine() are two permissionless functions that accept an arbitrary to parameter and forward the Yuga and machine splits to that address. An attacker can call these functions with their own address and in result steal accumulated splits that should have went to Yuga and the machine.

Either make the functions callable only by Yuga and the machine or don't allow passing arbitrary to and instead hardcode Yuga and the machine.

The totalRoyaltiesReceived variable in TheSplit contract tracks the amount of native token received. It's later used to determine the amount of royalties to be distributed.

uint256 newRoyalties = totalRoyaltiesReceived - totalRoyaltiesProcessed;

The variable is increased in receive() and claimBlurPool(). However, claimBlurPool() accepts an arbitrary pool and increases the variable with the value returned from its balanceOf() function.

    function claimBlurPool(address pool) external {
        uint256 claimable = IBlurPool(pool).balanceOf(address(this));
        if (claimable == 0) revert NothingToClaim();

        IBlurPool(pool).withdraw(claimable);
        unchecked {
            totalRoyaltiesReceived += claimable;
        }
        emit RoyaltiesReceived(claimable);
    }

A fake contract can be used to return any value. This allows anyone to inflate the received royalties and cause incorrect distribution, stealing from other recipients.

The variable can also be set to its maximum value of uint256.max to completely DOS any new royalties.

Use a hardcoded address of the pool.

TheSplit.claimPool() can be invoked to claim the balance of the contract held in the blur pool.

The claimable variable is assigned the value returned from balanceOf() and is afterwards received by calling withdraw(). Then the totalRoyaltiesReceived is increased with that value and will be later used to calculate how much new royalties are to be distributed. After that the RoyaltiesReceived even is emitted.

    function claimBlurPool(address pool) external {
        uint256 claimable = IBlurPool(pool).balanceOf(address(this));
        if (claimable == 0) revert NothingToClaim();

        IBlurPool(pool).withdraw(claimable);
        unchecked {
            totalRoyaltiesReceived += claimable;
        }
        emit RoyaltiesReceived(claimable);
    }

BlurPool.withdraw() sends native tokens to TheSplit

    function withdraw(uint256 amount) external {
        uint256 balance = _balances[msg.sender];
        require(balance >= amount, "Insufficient funds");
        unchecked {
            _balances[msg.sender] = balance - amount;
        }
        (bool success,) = payable(msg.sender).call{value: amount}("");
        require(success, "Transfer failed");
        emit Transfer(msg.sender, address(0), amount);
    }

Because of this, TheSplit.receive() will be executed. Inside of it, the totalRoyaltiesReceived is increased again and RoyaltiesReceived - emitted as well.

    receive() external payable {
        totalRoyaltiesReceived += msg.value;
        emit RoyaltiesReceived(msg.value);
    }

Therefore, any royalties claimed by the pool will be counted twice in totalRoyaltiesReceived and the event will be emitted twice. This will lead to incorrect royalties distribution for the different receivers.

Remove the duplicated logic from claimBlurPool

    function claimBlurPool(address pool) external {
        uint256 claimable = IBlurPool(pool).balanceOf(address(this));
        if (claimable == 0) revert NothingToClaim();

        IBlurPool(pool).withdraw(claimable);
-       unchecked {
-           totalRoyaltiesReceived += claimable;
-       }
-       emit RoyaltiesReceived(claimable);
    }

For the first mint of a tokenId the BoredApeYachtClub deploys a splitter for the user and passes owner() as one of the recipients.

    function _deployRoyaltySplitter(uint256 tokenId) internal {
        address payable splitter = payable(address(new UserOwnedRoyaltySplitter{salt: bytes32(tokenId)}()));
        UserOwnedRoyaltySplitter(splitter).initialize(msg.sender, royaltyMachine, owner());
        emit RoyaltySplitterDeployed(tokenId, msg.sender, address(splitter));
    }

Then UserOwnedRoyaltySplitter.initialize() reverts if any of the recipients is address(0).

    function initialize(address recipient1, address recipient2, address recipient3) external {
        if (recipientOne != address(0) || recipientTwo != address(0) || recipientThree != address(0)) {
            revert AlreadyInitialized();
        }
        if (recipient1 == address(0) || recipient2 == address(0) || recipient3 == address(0)) {
            revert InvalidRecipient();
        }
     ...
   }

Therefore, once the owner of the contract uses renounceOwnership() all new mints will be disabled, as they will start failing with InvalidRecipient().

If renouncing ownership is not a desirable feature, override the function to revert in order to disable renouncing.

Otherwise, use another address as fee collector instead of owner().

Two of the recipients in the UserOwnedRoyaltySplitter contract are the royaltyMachine and the owner of the club collection. Once set, these recipients cannot be changed, so if a modification happens in the BAYC contract, the royalties will keep going to the old addresses.

Instead of setting recipientTwo and recipientThree during deployment, read them from BAYC any time they are needed.

BoredApeYachtClub.setSplitContract() can be used by accounts with the SPLIT_MANAGEMENT_ROLE role to enable, disable or change the split contract. However, changing this variable leads to incorrect accounting for the old split.

Once changed, onMint() won't be called on the old split. This creates a discrepancy between the data in TheSplit because the contract reads the storage variables of the BAYC. Example:

• There is 400 minter reward to be split across 2 users, each owning 1 token
• The split is changed to another contract
• One of the users mints another NFT, onMint() is called only on the new splitter
• When this user calls TheSplit.claimMinter() on the old contract, they would be able to claim all of the 400 tokens, because to the split it appears like they own 2 NFT and their accumulated reward hasn't been updated

Either:

• make setSplitContract() callable only once and make TheSplit read machine and yugaLabs from BAYC instead of having them as immutables
• introduce a way to freeze the old splitter by caching all the needed variables from BAYC before migrating to a new splitter

In UserOwnedRoyaltySplitter, the value for _BLUR_POOL is assigned at the top but is never used, since withdrawBlurPool accepts a user-supplied pool address.

Unlike the split contract, the impact here is limited. However, allowing the contract to make an external call to an arbitrary user-provided address is still not considered best practice.

Consider using hardcoded blur pool address instead.

TheSplit is designed to work with native tokens only. Any ERC20s received as royalties will be stuck there since there is no way to take them out.

Consider whether support for ERC20s is needed.

BoredApeYachtClub.totalSupply() returns the amount of underlying NFTs staying in the contract.

    function totalSupply() external view returns (uint256) {
        return IERC721(BAYC_LEGACY).balanceOf(address(this));
    }

Anyone can donate their BAYC_LEGACY NFT token to increase totalSupply() without an actual club token being minted.

This is unusual for ERC721.totalSupply() and can be misused by third-party integrators.

Consider having an internal totalSupply tracker.

All royalties coming from a BAYC NFT sale that go to TheSplit are split evenly between the original owners of the tokens. Some tokens are traded at a much higher price than others, but due to the way TheSplit works, its rightful owner will receive as much as the other participants.

This also creates a MEV opportunity. For example, if a very expensive NFT is bought, an external party can frontrun the royalty transaction and enter the system before that in order to receive a part of that royalty.

Keep that design in mind and consider if it's acceptable.

The owner of the new BAYC would have to set the token type and list it on the transfer validator for transfers to be processed.

setTokenTypeOfCollection and a placeholder.

Limitbreak’s payment processor enforces the payment of royalties, but the exchange used by users may not. Regular monitoring would be needed to block such operators if BAYC adopts a blacklist approach. As per current documention, Opensea, X2Y2 enforce it while Blur doesn't.

Beware of these considerations

Technically, you should also add support for the IERC165 interface for this to work. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/239795bea728c8dca4deb6c66856dd58a6991112/contracts/utils/introspection/ERC165.sol#L23

Also according to Limitbreak’s implementation of ERC721C, they also return support for their legacy interface.

In practice, we’ve never seen anyone actually use supportsInterface in the first place, but we are noting the difference so you’re aware of it.

https://github.com/limitbreakinc/creator-token-standards/blob/980a63b33591d568b6e04b45f37deba05a55f787/src/erc721c/ERC721C.sol#L39-L40

Beware of this consideration and if deemed important consider fixing it.

The official ERC721C implementation by Limitbreak also includes a feature called AutomaticValidatorTransferApproval, which allows the transfer validator to be automatically approved as an operator.

We’re not sure whether this is something desired for BAYC, but we wanted to note it in case you weren’t aware.

https://github.com/limitbreakinc/creator-token-standards/blob/980a63b33591d568b6e04b45f37deba05a55f787/src/erc721c/ERC721C.sol#L23-L24

Beware of this feature and decide if its something that should be enabled for BAYC.

The new BAYC has an optional burnAllowed feature. If enabled, it allows users to burn their ERC721C token to receive the legacy BAYC.

However, this burn operation is not compatible with a split contract. Even if someone burns their token, there is no hook like onBurn (similar to onMint), so that address would continue receiving its share of royalties in perpetuity.

If burn is enabled, someone could even snipe NFT exchange order books and do buy → mint → burn → sell in a single transaction, while still retaining royalties forever.

They could also burn, transfer (sell), and have the buyer mint, effectively bypassing transfer validations.

We raised this to BAYC team, and it was noted that this feature most likely won't be used and is a emergency escape hatch.

BAYC_LEGACY value is unnecessarily assigned on top as immutable and then reassigned in constructor.

Consider keeping one of the assignments.

The limitbreak's implementation of ERC721C bypasses the validation if caller is validator itself.

 function _preValidateTransfer(
        address caller,
        address from,
        address to,
        uint256 tokenId,
        uint256 /*value*/) internal virtual override {
        address validator = getTransferValidator();

        if (validator != address(0)) {
            if (msg.sender == validator) {
                return;
            }

            ITransferValidator(validator).validateTransfer(caller, from, to, tokenId);
        }
    }

The BAYC's implementation doesn't. Hence if BAYC uses the default list provided, it may not allow transfers called by validators itself.

Beware of this difference, if transfers made from validator is something BAYC wants to allow, either consider adding validator to the list manually OR add override.

The current royalty architecture deploys one UserOwnedRoyaltySplitter contract per tokenId on first mint. While this works correctly, it introduces significant and unnecessary gas costs during minting. Importantly, the deployed splitter is not token-owned in practice:

• The splitter is initialized with the original minter as the primary recipient.
• If burning is disabled, the original minter relationship is permanent.
• All splitters have identical logic and identical recipient structure (minter / machine / Yuga).
• Different tokenIds minted by the same user ultimately route royalties to the same address(es).

As a result, per-token splitters provide no additional economic or functional guarantees beyond having a distinct contract address per tokenId. Their only real benefit is address-level isolation and per-token royalty history, which is primarily an indexing and analytics concern, not a correctness or security requirement.

If the offchain benefits from the current approach are not important to you, consider deploying a royalty minter per user only once and adding a mapping that tracks the original owner of each tokenID in order to route royaltyInfo() correctly.

The BoredApeYachtClub.hasRole() function is intended to be used by the CreatorTokenTransferValidator. If it's used for any other purposes, it's return value will likely not be correct. For example, any role besides the default admin will return false. If third party integrators are not aware of this behavior, they may take wrong decisions based on the returned value.

Consider documenting that this function shouldn't be used, or even revert if the caller is not the transfer validator.

Yuga deploys a new user-owned splitter for each mint. Instead, they could consider using a deterministic clone model, which keeps a single implementation contract with minimal proxy clones deployed for each mint.

This would lead to a reduction in gas costs for each mint, since much smaller bytecode would be deployed.

Consider this approach

The splitter contract includes a function to claim royalties from the Blur pool. For standard royalty payments, funds are received directly via the receive function and are instantly accounted for, immediately increasing totalReceived. However, Blur royalties are handled differently, the royalty amount is credited within the Blur pool, but the funds are not transferred to the splitter contract until claimBlurPool is called. Only at that point does totalReceived increase. If a user mints after a blur accounting has occurred but before claimBlurPool is called, they will still receive a share of those previously earned royalties. This results in incorrect royalty distribution and unfair payouts.

Since yuga team mentioned that they dont expect blur pool claims, this could be left as it is. Just beware of this case.

The TheSplit contract uses an accumulator pattern that only tracks ETH-based royalties via totalRoyaltiesReceived (incremented in receive()). When royalties are paid in ERC20 or ERC721 tokens, the Yuga team intends to rescue them via rescueERC20/rescueERC721, swap to ETH, and send the ETH back to the splitter.

However as per above, users who mint after the original ERC20/ERC721 was received but before the ETH swap is deposited will also get the same treatment as if those royalties came in after their mint. This means the timing of when non-ETH royalties are converted and deposited as ETH creates arbitrary reward distribution unrelated to when the original royalty was actually earned.

There is no easy solution. Even if the Yuga team acts quickly to swap and deposit, the vector exists during the mint period (until all 10,000 BAYC are minted).

Possible mitigations:

1. Accept this as a known limitation during the mint phase
2. Consider a more complex accounting system that tracks "pending non-ETH royalties" separately
3. Pause minting while significant ERC20/ERC721 royalties are being processed (operationally complex)

The TheSplit contract includes both rescueERC20 and rescueERC721 functions (TheSplit.sol:204-217) to handle royalties received in non-ETH tokens.

However, UserOwnedRoyaltySplitter only implements withdrawERC20 (UserOwnedRoyaltySplitter.sol:60-70) and has no mechanism to handle ERC721 tokens that may be sent to user-owned splitter contracts.

If an ERC721 is sent to a UserOwnedRoyaltySplitter as royalties, it will be permanently stuck.

Consider adding rescue for ERC721 here as well.

The BAYC contract allows the owner to rescind ownership by setting owner = address(0). In this case, the code intentionally treats RoyaltyMachine as the effective owner.

However, in the Split contract, this same returned owner (RoyaltyMachine when owner == address(0)) is responsible for rescuing stuck ERC20 and ERC721 tokens. If RoyaltyMachine does not implement methods to call the Split contract’s rescue functions, those assets become stuck once ownership is rescinded.

Beware of this constraint while developing royalty machine.

The hasRole() function in the BAYC contract reverts if the role being checked is not address(0). However, it checks the role against address(0) again on the next line.

    function hasRole(bytes32 role, address account) external view returns (bool) {
        if (role != bytes32(0)) revert UseHasAnyRoleInstead();

        return (role == bytes32(0) && hasAnyRole(account, TRANSFER_VALIDATOR_MANAGEMENT_ROLE));
    }

Remove the second check.

    function hasRole(bytes32 role, address account) external view returns (bool) {
        if (role != bytes32(0)) revert UseHasAnyRoleInstead();

-       return (role == bytes32(0) && hasAnyRole(account, TRANSFER_VALIDATOR_MANAGEMENT_ROLE));
+       return hasAnyRole(account, TRANSFER_VALIDATOR_MANAGEMENT_ROLE);
    }

TheSplit.updateRecipients() reads recipientTwo and recipientThree from BAYC and updates its storage variables. In BAYC, these are the royaltyMachine and the owner, or if there is no owner it's twice the royalty machine.

If the owner of the BAYC contract changes one of the recipients before claiming their share, the split royalties may be forwarded to the old addresses before updateRecipients() is called.

Consider removing the updateRecipients() function and instead fetch the appropriate recipient on each call to claimYugaLabs() and claimMachine().