Fractional v2 contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Asset permanently stuck in vault

Proof of Concept

Assume that the vault has a very high supply (i.e. 1E20). When the buyout price is divided by the supply, unless the buyout price is >1E20, fractionPrice = 0 due to precision loss. In this example, the buyout price would have to be at least 1E20 (100 ETH) for the price of the fraction to be !0. Any vault created with a high supply would then be nearly impossible to buyout because trying to buy it for any less would result in your fractions being sold for 0.

Tools Used

Recommended Mitigation Steps

Don't use fractionPrice. Always calculate using buyoutPrice to prevent precision loss like below:

uint256 ethAmount = buyoutPrice * _amount / totalSupply

Lines of code

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

Vulnerability details

Impact

User funds permanently locked

Proof of Concept

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

When buyout end is called, the current number of fractions and eth are sent back to the migration contract. Because of buying and selling, the number of fractions and eth returned can (and most likely will be) different from the original amount sent to the buyout.

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

When withdrawing from a failed buyout, the contract tries to give back eth and fractions one to one of what the user contributed to the proposal. If the number of eth and fractions are not the exact same then some users will not be able to withdraw their eth/fractions because the contract won't have enough eth/fractions to honor their withdrawal. It also may take eth/fractions from pending proposals to honor withdrawals.

Tools Used

Recommended Mitigation Steps

Contract Integration The first step should be to integrate the migration and buyout contracts. The buyout contract needs reference to the migration contract and the migration contract needs a new function that is called on failed buyout proposals. When the start function of buyout is called it needs to compare msg.sender to the migration contract address and track the proposalID. Then when the end function is called it needs to check the proposalID and if it's non zero then it needs to call the new function in migration to convey to migration how much of each migration is receiving back.

withdrawContribution changes: withdrawContribution should honor withdraws on a first come first serve basis, up to the amounts received from the failed buyout. If a user deposited fractions they should get fractions back unless there isn't enough. Any outstanding fractions that can't be redeemed should be redeemed for their corresponding amount of eth based on the target price of the proposal

Lines of code

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

Vulnerability details

Impact

Fractions stolen from proposal contributors

Proof of Concept

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

When commit is called, it calls the start function from the buyout module.

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

The following lines transfer ALL fractions from the migration contract. This includes fractions that are being held by other proposals. This effectively steals all the fractions and uses them to buyout the vault.

Tools Used

Recommended Mitigation Steps

The start function in buyout needs to be changed so that it allows the number of fractions to be specified. The migration contract should then call this new function with the proper number of fractions in the proposal.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L334-L350 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L358-L374 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L383-L401

Vulnerability details

Impact

Vault tokens are lost

Proof of Concept

None of migrateVault functions check that proposalID supplied actually matches the proposalID of the winning proposal. This means that anyone can either reference another failed proposalID or create a new one then call that proposalID

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

Since newVault has never been set, it will return address(0) as the address to send the tokens to. If the token being transferred does not revert on transfers to address(0) then the transfer will go through and the token will be sent there

Tools Used

Recommended Mitigation Steps

Functions should revert if newVault = address(0)

Lines of code

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

Vulnerability details

Vulnerability details

Impact

Precision loss causing loss of user value and potentially cause complete loss to vault

Proof of Concept

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

If the supply of the fraction is set to say 10 then any user that uses migrateFractions with less than 10% of the contributions will receive no shares at all due to precision loss. Under certain conditions it may even cause complete loss of access to the vault. In this same example, if less than 5 fractions can be redeemed (i.e. not enough people have more than 10% to overcome the precision loss) then the vault would never be able to be bought out and the vault would forever be frozen.

Tools Used

Recommended Mitigation Steps

When calling propose require that _newFractionSupply is greater than some value (i.e. 1E18)

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L141-L173 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L105-L136

Vulnerability details

Impact

Proposal participants are unable to call leave or join when ANY buyout is active or successful

Proof of Concept

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

The above lines require that the status of the buyout for the vault to be inactive. This means that join and leave will always revert even when the current buyout is not related to migration at all. This is problematic for the join function because it keeps any potential proposals from being arranged while there is an active buyout. This should be allowed because there is no reason to assume that the buyout underway will succeed and proposals will likely take time to assemble. The bigger problem is for users who have already joined a proposal as they will not be able to withdraw their fractions or eth, even though the buyout is potentially not related to them at all. Worst yet, if the buyout succeeds then the status of the vault will forever be state.SUCCESS and all users who have a deposit in a proposal for that vault will be forever unable to withdraw their contributions either with leave or withdrawContribution

It also affects the user when there are multiple proposals active on migration at the same time. Imagine there are two proposals, Prop A and Prop B, and three users, User 1, User 2 and User 3. User 1 contributes to Prop A and Users 2 and 3 contribute to Prop B, bringing it over the funding threshold. User 2 then calls commit and starts the buyout process using Prop B. When User 1 calls leave to withdraw their funds from Prop A it will revert because L148 will return state.ACTIVE causing a revert in L150. If User 1 tries to call withdrawContribution it will also revert in L303. User 1 will be unable to withdraw from Prop A even though it is not the active proposal/buyout

Tools Used

Recommended Mitigation Steps

A bool (committed) should be added to the migration information and changed to true when commit is called for that proposal. Leave and join should revert if migrationInfo[_vault][_proposalId].committed == true. withdrawContribution should also check this bool and revert if migrationInfo[_vault][_proposalId].committed != true && current != state.INACTIVE

Lines of code

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

Vulnerability details

Impact

Failed proposal can be committed again and eth stolen from migration contract in combination with other vulnerabilities submitted

Proof of Concept

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

commit can be called as long as it has been less than 7 days from the start time. The buyout period is specified as 4 days in the buyout contract. This means that as long as proposal is committed within 3 days of starting, commit can be called again after a failed buyout (4 days) because the current time will still be less than 7 days from the start time.

This can be used in combination with a vulnerability I previously reported. The contract does not account for the actual number of fractions that it receives back from a failed buyout. If it sent 10 fractions and 3 eth to a buyout it may receive back 15 fractions and 2 eth due to trading against the buyout. Because commit can called again on the same proposal, the second time it will try to send the fraction balance of the contract, now 15, and the value of the eth in the proposal, 3 eth. This transaction will either revert due to not having enough eth or it will send 3 eth pulling from eth deposited to other migration proposals.

This could be exploited by creating a vault and immediately migrating it. Once the migration starts the user could sell fractions to themselves and get eth, making sure to keep the number of fractions under 51%, to prevent a successful buyout. After the buyout fails they can then call the commit function again and more eth will be sent. They can then sell fractions to themselves netting more eth than they initially supplied. This could be done repeatedly until all eth has been stolen from the migration contract.

Tools Used

Recommended Mitigation Steps

Change the length of either the migration period or the buyout period to match so that a proposal can't be replayed

Lines of code

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

Vulnerability details

Impact

All new shares drained

Proof of Concept

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

userProposalEth[_proposalId][msg.sender] and userProposalFractions[_proposalId][msg.sender] are used to calculate the number of shares to issue to the user but they are never set to 0 after claiming. This means that the functions can be called over and over until all new shares are in possession of the attacker

Tools Used

Recommended Mitigation Steps

Set userProposalEth[_proposalId][msg.sender] and userProposalFractions[_proposalId][msg.sender] = 0 after calculating balanceContributedInEth

Lines of code

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

Vulnerability details

Impact

Expired proposals can be joined wasting gas and time

Proof of Concept

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

commit can be called as long as it has been less than 7 days from the start time of the proposal. Join never does a similar check meaning that users can join a proposal that can't be submitted.

Tools Used

Recommended Mitigation Steps

Add the same check in L194-195 of commit to join

Lines of code

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

Vulnerability details

Impact

A real buyout never succeeds

Proof of Concept

start allows anyone to call a buyout and they can choose any price they want. This would allow a malicious user to always frontrun buyout attempts with a tiny eth deposit. Since only one buyout can exist at the same time, the legitimate buyout will never succeed. Locking the assets as long as the attacker wants.

Tools Used

Recommended Mitigation Steps

Doesn't seem like much can be done, other than adding the ability for buyouts offering a higher buyout price to replace an existing buyout. Implementing that would come with it's own set of challenges.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L58-L70 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L77-L89 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L98-L117

Vulnerability details

Impact

Large amounts of user gas wasted on input error

Proof of Concept

If _tokens is longer than any of the other input arrays then it will cause the function to revert when i is greater than the length of the short array. Since these are transfers a large amount of user gas would be wasted in a situation like that, which would be costly.

Tools Used

Recommended Mitigation Steps

Add require statements to each function to confirm that all input arrays are the same length