Fractional v2 contest - 0xf15ers's results

1. Unused interface imports

• The interfaces (IERC1155, IERC20, IERC1155) imported in Transfer.sol is not used anywhere inside the contract.

2. Address(0) checks

• It is best practice to add guard checks for address(0) in important settings like constructors.
• BaseVault.sol#L25
• Minter.sol#L18

3. Unhandeled return value

• It is recommended to add require() to the boolean return values.
• execute() returns boolean values.

4. Unused receive() function will lock ether in contract

• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L53

5. Less external call be made by calling the token contract's methods directly

• In start() function of Buyout.sol, while calulating totalSupply of vault IVaultRegistry(registry).totalSupply(_vault). This could be calculated directly by calling FERC1155(token).totalSupply(id) directly which will make one less external call.

// before
(address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
....
uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
....


// After 
....
(address token, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
.........
uint256 totalSupply = IERC1155(token).totalSupply(id);

6. Defined error is not used

• error MethodNotFound() in IVault.sol is not used in the inherited contracts.

7. Use a single modifier instead of using the same guard checks multiple times

• if (owner != msg.sender) revert NotOwner(owner, msg.sender) is used in multiple methods inside vault.sol. It is recommended to use a single modifer for better code resue.

8. Use two-step process for transfering ownership

• Unrecoverable critical process like transferOwnership() could use two step process to prevent unintended mistakes.
• same for transferController()
• First, nominate the address, and second accept the nomination from that address ensuring that the access is indeed secured.

9. Use modifier to check the vault status

• There are several instances where a function needs to revert unless the vault is at a certain state. This code is repeated throught several functions.
• This can be refactored into a single modifier to make the code look nice and more readable.

for eg.

    modifier vaultState(address _vault, State required) {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
        if (id == 0) revert NotVault(_vault);
        // Reverts if auction state is not successful
        (, , State current, , , ) = this.buyoutInfo(_vault);
        if (current != required) revert InvalidState(required, current);
    }

Now this modifier can be used in any function that needs to check the vault is in a certain state before performing certain actions.

for eg, lets use function porpose of Migration.sol

    function propose(
        address _vault,
        address[] calldata _modules,
        address[] calldata _plugins,
        bytes4[] calldata _selectors,
        uint256 _newFractionSupply,
        uint256 _targetPrice
    ) external vaultState(_vault, State.INACTIVE) {
    ...
    ...
    }