QA Report - Githubissues

[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

code-423n4 / 2022-07-fractional-findings

QA Report #391