Joyn contest - Dravee's results

Judge has assessed an item in Issue #110 as High risk. The relevant finding follows:

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

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/SplitFactory.sol#L120 https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L67

Vulnerability details

Impact

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.

Here, no timelock capabilities seem to be used

I believe this impacts multiple users enough to make them want to react / be notified ahead of time.

Recommended Mitigation Steps

Consider adding a timelock to setPlatformFee()

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

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • Findings
    • Version
      • Upgrade pragma to at least 0.8.4
    • Contract size
      • Contract is Ownable but owner capabilites are not used
    • Storage
      • Caching storage values in memory
    • Variables
      • No need to explicitly initialize variables with default values
    • Comparisons
      • Boolean comparisons
      • > 0 is less efficient than != 0 for unsigned integers (with proof)
    • For-Loops
      • An array's length should be cached to save gas in for-loops
      • ++i costs less gas compared to i++ or i += 1
      • Increments can be unchecked
    • Arithmetics
      • Unchecking arithmetics operations that can't underflow/overflow
    • Visibility
      • Consider making some constants as non-public to save gas
    • Errors
      • Reduce the size of error messages (Long revert Strings)
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Findings

Version

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
• Optimizer improvements in packed structs (>= 0.8.3)
• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4:

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;

Contract size

Contract is Ownable but owner capabilites are not used

Reduce contract size by removing Ownable given that its functionalities are not used here:

core-contracts/contracts/CoreProxy.sol:4:import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
core-contracts/contracts/CoreProxy.sol:8:contract CoreProxy is Ownable {
royalty-vault/contracts/ProxyVault.sol:6:import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
royalty-vault/contracts/ProxyVault.sol:8:contract ProxyVault is VaultStorage, Ownable {

Storage

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

core-contracts/contracts/CoreCollection.sol:
  225          startingIndex =
  226              (uint256(
  227                  keccak256(abi.encodePacked("CoreCollection", block.number))
  228              ) % maxSupply) +
  229:             1; //@audit gas: this calculation should be cached and saved in memory before being saved in startingIndex, so that the cached value can be emitted Line L231
  230          startingIndexBlock = uint256(block.number);
  231:         emit StartingIndexSet(startingIndex); //@audit gas: should emit the suggested cached memory variable in comment L229 instead of making a storage read (SLOAD) here

  304:             royaltyVault != address(0) && //@audit gas: royaltyVault should get cached in memory
  305:             IRoyaltyVault(royaltyVault).getVaultBalance() > 0 //@audit gas: should use the suggested cached royaltyVault
  306          ) {
  307:             IRoyaltyVault(royaltyVault).sendToSplitter(); //@audit gas: should use the suggested cached royaltyVault

royalty-vault/contracts/RoyaltyVault.sol:
  38:         require(splitterProxy != address(0), "Splitter is not set"); //@audit gas: splitterProxy should get cached in memory
  
  44:             IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true, //@audit gas: should use the suggested cached splitterProxy //@audit gas: royaltyAsset should get cached in memory 
  45              "Failed to transfer royalty Asset to splitter"
  46          );
  47          require(
  48:             ISplitter(splitterProxy).incrementWindow(splitterShare) == true, //@audit gas: should use the suggested cached splitterProxy
  49              "Failed to increment splitter window"
  50          );
  51          require(
  52:             IERC20(royaltyAsset).transfer( //@audit gas: should use the suggested cached royaltyAsset 
  53:                 platformFeeRecipient, //@audit gas: platformFeeRecipient should get cached in memory
  54                  platformShare
  55              ) == true,
  56              "Failed to transfer royalty Asset to platform fee recipient"
  57          );
  58  
  59:         emit RoyaltySentToSplitter(splitterProxy, splitterShare); //@audit gas: should use the suggested cached splitterProxy
  60:         emit FeeSentToPlatform(platformFeeRecipient, platformShare); //@audit gas: should use the suggested cached platformFeeRecipient

Variables

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

core-contracts/contracts/CoreCollection.sol:279:        for (uint256 i = 0; i < _amount; i++) {
splits/contracts/Splitter.sol:49:        uint256 amount = 0;
splits/contracts/Splitter.sol:50:        for (uint256 i = 0; i < currentWindow; i++) {
splits/contracts/Splitter.sol:274:        for (uint256 i = 0; i < proof.length; i++) {

I suggest removing explicit initializations for default values.

Comparisons

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:

royalty-vault/contracts/RoyaltyVault.sol:44:            IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
royalty-vault/contracts/RoyaltyVault.sol:48:            ISplitter(splitterProxy).incrementWindow(splitterShare) == true,
royalty-vault/contracts/RoyaltyVault.sol:55:            ) == true,

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

core-contracts/contracts/CoreCollection.sol:
   52          require(
   53:             _maxSupply > 0,

  146:         require(amount > 0, "CoreCollection: Amount should be greater than 0");

core-contracts/contracts/CoreFactory.sol:
  74      require(
  75:       _collections.length > 0,

royalty-vault/contracts/RoyaltyVault.sol:
  34          require(
  35:             balanceOfVault > 0,

splits/contracts/Splitter.sol:
  164:         require(royaltyAmount > 0, "No additional funds for window");

Also, please enable the Optimizer.

For-Loops

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

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

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

core-contracts/contracts/CoreCollection.sol:279:        for (uint256 i = 0; i < _amount; i++) {
core-contracts/contracts/CoreFactory.sol:79:    for (uint256 i; i < _collections.length; i++) {
splits/contracts/Splitter.sol:50:        for (uint256 i = 0; i < currentWindow; i++) {
splits/contracts/Splitter.sol:166:        currentWindow += 1;
splits/contracts/Splitter.sol:274:        for (uint256 i = 0; i < proof.length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

core-contracts/contracts/CoreCollection.sol:279:        for (uint256 i = 0; i < _amount; i++) {
core-contracts/contracts/CoreFactory.sol:79:    for (uint256 i; i < _collections.length; i++) {
splits/contracts/Splitter.sol:50:        for (uint256 i = 0; i < currentWindow; i++) {
splits/contracts/Splitter.sol:274:        for (uint256 i = 0; i < proof.length; i++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

Arithmetics

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tags for more details):

File: RoyaltyVault.sol
40:         uint256 platformShare = (balanceOfVault * platformFee) / 10000;
41:         uint256 splitterShare = balanceOfVault - platformShare; //@audit gas: should be unchecked (as L40: platformFee == 500 < 10000 so platformShare < balanceOfVault and I don't believe platformFee would ever be set to >= 10000)

Visibility

Consider making some constants as non-public to save gas

Reducing from public to private or internal can save gas when a constant isn't used outside of its contract. I suggest changing the visibility from public to internal or private here:

splits/contracts/Splitter.sol:14:    uint256 public constant PERCENTAGE_SCALE = 10e5;
splits/contracts/Splitter.sol:15:    bytes4 public constant IID_IROYALTY = type(IRoyaltyVault).interfaceId;

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

core-contracts/contracts/CoreCollection.sol:47:        require(!initialized, "CoreCollection: Already initialized");
core-contracts/contracts/CoreCollection.sol:54:            "CoreCollection: Max supply should be greater than 0"
core-contracts/contracts/CoreCollection.sol:146:        require(amount > 0, "CoreCollection: Amount should be greater than 0");
core-contracts/contracts/CoreCollection.sol:191:            "CoreCollection: Only Split Factory or owner can initialize vault."
core-contracts/contracts/CoreCollection.sol:206:            "CoreCollection: Hashed Proof is set"
core-contracts/contracts/CoreCollection.sol:222:            "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:120:            "NFT has already claimed the given window" 

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

core-contracts/contracts/CoreCollection.sol:42:        require(initialized, "CoreCollection: Not initialized");
core-contracts/contracts/CoreCollection.sol:47:        require(!initialized, "CoreCollection: Already initialized");
core-contracts/contracts/CoreCollection.sol:52:        require(
core-contracts/contracts/CoreCollection.sol:60:        require(_exists(_tokenId), "CoreCollection: Invalid token id");
core-contracts/contracts/CoreCollection.sol:146:        require(amount > 0, "CoreCollection: Amount should be greater than 0");
core-contracts/contracts/CoreCollection.sol:147:        require(
core-contracts/contracts/CoreCollection.sol:153:            require(claimableSet(), "CoreCollection: No claimable");
core-contracts/contracts/CoreCollection.sol:154:            require(
core-contracts/contracts/CoreCollection.sol:160:            require(isForSale, "CoreCollection: Not for sale");
core-contracts/contracts/CoreCollection.sol:189:        require(
core-contracts/contracts/CoreCollection.sol:204:        require(
core-contracts/contracts/CoreCollection.sol:220:        require(
core-contracts/contracts/CoreFactory.sol:35:    require(
core-contracts/contracts/CoreFactory.sol:43:    require(
core-contracts/contracts/CoreFactory.sol:51:    require(
core-contracts/contracts/CoreFactory.sol:74:    require(
core-contracts/contracts/ERC721Claimable.sol:13:    require(root != bytes32(0), 'ERC721Claimable: Not valid root');
core-contracts/contracts/ERC721Claimable.sol:18:    require(claimableSet(), 'ERC721Claimable: No claimable');
core-contracts/contracts/ERC721Claimable.sol:23:    require(!claimableSet(), 'ERC721Claimable: Claimable is already set');
core-contracts/contracts/ERC721Claimable.sol:63:    require(
core-contracts/contracts/ERC721Payable.sol:21:    require(
core-contracts/contracts/ERC721Payable.sol:29:    require(
royalty-vault/contracts/RoyaltyVault.sol:34:        require(
royalty-vault/contracts/RoyaltyVault.sol:38:        require(splitterProxy != address(0), "Splitter is not set");
royalty-vault/contracts/RoyaltyVault.sol:43:        require(
royalty-vault/contracts/RoyaltyVault.sol:47:        require(
royalty-vault/contracts/RoyaltyVault.sol:51:        require(
splits/contracts/SplitFactory.sol:48:    require(
splits/contracts/SplitFactory.sol:81:    require(
splits/contracts/SplitFactory.sol:136:    require(_vault != address(0), 'Invalid vault');
splits/contracts/SplitFactory.sol:137:    require(
splits/contracts/Splitter.sol:40:        require(
splits/contracts/Splitter.sol:117:        require(currentWindow > window, "cannot claim for a future window");
splits/contracts/Splitter.sol:118:        require(
splits/contracts/Splitter.sol:125:        require(
splits/contracts/Splitter.sol:152:        require(
splits/contracts/Splitter.sol:156:        require(
splits/contracts/Splitter.sol:162:        require(wethBalance >= royaltyAmount, "Insufficient funds");
splits/contracts/Splitter.sol:164:        require(royaltyAmount > 0, "No additional funds for window");
splits/contracts/Splitter.sol:237:        require(didSucceed, "Failed to transfer ETH");

I suggest replacing revert strings with custom errors.