ERC721OwnerFeeSplitManager.sol

Similar to the AddressFeeSplitManager, except this implementation allows holders of specific NFT collection tokens to make claims relative to their holdings.

These results only pertain to changes required in the ERC721OwnerFeeSplitManager.sol. Inherited files are reported separately.

[CRITICAL] Malicious ClaimParams can result in ETH drain

If a user enters malicious ClaimParams into their claim that included the same collection and tokenIds multiple times, they would be still deemed a valid recipient and have an incorrect share calculated.

This should be resolved by validating the ClaimParams in each function via an internal function, or just directly when calculating isValidRecipient as this will be the precursory function call.

Example Attack

address[] memory erc721 = new erc721[](3);
uint[] memory tokenIds = new tokenIds[][](3);

// Build a `ClaimParams` that claims against the same tokenId1 three times in a single call
for (uint i; i < erc721.length; ++i) {
    erc721[i] = NFT_COLLECTION_1;
    tokenIds[i] = new uint[](1);
    tokenIds[i][0] = 1;
}

// This will claim a share 3x what we should be able to claim
manager.claim(
    ERC721OwnerFeeSplitManager.ClaimParams(erc721, tokenIds)
);

Proposed Solution

To avoid this from happening, this is (untested) code that will iterate over all of the ClaimParams and ensure that the user has not provided multiple ERC721 + tokenId combinations. This gives the flexbility to allow for the same ERC721 contract to be passed multiple times, but never including the same tokenId.

error InvalidClaimParams();
error DuplicateTokenId(address erc721, uint256 tokenId);

/**
 * @notice Validates claim parameters to ensure no duplicate ERC721 + tokenId combinations
 *
 * @dev Uses a mapping approach for O(n) efficiency where n is the total number of tokenIds
 *
 * @param _claimParams The claim parameters to validate
 */
function _validateClaimParams(ClaimParams memory _claimParams) internal pure {
    // Basic validation
    if (_claimParams.erc721.length == 0 || 
        _claimParams.erc721.length != _claimParams.tokenIds.length) {
        revert InvalidClaimParams();
    }
    
    // Use a nested mapping to track which tokenIds have been seen for each erc721 contract
    // We'll use memory for this since it's a stateless validation
    mapping(address => mapping(uint256 => bool)) memory seen;
    
    // Iterate through each erc721 contract
    for (uint256 i = 0; i < _claimParams.erc721.length; i++) {
        address erc721Contract = _claimParams.erc721[i];
        uint256[] memory ids = _claimParams.tokenIds[i];
        
        // For each token ID in this contract
        for (uint256 j = 0; j < ids.length; j++) {
            uint256 tokenId = ids[j];
            
            // Check if this combination has been seen before
            if (seen[erc721Contract][tokenId]) {
                revert DuplicateTokenId(erc721Contract, tokenId);
            }
            
            // Mark this combination as seen
            seen[erc721Contract][tokenId] = true;
        }
    }
}

[INFO] Misleading contract code commenting

The contract code commenting includes references to Chainlink functionality, though no Chainlink functionality exists. This should be removed to avoid misleading readers.

Proposed Solution

This contract allows holders of any number of same-chain ERC721 contracts to receive an allocation from all memestreams held within the manager.

[INFO] Enumerable.Set imported but not used

The EnumerableSet library is imported from OpenZeppelin and the contract is using EnumerableSet for EnumerableSet.UintSet, but there is no usage of the library in the contract.