Joyn contest - robee's results

Lines of code

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

Vulnerability details

the balance of outputToken is checked to be exactly a specified value that is not declared in this specific function. Therefore, a malicious user can transfer to the contract address tiny amount of tokens and the user transactions will always revert.

    Potential DoS in RoyaltyVault.sol, 43
    Potential DoS in RoyaltyVault.sol, 47
    Potential DoS in RoyaltyVault.sol, 51

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    RoyaltyVaultFactory.setPlatformFeeRecipient (_platformFeeRecipient)
    RoyaltyVaultFactory.setPlatformFee (_platformFee)
    SplitFactory.setPlatformFeeRecipient (_platformFeeRecipient)
    CoreCollection.initialize (_mintFee)
    RoyaltyVault.setPlatformFeeRecipient (_platformFeeRecipient)
    RoyaltyVault.setPlatformFee (_platformFee)
    SplitFactory.setPlatformFee (_platformFee)

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    Splitter.sol: function attemptETHTransfer parameter value isn't used. (attemptETHTransfer is private)

Title: Not verified claimer Severity: Low Risk

If a functions gets as input a claimer param, then it should make sure the claimer address is not address(0). Otherwise it will cause to loss of the funds or access.

    ERC721Claimable.sol._claim claimer

Title: Missing commenting Severity: Low Risk

    The following functions are missing commenting as describe below:
    
    MultiSigWallet.sol, external_call (private), parameters destination, value, dataLength, data not commented
    SplitProxy.sol, splitter (public), @return is missing
    MultiSigWallet.sol, external_call (private), @return is missing

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    ICoreCollection.sol

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    
    MultiSigWallet.sol.replaceOwner owner
    MultiSigWallet.sol.removeOwner owner
    MultiSigWallet.sol.addOwner owner
    MultiSigWallet.sol.replaceOwner newOwner

Title: Duplicates in array Severity: Low Risk

You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.

MultiSigWallet.addOwner pushed (owner) 

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    MockCollection.sol.setRoyaltyVault _royaltyVault
    Splitter.sol.setClaimed who
    CoreCollection.sol.setRoyaltyVault _royaltyVault
    RoyaltyVault.sol.setPlatformFeeRecipient _platformFeeRecipient

Title: Require with empty message Severity: Low Risk

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: MultiSigWallet.sol, In line 99 with Empty Require message.
    Solidity file: CoreMultiSig.sol, In line 21 with Empty Require message.

Title: More efficient Struct packing of Transaction in the contract MultiSigWallet.sol Severity: GAS

The following structs could change the order of their stored elements to decrease memory uses and number of occupied slots. Therefore will save gas at every store and load from memory.

In MultiSigWallet.sol, Transaction is optimized to: 3 slots from: 4 slots. The new order of types (you choose the actual variables):

    1. uint256
    2. bytes
    3. address
    4. bool

Title: Unnecessary equals boolean Severity: GAS

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

    RoyaltyVault.sol, 55: ) == true,
    RoyaltyVault.sol, 48: ISplitter(splitterProxy).incrementWindow(splitterShare) == true,
    MultiSigWallet.sol, 270: if (count == required) return true;
    RoyaltyVault.sol, 44: IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,

Title: Unnecessary cast Severity: Gas

    Collection CoreFactory.sol.addCollection - unnecessary casting Collection(_collection)

Title: Change transferFrom to transfer Severity: GAS

'transferFrom(address(this), , **)' could be replaced by the following more gas efficient 'transfer(, **)' This replacement is more gas efficient and improves the code quality.

    CoreCollection.sol, 175 : payableToken.transferFrom(address(this), msg.sender, amount);

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    platformFee in SplitFactory.sol
    platformFee in RoyaltyVaultFactory.sol
    platformFeeRecipient in RoyaltyVaultFactory.sol
    royaltyVault in ProxyVault.sol
    platformFeeRecipient in SplitFactory.sol

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: CoreCollection.sol, In line 204, Require message length to shorten: 35, The message: CoreCollection: Hashed Proof is set
    Solidity file: RoyaltyVault.sol, In line 47, Require message length to shorten: 35, The message: Failed to increment splitter window

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: Splitter.sol, i, 274
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 98
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 357
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 393
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 314
    change to prefix increment and unchecked: CoreFactory.sol, i, 79
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 363
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 127
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 147
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 384
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 268
    change to prefix increment and unchecked: Splitter.sol, i, 50
    change to prefix increment and unchecked: MultiSigWallet.sol, i, 330
    change to prefix increment and unchecked: CoreCollection.sol, i, 279

Title: Use bytes32 instead of string to save gas whenever possible Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    CoreCollection.sol (L27), string public HASHED_PROOF = ""; 

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    CoreCollection.setCollectionMeta (_collectionName)
    MultiSigWallet.external_call (data)
    SplitFactory.createSplit (_splitId)
    CoreCollection.initialize (_collectionName)
    CoreFactory.addCollection (_projectId)
    CoreCollection.initialize (_collectionSymbol)
    CoreCollection.initialize (_collectionURI)
    CoreCollection.setCollectionMeta (_collectionSymbol)
    CoreFactory.getProject (_projectId)

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    CoreCollection.sol, _baseURI, { return _baseUri; }
    Splitter.sol, getClaimHash, { return keccak256(abi.encodePacked(who, window)); }
    ERC721Claimable.sol, getNode, { return keccak256(abi.encodePacked(who, claimableAmount)); }

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    MultiSigWallet.sol, 127
    MultiSigWallet.sol, 330
    CoreCollection.sol, 279
    Splitter.sol, 50
    Splitter.sol, 274
    MultiSigWallet.sol, 314
    MultiSigWallet.sol, 98
    MultiSigWallet.sol, 268
    MultiSigWallet.sol, 147

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    VaultStorage.sol, platformFeeRecipient
    SplitStorage.sol, balanceForWindow
    SplitStorage.sol, depositedInWindow
    Splitter.sol, PERCENTAGE_SCALE
    SplitStorage.sol, merkleRoot
    ERC721Payable.sol, mintFee
    SplitStorage.sol, _splitter
    ERC721Payable.sol, splitFactory
    SplitStorage.sol, claimed
    ERC721Payable.sol, isForSale
    SplitStorage.sol, splitAsset
    SplitStorage.sol, currentWindow
    VaultStorage.sol, royaltyAsset
    VaultStorage.sol, splitterProxy

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    ERC721Claimable.sol, _claim
    ERC721Claimable.sol, _setMerkelRoot
    CoreCollection.sol, _baseURI
    Splitter.sol, attemptETHTransfer
    Splitter.sol, amountFromPercent
    ERC721Payable.sol, _handlePayment

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    MultiSigWallet.sol, addTransaction
    CoreCollection.sol, _beforeTokenTransfer
    CoreCollection.sol, batchMint
    ERC721Claimable.sol, getNode
    MultiSigWallet.sol, external_call

Title: Unnecessary array boundaries check when loading an array element twice Severity: GAS

There are places in the code (especially in for-each loops) that loads the same array element more than once. 
In such cases, only one array boundaries check should take place, and the rest are unnecessary.
Therefore, this array element should be cached in a local variable and then be loaded
again using this local variable, skipping the redundant second array boundaries check: 

    MultiSigWallet.sol.constructor - double load of owners[i]
    MultiSigWallet.sol.constructor - double load of _owners[i]

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    MultiSigWallet.sol, addTransaction
    CoreCollection.sol, _beforeTokenTransfer
    ERC721Claimable.sol, _claim
    ERC721Claimable.sol, _setMerkelRoot
    CoreCollection.sol, _baseURI
    ERC721Payable.sol, _handlePayment

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    MultiSigWallet.sol, owners, 147
    MultiSigWallet.sol, owners, 314
    Splitter.sol, proof, 274
    MultiSigWallet.sol, _owners, 98
    MultiSigWallet.sol, owners, 268
    CoreFactory.sol, _collections, 79
    MultiSigWallet.sol, owners, 357

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    CoreCollection.sol.constructor
    MockSplitFactory.sol.constructor
    MyNFT.sol.constructor
    CoreMultiSig.sol.constructor

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    CoreCollection.sol, 146: change 'amount > 0' to 'amount != 0'
    CoreMultiSig.sol, 21: change 'balance > 0' to 'balance != 0'
    Splitter.sol, 164: change 'royaltyAmount > 0' to 'royaltyAmount != 0'
    RoyaltyVault.sol, 35: change 'balance > 0' to 'balance != 0'

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    MockRoyaltyVault.sol; the inherited contracts RoyaltyVault not used
    ProxyVault.sol; the inherited contracts ProxyVault not used
    MockSplitter.sol; the inherited contracts Splitter not used

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    ERC721Claimable.sol, canClaim
    CoreCollection.sol, name
    MultiSigWallet.sol, replaceOwner
    CoreCollection.sol, symbol
    MultiSigWallet.sol, removeOwner
    MultiSigWallet.sol, revokeConfirmation
    MultiSigWallet.sol, getTransactionIds
    MultiSigWallet.sol, addOwner
    Splitter.sol, isClaimed
    MyNFT.sol, mintNFT
    MultiSigWallet.sol, isConfirmed
    ERC721Claimable.sol, claimableSet
    CoreCollection.sol, baseURI
    MultiSigWallet.sol, getConfirmationCount
    Splitter.sol, incrementWindow
    MultiSigWallet.sol, submitTransaction
    MultiSigWallet.sol, getConfirmations
    RoyaltyVault.sol, getSplitter
    RoyaltyVault.sol, supportsInterface
    MultiSigWallet.sol, getTransactionCount
    MultiSigWallet.sol, getOwners
    ERC721Claimable.sol, processProof

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    CoreCollection.sol, line 7, import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
    RoyaltyVaultFactory.sol, line 3, import {ProxyVault} from "./ProxyVault.sol";
    CoreProxy.sol, line 5, import {ICoreFactory} from "../interfaces/ICoreFactory.sol";
    MockCollection.sol, line 3, import {ICoreCollection} from "../../interfaces/ICoreCollection.sol";