Fractional v2 contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Buyout.sol#L87

Vulnerability details

Impact

When calculating the buyout price, the percentage is rounded down to the next integer. This can lead to lost ETH that nobody can claim (see Proof Of Concept).

Proof Of Concept

Consider this extreme scenario: The total supply of the FERC1155 token is 10,000,000. Bob owns 9,999,999 tokens and Alice 1. Bob starts a payout and is willing to pay 1 ETH for Alice's remaining token. We therefore have buyoutPrice = (1 ether * 100) / (100 - ((9_999_999 * 100) / 10_000_000)) = (1 ether * 100) / (100 - 99) = 1 ether * 100. Now, Alice sells her token (either during the period with sellFractions or afterwards with cash, that does not matter) and she gets exactly the fractionPrice, i.e. (1 ether * 100) / 10_000_000 = 0.00001 ether. However, the other 0.99999 are lost, Bob cannot get them back.

Recommended Mitigation Steps

Calculate with a higher precision.

Lines of code

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

Vulnerability details

Impact

In the migrateVault functions, it is not checked whether the newVault is set. Therefore, if a non-existing _proposalId is passed (or the function is called before settleVault is), a transfer to the address 0 will be attempted. Depending on the token implementation, this can succeed (as EIP20 for instance does not force the implementation to throw on a transfer to 0), resulting in burned tokens.

Recommended Mitigation Steps

Check that newVault is not 0.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L310

Vulnerability details

Impact

In withdrawContribution, proposal.totalEth. and proposal.totalFractions are not updated when a user withdraws his contributions. This leads to problems when the function is called before commit, as the calculation in commit is done with the outdated values and the system tries to transfer the outdated values when calling start on the buyout. When there are no other proposals, this causes a reversal (as there are is too little ETH in the contract). In this case, the proposal reverts although it might would have succeeded, when the balance would have been updated (as the updated totalEth and totalFractions could still be enough for a succesful buyout).

Even worse, when people have deposited their ETH for different proposals, some of their deposited ETH will be used for this proposal, as the start call will not revert.

Proof Of Concept

Consider the following diff

--- a/test/Migration.t.sol
+++ b/test/Migration.t.sol
@@ -241,7 +241,9 @@ contract MigrationTest is TestUtil {
         alice.migrationModule.join{value: 1 ether}(vault, 1, 1000);

         vm.warp(proposalPeriod + 1);
-        // bob calls commit to kickoff the buyout process
+        // Alice withdraws her token before bob calls commit
+        alice.migrationModule.withdrawContribution(vault, 1);
+        // bob calls commit to kickoff the buyout process -> now fails, because totalETH is not updated
         bool started = bob.migrationModule.commit(vault, 1);
         assertTrue(started);

In commit, the migration module tries to deposit 2 ETH into the buyout module. This reverts here (because the Migration module does not have more ETH), but would not revert if other people also deposited for different proposals.

Recommended Mitigation Steps

Update the values.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L303

Vulnerability details

Impact

In Migration.sol, it is required that currently no buyout is active (or succeeded). However, it is possible that the buyout is active (or even succeeded) because another one was started.

Proof Of Concept

Alice deposited to an unsuccessful migration and wants to withdraw her contribution. However, after the unsuccessful migration failed, another buyout was started. As long as this buyout is active, Alice cannot call withdrawContribution. If the new buyout succeeds, this is even worse, as she will never be able to call withdrawContribution, because the buyout info of the vault will remain in the status SUCCESS.

Similarly, this can be problematic when two proposals for the same vault are created. One is committed, which initiates a buyout. For the other one, the users want to withdraw their money, which is not possible.

Recommended Mitigation Steps

Track the success of the individual buyouts, not just the last status.

Lines of code

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

Vulnerability details

Impact

After a succesful migration, a user can call migrateFractions to get a multiple of the tokens that are intended for him (or even drain all tokens).

Proof Of Concept

The following diff illustrates the problem:

--- a/test/Migration.t.sol
+++ b/test/Migration.t.sol
@@ -754,6 +754,7 @@ contract MigrationTest is TestUtil {

         assertEq(getFractionBalance(alice.addr), 4000);
         alice.migrationModule.migrateFractions(vault, 1);
+        alice.migrationModule.migrateFractions(vault, 1);
         assertEq(IERC1155(newToken).balanceOf(alice.addr, id), 6000);

         assertEq(getFractionBalance(bob.addr), 0);

Recommended Mitigation Steps

Only allow one call per user / proposal for migrateFractions.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L132

Vulnerability details

Impact

In Vault.sol, it is checked that the owner variable is unchanged after the delegatecall to the target. However, this is not sufficient. An attacker can for instance reset the nonce and afterwards reinitialize the vault, making him the new owner. Afterwards, he has complete control over the vault.

Other variables (merkleRoot, methods) could also be changed, which can be exploited.

Proof Of Concept

Consider the following test diff where the owner is changed to Alice by reinitalizing the vault after a nonce reset:

--- a/test/Vault.t.sol
+++ b/test/Vault.t.sol
@@ -214,6 +214,26 @@ contract VaultTest is TestUtil {
         );
         vaultProxy.execute(address(targetContract), data, proof);
     }
+
+    function testResetNonce() public {
+        vaultProxy.init();
+        assertEq(vaultProxy.owner(), address(this));
+        TargetContract2 targetContract = new TargetContract2();
+        bytes32[] memory proof = new bytes32[](1);
+        proof[0] = keccak256(
+            abi.encode(
+                alice.addr,
+                address(targetContract),
+                TargetContract2.setNonce.selector
+            )
+        );
+        vaultProxy.setMerkleRoot(proof[0]);
+        bytes memory data = abi.encodeCall(targetContract.setNonce, (0));
+        vaultProxy.execute(address(targetContract), data, proof);
+        assertEq(vaultProxy.nonce(), 0);
+        alice.vaultProxy.init();
+        assertEq(vaultProxy.owner(), alice.addr);
+    }
 }

 contract TargetContract {
@@ -227,3 +247,13 @@ contract TargetContract {
         Vault(payable(address(this))).transferOwnership(address(1));
     }
 }
+
+contract TargetContract2 {
+    address public owner;
+    bytes32 public merkleRoot;
+    uint256 public nonce;
+
+    function setNonce(uint256 _nonce) public {
+        nonce = _nonce;
+    }
+}

Recommended Mitigation Steps

Also check for the other contract variables that they are unchanged.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L202 https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L528

Vulnerability details

Impact

When proposal.totalFractions is equal to the total supply (meaning that all token holders want to participate in a migration), there is a division by zero in _calculateTotal.

In contrast to a buyout, where it does not make sense to initiate a buyout if all tokens are held (because there is a dedicated method for that), it does make sense to have a migration that all token holders join. Therefore, this case should be handled.

Proof Of Concept

--- a/test/Migration.t.sol
+++ b/test/Migration.t.sol
@@ -238,7 +238,7 @@ contract MigrationTest is TestUtil {
         // Bob joins the proposal
         bob.migrationModule.join{value: 1 ether}(vault, 1, HALF_SUPPLY);
         // Alice joins the proposal
-        alice.migrationModule.join{value: 1 ether}(vault, 1, 1000);
+        alice.migrationModule.join{value: 1 ether}(vault, 1, HALF_SUPPLY);

         vm.warp(proposalPeriod + 1);
         // bob calls commit to kickoff the buyout process

Recommended Mitigation Steps

In such a case, redeem can be used instead of starting a buyout.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L470

Vulnerability details

Impact

migrateFractions retrieves the new total supply instead of using the saved newFractionSupply of the proposal. If this value changes after the migration (because new tokens are minted using Minter), the migration will fail for some users.

Proof Of Concept

Alice and Bob participate both 50% in a migration with a new fraction supply of 100,000. Alice calls migrateFractions first and retrieves 50,000 tokens. Afterwards, 10,000 new tokens are minted to Charlie. Then, Bob calls migrateFractions. Because newTotalSupply is now 110,000 the variable shareAmount will be 55,000. This causes the transfer to fail, as the Migration module does not have this many tokens left (only 50,000). Therefore, Bob cannot retrieve his tokens.

Recommended Mitigation Steps

Use the saved newFractionSupply of the proposal for the calculations.

Lines of code

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

Vulnerability details

Impact

A user can prevent that a buyout for a vault ever succeeds by continually placing very low bids, see the PoC for details.

Proof Of Concept

Bob wants to avoid at all costs that a buyout succeeds. He currently owns 1 FERC1155 tokens He uses that to initiate a buyout with a deposit of 1 wei. The fractionPrice will be rounded down to 0. Immediately afterwards (in the same transaction), he "buys" the token for free. Of course no one is willing to sell tokens for free, so the auction does not succeed. Just after the end of the buyout period, he submits a Flashbots bundle to call end (where he gets his wei back) and immediately initiates a new buyout using the same method. He can repeat this procedure forever, effectively stopping any buyouts (or migrations).

Recommended Mitigation Steps

You could consider to allow starting a new buyout period when a user submits more money, require a minimum deposit for a buyout, or have some grace period after a buyout (e.g., 2 days) where all users can propose a deposit and the highest value is taken.

• For a migration, shouldn't it be sufficient if the currentPrice is greater than or equal to the target price? (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Migration.sol#L206) At least that is what I would imagine after reading the documentation ("If the target price is reached then a buyout can be triggered")
• For the buyout, the documentation does not match the implementation. In the documentation (https://docs.fractional.art/fractional-v2-1/smart-contracts/modules/buyout), it is mentioned that "If a pool has more than 51% of the total supply after 4 days, ...". However, 51% are not needed, the buyout actually suceeds with 50.1% in the implementation (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/Buyout.sol#L211)
• For FERC1155, once a signature was given to someone, there is no way to cancel the permission, which can be undesirable. Consider adding a cancel method to the contract (that stores the cancelled signatures).
• For FERC1155, when a user gives out multiple signatures (to different accounts at the same time), only the first redeem call will succeed (because of the nonce). If the user wants to avoid that and increases the nonces himself (e.g., nonce 1 for user A, nonce 2 for user B), user A has to use the signature before user B, which is undesirable. Consider optimizing the nonce management, e.g. storing all the used nonces (instead of requiring sequential ones).
• Because of malleable signatures, it is considered best practice to check for invalid s and v values, as OpenZeppelin is doing: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/5e007871991e4f04e871bf5fb1200668ff16b35f/contracts/utils/cryptography/ECDSA.sol#L142 In FERC1155.sol, ecrecover is called without these checks.
• In setApprovalFor (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/FERC1155.sol#L191), only a boolean can be provided. This can be very undesirable for an ERC1155 token where a user might also want to specify an amount for the allowance. Consequently, the approvals are not decreased or reset in safeTransferFrom.
• ERC1155 inherits safeBatchTransferFrom from Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC1155.sol#L78), but isApproved is not checked in this function, meaning that someone who got the approval for all the provided ids still cannot call safeBatchTransferFrom. Consider also overriding this function and adding the check that all ids are approved as an alternative.
• In ERC1155, it is possible to set arbitrarily high royalties (even >100%), which does not make sense and leads to wrong values in royaltyInfo. Consider adding a limit.
• In ERC1155, although the royalty is a uint256 value, the actual granularity for the calculation is very small and only whole percent values can be provided (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/FERC1155.sol#L247). Consider increasing the granularity, e.g. dividing by 10,000.
• In _execute of Vault, returning success (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L117) is not necessary. The execution will always revert when it was not successful, i.e. there is no way that sucess can be false.
• BaseVault hardcodes the number of hashes to 6 (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/protoforms/BaseVault.sol#L128), which seems to be the number when modules contains BaseVault and BuyoutModule. However, depending on the modules that are passed, the number of hashes can be greater. Therefore, the array size should be dynamic.
• It seems strange to me that there is no authentication for plugins, whereas the whole Merkle Tree authentication system exists for modules. Therefore, if an owner would (accidentally) add a module as a plugin, everyone could call it and the whole module authentication would be circumvented. This also contradicts the sentence "An NFT within a given Vault cannot be withdrawn unless its modules allow for it." in the documentation. An owner can simply add a new plugin and withdraw all tokens (meaning there is a lot of trust in the owner).
• There is currently no way to transfer airdrops (e.g., ERC20 tokens that are distributed to all holders of a NFT) out of a Vault without starting a migration. Consider adding a module that would allow to transfer them.
• The WETH address (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/utils/SafeSend.sol#L12) is hardcoded, which can make testing and deploying on other networks more difficult. Consider making the address configurable.
• In the documentation, it is mentioned "These modules are set by the curator upon fractionalization and can be adjusted via governance vote of token holders.". However, this does not seem to be true. An owner can always set a new merkle root (via setMerkleRoot) which allows new modules.
• An attacker could call createInCollection with some malicious tokens that he created (conforming to the FERC1155 calls) and therefore create a vault with a malicous, worthless tokens. He could then sell those tokens and for normal users it would appear as if they are normal fractionalization tokens of a vault.
• It seems strange to me to have a functionality (createInCollection) for creating a new vault with an existing token. This could result in some scenarios that are undesired. For example, when a buyout is active for both vaults, there is an arbitrage opportunity (buy the tokens with the lower price, sell the ones with the higher) and the person that initiated the one with the lower price will not succeed, although it might have (because people would have been willing to sell at this price) when only vault with this token had existed.

• Some mathematical operations can be unchecked because an underflow / overflow is not possible:
  • https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/FERC1155.sol#L62
  • https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L78
  • https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L104
  • https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/modules/protoforms/BaseVault.sol#L107
• In Vault, using a uint256 for the nonce (https://github.com/code-423n4/2022-07-fractional/blob/f862c14f86adf7de232cd4e9cca6b611e6023b98/src/Vault.sol#L17), which is a variable that will only take the value 0 or 1, is wasteful. A boolean isInitialized would also work.