Joyn contest - 0x's results

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L14 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L103

Vulnerability details

Impact

PERCENTAGE_SCALE in Splitter.sol is defined as 1e6 and this is not aligned with used percentage scale, which is an inverse of a basis point - 1e4.

Contracts rely on the values displayed by external function of a contract. They may align a percentage to the read scale, while the used one will be drastically (100x) different, causing loss of funds if the contract is prepared to only claim more than 1% of the tokens.

Also users who interact with the contract can call it and the transaction will be reverted due to not enough allowance in the merkle tree.

Thus marked severity is medium - an accounting bug in core functionality, can impact protocol availability.

Tools used

Manual analysis

Recommended Mitigation Steps

Use PERCENTAGE_SCALE in calculations instead of other hardcoded values.

You also may want to change the value of PERCENTAGE_SCALE to 1e4 if percentages are supposed to be in basis points.

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