Fractional v2 contest - chatch's results

Low severity: BaseVault.sol deployVault() will revert if there are more than 6 Permissions in _modules

The list of Permission hashes is fixed at length 6 in generateMerkleTree here:

hashes = new bytes32[](6);

However the _modules passed to deployVault might have more Permissions than 6.

If there are more than 6 Permissions deployVault() will revert with raiseOutOfBounds.

Consider counting up the number of Permissions first before creating the hashes array. Migration.generateMerkleTree does this here:

uint256 treeLength;
uint256 modulesLength = _modules.length;

unchecked {
    for (uint256 i; i < modulesLength; ++i) {
        treeLength += IModule(_modules[i]).getLeafNodes().length;
    }
}

Marking low severity as if this limit is hit, a new protoform contract could be built, deployed and used. ie. it's not built into core contracts like the VaultRegistry.

Non-critical (until gas costs change): Vault.sol gas reserve setting not future proof'd like the one in PRBProxy

From the prb-proxy repo README.md A minimum gas reserve is saved in storage such that the proxy does not become unusable if EVM opcode gas costs change in the future.

In Vault.sol it's a constant so can't be modified in the future without redeploying the Vault and VaultFactory contracts.

This was probably done to save gas but consider making it modifiable for the future. Some Vault's will continue working for a long time and it's possible gas costs will change again in the future.

In the worst case the Vaults would fail to execute transactions due to out of gas failures.

I marked this non-critical but if opcode costs were to change this could potentially become a high or critical.

NOTE: This issue could also apply to COST_PER_WORD in Transfer.sol however there is a path to swap plugins and migrate Vaults to use new modules so it's less of an issue in contracts like Transfer.

Non-critical: FERC1155.sol implements EIP-2981 but does not include it in supportsInterface

There is no mention of the EIP2981 standard in the docs or code but the FERC1155 does implement that standard.

So it would benefit from returning true for calls to supportsInterface(0x2a55205a). see https://eips.ethereum.org/EIPS/eip-2981 for details of the interface.

With that in place exchanges can see the standard is supported and attempt to honor the royalty setting.

Non-critical: Buyout percentage inconsistency between docs and code

The docs mention 51% as the required buyout threshold multiple times. For example If a pool has more than 51% of the total supply after 4 days, the buyout is successful. See https://docs.fractional.art/fractional-v2-1/smart-contracts/modules/buyout.

However the Buyout.sol logic uses 50%. For example see the check in the end() function here: https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Buyout.sol#L211.

Update either the docs or the code so they are consistently using one of either 50% or 51%.

Non-critical: Vault.sol state variable nonce can be bool

nonce in Vault.sol is used as a flag to indicate if the Vault has been initialized or not. However it's stored as a uint256.

Changing it to a bool would make it's purpose and usage a little clearer. It would also save 20k deployment gas if the nonce declaration was moved above merkleRoot when changed to bool. This is because it can share a slot with the address owner.

Non-critical: IBuyout.sol duplicates 2 functions from the parent IModule.sol

Functions getLeafNodes an getPermissions can be removed from IBuyout.sol as they are already defined in the parent contract IModule.sol.