Fractional v2 contest - bbrho's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L38

Vulnerability details

Impact

Vault owners can install plugins via Vault.install(), with calls to the installed plugins made through the vault's fallback function. Unlike the vault's external Vault.execute() function, fallback() imposes no checks on the permissions of the caller, assuming proper installation of the plugin by the owner at install time.

While this design seems intentional given NFTReceiver.sol, it can lead to unintended vulnerabilities, like loss of vault NFTs, if the vault owner:

• Mistakenly installs a plugin not intended for unpermissioned use or installs a malicious plugin
• Is transferred vault ownership from a prior owner who misconfigured vault plugin installations

Proof of Concept

Example successful test similar to those from Vault.t.sol below.

The original vault owner installs a transfer target plugin, with selector ERC721TransferFrom on the vault. Ownership is then transferred to bob, but the original owner uses the installed transfer plugin to steal the NFT deposited in the vault and send it to alice (without bob's permission):

function testFallbackWithTransferPlugin() public {
    bytes memory data = setUpExecute(alice);

    // install transfer from on vault
    address[] memory plugins = new address[](1);
    bytes4[] memory selectors = new bytes4[](1);

    plugins[0] = address(transferTarget);
    selectors[0] = transferTarget.ERC721TransferFrom.selector;
    vaultProxy.install(selectors, plugins);

    // set bob as the owner
    vaultProxy.transferOwnership(bob.addr);

    // check vault originally has the nft
    assertEq(IERC721(erc721).balanceOf(vault), 1);
    assertEq(IERC721(erc721).balanceOf(alice.addr), 0);

    // execute the nft transfer via plugin with fallback
    (bool success,) = address(vaultProxy).call(data);

    // check this contract transferred nft out of vault to alice
    // even after bob became owner
    assertEq(IERC721(erc721).balanceOf(vault), 0);
    assertEq(IERC721(erc721).balanceOf(alice.addr), 1);
} 

Tools Used

Editor

Recommended Mitigation Steps

Consider tracking which installed plugins might require permissions alongside the methods mapping in storage. Potentially:

/// @notice Mapping of function selector to plugin address
mapping(bytes4 => address) public methods;

/// @notice Mapping of plugin address to whether permissions required
mapping(address => bool) public auths;

with auths[plugin] used in fallback().

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/protoforms/BaseVault.sol#L128

Vulnerability details

Impact

Deployment of BaseVault through deployVault() fails when _modules.length * leaves.length > 6.

BaseVault.generateMerkleTree() returns the hashes list of fixed length 6, initialized in L128. But _modules.length * leaves.length is variable in general (given the allowed deploy vault _modules input), and could be > 6 resulting in an index error and failed deployment of the BaseVault instance. Migration.sol::generateMerkleTree() seems to implement the logic around this appropriately.

Proof of Concept

The following test reverts at vault deployment with an Reason: Index out of bounds:

    function testGenerateMerkleTree() public {
        address[] memory nmodules = new address[](3);
        nmodules[0] = address(baseVault);
        nmodules[1] = address(buyoutModule);
        nmodules[2] = address(minter);

        vault = alice.baseVault.deployVault(
            TOTAL_SUPPLY,
            nmodules,
            nftReceiverPlugins,
            nftReceiverSelectors,
            mintProof
        );
        (token, tokenId) = registry.vaultToToken(vault);

        assertEq(getFractionBalance(alice.addr), TOTAL_SUPPLY);
    }

Tools Used

Editor

Recommended Mitigation Steps

Would replicate the logic in Migration.sol::generateMerkleTree() which seems to implement the check of modules * leaves in initializing the hashes list appropriately.

If a length of 6 for hashes list should be enforced for BaseVault as a protoform template, should add a revert in the case _modules.length * leaves.length != 6 and/or simply require the modules given as input to deployVault to be minter and buyout. If not adding this revert, would further check whether empty elements for < 6 causes any issues with merkleRoot generation and module permissions.