Gas Optimizations

[code423n4]

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 code-423n4/2021-11-fei-findings#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 code-423n4/2021-12-maple-findings#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";

[sofianeOuafir]

high quality report

[deluca-mike]

• "MultiSigWallet.sol, 270: if (count == required) return true;" is not valid, since you don't want to return count == required in the for loop.
• "Collection CoreFactory.sol.addCollection - unnecessary casting Collection(_collection)" is unclear.
• "In the following files there are state variables that could be set immutable to save gas." Not really since immutable is usually not compatible with the proxy pattern used here, and would prevent upgradeability.
• "Unused state variables" lacks understanding of the current proxy pattern
• "Unnecessary functions" not all of them
• "Inline one time use functions" not all of them, like _beforeTokenTransfer
• "The following functions could be set private to save gas and improve code quality" not all of them
• "Unnecessary constructor" nope
• "Unused inheritance" really seems like this warden is not understanding the proxy pattern, or mocks
• "Public functions to external" not all of them