QA Report

[code423n4]

QA Report

Low Issues

Unsafe ERC20 transfers

ERC20 operations can be unsafe due to different implementations and vulnerabilities in the standard.

It is therefore recommended to always either use OpenZeppelin's SafeERC20 library or at least to wrap each operation in a require statement.

Following functions have unsafe ERC20 transfers:

• CoreCollection::withdraw
• ERC721Payable::_handlePayment

Missing upper limit for platform fee

The platform fee can be set to arbitraty values in the RoyaltyVault::setPlatformFee function. The highest logical value is 10,000 = 100%.

Consider introducing an upper limit for the platform fee through a require statement.

Note that it's recommended to set the upper limit far lower than 100% to disable rug vectors.

Issues with comments

The Splitter::transferSplitAsset function has a faulty comment stating // Try to transfer ETH to the given recipient., eventhough an ERC20 token is transfered. The error message in case of failure is off too.

A parameter document for function RoyaltyVaultFactory::setPlatformFee states 5% = 200 as example for the percentage scale. This conversion rate is false.

Gas Optimizations

Don't Initialize Variables with Default Value

Issue Information: G001

Findings:

core-contracts/contracts/CoreCollection.sol::280 => for (uint256 i = 0; i < _amount; i++) {
splits/contracts/Splitter.sol::52 => uint256 amount = 0;
splits/contracts/Splitter.sol::53 => for (uint256 i = 0; i < currentWindow; i++) {
splits/contracts/Splitter.sol::278 => for (uint256 i = 0; i < proof.length; i++) {

Tools used

c4udit

Cache Array Length Outside of Loop

Issue Information: G002

Findings:

core-contracts/contracts/CoreFactory.sol::79 => for (uint256 i; i < _collections.length; i++) {
splits/contracts/Splitter.sol::278 => for (uint256 i = 0; i < proof.length; i++) {

Tools used

c4udit

Long Revert Strings

Issue Information: G007

Findings:

core-contracts/contracts/CoreCollection.sol::47 => require(!initialized, "CoreCollection: Already initialized");
core-contracts/contracts/CoreCollection.sol::146 => require(amount > 0, "CoreCollection: Amount should be greater than 0");
core-contracts/contracts/CoreCollection.sol::192 => "CoreCollection: Only Split Factory or owner can initialize vault."
core-contracts/contracts/CoreCollection.sol::207 => "CoreCollection: Hashed Proof is set"
core-contracts/contracts/CoreCollection.sol::223 => "CoreCollection: Starting index is already set"
royalty-vault/contracts/RoyaltyVault.sol::36 => "Vault does not have enough royalty Asset to send"
royalty-vault/contracts/RoyaltyVault.sol::45 => "Failed to transfer royalty Asset to splitter"
royalty-vault/contracts/RoyaltyVault.sol::49 => "Failed to increment splitter window"
royalty-vault/contracts/RoyaltyVault.sol::56 => "Failed to transfer royalty Asset to platform fee recipient"
splits/contracts/Splitter.sol::123 => "NFT has already claimed the given window"

Tools used

c4udit

Unspecific Compiler Version Pragma

Issue Information: L003

All contracts use a floating pragma. Consider specifying a concrete solidity version for non-interface contracts.

Tools used

c4udit

[sofianeOuafir]

high quality report

[deluca-mike]

Gas Optimizations became #146