Fractional v2 contest - dipp's results

Lines of code

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

Vulnerability details

Impact

The withdrawContributions function in Migration.sol takes any vault as input. As long as the vault is valid and has an inactive buyout, a user may call withdrawContributions even if the proposal they contributed to is LIVE.

This may lead to users not being able to commit their proposals if the Migrations.sol contract does not contain enough ETH for their commits.

A malicious user may also swap their vault tokens for any other vault tokens in the Migration.sol contract since withdrawContributions sends the tokens of the vault given as input. So if a user joined a proposal with 100 tokens of vault A and calls withdrawContributions with _vault set to vault B, the user will receive 100 vault B tokens.

Proof of Concept

Scenario 1: Malicious user withdraws ETH so that other proposals may not be commited

1. A user joins a proposal for vault A with 1 ETH, which is then commited and the ETH is sent to Buyout.sol.

2. Another user joins a proposal for vault B with 1 ETH, which is not yet commited so the ETH stays in Migration.sol.

3. The first user calls withdrawContributions with the proposalId they called join with and the vault set to vault B (it could be any valid vault).

4. The withdrawContributions will not revert since the buyout state for vault B is INACTIVE (assuming no one starts a buyout) and the '''migrationInfo[_vault][_proposalId].newVault``` should return a 0 address since the proposalId is valid only for vault A due to the incrementing of the proposalId.

5. Since the userProposalEth returns 1 ETH, the first user will receive 1 ETH and Migration.sol will lose 1 ETH. Migration.sol now has 0 ETH.

6. The second user will now not be able to commit their proposal since the Migration.sol contract contains less ETH than the totalEth returned by migrationInfo for that proposalId. Migration.sol does not have enough ETH for the user to call commit.

Scenario 2: Malicious user swaps worthless vault A tokens for valuable vault B tokens

1. A malicious user joins a proposal for vault A with 1 ETH and 100 vault A tokens.

2. A proposal for vault B has 100 vault B tokens and is not commited yet.

3. The proposal for vault A is commited, sending totalEth and totalFractions of the proposal for vault A to the Buyout.sol contract.

4. The malicious user calls withdrawContributions with _vault set to vault B and using the vault A proposalId.

5. The withdrawContributions will not revert since the buyout state for vault B is INACTIVE (assuming no one starts a buyout) and the '''migrationInfo[_vault][_proposalId].newVault``` should return a 0 address since the proposalId is valid only for vault A due to the incrementing of the proposalId.

6. The userProposalFractions for the malicious user will return 100 since they joined the vault A proposal with 100 tokens.

7. Migration.sol will send the vault B tokens to the malicious user instead of the correct vault A tokens. (Assuming the contract has enough ETH to send to the malicious user as well)

Recommended Mitigation Steps

Since no 2 vaults will have the same proposalId, check that the migrationInfo does not return an empty proposal in withdrawContributions.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L105-L136 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L57-L107 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L184-L239 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Buyout.sol#L278-L303 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L141-L173 https://github.com/code-423n4/2022-07-fractional/blob/main/src/modules/Migration.sol#L292-L326

Vulnerability details

Impact

If a user joins a proposal (valid or invalid) and the vault is then bought out or redeemed by someone other than the Migration.sol contract before the user withdraws their contribution then they will be unable to retrieve their funds from the Migration.sol contract. This is due to the withdrawContributions function in Migration.sol reveting if the state of the vault's buyout is SUCCESSS, which it is if a buyout succeeds or redeem is successfully called in Buyout.sol.

After a vault is bought out or redeemed and its buyout state is set to SUCCESS, the state should not be able to change back to INACTIVE and thus the funds locked in Migrations.sol does not seem to be recoverable.

This scenario seems more likely when you consider that users are able to join proposals that have expired or proposals that do not exist.

Proof of Concept

1. User A joins a proposal (valid or invalid) and sends ETH and/or fractional tokens to Migration.sol.

2. User B starts a buyout in Buyout.sol before the proposal is commited in Migration.sol. (A buyout may also occur after a proposal is commited and fails. As long as the buyout state of a vault is SUCCESS and the proposer is not the Migration.sol contract, joined funds will not be withdrawable after.)

3. After the rejection period, user B ends the buyout successfully, changing the buyout state to SUCCESS.

4. User A now wants to withdraw their deposited funds from Migration.sol.

5. Since the buyout state of the vault is now SUCCESS, user A is unable to call leave or withdrawContributions due to the conditions (line 150 and line 303) that the state should be INACTIVE.

_

In order to call Buyout:start in Migration.t.sol, I added 2 approvals for the buyout module in the initializeMigration function in TestUtil.sol.

setApproval(_user1, buyout, _approval);                                 // Add for migration test
setApproval(_user2, buyout, _approval);                                 // Add for migration test

Added test code in Migration.t.sol:

The test code below (testJoinNonExistentProposal) shows a user joining a non-existent proposal, the vault is then bought out by another user before the migration proposal may be commited and the first user is then unable to withdraw from Migration.sol.

function testJoinNonExistentProposal() public {
        // Setup
        initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
        
        // Check proposal does not exist
        uint256 proposalId = 100;
        (uint256 startTime,,,,,,,,) = bob.migrationModule.migrationInfo(vault, proposalId);
        assertEq(startTime, 0);

        // Join non-existent proposal
        assertEq(address(migrationModule).balance, 0);
        (address token, uint256 id) = registry.vaultToToken(vault);
        bob.migrationModule.join{value: 0.5 ether}(vault, proposalId, 0);

        // Check migration module gains ETH
        assertEq(address(migrationModule).balance, 0.5 ether);


        // Some other user starts a buyout
        alice.buyoutModule.start{value: 1 ether}(vault);

        // End a successful buyout
        bob.buyoutModule.sellFractions(vault, IERC1155(token).balanceOf(bob.addr, id));         // Send rest of tokens to buyout so that the buyout succeeds
        vm.warp(rejectionPeriod + 1);

        alice.buyoutModule.end(vault, burnProof);

        (,,State state,,,) = alice.buyoutModule.buyoutInfo(vault);

        assert(state == State.SUCCESS);
        
        // Try to withdraw from migration module
        assertEq(address(migrationModule).balance, 0.5 ether);  // Check user still has outstanding contributions
        vm.expectRevert(
            abi.encodeWithSelector(IBuyout.InvalidState.selector, 0, 2)
        );
        bob.migrationModule.leave(vault, proposalId); // Cant leave propsoal since state==SUCCESS
        vm.stopPrank();
        vm.expectRevert(
            abi.encodeWithSelector(IMigration.NoContributionToWithdraw.selector)
        );
        bob.migrationModule.withdrawContribution(vault, proposalId); // Cant withdraw since state==SUCCESS
    }

Recommended Mitigation Steps

A migration proposal is not successful if the state of the vault buyout is SUCCESS and the proposer is not the Migration.sol contract. If the buyout state of a vault is SUCCESS but the proposer is not Migration.sol, the proposal should be considered as a failure and users who contributed to the proposal should be able to withdraw their contributions.

The condition on line 303 of Migration.sol should consider if the proposer is Migration.sol. withdrawContributions should fail if the state is not INACTIVE and the proposer is Migration.sol. In the case that the vault is bought out successfully by another user, state == SUCCESS and proposer != Migration.sol so withdrawal should be allowed.

It might also be beneficial to not allow a user to join expired or non-existent migration proposals. Joining an invalid proposal does not seem to be an issue as long as the user is able to withdraw their funds.

Lines of code

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

Vulnerability details

Impact

The migrateFractions function in the Migration.sol contract is used to send new vault tokens to the user calculated based on the amount of ETH and fractions the user contributed to the migration proposal. After it is called once the user should have all the new vault tokens owed to them.

Since the function does not check if the user had already called it, a user may call it more than once, allowing them to gain more new vault tokens than they are owed. If a user repeatedly uses this function to gain new tokens then other users may not be able to get their new tokens.

Proof of Concept

Test code added to Migrations.t.sol:

The test code below shows the first user (Alice) migrating their tokens to the new vault twice before the second user (Bob) calls migrateFractions which then fails.

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

        // First user migrates fractions twice
        assertEq(IERC1155(newToken).balanceOf(address(migrationModule), id), TOTAL_SUPPLY * 2);            // Confirm Migration has all new tokens

        assertEq(getFractionBalance(alice.addr), 4000);                         // Alice joined with 1 ether and 1000 fractions
        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 6000);           // Alice's shares == 6000

        assertEq(IERC1155(newToken).balanceOf(address(migrationModule), id), TOTAL_SUPPLY * 2 - 6000);            // Confirm Migration loses new tokens

        alice.migrationModule.migrateFractions(vault, 1);
        assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 12000);          // Confirm Alice gains 6000 new tokens again

        assertEq(IERC1155(newToken).balanceOf(address(migrationModule), id), 8000);            // Confirm Migration loses new tokens

        // Second user attempts to migrate fractions
        assertEq(getFractionBalance(bob.addr), 0);                              // Bob joined with 1 ether and 5000 fractions (all of his fractions)
        vm.expectRevert(stdError.arithmeticError);
        bob.migrationModule.migrateFractions(vault, 1);                         // Bob is unable to call migrateFractions and gain new tokens because the migration module does not contain enough tokens
        assertEq(IERC1155(newToken).balanceOf(bob.addr, id), 0);                // Confirm Bob does not gain any new tokens (supposed to gain 14,000 tokens)
    }

Recommended Mitigation Steps

A possible fix might be to set the userProposalEth and userProposalFractions to 0 after the user's tokens have been migrated.

1. Unable to leave proposal if receivers gas cost is too high for transfer function

Line References

Migration.sol#L172

Migration.sol#L325

Impact

The payable(address).transfer function has a limit of 2300 gas (source). If the receiver has a fallback/receive function that requires more gas then the transaction would revert, not allowing the sender to withdraw or leave a proposal.

Recommended Mitigation Steps

Consider using the call function instead of transfer to send ETH or use the SafeSend.sol _sendEthOrWeth function like in Buyout.sol.

2. Unsafe reentrancy pattern

Line References

Migration.sol#L272-L279

Impact

If a reentrancy were possible through the _mintFractions function call in settleFractions (Migration.sol), an attacker could repeatedly call settleFractions and inflate the token supply. This is due to the fractionsMigrated property being set to true after the _mintFractions call.

The _mintFractions function does not seem vulnerable to reentrancy so there is no real impact, however it is recommended to conform to the safe 'Checks Effects Interactions' pattern to avoid any issues in the future.

Recommended Mitigation Steps

Move line 279 to before _mintFractions is called.