QA Report #57

code423n4 · 2022-04-01T10:11:59Z

Low Risk Issues

Unused `receive()` function will lock Ether in contract

If the intention is for the Ether to be used, the function should emit or call another function, otherwise it should revert

File: royalty-vault/contracts/ProxyVault.sol (line 47)

    receive() external payable {}

File: splits/contracts/SplitProxy.sol (line 51)

    receive() external payable {}

Using `payable` with the `fallback()` function will lock Ether in the contract

The Ether is never used by any of the proxied contracts - only WETH is

File: royalty-vault/contracts/ProxyVault.sol (line 28)

    fallback() external payable {

File: splits/contracts/SplitProxy.sol (line 24)

    fallback() external payable {

Missing checks for `address(0x0)` when assigning values to `address` state variables

File: core-contracts/contracts/CoreProxy.sol (line 12)

        _implement = _imp;

File: core-contracts/contracts/CoreFactory.sol (line 28)

    collection = _collection;

File: core-contracts/contracts/CoreFactory.sol (line 29)

    splitFactory = _splitFactory;

File: splits/contracts/SplitFactory.sol (line 61)

    royaltyVault = _royaltyVault;

File: splits/contracts/SplitFactory.sol (line 86)

    splitAsset = _splitAsset;

File: splits/contracts/SplitFactory.sol (line 87)

    royaltyAsset = _splitAsset;

File: splits/contracts/SplitFactory.sol (line 108)

    splitAsset = _splitAsset;

File: splits/contracts/SplitFactory.sol (line 109)

    royaltyAsset = _splitAsset;

File: splits/contracts/SplitFactory.sol (line 169)

    splitterProxy = splitProxy;

Misleading `revert()` strings and variable names

The contract is not specific to either ETH or WETH. This appears to be left over from forking from mirror-xyz/splits

File: splits/contracts/Splitter.sol (line 237)

        require(didSucceed, "Failed to transfer ETH");

File: splits/contracts/Splitter.sol (line 150)

        uint256 wethBalance;

No fees may be sent if `payableToken` does not have enough decimals and amounts are small

There are tokens such as ERC884 tokens, which are ERC20-compatible and are required to have zero decimals. The fix would be to require a minimum number of decimals.

File: splits/contracts/Splitter.sol (line 103)

        scaledAmount = (amount * scaledPercent) / (10000);

Non-critical Issues

`public` functions not called by the contract should be declared `external` instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

File: core-contracts/contracts/CoreCollection.sol (line 244)

    function baseURI() public view returns (string memory) {

File: splits/contracts/Splitter.sol (line 149)

    function incrementWindow(uint256 royaltyAmount) public returns (bool) {

`constant`s should be defined rather than using magic numbers

File: royalty-vault/contracts/RoyaltyVault.sol (line 40)

        uint256 platformShare = (balanceOfVault * platformFee) / 10000;

File: splits/contracts/SplitFactory.sol (line 62)

    platformFee = 500; // 5%

File: splits/contracts/SplitFactory.sol (line 63)

    platformFeeRecipient = 0x70388C130222eae55a0527a2367486bF5D12d6e7;

File: splits/contracts/Splitter.sol (line 103)

        scaledAmount = (amount * scaledPercent) / (10000);

File: splits/contracts/Splitter.sol (line 223)

        return (amount * percent) / 100;

File: splits/contracts/Splitter.sol (line 255)

        (bool success, ) = to.call{value: value, gas: 30000}("");

Different versions of solidity used

File: royalty-vault/contracts/RoyaltyVault.sol (line 2)

pragma solidity ^0.8.4;

File: core-contracts/contracts/CoreProxy.sol (line 2)

pragma solidity ^0.8.0;

Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

File: splits/contracts/Splitter.sol (line 2)

pragma solidity ^0.8.4;

Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

File: core-contracts/contracts/ERC721Claimable.sol (line 2)

pragma solidity ^0.8.0;

File: core-contracts/contracts/CoreCollection.sol (line 2)

pragma solidity ^0.8.0;

File: core-contracts/contracts/CoreFactory.sol (line 2)

pragma solidity ^0.8.0;

Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

File: core-contracts/contracts/CoreCollection.sol (line 27)

    string public HASHED_PROOF = "";

Non-library/interface files should use fixed compiler versions, not floating ones

File: core-contracts/contracts/CoreProxy.sol (line 2)

pragma solidity ^0.8.0;

File: core-contracts/contracts/CoreCollection.sol (line 2)

pragma solidity ^0.8.0;

File: core-contracts/contracts/CoreFactory.sol (line 2)

pragma solidity ^0.8.0;

File: royalty-vault/contracts/RoyaltyVault.sol (line 2)

pragma solidity ^0.8.4;

File: royalty-vault/contracts/ProxyVault.sol (line 2)

pragma solidity ^0.8.4;

File: splits/contracts/SplitFactory.sol (line 2)

pragma solidity ^0.8.4;

File: splits/contracts/SplitProxy.sol (line 2)

pragma solidity ^0.8.4;

File: splits/contracts/Splitter.sol (line 2)

pragma solidity ^0.8.4;

Typos

File: core-contracts/contracts/CoreCollection.sol (line 181)

     * @dev All revenue (Primary sales + royalties from secondardy sales)

secondardy

File: core-contracts/contracts/CoreFactory.sol (line 139)

   * @notice Instanciates/Deploys a collection

Instanciates

File: splits/contracts/SplitFactory.sol (line 16)

  /**** Mmutable storage ****/

Mmutable

Grammar

File: core-contracts/contracts/CoreCollection.sol (line 134)

     * @param isClaim Whether the user want claim a token that has been airdropped to him or want to purchase the token

user want claim -> user wants to claim

NatSpec is incomplete

File: core-contracts/contracts/ERC721Claimable.sol (lines 55-62)

   * @param merkleProof Proof
   */
  function canClaim(
    address who,
    uint256 claimableAmount,
    uint256 claimedAmount,
    bytes32[] calldata merkleProof
  ) public view returns (bool) {

Missing: @return

File: core-contracts/contracts/CoreCollection.sol (lines 258-260)

     * @param _to Token recipient
     */
    function mint(address _to) private returns (uint256 tokenId) {

Missing: @return

File: core-contracts/contracts/CoreFactory.sol (lines 106-111)

   * @param _collection Collection that needs to be deployed
   */
  function addCollection(
    string memory _projectId,
    Collection memory _collection
  ) external onlyProjectOwner(_projectId) returns (address) {

Missing: @return

File: core-contracts/contracts/CoreFactory.sol (lines 140-145)

   * @param _collection Collection that needs to be deployed
   */
  function _createCollection(Collection memory _collection)
    private
    onlyAvailableCollection(_collection.id)
    returns (address)

Missing: @return

File: splits/contracts/SplitFactory.sol (lines 73-80)

   * @param _splitId The split identifier.
   */
  function createSplit(
    bytes32 _merkleRoot,
    address _splitAsset,
    address _collectionContract,
    string memory _splitId
  ) external onlyAvailableSplit(_splitId) returns (address splitProxy) {

Missing: @return

File: splits/contracts/SplitFactory.sol (lines 100-106)

   * @param _splitId The split identifier.
   */
  function createSplit(
    bytes32 _merkleRoot,
    address _splitAsset,
    string memory _splitId
  ) external onlyAvailableSplit(_splitId) returns (address splitProxy) {

Missing: @return

File: splits/contracts/Splitter.sol (lines 229-233)

     * @param value {uint256} Amount to transfer.
     */
    function transferSplitAsset(address to, uint256 value)
        private
        returns (bool didSucceed)

Missing: @return

Event is missing `indexed` fields

Each event should use three indexed fields if there are three or more fields

File: core-contracts/contracts/ERC721Payable.sol (lines 11-16)

  event NewPayment(
    address from,
    address to,
    uint256 amount,
    bool royaltyVaultPayment
  );

File: core-contracts/contracts/CoreCollection.sol (line 29)

    event ClaimInitialized(bytes32 root);

File: core-contracts/contracts/CoreCollection.sol (line 30)

    event NewCollectionMeta(string name, string symbol);

File: core-contracts/contracts/CoreCollection.sol (line 31)

    event NewClaim(address claimedBy, address to, uint256 tokenId);

File: core-contracts/contracts/CoreCollection.sol (line 32)

    event StartingIndexSet(uint256 index);

File: core-contracts/contracts/CoreCollection.sol (line 33)

    event RoyaltyVaultInitialized(address royaltyVault);

File: core-contracts/contracts/CoreCollection.sol (line 34)

    event NewHashedProof(string proof);

File: core-contracts/contracts/CoreCollection.sol (line 35)

    event NewWithdrawal(address to, uint256 amount);

File: core-contracts/contracts/CoreFactory.sol (line 15)

  event NewProject(string id, address creator);

File: core-contracts/contracts/CoreFactory.sol (lines 16-20)

  event NewCollection(
    string collectionId,
    address collection,
    string projectId
  );

File: royalty-vault/contracts/RoyaltyVault.sol (line 13)

    event RoyaltySentToSplitter(address indexed splitter, uint256 amount);

File: royalty-vault/contracts/RoyaltyVault.sol (lines 14-17)

    event FeeSentToPlatform(
        address indexed platformFeeRecipient,
        uint256 amount
    );

File: royalty-vault/contracts/RoyaltyVault.sol (line 18)

    event NewRoyaltyVaultPlatformFee(uint256 platformFee);

File: royalty-vault/contracts/RoyaltyVault.sol (line 19)

    event NewRoyaltyVaultPlatformFeeRecipient(address recipient);

File: splits/contracts/SplitFactory.sol (line 30)

  event SplitCreated(address indexed splitter, string splitId);

File: splits/contracts/SplitFactory.sol (lines 32-37)

  event VaultCreated(
    address indexed vault,
    address indexed splitter,
    uint256 platformFee,
    address platformFeeRecipient
  );

File: splits/contracts/Splitter.sol (lines 18-25)

    event TransferETH(
        // The account to which the transfer was attempted.
        address account,
        // The amount for transfer that was attempted.
        uint256 amount,
        // Whether or not the transfer succeeded.
        bool success
    );

File: splits/contracts/Splitter.sol (line 28)

    event WindowIncremented(uint256 currentWindow, uint256 fundsAvailable);

Non-exploitable reentrancies

Reentrancy in ProxyVault.constructor() (contracts/ProxyVault.sol#16-22):
	External calls:
	- royaltyVault = IVaultFactory(msg.sender).royaltyVault() (contracts/ProxyVault.sol#17)
	- splitterProxy = IVaultFactory(msg.sender).splitterProxy() (contracts/ProxyVault.sol#18)
	State variables written after the call(s):
	- splitterProxy = IVaultFactory(msg.sender).splitterProxy() (contracts/ProxyVault.sol#18)
Reentrancy in ProxyVault.constructor() (contracts/ProxyVault.sol#16-22):
	External calls:
	- royaltyVault = IVaultFactory(msg.sender).royaltyVault() (contracts/ProxyVault.sol#17)
	- splitterProxy = IVaultFactory(msg.sender).splitterProxy() (contracts/ProxyVault.sol#18)
	- royaltyAsset = IVaultFactory(msg.sender).royaltyAsset() (contracts/ProxyVault.sol#19)
	State variables written after the call(s):
	- royaltyAsset = IVaultFactory(msg.sender).royaltyAsset() (contracts/ProxyVault.sol#19)
Reentrancy in ProxyVault.constructor() (contracts/ProxyVault.sol#16-22):
	External calls:
	- royaltyVault = IVaultFactory(msg.sender).royaltyVault() (contracts/ProxyVault.sol#17)
	- splitterProxy = IVaultFactory(msg.sender).splitterProxy() (contracts/ProxyVault.sol#18)
	- royaltyAsset = IVaultFactory(msg.sender).royaltyAsset() (contracts/ProxyVault.sol#19)
	- platformFee = IVaultFactory(msg.sender).platformFee() (contracts/ProxyVault.sol#20)
	State variables written after the call(s):
	- platformFee = IVaultFactory(msg.sender).platformFee() (contracts/ProxyVault.sol#20)
Reentrancy in ProxyVault.constructor() (contracts/ProxyVault.sol#16-22):
	External calls:
	- royaltyVault = IVaultFactory(msg.sender).royaltyVault() (contracts/ProxyVault.sol#17)
	- splitterProxy = IVaultFactory(msg.sender).splitterProxy() (contracts/ProxyVault.sol#18)
	- royaltyAsset = IVaultFactory(msg.sender).royaltyAsset() (contracts/ProxyVault.sol#19)
	- platformFee = IVaultFactory(msg.sender).platformFee() (contracts/ProxyVault.sol#20)
	- platformFeeRecipient = IVaultFactory(msg.sender).platformFeeRecipient() (contracts/ProxyVault.sol#21)
	State variables written after the call(s):
	- platformFeeRecipient = IVaultFactory(msg.sender).platformFeeRecipient() (contracts/ProxyVault.sol#21)
Reentrancy in RoyaltyVaultFactory.createVault(address,address) (contracts/RoyaltyVaultFactory.sol#37-50):
	External calls:
	- vault = address(new ProxyVault()) (contracts/RoyaltyVaultFactory.sol#44-46)
	State variables written after the call(s):
	- delete royaltyAsset (contracts/RoyaltyVaultFactory.sol#49)
	- delete splitterProxy (contracts/RoyaltyVaultFactory.sol#48)

Reentrancy in RoyaltyVault.sendToSplitter() (contracts/RoyaltyVault.sol#31-61):
	External calls:
	- require(bool,string)(IERC20(royaltyAsset).transfer(splitterProxy,splitterShare) == true,Failed to transfer royalty Asset to splitter) (contracts/RoyaltyVault.sol#43-46)
	- require(bool,string)(ISplitter(splitterProxy).incrementWindow(splitterShare) == true,Failed to increment splitter window) (contracts/RoyaltyVault.sol#47-50)
	- require(bool,string)(IERC20(royaltyAsset).transfer(platformFeeRecipient,platformShare) == true,Failed to transfer royalty Asset to platform fee recipient) (contracts/RoyaltyVault.sol#51-57)
	Event emitted after the call(s):
	- FeeSentToPlatform(platformFeeRecipient,platformShare) (contracts/RoyaltyVault.sol#60)
	- RoyaltySentToSplitter(splitterProxy,splitterShare) (contracts/RoyaltyVault.sol#59)

The text was updated successfully, but these errors were encountered:

sofianeOuafir · 2022-04-15T16:26:55Z

high quality report

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Apr 1, 2022

code423n4 added a commit that referenced this issue Apr 1, 2022

IllIllI issue #57

959e6f5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #57

QA Report #57

code423n4 commented Apr 1, 2022

sofianeOuafir commented Apr 15, 2022

QA Report #57

QA Report #57

Comments

code423n4 commented Apr 1, 2022

Low Risk Issues

Unused receive() function will lock Ether in contract

Using payable with the fallback() function will lock Ether in the contract

Missing checks for address(0x0) when assigning values to address state variables

Misleading revert() strings and variable names

No fees may be sent if payableToken does not have enough decimals and amounts are small

Non-critical Issues

public functions not called by the contract should be declared external instead

constants should be defined rather than using magic numbers

Different versions of solidity used

Use a more recent version of solidity

Use a more recent version of solidity

Variable names that consist of all capital letters should be reserved for const/immutable variables

Non-library/interface files should use fixed compiler versions, not floating ones

Typos

Grammar

NatSpec is incomplete

Event is missing indexed fields

Non-exploitable reentrancies

sofianeOuafir commented Apr 15, 2022

Unused `receive()` function will lock Ether in contract

Using `payable` with the `fallback()` function will lock Ether in the contract

Missing checks for `address(0x0)` when assigning values to `address` state variables

Misleading `revert()` strings and variable names

No fees may be sent if `payableToken` does not have enough decimals and amounts are small

`public` functions not called by the contract should be declared `external` instead

`constant`s should be defined rather than using magic numbers

Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

Event is missing `indexed` fields