QA Report

[code423n4]

QA Report

1. Immutable addresses should be 0-checked

Consider adding an address(0) check in the constructors for these variables:

core-contracts/contracts/CoreFactory.sol:
  22:   address public immutable collection;
  23:   address public immutable splitFactory;

core-contracts/contracts/CoreProxy.sol:
  9:     address private immutable _implement;

splits/contracts/SplitFactory.sol:
  11:   /**** Immutable storage ****/
  13:   address public immutable splitter;
  14:   address public immutable royaltyVault;

2. Missing comments

The following comments are missing (see @audit tags):

core-contracts/contracts/CoreCollection.sol:
  254      /**
  255       * @notice Mint token
  256       * @dev A starting index is calculated at the time of first mint
  257       * returns a tokenId
  258:      * @param _to Token recipient //@audit missing @return uint256 tokenId
  259       */
  260      function mint(address _to) private returns (uint256 tokenId) {

core-contracts/contracts/CoreFactory.sol:
  101    /**
  102     * @notice Allows to add a collection to a project
  103     * @dev Can only be called by project creator
  104     * Collection's ownership is transferred to the caller
  105     * @param _projectId Project id which is a unique identifier
  106:    * @param _collection Collection that needs to be deployed //@audit missing @return address
  107     */
  108    function addCollection(
  109      string memory _projectId,
  110      Collection memory _collection  
  111    ) external onlyProjectOwner(_projectId) returns (address) {

  138    /**
  139     * @notice Instanciates/Deploys a collection
  140:    * @param _collection Collection that needs to be deployed //@audit missing @return address
  141     */
  142    function _createCollection(Collection memory _collection)
  143      private
  144      onlyAvailableCollection(_collection.id)
  145      returns (address)

core-contracts/contracts/ERC721Claimable.sol:
  49    /**
  50     * @notice Verifies whether an address can claim tokens
  51     * @dev 
  52     * @param who Claimer address
  53     * @param claimableAmount Amount airdropped to claimer
  54     * @param claimedAmount Amount of tokens claimer wants to claim
  55:    * @param merkleProof Proof //@audit missing @return bool
  56     */
  57    function canClaim(
  58      address who,
  59      uint256 claimableAmount,
  60      uint256 claimedAmount,
  61      bytes32[] calldata merkleProof
  62    ) public view returns (bool) {

splits/contracts/SplitFactory.sol:
   55    /**
   56     * @dev Constructor
   57:    * @param _splitter The address of the Splitter contract. //@audit missing @param _royaltyVault
   58     */
   59    constructor(address _splitter, address _royaltyVault) {

   68    /**
   69     * @dev Deploys a new SplitProxy and initializes collection's royalty vault.
   70     * @param _merkleRoot The merkle root of the asset.
   71     * @param _splitAsset The address of the asset to split.
   72     * @param _collectionContract The address of the collection contract.
   73:    * @param _splitId The split identifier. //@audit missing @return address splitProxy
   74     */
   75    function createSplit(
   76      bytes32 _merkleRoot,
   77      address _splitAsset,
   78      address _collectionContract,
   79      string memory _splitId
   80    ) external onlyAvailableSplit(_splitId) returns (address splitProxy) {

   96    /**
   97     * @dev Deploys a new SplitProxy.
   98     * @param _merkleRoot The merkle root of the asset.
   99     * @param _splitAsset The address of the asset to split.
  100:    * @param _splitId The split identifier. //@audit missing @return address splitProxy
  101     */
  102    function createSplit(
  103      bytes32 _merkleRoot,
  104      address _splitAsset,
  105      string memory _splitId
  106    ) external onlyAvailableSplit(_splitId) returns (address splitProxy) {

splits/contracts/Splitter.sol:
  226      /**
  227       * @dev Function to transfer split asset to the given address.
  228       * @param to {address} Address to transfer the split asset to.
  229:      * @param value {uint256} Amount to transfer. //@audit missing @return bool didSucceed
  230       */
  231      function transferSplitAsset(address to, uint256 value)
  232          private
  233          returns (bool didSucceed)

3. Avoid floating pragmas / The pragmas used are not the same everywhere: the version should be locked mandatorily at >= 0.8.4 as Custom Errors are only introduced there and several contracts wouldn't compile at an older version than this:

core-contracts/contracts/CoreCollection.sol:
  2: pragma solidity ^0.8.0;

core-contracts/contracts/CoreFactory.sol:
  2: pragma solidity ^0.8.0;

core-contracts/contracts/CoreProxy.sol:
  2: pragma solidity ^0.8.0;

core-contracts/contracts/ERC721Claimable.sol:
  2: pragma solidity ^0.8.0;

core-contracts/contracts/ERC721Payable.sol:
  2: pragma solidity ^0.8.0;

royalty-vault/contracts/ProxyVault.sol:
  2: pragma solidity ^0.8.4;

royalty-vault/contracts/RoyaltyVault.sol:
  2: pragma solidity ^0.8.4;

splits/contracts/SplitFactory.sol:
  2: pragma solidity ^0.8.4;

splits/contracts/SplitProxy.sol:
  2: pragma solidity ^0.8.4;

splits/contracts/Splitter.sol:
  2: pragma solidity ^0.8.4;

4. CoreCollection.sol should use implement a 2-step ownership transfer pattern instead of using Ownable's default one.

5. platformFee should be upper bounded to avoid DoS and excessive fees

platformFee can take a value of 10000 (100%) which could be seen as a trust issue:

File: RoyaltyVault.sol
67:     function setPlatformFee(uint256 _platformFee) external override onlyOwner {
68:         platformFee = _platformFee; //@audit low should be upperbounded to 10000 or L41 will get DOSed by an underflow. A reasonable upperbound should be declared for trust
69:         emit NewRoyaltyVaultPlatformFee(_platformFee);
70:     }

Also, although unlikely and remediable by calling again setPlatformFee with another value, sendToSplitter can get DOSed by the admin by setting platformFee to more than 10000:

File: RoyaltyVault.sol
40:         uint256 platformShare = (balanceOfVault * platformFee) / 10000;
41:         uint256 splitterShare = balanceOfVault - platformShare; //@audit DOSed by the admin if platformFee > 10000, which is possible

[sofianeOuafir]

high quality report

[deluca-mike]

last point became #147