Fractional v2 contest - zzzitron's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultFactory.sol#L20-L22

Vulnerability details

Impact

HIGH - Assets can be lost directly Anybody can initialize the Vault's implementation contract. The worst case would be to selfdestruct and make all the (already deployed and to be deployed) Vault's proxies useless and assets in the deployed proxies will be lost.

Proof of Concept

• proof of concept
  • Notes: part of destroying code is in the setUp, due to limitation of foundry. The code size will be checked in the testInitAndDestroy_poc.

In the proof of concept, alice calls init on the Vault implementation and takes the ownership (line 38). Alice can now install a malicious plugin, the SelfdestructPlugin located in the above of the test contract (line 6-10). Then simply calling the destroy function on the Vault implementation (line 54). Alice could simply renounce her ownership to zero address to bypass the check on ownership change.

The main cause is the Vault implementation was not initialized. The deploy scripts also do not call init on deployed Vault implementation. It is safer and easier to init right after the contract is deployed.

// VaultFactory::constructor
// Vault implementation is deployed but not initialized

 20     constructor() {
 21         implementation = address(new Vault());
 22     }

The problem could be exploited even further because any functionality could be installed as plugins. Also lack of zero address check for transferring ownership made it simple to bypass the check on ownership change.

Tools Used

foundry

Recommended Mitigation Steps

Add init after deploy the vault implementation.

// VaultFactory::constructor
// to mitigate, add init after deploy

 20     constructor() {
 21         implementation = address(new Vault());
 //	    implementation.init();
 22     }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L211 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L343 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L367 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L393 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L420

Vulnerability details

Impact

HIGH - Assets can be stolen directly When there are multiple proposals for a vault, a failed proposal can withdraw the assets.

Proof of Concept

• proof of concept: testMultiProposal_poc

The proof of concept shows a scenario alice is taking assets.

1. setup: bob makes a vault and sends erc721 id 2 asset to the vault
2. distribution of fractional tokens
3. alice (attacker) proposed for the vault and her proposalId is 1
4. bob (victim) proposed for the vault and his proposalId is 2
5. alice commits to kickoff buyout but fails
6. alice call end on the buyoutmodule
7. bob commits for proposalId 2
8. bob's buyout proposal was successful and bob ends it
9. now alice can withdraw to a vault she proposed

Alice can install any plugins and modules she wants in her migration proposal. So as soon as the assets are in her vault, she can easily transferred out of it.

The cause of it is

1. proposal.isCommited is never unset even after the proposal fails. It enabled to reuse failed old proposal to withdraw
2. the functions to withdraw does not check whether the proposal was successful.
3. the buyout module does not know different proposals, it will send assets regardless which proposalId is withdrawing as long as the buyout was called by migration contract.

Tools Used

foundry

Recommended Mitigation Steps

Functions which withdraws from old vault to new vault should explicitly check the status of proposal, i.e. it is the proposal with successful buyout.

Lines of code

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

Vulnerability details

Impact

HIGH - Assets can be stolen directly. An attacker can steal eth from buyout module

Proof of Concept

• proof of concept1: testCashShare_poc
• proof of concept2: testCashRepeat_poc

The proof of concept1 shows that the same amount of fractions will result in different amount of eth upon cash out. The first cash out for 1000 FERC1155 token will result in 0.2 ETH. Bob cashes out 1000 FERC1155 again, and he will get 0.2666.. ETH. The increased payout is coming from other buyout processes, so essentially bob is stealing eth by dividing the payout.

The proof of concept2 demonstrates a scenario, where one can drain eth from buyout module using a vault with supply target as plugin. The plugin enables to mint more FERC1155 tokens on the way.

1. setup: other people are using buyout module so there are some eth in the module
2. deploy a vault with the target supply as a plugin, mint function as the selector
3. start the buyout process for the vault
4. end the buyout process successfully
5. now the attacker can mint and cash multiple times, possibly until all the eth are drained from the buyout module

For both cases the main issue is that the ethBalance is not properly updated:

// modules/Buyout.sol::cash
// the balance should be updated
// Mitigation idea: update the ethBalance
// ethBalance -= buyoutShare;

266         // Transfers buyout share amount to caller based on total supply
267         uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
268         uint256 buyoutShare = (tokenBalance * ethBalance) /
269             (totalSupply + tokenBalance);
270         _sendEthOrWeth(msg.sender, buyoutShare);

However, the existence of plugins made the issue a lot easier to exploit.

Tools Used

foundry

Recommended Mitigation Steps

Update the ethBalance before sending ether.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Migration.sol#L334-L350 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Buyout.sol#L343-L367

Vulnerability details

Impact

HIGH - Assets can be lost directly After proposal and proposed buyout is successful, anyone can transfer ERC20 asset in the vault to the zero address and the asset will be lost.

Proof of Concept

• proof of concept: testAnyoneCanThrowERC20_poc

The proof of concept demonstrates a scenario, where alice is sending ERC20 asset from bob's vault to zero address, so the asset is lost.

1. set up vault with ERC20
2. bob proposes (proposalId 1)
3. bob commits to kickoff the buyout process
4. after the buyout succeeds bob calls end on buyoutModule
5. now alice can call migrateVaultERC20 with proposalId 2 and send ERC20 to the zero address

// modules/Migration.sol
// in the line 341, zero address for newVault was not checked

334     function migrateVaultERC20(
335         address _vault,
336         uint256 _proposalId,
337         address _token,
338         uint256 _amount,
339         bytes32[] calldata _erc20TransferProof
340     ) external {
341         address newVault = migrationInfo[_vault][_proposalId].newVault;
342         // Withdraws an ERC-20 token from the old vault and transfers to the new vault
343         IBuyout(buyout).withdrawERC20(
344             _vault,
345             _token,
346             newVault,
347             _amount,
348             _erc20TransferProof
349         );
350     }

The migrateVaultERC20 does not check whether the newVault is zero address or not. It just sends ERC20 to the newVault. The buyout module does not check whether to address is zero either: (code). The target Transfer does not check it, either. Some ERC20 tokens will refuse to send to zero address, but some will just send it. For example, the MockERC20 based on solmate's ERC20 does not check for zero address. And The standard does not require to throw upon sending to zero address. Migrate functions for ERC721 and ERC1155 are safe for this issue because they must throw upon sending to zero address. Also The Target Transfer for ERC1155 uses safeTransferFrom.

Tools Used

foundry

Recommended Mitigation Steps

Add zero address check in the migrateVaultERC20 for the address newVault. Also add to check whether the propose was successful. The lack of check can be exploited in a different way combined with other issues. (see the issue Migration Module: The assets can be taken by a failed proposal)

Lines of code

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

Vulnerability details

Impact

HIGH - Assets can be compromised directly. One can use eth from the module for buyout process. (Basically sending somebody else's eth from migration module to buyout module)

Proof of Concept

• proof of concept

The proof of concepts shows a scenario where one can use the eth from module for buyout. 0. setup: some people are using migration module, so some eth are depositted in the module.

1. alice makes a vault and propose
2. alice calls on join and send some eth
3. Normally alice would use leave function to leave, but instead she uses withdrawContribution
4. when she calls commit, somebody else's eth will be used in buyout.

Alice will not be able to get her fractions. But it can be combined with other issues of buyout, where one can drain eth from buyout module. Also, from the point of view of the other users using the migration modules, their money is stolen.

// modules/Migration.sol::withdrawContribution
// The check below allows to use withdrawContribution even before buyout kickoff
// Mitigation idea: add a check `proposal.isCommited == true`

302         if (
303             current != State.INACTIVE ||
304             migrationInfo[_vault][_proposalId].newVault != address(0)
305         ) revert NoContributionToWithdraw();

// modules/Migration.sol::withdrawContribution
// `proposal.totalEth` is not updated
// Mitigation idea: update `proposal.totalEth -= userEth;`

320         // Temporarily store user's eth for the transfer
321         uint256 userEth = userProposalEth[_proposalId][msg.sender];
322         // Udpates ether balance of caller
323         userProposalEth[_proposalId][msg.sender] = 0;

Tools Used

foundry

Recommended Mitigation Steps

1. check whether the proposal was committed by checking proposal.isCommited == true
2. update proposal.totalEth

Lines of code

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

Vulnerability details

Impact

HIGH - Assets can be compromised directly. One can drain eth out from migration module to buyout module using custom made FERC1155 token.

Proof of Concept

• proof of concept: testCommitReenter_poc
• custom made FERC1155 for the attack

The proof of concept shows a scenario where alice is draining migration module using custom made FERC1155 token.

1. setup: other people are using migration module and they deposited some eth. (using alice and bob just to simplify the set up process)
2. alice prepared the custom FERC1155 (let's say evil_token)
3. alice create a vault with the evil_token
4. alice proposes and joins with 0.5 ether
5. when alice calls commit, the evil_token will reenter commit and send money to buyout module

Note: For a simplicity, the evil_token reenters for a fixed number of times. But one can adjust to drain all the eth in the migration module. Note2: For now the eth is in the buyout module, but given the current implementation of buyout module, the same actor can drain eth from buyout.

The commit function is not written in Checks, Effects, Interactions (CEI) patterns.

// modules/Migration.sol::commit
// proposal.isCommited and started are set after the out going calls (i.e. start, setApprovalFor)
// Mitigation idea: set the values before the out going calls

206         if (currentPrice > proposal.targetPrice) {
207             // Sets token approval to the buyout contract
208             IFERC1155(token).setApprovalFor(address(buyout), id, true);
209             // Starts the buyout process
210             IBuyout(buyout).start{value: proposal.totalEth}(_vault);
211             proposal.isCommited = true;
212             started = true;
213         }

Tools Used

foundry

Recommended Mitigation Steps

Follow Checks, Effects, Interactions patterns. One can also consider adding reentrancy guard.

Lines of code

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

Vulnerability details

Impact

MED - the function of the protocol could be impacted. Anyone can call Buyout::start to disable to join, leave, commit functions for migration proposal

Proof of Concept

The Buyout module is unaware of migration module. So, even when some migration is proposed, anybody can start a buyout process. On the other hand, one cannot use propose, leave, join, commit, withdrawContribution from Migration module when a buyout for the relevant vault is active. Using this, a malicious actor can start buyout process just to prevent people from using these functions. For example, somebody proposed a migration, and an attacker starts a buyout so that none can join. For another example, somebody proposed a migration and sent some eth with join function. Then an attacker starts a buyout. As the result, the eth is locked during the buyout period. The attacker can call end and start to keep the asset locked.

// modules/Migration.sol::join
// The buyout state is checked

115         // Reverts if buyout state is not inactive
116         (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
117         State required = State.INACTIVE;
118         if (current != required) revert IBuyout.InvalidState(required, current);

Tools Used

none

Recommended Mitigation Steps

One idea for mitigation would be to prevent to start buyout when there is a proposal. But this weakens the modularity and it can be used to disable the buyout functionality. Another idea would be not checking the buyout state for functions like join, leave. However, it may open up some attack vectors.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/FERC1155.sol#L79-L87

Vulnerability details

Impact

MED - a hypothetical attack path with stated assumptions, but external requirements.

Attacker can create a vault with successful buyout status and non zero supply. The attacker can sell the fractions and then simply withdraw the assets.

Proof of Concept

• proof of concept
• evil redeemer

1. deploy evil_redeemer: it will deployVault and calls redeem when the minted FERC1155 token is received
2. calling start will start the process : baseVault.deployVault
3. the baseVault will deploy vault and eventually mint the FERC1155 token to the evil_redeemer
4. when the FERC1155 is received, the evil_redeemer calls redeem and set the state to SUCCESS
5. after the redeem, the tatalSupply of the FERC1155 is set.

Now, the attacker can send in some assets to the vault and sell the fractions. Then, the attacker can withdraw any asset at any time from the vault using the buyout module, because the state is already SUCCESS.

Note: An attacker can achieve the similar result with plugins. However, users might just refuse to buy tokens associated with such vaults and plugins. With the current issue, the users who is only looking at the vault will not notice this possibility unless they also check the status in the buyout module for the vault.

// FERC1155.sol::mint
// totalSupply is updated after _mint
// _mint contains out going call if the `_to` address's codesize is non zero
// Mitigation idea: update totalSupply before _mint

 79     function mint(
 80         address _to,
 81         uint256 _id,
 82         uint256 _amount,
 83         bytes memory _data
 84     ) external onlyRegistry {
 85         _mint(_to, _id, _amount, _data);
 86         totalSupply[_id] += _amount;
 87     }

Tools Used

foundry

Recommended Mitigation Steps

update totalSupply before _mint.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L73-L82 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/VaultRegistry.sol#L102-L112

Vulnerability details

Impact

HIGH - Assets can be stolen/compromised/lost directly. The creator of vault can add any functionality they want by plugins. Also they can bring any tokens for the vault. It can be used against users, or it will make exploits easier to execute.

Proof of Concept

Plugins are very powerful and can be used maliciously.

• use case 1. in the middle of buyout, mint to the creator so that they can sell the fraction for eth.
• use case 1.5 more supply means, for buyout proposer, they need to buy more share to be successful
• use case 2. by adding transfer function, the creator can always withdraw without buyout process
• use case 3. creator can burn other users' fractions after sell the fractions for eth
• use case 4. add install functionality so the creator can add some malicious plugins additionally later.

As a concrete example, a plugin with minting functionality can be combined with the existing bug to make it much more effective to drain eth from buyout module (for more detailed report, see "Buyout Module: ethBalance is not properly updated"). Here is the summary:

• case1: without plugin use
  • cash out with divided fraction will allow to withdraw more than intended
• case2: with mint plugin
  • one can simply repeat cash and mint process until all eth are withdrawn

Users would have to check which plugins are used in a vault to know whether it is secure, which will hurt the confidence of the users.

Similarly a vault creator can bring their own token by using createInCollection. This enables many attack vectors possible. For example, this proof of concept uses a custom made token to make the attack possible. (More detailed explanation is in the report "Migration Module: Re-enter commit using custom token").

Tools Used

none

Recommended Mitigation Steps

Limit the usage of plugins. idea 1. only certain white listed plugins can be used idea 2. only white listed creators can use plugins idea 3. get rid of the plugin functionalities

Fractional V2 QA Report

Summary

Low

L00: Not following Check-Effect-Interaction patterns

There are multiple occasions where Check-Effect-Interaction pattern was not followed. The pattern is a way to avoid reentrancy attack.

L01: Usage of transfer to send eth

Using transfer to send eth is not anymore recommended as it has a fixed gas stipend of 2300. Use call instead.

L02: Lack of zero address check

There are missing zero address check.

L03: Unresolved TODO

link

./utils/MerkleBase.sol-22-
./utils/MerkleBase.sol-23-        assembly {
./utils/MerkleBase.sol:24:            // TODO: This can be aesthetically simplified with a switch. Not sure it will
./utils/MerkleBase.sol-25-            // save much gas but there are other optimizations to be had in here.
./utils/MerkleBase.sol-26-            if or(lt(_left, _right), eq(_left, _right)) {