Fractional v2 contest - 0x29A's results

Lines of code

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

Vulnerability details

Impact

On the withdrawContribution function the proposal.totalFractions and proposal.totalEth don't update like in the function leave L156 and L160 This broke the commit function when calculates the currentPrice and the migrateFractions function when calculates the totalInEth

Proof of Concept

The withdrawContribution function don't update the proposal.totalFractions and proposal.totalEth

Tools Used

Manual Review

Recommended Mitigation Steps

Use the same pattern of the leave function:

    /// @notice Retrieves ether and fractions deposited from an unsuccessful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the failed proposal
    function withdrawContribution(address _vault, uint256 _proposalId)
        external
    {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
        if (id == 0) revert NotVault(_vault);
        // Reverts if caller has no fractional balance to withdraw
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (
            current != State.INACTIVE ||
            migrationInfo[_vault][_proposalId].newVault != address(0)
        ) revert NoContributionToWithdraw();

+       // Gets the migration proposal for the given ID
+       Proposal storage proposal = migrationInfo[_vault][_proposalId];
        // Temporarily store user's fractions for the transfer
        uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
+       proposal.totalFractions -= userFractions;
        // Updates fractional balance of caller
        userProposalFractions[_proposalId][msg.sender] = 0;
        // Withdraws fractional tokens from contract back to caller
        IFERC1155(token).safeTransferFrom(
            address(this),
            msg.sender,
            id,
            userFractions,
            ""
        );

        // Temporarily store user's eth for the transfer
        uint256 userEth = userProposalEth[_proposalId][msg.sender];
+       proposal.totalEth -= userEth;
        // Udpates ether balance of caller
        userProposalEth[_proposalId][msg.sender] = 0;
        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(userEth);
    }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L197-L203

Vulnerability details

Impact

Given a low scalar of 100 on _calculateTotal the current price could be with low precision.

Proof of Concept

With a _scalar of 100 if you got a small _totalEth, a big _lastTotalSupply and a small _totalFractions you will end up with a rounding error.

Edge case

_totalEth = 1
_totalFractions = 10
_lastTotalSupply = 10000

Tools Used

Manual Review

Recommended Mitigation Steps

Increase the scalar

        uint256 currentPrice = _calculateTotal(
-           100,
+           1 ether,
            IVaultRegistry(registry).totalSupply(_vault),
            proposal.totalEth,
            proposal.totalFractions
        );

Lines of code

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

Vulnerability details

Impact

Anyone can manipulate the tokens (ERC20, ERC777, ERC721 or ERC1155) of BaseVault.sol, also, of anyone who has approved (approve, authorizeOperator or setApprovalForAll) the BaseVault.sol contract to manipulate their tokens

Proof of Concept

The batchDepositERC20, batchDepositERC721 and batchDepositERC1155 functions don't have any authorization check

Tools Used

Manual Review

Recommended Mitigation Steps

The best solution it's remove this functions, other option it's check if the msg.sender have the authorization to manipulate the tokens, for example, check allowance in erc20

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L289-L326

Vulnerability details

Impact

A contract can attack the Migration contract using a reentrancy attack, calling the withdrawContribution function who send the IFERC1155 tokens to the contract before update the userProposalEth[_proposalId][msg.sender] and this one can manipulate the balanceContributedInEth in the migrateFractions function

Proof of Concept

The withdrawContribution function make a safeTransferFrom, the msg.sender can be a contract who manipulate the balanceContributedInEth in the migrateFractions function becouse the userProposalEth[_proposalId][msg.sender] update after the safeTransferFrom call

Tools Used

Manual Review

Recommended Mitigation Steps

Use the same pattern of the leave function:

    /// @notice Retrieves ether and fractions deposited from an unsuccessful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the failed proposal
    function withdrawContribution(address _vault, uint256 _proposalId)
        external
    {
        // Reverts if address is not a registered vault
        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
            _vault
        );
        if (id == 0) revert NotVault(_vault);
        // Reverts if caller has no fractional balance to withdraw
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (
            current != State.INACTIVE ||
            migrationInfo[_vault][_proposalId].newVault != address(0)
        ) revert NoContributionToWithdraw();

        // Temporarily store user's fractions for the transfer
        uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
        // Updates fractional balance of caller
        userProposalFractions[_proposalId][msg.sender] = 0;
-       // Withdraws fractional tokens from contract back to caller
-       IFERC1155(token).safeTransferFrom(
-           address(this),
-           msg.sender,
-           id,
-           userFractions,
-           ""
-       );
-
        // Temporarily store user's eth for the transfer
        uint256 userEth = userProposalEth[_proposalId][msg.sender];
        // Udpates ether balance of caller
        userProposalEth[_proposalId][msg.sender] = 0;
+
+       // Withdraws fractional tokens from contract back to caller
+       IFERC1155(token).safeTransferFrom(
+           address(this),
+           msg.sender,
+           id,
+           userFractions,
+           ""
+       );
        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(userEth);
    }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/FERC1155.sol#L31-L34

Vulnerability details

Impact

The EIP-2981: NFT Royalty Standard implementation is incomplete, missing the implementation of function supportsInterface(bytes4 interfaceID) external view returns (bool); from the EIP-165: Standard Interface Detection

Proof of Concept

A marketplace implemented royalties could check if the NFT have royalties, but if don't add the interface of ERC2981 on the _registerInterface, the marketplace can't know if this NFT haves

Tools Used

Manual Review

Recommended Mitigation Steps

Like in solmate ERC1155.sol add the ERC2981 interfaceId on the FERC1155 contract

    /*//////////////////////////////////////////////////////////////
                              ERC165 LOGIC
    //////////////////////////////////////////////////////////////*/

    function supportsInterface(bytes4 interfaceId) public view  override returns (bool) {
        return
            super.supportsInterface(interfaceId) ||
            interfaceId == 0x2a55205a; // ERC165 Interface ID for ERC2981
    }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L175-L214 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L430-L482 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L520-L529

Vulnerability details

Impact

If you got all the fractions you got a div 0 when _calculateContribution in the commit and migrateFractions functions

Tools Used

Manual Review

Recommended Mitigation Steps

Add if that returns _totalEth if _totalFractions == _lastTotalSupply

    function _calculateTotal(
        uint256 _scalar,
        uint256 _lastTotalSupply,
        uint256 _totalEth,
        uint256 _totalFractions
    ) private pure returns (uint256) {
        if (_totalFractions == _lastTotalSupply) {
            return _totalEth;
        }
        return
            (_totalEth * _scalar) /
            (_scalar - ((_totalFractions * _scalar) / _lastTotalSupply));
    }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L325

Vulnerability details

Impact

Although transfer() and send() have been recommended as a security best-practice to prevent reentrancy attacks because they only forward 2300 gas, the gas repricing of opcodes (such as with EIP-2929 in the recent Berlin fork) may break deployed contracts.

There are three usages of transfer() which may fail if the destination is a contract which uses repriced opcodes.

See reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual Review

Recommended Mitigation Steps

Use call() instead, without hardcoded gas limits along with checks-effects-interactions pattern or reentrancy guards for reentrancy protection.

QA report

Non-critical

[N-01] Unused imports

Remove this imports:

• IBaseVault.sol:L04 import {IModule} from "./IModule.sol";
• IMigration.sol:L04 import {Permission} from "./IVaultRegistry.sol";
• IMigration.sol:L05 import {State} from "./IBuyout.sol";
• Migration.sol:L04 remove Auction
• Migration.sol:L05 import {IERC20} from "../interfaces/IERC20.sol";
• Migration.sol:L06 import {IERC721} from "../interfaces/IERC721.sol";
• Migration.sol:L07 import {IERC1155} from "../interfaces/IERC1155.sol";
• Migration.sol:L11 import {IVault} from "../interfaces/IVault.sol";
• Supply.sol:L05 import {IVaultRegistry} from "../interfaces/IVaultRegistry.sol";
• Transfer.sol:L04 import {IERC20} from "../interfaces/IERC20.sol";
• Transfer.sol:L05 import {IERC721} from "../interfaces/IERC721.sol";
• Transfer.sol:L06 import {IERC1155} from "../interfaces/IERC1155.sol";

Remove IProtoform.sol:L05 import {Permission} from "./IVaultRegistry.sol"; and in BaseVault.sol:L8 import the IModule from IModule.sol, import {IModule} from "../../interfaces/IModule.sol";

[N-02] Remove payable cast

Remove unnecessary cast from address to payable address:

• Buyout.sol:

L219: IVault(payable(_vault)).execute(supply, data, _burnProof);
L264: IVault(payable(_vault)).execute(supply, data, _burnProof);
L294: IVault(payable(_vault)).execute(supply, data, _burnProof);
L334: IVault(payable(_vault)).execute(transfer, data, _erc20TransferProof);
L366: IVault(payable(_vault)).execute(transfer, data, _erc721TransferProof);
L403: IVault(payable(_vault)).execute(transfer, data, _erc1155TransferProof);
L440: IVault(payable(_vault)).execute(

• Minter.sol, L60: IVault(payable(_vault)).execute(supply, data, _mintProof);

IVault(payable(_vault)) to IVault(_vault)

• In Migration.sol: L37 address payable public buyout; to address public buyout; L58 buyout = payable(_buyout); to buyout = _buyout; and IMigration.sol L94 function buyout() external view returns (address payable); to function buyout() external view returns (address);

• In VaultFactory.sol, L87: address(vault) to vault

[N-03] Use "" instead of abi.encodePacked()

In VaultFactory.sol: L40: .cloneCreationCode(abi.encodePacked()); to .cloneCreationCode(""); L68-L69:

bytes memory data = abi.encodePacked();
vault = implementation.clone(salt, data);

To

vault = implementation.clone(salt, "");

[N-04] Low accuracy when calculates royaltyAmount

In the FERC1155 contract the royaltyInfo equation have a low accuracy, only 100, add more digits in the BASE and avoid magic numbers

[N-04] The leafNodes are limited by 6

In BaseVault.sol the _modules are limited to total 6 leafNodes: hashes = new bytes32[](6);

[N-05] The multicall function of Multicall.sol its not payable as Uniswap Multicall

Take in mind that since multicall it's top payable you couldn't do a multicall of payable functions.

Multicall.sol

• [N] L11 tendria que ser payable?? la de Uniswap lo es, el de makerdao no lo es pero usa .call en vez de .delegatecall

Low Risk

[L-01] Critical function setContractURI should emit event

L198 setContractURI

[L-02] Missing input validation on array lengths

On calling with arrays of different lengths various malfunctions are possible as the arrays are used as given. Recommendation is to require to all array match in length

BaseVault.sol#L61-L62 BaseVault.sol#L80-L81 BaseVault.sol#L101-L104

[L-03] WETH transfer return values not checked

On SafeSend.sol#L33 the WETH.transfer() and WETH.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success.

We recommend checking the success boolean of all .transfer and .transferFrom calls;

diff --git a/src/utils/SafeSend.sol b/src/utils/SafeSend.sol
index 60d333a..057eade 100644
--- a/src/utils/SafeSend.sol
+++ b/src/utils/SafeSend.sol
@@ -30,7 +30,8 @@ abstract contract SafeSend {
     function _sendEthOrWeth(address _to, uint256 _value) internal {
         if (!_attemptETHTransfer(_to, _value)) {
             WETH(WETH_ADDRESS).deposit{value: _value}();
-            WETH(WETH_ADDRESS).transfer(_to, _value);
+            require(WETH(WETH_ADDRESS).transfer(_to, _value), "Failed to send WETH");
         }
     }
 }

[L-04] Critical function setMerkleRoot on Vault.sol doesnt emit event

Function setMerkleRoot on Vault.sol#L86-L89 should emit event

[L-05] Wrong comment more than 51%

In the Buyout.sol#L21 contract the documentation says: /// - If a pool has more than 51% of the total supply after 4 days, the buyout is successful and the proposer but the L211 have 500 this represent more than 50% Use 510instead of 500 or change the documentation to more than 50%

[L-06] Don't return success in execute function

In Vault contract, execute function: The success returned from _execute function always will be true in other case the _execute function reverts, even remove the return success parameter of _execute function

[L-07] Assign the WETH in the constructor

The WETH may change between the chains, send the WETH address as a parameter of the constructor and assign it there

[L-08] A malicius plugin on Vault could mess up with state

The _execute function on Vault.sol#L115 runs a delegatecall, after the delegatecall it checks if module has change the owner on Vault.sol#L132, but a malicius plugin could also change the state variable merkleRoot, nonce or add a plugin adding var to the mapping methods and more

[L-09] Royalties with high percentage

In the FERC1155 contract the a controller can set a high royalty percentage with the setRoyalties function Add a require to check the max royalty percentage

[L-10] Owner has full control and can rug

By definition the owner of the contract has total control of assets and can hard rug all users, consider limit owner priviliges and or a timelock mechanism on critical assets management functions

Gas report

[G-01] Use immutable

Marking these as immutable (as they never change outside the constructor) would avoid them taking space in the storage:

BaseVault.sol, L19: address public registry;
Minter.sol, L14: address public supply;
Buyout.sol, L29: address public registry;
Buyout.sol, L31: address public supply;
Buyout.sol, L33: address public transfer;
Migration.sol, L37: address public buyout;
Migration.sol, L39: address public registry;
VaultFactory.sol, L15: address public implementation;

[G-02] On Buyout.sol cache variable registry

Gas savings according to tests; Overall gas change: -8342 (-0.001%)

diff --git a/src/modules/Buyout.sol b/src/modules/Buyout.sol
index 1557233..79abcf5 100644
--- a/src/modules/Buyout.sol
+++ b/src/modules/Buyout.sol
@@ -182,8 +182,9 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
     /// @param _vault Address of the vault
     /// @param _burnProof Merkle proof for burning fractional tokens
     function end(address _vault, bytes32[] calldata _burnProof) external {
+        IVaultRegistry _registry = IVaultRegistry(registry);
         // Reverts if address is not a registered vault
-        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
+        (address token, uint256 id) = _registry.vaultToToken(
             _vault
         );
         if (id == 0) revert NotVault(_vault);
@@ -207,16 +208,14 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         // Checks totalSupply of auction pool to determine if buyout is successful or not
         if (
             (tokenBalance * 1000) /
-                IVaultRegistry(registry).totalSupply(_vault) >
+                _registry.totalSupply(_vault) >
             500
         ) {
-            // Initializes vault transaction
-            bytes memory data = abi.encodeCall(
+            // Executes burn of fractional tokens from pool
+            IVault(_vault).execute(supply, abi.encodeCall(
                 ISupply.burn,
                 (address(this), tokenBalance)
-            );
-            // Executes burn of fractional tokens from pool
-            IVault(payable(_vault)).execute(supply, data, _burnProof);
+            ), _burnProof);
             // Sets buyout state to successful
             buyoutInfo[_vault].state = State.SUCCESS;
             // Emits event for ending successful auction
@@ -242,8 +241,9 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
     /// @param _vault Address of the vault
     /// @param _burnProof Merkle proof for burning fractional tokens
     function cash(address _vault, bytes32[] calldata _burnProof) external {
+        IVaultRegistry _registry = IVaultRegistry(registry);
         // Reverts if address is not a registered vault
-        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
+        (address token, uint256 id) = _registry.vaultToToken(
             _vault
         );
         if (id == 0) revert NotVault(_vault);
@@ -264,7 +264,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         IVault(payable(_vault)).execute(supply, data, _burnProof);
 
         // Transfers buyout share amount to caller based on total supply
-        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
+        uint256 totalSupply = _registry.totalSupply(_vault);
         uint256 buyoutShare = (tokenBalance * ethBalance) /
             (totalSupply + tokenBalance);
         _sendEthOrWeth(msg.sender, buyoutShare);
@@ -276,8 +276,9 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
     /// @param _vault Address of the vault
     /// @param _burnProof Merkle proof for burning fractional tokens
     function redeem(address _vault, bytes32[] calldata _burnProof) external {
+        IVaultRegistry _registry = IVaultRegistry(registry);
         // Reverts if address is not a registered vault
-        (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
+        (, uint256 id) = _registry.vaultToToken(_vault);
         if (id == 0) revert NotVault(_vault);
         // Reverts if auction state is not inactive
         (, , State current, , , ) = this.buyoutInfo(_vault);
@@ -285,7 +286,7 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         if (current != required) revert InvalidState(required, current);
 
         // Initializes vault transaction
-        uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
+        uint256 totalSupply = _registry.totalSupply(_vault);
         bytes memory data = abi.encodeCall(
             ISupply.burn,
             (msg.sender, totalSupply)

[G-03] On Migration.sol cache external call getLeafNodes

Gas savings according to tests Overall gas change: -226592 (-0.020%)

diff --git a/src/modules/Migration.sol b/src/modules/Migration.sol
index d81d0ef..5b68ce1 100644
--- a/src/modules/Migration.sol
+++ b/src/modules/Migration.sol
@@ -492,19 +492,22 @@ contract Migration is
         uint256 treeLength;
         uint256 modulesLength = _modules.length;
 
+        
+        bytes32[][] memory _leaves = new bytes32[][](modulesLength);
+
         unchecked {
             for (uint256 i; i < modulesLength; ++i) {
-                treeLength += IModule(_modules[i]).getLeafNodes().length;
+                _leaves[i] = IModule(_modules[i]).getLeafNodes();
+                treeLength += _leaves[i].length;
             }
         }
-
+        
         uint256 counter;
         hashes = new bytes32[](treeLength);
         unchecked {
             for (uint256 i; i < modulesLength; ++i) {
-                bytes32[] memory leaves = IModule(_modules[i]).getLeafNodes();
-                uint256 leavesLength = leaves.length;
-                for (uint256 j; j < leavesLength; ++j) {
+                bytes32[] memory leaves = _leaves[i];
+                for (uint256 j; j < leaves.length; ++j) {
                     hashes[counter++] = leaves[j];
                 }
             }

[G-04] Move creationHash to contract var

Move the VaultFactory.sol, L39-L45 to the constructor and make the creationHash immutable contract var

[G-05] On Migration.sol dont use buyoutInfo as public function

Use Auction storage auction = buyoutInfo[_vault]; instead of (, , , , , ) = this.buyoutInfo(_vault); This avoid a call to the contract himself and use the buyoutInfo internally

L66: (, , State current, , , ) = this.buyoutInfo(_vault);
L118-119: 
    (uint256 startTime, , State current, uint256 fractionPrice, , ) = this
             .buyoutInfo(_vault);
L156-157: 
    (uint256 startTime, , State current, uint256 fractionPrice, , ) = this
             .buyoutInfo(_vault);
L191-198: 
        (
            uint256 startTime,
            address proposer,
            State current,
            ,
            uint256 ethBalance,

        ) = this.buyoutInfo(_vault);
L251: (, , State current, , uint256 ethBalance, ) = this.buyoutInfo(_vault);
L283: (, , State current, , , ) = this.buyoutInfo(_vault);
L322: (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
L354: (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
L390: (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
L427: (, address proposer, State current, , , ) = this.buyoutInfo(_vault);

[G-06] On Vault.sol dont use nonce as initializer checker

Vault.sol#L16-L29:

    ...
-   /// @notice Initializer value
-   uint256 public nonce;
    /// @dev Minimum reserve of gas units
    uint256 private constant MIN_GAS_RESERVE = 5_000;
    /// @notice Mapping of function selector to plugin address
    mapping(bytes4 => address) public methods;

    /// @dev Initializes nonce and proxy owner
    function init() external {
-       if (nonce != 0) revert Initialized(owner, msg.sender, nonce);
-       nonce = 1;
+       if (owner != address(0)) revert Initialized(owner, msg.sender);
        owner = msg.sender;
        emit TransferOwnership(address(0), msg.sender);
    }
    ...

IVault.sol#L9: error Initialized(address _owner, address _newOwner, uint256 _nonce); to error Initialized(address _owner, address _newOwner);

[G-07] ++i costs less gas compared to i++ or i += 1 and use unchecked for boost save gas

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Also using unchecked arithmetic will cost less gass because it wouldnt run a check for under/overflow and revert if it is detected.

Take in mind you shoul only use unchecked when its safe to increment without overflow, like in loops iterator.

Use this pattern;

for (uint256 i; i < length;) {
    // code here
    unchecked { ++i; }
}

Vault.sol#L78: for (uint256 i = 0; i < length; i++) { Vault.sol#L104: for (uint256 i = 0; i < length; i++) {

[G-08] A require with multiple || cost more gas than split conditions into multiple require

FERC1155.sol#L263-L268 Change

require(
    msg.sender == _from ||
        isApprovedForAll[_from][msg.sender] ||
        isApproved[_from][msg.sender][_id],
    "NOT_AUTHORIZED"
);

To:

require(msg.sender == _from, "NOT_AUTHORIZED");
require(isApprovedForAll[_from][msg.sender], "NOT_AUTHORIZED");
require(isApproved[_from][msg.sender][_id], "NOT_AUTHORIZED");

[G-09] Unnecessary abi.encodePacked operation, replace it for ""

On VaultFactory.sol#L68-L69: Recommendation:

diff --git a/src/VaultFactory.sol b/src/VaultFactory.sol
index 0902ebb..cc66cf2 100644
--- a/src/VaultFactory.sol
+++ b/src/VaultFactory.sol
@@ -65,8 +65,7 @@ contract VaultFactory is IVaultFactory {
         // Prevent front-running the salt by hashing the concatenation of tx.origin and the user-provided seed.
         bytes32 salt = keccak256(abi.encode(tx.origin, seed));
 
-        bytes memory data = abi.encodePacked();
-        vault = implementation.clone(salt, data);
+        vault = implementation.clone(salt, "");
         Vault(vault).init();

[G-09] Unnecesary loop on function getLeafNodes

You could go with a similar strategy as in getPermissions for the function getLeafNodesand avoid the loop;

diff --git a/src/modules/Buyout.sol b/src/modules/Buyout.sol
index 1557233..d00754a 100644
--- a/src/modules/Buyout.sol
+++ b/src/modules/Buyout.sol
@@ -451,14 +451,13 @@ contract Buyout is IBuyout, Multicall, NFTReceiver, SafeSend, SelfPermit {
         nodes = new bytes32[](5);
         // Gets list of permissions from this module
         Permission[] memory permissions = getPermissions();
-        for (uint256 i; i < permissions.length; ) {
-            // Hashes permission into leaf node
-            nodes[i] = keccak256(abi.encode(permissions[i]));
-            // Can't overflow since loop is a fixed size
-            unchecked {
-                ++i;
-            }
-        }
+        
+        nodes[0] = keccak256(abi.encode(permissions[0]));
+        nodes[1] = keccak256(abi.encode(permissions[1]));
+        nodes[2] = keccak256(abi.encode(permissions[2]));
+        nodes[3] = keccak256(abi.encode(permissions[3]));
+        nodes[4] = keccak256(abi.encode(permissions[4]));
+        
     }
 
     /// @notice Gets the list of permissions installed on a vault