Fractional v2 contest - rbserver's results

[L-01] MISSING CHECKS FOR INPUT ARRAY LENGTHS

For a function, when multiple inputs are arrays, and their items correspond to each other, the lengths of these arrays should be checked to be the same.

src\Vault.sol:73 -> function install(bytes4[] memory _selectors, address[] memory _plugins)
src\modules\Buyout.sol:417-149 -> 
    uint256[] calldata _ids,
    uint256[] calldata _values,
    bytes32[] calldata _erc1155BatchTransferProof
src\modules\protoforms\BaseVault.sol:61-62 ->
    address[] calldata _tokens,
    uint256[] calldata _amounts
src\modules\protoforms\BaseVault.sol:80-81 ->
    address[] calldata _tokens,
    uint256[] calldata _ids
src\modules\protoforms\BaseVault.sol:101-104 ->
    address[] calldata _tokens,
    uint256[] calldata _ids,
    uint256[] calldata _amounts,
    bytes[] calldata _datas
src\targets\Transfer.sol:478-479 ->
    uint256[] calldata, /*_ids*/
    uint256[] calldata /*_amounts*/

[L-02] MISSING ZERO-ADDRESS CHECK

Addresses should be checked against address(0).

src\modules\Buyout.sol:43-45 ->
    address _registry,
    address _supply,
    address _transfer
src\modules\Migration.sol:54-56 ->
    address _buyout,
    address _registry,
    address _supply
src\modules\Minter.sol:17 -> constructor(address _supply)
src\modules\protoforms\BaseVault.sol:24 -> constructor(address _registry, address _supply) Minter(_supply)
src\references\SupplyReference.sol:15 -> constructor(address _registry)
src\targets\Supply.sol:16 -> constructor(address _registry)
src\utils\Metadata.sol:16 -> constructor(address _token)

[L-03] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers.

src\modules\Buyout.sol:86-87 ->
    uint256 buyoutPrice = (msg.value * 100) /
        (100 - ((depositAmount * 100) / totalSupply));
src\modules\Buyout.sol:209-211 ->
    (tokenBalance * 1000) /     
        IVaultRegistry(registry).totalSupply(_vault) >
    500

[L-04] MISSING CHECK FOR TRANSFERFROM RETURN VALUE OR SAFETRANSFERFROM CAN BE USED INSTEAD

To monitor token transfers that may fail silently, the return value of transferFrom can be checked. Alternatively, safeTransferFrom can be used instead of transferFrom.

src\modules\protoforms\BaseVault.sol:65 -> IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

[L-05] SAME CODE CAN BE IN REUSABLE MODIFIER

To improve readability and maintainability, the same code that are used in multiple functions can be in a reusable modifier.

src\modules\Buyout.sol:318-326 ->
src\modules\Buyout.sol:350-358 ->
src\modules\Buyout.sol:386-394 ->
src\modules\Buyout.sol:423-431 ->
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if auction state is not successful
    (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
    State required = State.SUCCESS;
    if (current != required) revert InvalidState(required, current);
    // Reverts if caller is not the auction winner
    if (msg.sender != proposer) revert NotWinner(); 

[N-01] MISSING INDEXED EVENT FIELDS

The following event is missing indexed fields:

src\interfaces\IVault.sol:29 -> event InstallPlugin(bytes4[] _selectors, address[] _plugins);

[N-02] LOWER CASE LETTERS ARE USED FOR CONSTANTS

As a best practice, only capital letters should be used for constants.

src\targets\Transfer.sol:366 -> ERC1155_SAFE_TRANSFER_FROM_signature

[G-01] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it.

src\Vault.sol:78 -> for (uint256 i = 0; i < length; i++)
src\Vault.sol:104 -> for (uint256 i = 0; i < length; i++)
src\modules\protoforms\BaseVault.sol:64 -> for (uint256 i = 0; i < _tokens.length; )
src\modules\protoforms\BaseVault.sol:83 -> for (uint256 i = 0; i < _tokens.length; )
src\modules\protoforms\BaseVault.sol:107 -> for (uint256 i = 0; i < _tokens.length; ++i)
src\utils\MerkleBase.sol:51 -> for (uint256 i = 0; i < _proof.length; ++i)

[G-02] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration.

src\modules\Buyout.sol:454 -> for (uint256 i; i < permissions.length; )
src\modules\protoforms\BaseVault.sol:64 -> for (uint256 i = 0; i < _tokens.length; )
src\modules\protoforms\BaseVault.sol:83 -> for (uint256 i = 0; i < _tokens.length; )
src\modules\protoforms\BaseVault.sol:107 -> for (uint256 i = 0; i < _tokens.length; ++i)
src\modules\protoforms\BaseVault.sol:130 -> for (uint256 i; i < _modules.length; ++i)
src\modules\protoforms\BaseVault.sol:132 -> for (uint256 j; j < leaves.length; ++j)
src\utils\MerkleBase.sol:51 -> for (uint256 i = 0; i < _proof.length; ++i) {
src\utils\MerkleBase.sol:110 -> for (uint256 i; i < result.length; ++i) {

[G-03] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++.

src\Vault.sol:78 -> for (uint256 i = 0; i < length; i++)
src\Vault.sol:104 -> for (uint256 i = 0; i < length; i++)
src\utils\MerkleBase.sol:188 -> ceil++;

[G-04] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these. For loops, if increasing the counter variable is very unlikely to overflow, then unchecked {++i} at the end of the loop block can be used, where i is the counter variable.

src\Vault.sol:78 -> for (uint256 i = 0; i < length; i++)
src\Vault.sol:104 -> for (uint256 i = 0; i < length; i++)

[G-05] X = X + Y AND X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y and x = x -y costs less gas than x += y and x -= y.

src\FERC1155.sol:86 -> totalSupply[_id] += _amount;
src\FERC1155.sol:271 -> balanceOf[_to][_id] += _amount;
src\FERC1155.sol:62 -> totalSupply[_id] -= _amount;
src\FERC1155.sol:270 -> balanceOf[_from][_id] -= _amount;
src\modules\Buyout.sol:176 -> buyoutInfo[_vault].ethBalance += msg.value;
src\modules\Buyout.sol:139 -> buyoutInfo[_vault].ethBalance -= ethAmount;
src\modules\Buyout.sol:139 -> buyoutInfo[_vault].ethBalance -= ethAmount;
src\modules\Migration.sol:123 -> proposal.totalEth += msg.value;
src\modules\Migration.sol:124 -> userProposalEth[_proposalId][msg.sender] += msg.value;
src\modules\Migration.sol:134 -> proposal.totalFractions += _amount;
src\modules\Migration.sol:135 -> userProposalFractions[_proposalId][msg.sender] += _amount;
src\modules\Migration.sol:497 -> treeLength += IModule(_modules[i]).getLeafNodes().length;
src\modules\Migration.sol:156 -> proposal.totalFractions -= amount;
src\modules\Migration.sol:160 -> proposal.totalEth -= ethAmount;
src\utils\MerkleBase.sol:147 -> for (uint256 i; i < length - 1; i += 2)
src\utils\MerkleBase.sol:190 -> ceil -= pOf2;

[G-06] REVERT REASON STRINGS CAN BE SHORTENED TO FIT IN 32 BYTES

Revert reason strings that are longer than 32 bytes need more memory space and more mstore operations than these that are shorter than 32 bytes.

src\utils\MerkleBase.sol:62 -> require(_data.length > 1, "wont generate root for single leaf");
src\utils\MerkleBase.sol:78 -> require(_data.length > 1, "wont generate proof for single leaf");

[G-07] CUSTOM ERRORS CAN BE USED INSTEAD OF REVERT REASON STRINGS

Custom errors are more gas-efficient than revert reason strings.

src\FERC1155.sol:263-268 ->
    require(
        msg.sender == _from ||
        isApprovedForAll[_from][msg.sender] ||
        isApproved[_from][msg.sender][_id],
    "NOT_AUTHORIZED"
);
src\FERC1155.sol:275-286 ->
    require(
        _to.code.length == 0
        ? _to != address(0)
        : INFTReceiver(_to).onERC1155Received(
        msg.sender,
        _from,
        _id,
        _amount,
        _data
    ) == INFTReceiver.onERC1155Received.selector,
        "UNSAFE_RECIPIENT"
    );
src\FERC1155.sol:297 -> require(metadata[_id] != address(0), "NO METADATA");
src\utils\MerkleBase.sol:62 -> require(_data.length > 1, "wont generate root for single leaf");
src\utils\MerkleBase.sol:78 -> require(_data.length > 1, "wont generate proof for single leaf");
src\utils\Multicall.sol:23 -> if (result.length == 0) revert();

[G-08] CONSTANTS CAN BE PRIVATE IF POSSIBLE

For constants that are used only inside the contract, they can be private than public to cost less gas.

src\utils\SafeSend.sol:11 -> address payable public constant WETH_ADDRESS

[G-09] REDUNDANT TYPE CONVERSION CAN BE AVOIDED

Converting a variable's type when not needed costs more unnecessary gas.

src\VaultFactory.sol:87 -> address(vault)

[G-10] STATE VALUES THAT NEED TO BE READ FOR MULTIPLE TIMES CAN BE CACHED IN MEMORY

When state values need to be read and accessed for multiple times, caching them in memory can save gas.

For the following code, nextId[_token] can be cached and then increased by 1 in memory. Afterwards, the cached value can be assigned to the corresponding state variable and used as the input for VaultInfo. On the last line, the cheaper mload operation can be used instead of the sload operation by loading the cached value.

src\VaultRegistry.sol:172-176 ->
    vaultToToken[vault] = VaultInfo(_token, ++nextId[_token]);
    ...
    emit VaultDeployed(vault, _token, nextId[_token]);