Fractional v2 contest - robee's results

Mult instead div in compares

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

Code instance:

    Buyout.sol, 208, if ( (tokenBalance * 1000) / IVaultRegistry(registry).totalSupply(_vault) > 500 ) { 

Missing commenting

    The following functions are missing commenting as describe below:
    

Code instances:

    FERC1155.sol, INITIAL_CONTROLLER (public), @return is missing
    FERC1155.sol, VAULT_REGISTRY (public), @return is missing
    FERC1155.sol, uri (public), @return is missing

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-07-fractional/tree/main/src/utils/Metadata.sol#L24
    https://github.com/code-423n4/2022-07-fractional/tree/main/src/FERC1155.sol#L198
    https://github.com/code-423n4/2022-07-fractional/tree/main/src/modules/Migration.sol#L220

Not verified input

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.

Code instances:

    MockERC20.sol.mint to
    Migration.sol.migrateVaultERC721 _token
    BaseVault.sol.batchDepositERC1155 _from

Hardcoded WETH

WETH address is hardcoded but it may differ on other chains, e.g. Polygon, so make sure to check this before deploying and update if necessary You should consider injecting WETH address via the constructor. (previous issue: https://github.com/code-423n4/2021-10-ambire-findings/issues/54)

Code instance:

    Hardcoded weth address in SafeSend.sol

Not verified owner

    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.
    
    

Code instances:

    VaultRegistry.sol.createFor _owner
    FERC1155.sol.permitAll _owner
    FERC1155.sol.permit _owner
    VaultFactory.sol.deployFor _owner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    MerkleBase.sol, log2ceil_naive
    Multicall.sol, multicall
    MerkleBase.sol, hashLeafPairs

Two Steps Verification before Transferring Ownership

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

Code instance:

    IVault.sol

Missing non reentrancy modifier

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

Code instances:

    Migration.sol, migrateVaultERC721 is missing a reentrancy modifier
    Migration.sol, withdrawContribution is missing a reentrancy modifier
    Migration.sol, migrateVaultERC1155 is missing a reentrancy modifier
    Migration.sol, leave is missing a reentrancy modifier

Open TODOs

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Code instance:

Open TODO in MerkleBase.sol line 23 : // TODO: This can be aesthetically simplified with a switch. Not sure it will

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-07-fractional/tree/main/src/targets/Transfer.sol#L481
    https://github.com/code-423n4/2022-07-fractional/tree/main/src/modules/Buyout.sol#L75

transfer return value of a general ERC20 is ignored

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):

Code instances:

    TransferReference.sol, 22 (ERC20Transfer), IERC20(_token).transfer(_to, _value);
    BaseVault.sol, 65 (batchDepositERC20), IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

State variables that could be set immutable

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

Code instances:

    registry in BaseVault.sol
    supply in Minter.sol

Caching array length can save 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++) { ... }

Code instances:

    BaseVault.sol, _tokens, 83
    Buyout.sol, permissions, 454
    BaseVault.sol, _tokens, 64

Unnecessary index init

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:

Code instances:

    BaseVault.sol, 83
    BaseVault.sol, 64

Use bytes32 instead of string to save gas whenever possible

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

Code instances:

    FERC1155.sol (L15), string public constant NAME = "FERC1155"; 
    FERC1155.sol (L17), string public constant VERSION = "1"; 

Short the following require messages

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:

Code instances:

    Solidity file: MerkleBase.sol, In line 62, Require message length to shorten: 34, The message: wont generate root for single leaf
    Solidity file: MerkleBase.sol, In line 78, Require message length to shorten: 35, The message: wont generate proof for single leaf

Use != 0 instead of > 0

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

Code instance:

    MerkleBase.sol, 186: change 'x > 0' to 'x != 0'

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instance:

    Migration.sol (L#194) - if (block.timestamp > proposal.startTime + PROPOSAL_PERIOD)

Inline one time use functions

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

Code instances:

    Migration.sol, _calculateContribution
    FERC1155.sol, _computePermitStructHash
    SafeSend.sol, _attemptETHTransfer
    FERC1155.sol, _computePermitAllStructHash
    Multicall.sol, _revertedWithReason