ERC721FeeSplitManager.sol

[LOW] _validateClaimParams contains superfluous, gas-intensive code

Inside the _validateClaimParams function, there is a substantial loop over all tokens being claimed to detect duplicates, that includes multiple writes per successful iteration. This validation step is subsequently rendered pointless by the actual claim flow, as the claim against a tokenId is registered before the amount is allocated, so a duplicated token would not actually have anything extra to claim.

By removing this additional validation a large amount of gas would be saved for users making claims against large numbers of ERC721 tokens.

Recommended solution:

function _validateClaimParams(ClaimParams memory _claimParams) internal pure {
    // Basic validation to ensure lengths are correct
    if (_claimParams.erc721.length == 0 || _claimParams.erc721.length != _claimParams.tokenIds.length) {
        revert InvalidClaimParams();
    }
}

[LOW] "ERC721" contract may not support ERC721 Interface

If a specified ERC721 contract is initialized, but doesn't correctly support ERC721, then it may revert when transactions are made against it (e.g. ownerOf calls). It may be beneficial to validate that the ERC721 contracts actually implement the ERC721 interface during initialization. If not, then it would be recommended to highlight this on the frontend if an invalid address is entered.

Recommended solution:

// Add interface validation
for (uint i; i < erc721SharesLength; ++i) {
  erc721Share = params.shares[i];
  if (erc721Share.erc721 == address(0) || erc721Share.totalSupply == 0 || erc721Share.share == 0) {
    revert InvalidInitializeParams();
  }
  
  // Validate ERC721 interface
  try IERC721(erc721Share.erc721).supportsInterface(0x80ac58cd) returns (bool supported) {
    if (!supported) {
      revert InvalidERC721Interface();
    }
  } catch {
    revert InvalidERC721Interface();
  }
}

[LOW] Precision Loss in Share Calculations

The share calculations use division which can lead to precision loss. This can result in rounding errors that accumulate over time:

recipientShare_ += (erc721Share.share / erc721Share.totalSupply) * tokenIds;

Recommended solution:

// Use FullMath for better precision
recipientShare_ += FullMath.mulDiv(erc721Share.share, tokenIds, erc721Share.totalSupply);

[LOW] Missing Input Validation

The balances function with _data parameter doesn't validate the input data structure. This could result in the call reverting, rather than returning a zero value.

function balances(address _recipient, bytes calldata _data) public view returns (uint balance_) {
    (ClaimParams memory claimParams) = abi.decode(_data, (ClaimParams));
    // No validation of claimParams structure
}

Recommended solution:

function balances(address _recipient, bytes calldata _data) public view returns (uint balance_) {
  if (_data.length == 0) {
    return balances(_recipient);
  }
  
  try decodeClaimParams(_data) returns (ClaimParams memory claimParams) {
    // Validate the decoded params
    if (claimParams.erc721.length == 0 || claimParams.erc721.length != claimParams.tokenIds.length) {
      return 0;
    }
      
    // Continue with calculation...
  } catch {
    return 0;
  }
}