Fractional v2 contest - ElKu's results

Lines of code

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

Vulnerability details

Impact

If the vault wants to migrate to a new set of permissions, one of the fractional holders can create a proposal and other holders can show the support(vote) by sending their fractions and ether. When a targetPrice is reached, the Migration contract can start a buyout to resolve the outcome. But there is an error in the Migration contract which enables someone to sent their contribution, thus joining the proposal. And immediately he can withdraw it without affecting the state variables which are used for determining whether the target price of the proposal has reached.

Proof of Concept

Alice proposes a new migration with the propose function with a _targetPrice of 1 ether and newFractionSupply of 20000. The current vault has a totalSupply of 15000.

Alice joins this proposal via join function with 0.5 ether and 5000 fractions. The state variables look like this now:

proposal.totalEth = 0.5;
userProposalEth[_proposalId][Alice] = 0.5;
proposal.totalFractions = 5000;
userProposalFractions[_proposalId][Alice] = 5000;

If the commit function is called now, the currentPrice will be calculate as follows:

currentPrice = (proposal.totalEth * 100) /
            (100 - ((proposal.totalFractions * 100) / IVaultRegistry(registry).totalSupply(_vault)));

so currentPrice = (0.5 * 100) / (100 - 5000*100 / 15000) = 50 / (100 - 33) = 50 / 67 = 0.0.

At the moment the currentPrice is lower than the targetPrice and so commit will not go through.

if (currentPrice > proposal.targetPrice) Code link

Now Bob joins the same proposal via join function with another 0.5 ether and 5000 fractions. The state variables look like this now:

proposal.totalEth = 0.5 + 0.5 = 1;
userProposalEth[_proposalId][Alice] = 0.5;
userProposalEth[_proposalId][Bob] = 0.5;
proposal.totalFractions = 5000 + 5000 = 10000;
userProposalFractions[_proposalId][Alice] = 5000;
userProposalFractions[_proposalId][Bob] = 5000;

If the commit function is called again, then the currentPrice is calculated as follows:

currentPrice = (proposal.totalEth * 100) /
            (100 - ((proposal.totalFractions * 100) / IVaultRegistry(registry).totalSupply(_vault)));

so currentPrice = (1.0 * 100) / (100 - 10000*100 / 15000) = 100 / (100 - 66) = 100 / 34 = 2.0. Which is greater than the targetPrice of 1.0 and commit will go through.

So far everything is good.

But consider another scenario. Alice starts a proposal , Alice joins it and Bob joins it, just as mentioned in the previous scenario.

But before the commit is called Bob changes his mind and calls the withdrawContribution function. He withdraws all his eth and fractions back to his account. This call to withdrawContribution will NOT revert because the buyout current State is still INACTIVE and newVault is still address(0). So the below revert statement is not reached when Bob tries to withdraw his assets.

(, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (
            current != State.INACTIVE ||
            migrationInfo[_vault][_proposalId].newVault != address(0)
        ) revert NoContributionToWithdraw();

In the withdrawContribution function, state variables related to Bob are reset like shown below:

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

But the proposal.totalEth and proposal.totalFractions are not modified.

So after Bob withdraws his assets, the state variables will have the following values:

proposal.totalEth = 1;   //should be 0.5
userProposalEth[_proposalId][Alice] = 0.5;
userProposalEth[_proposalId][Bob] = 0;
proposal.totalFractions = 10000;    //should be 5000
userProposalFractions[_proposalId][Alice] = 5000;
userProposalFractions[_proposalId][Bob] = 0;

And now if commit function is called, then the currentPrice which is calculated as per the following formula will have a value of 2.0(which is greater than the target price of 1.0) and proposal will go through.

currentPrice = (proposal.totalEth * 100) /
            (100 - ((proposal.totalFractions * 100) / IVaultRegistry(registry).totalSupply(_vault)));

Tools Used

VSCode

Recommended Mitigation Steps

Update the proposal.totalEth and proposal.totalFractions state variables in the withdrawContribution function as follows:

proposal.totalFractions -= userFractions;
proposal.totalEth -= userEth;

Why is this not a High Risk finding?

Even though the contract can be tricked to start a buyout process, the buyout has to be successful (that is, enter the SUCCESS state) for the vault to be migrated with new permissions. So trickster holder still needs other holder's support for the migration.

Lines of code

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

Vulnerability details

Impact

The cash function in the Buyout.sol contract has an error which doesn't allow all fraction holders to withdraw eth after a successful buyout. The following points could be noted about this issue:

1. Note that if all the remainig fractions are held by just a single holder, then this error doesn't affect the cash out process.
2. The first person to cash out doesn't get affected as well.
3. Depending on the eth in the pool and the fractions they hold, the second person onwards has a chance to get their funds stuck in the contract pool.

Proof of Concept

The eth which needs to be sent to the holder of fractions is calculated in the following formula:

        uint256 buyoutShare = (tokenBalance * ethBalance) /
            (totalSupply + tokenBalance);

The ethBalance is assigned at line 251 and points to the final eth present in the buyout pool just after the buyout has ended.

(, , State current, , uint256 ethBalance, ) = this.buyoutInfo(_vault);

The issue is that after a holder is sent his share of eth, the ethBalance in the pool is not updated.

Let's take an example. Suppose after the buyout, the pool has 5000 eth. Which has to be shared among two holders each holding 2000 fractions(holder A) and 3000 fractions(holder B) respectively.

When holder A cashes out, his 2000 fractions are burned first and then buyoutShare is calculated. Notice that totalSupply is only 3000, because 2000 fractions of holder A was just burned. so 5000-2000=3000.

buyoutShare = (tokenBalance * ethBalance) / (totalSupply + tokenBalance);
            = (2000 * 5000) / (3000 + 2000)
	    = 10000 / 5000 = 2000 eth will be sent to holder A. Which is correct.

At this point the pool has only 5000-2000=3000 eth, though the buyoutInfo(_vault).ethBalance will still show a value of 5000.

Next, holder B wants to cash out. His 2000 fractions are burned first and then buyoutShare is calculated. Notice that totalSupply is 0, because 3000 fractions of holder A was just burned. so 3000-3000=0.

buyoutShare = (tokenBalance * ethBalance) / (totalSupply + tokenBalance);
            = (3000 * 5000) / (0 + 3000)
	    = 15000 / 3000 = 5000.

Which means the cash function will try to send 5000 eth to holder B. Which is incorrect. As the contract pool doesn't have this much eth, the transaction will revert, causing holder B's funds to be forever stuck in the pool.

Tools Used

VSCode.

Recommended Mitigation Steps

This can be resolved by updating the pool ethBalance in the cash function as follows:

buyoutInfo[_vault].ethBalance = ethBalance - buyoutShare;

1) Use modifier for frequently used access restrictions.

if (owner != msg.sender) revert NotOwner(owner, msg.sender); is used multiple times in Vault.sol. Create a modifier for it to avoid redundancy. Functions where its used are:

1. install
2. setMerkleRoot
3. transferOwnership
4. uninstall

2) When Changing owners the new owner should be zero-checked to avoid accidental loss or ownership.

Instance in Vault.sol:

transferOwnership function

3) Missing emit statement in several functions in Migration.sol.

Functions where an emit statement is missing are:

1. propose
2. join
3. leave
4. commit
5. withdrawContribution
6. migrateFractions

1) State variable which are modified only inside constructor should be declared as immutable.

address public implementation;

2) Reshaping struct elements to save storage slot.

In IMigration.sol, the struct Proposal can be reshaped so that bool fractionsMigrated; is moved to under the isCommited declaration. This will save you one storage slot.

3) Rearranging equations for saving gas and better readability.

uint256 buyoutPrice = (msg.value * 100) /
            (100 - ((depositAmount * 100) / totalSupply));
        uint256 fractionPrice = buyoutPrice / totalSupply;

These equations from Buyout.sol can be rearranged and gas optimized as follows:

uint256 fractionPrice = msg.value / (totalSupply - depositAmount);
uint256 buyoutPrice = fractionPrice * totalSupply;
        

2. In Migration.sol, function _calculateTotal can be simplified. Assuming scalar is used to keep the precision loss to a minimum we can just rewrite it as:

return (_totalEth * _lastTotalSupply) /
      (_lastTotalSupply - _totalFractions);

If the _scalar need to be used it has to be a high value and not something smaller like 100.