Fractional v2 contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the end function of the Buyout contract, when the buyout fails, ERC1155 tokens are sent to the proposer. A malicious proposer can start a buyout using a contract that cannot receive ERC1155 tokens, and if the buyout fails, the end function fails because it cannot send ERC1155 tokens to the proposer. This prevents a new buyout from being started.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider saving the status of the proposer after a failed buyout and implementing functions to allow the proposer to withdraw the ERC1155 tokens and eth

Lines of code

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

Vulnerability details

Impact

BuyoutInfo[_vault].ethBalance was not updated in the cash function of the Buyout contract, resulting in incorrect eth distribution. Considering that there are currently 40 eth in the contract, the total supply of ERC1155 tokens is still 400. User A uses 200 ERC1155 tokens to withdraw eth, User A will get 40 * 200/400 = 20 eth. User B uses 100 ERC1155 tokens to withdraw eth, and User B will get 40 * 100/200 = 20 eth. When user C uses 100 ERC1155 tokens to withdraw eth, the eth cannot be withdrawn because the eth in the contract has been exhausted

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

        uint256 buyoutShare = (tokenBalance * ethBalance) /
            (totalSupply + tokenBalance);
+     buyoutInfo[_vault].ethBalance -= buyoutShare;
        _sendEthOrWeth(msg.sender, buyoutShare);

Lines of code

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

Vulnerability details

Impact

In the Migration contract, when a proposal is committed and successfully bought out, the settleVault function can be called to set the newVault, and the MigrateVault* function can be called to transfer the assets to the newVault. However, since the MigrateVault* function does not check if the newVault is address 0, a malicious user can call the MigrateVault* function immediately after a successful buyout to send the assets to address 0.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Add 0 address check to the newVault in the MigrateVault* function of the Migration contract

Lines of code

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

Vulnerability details

Impact

The _execute function of the Vault contract will use delegatecall to call functions of other contracts. In order to avoid the delegatecall from modifying the owner variable, the function checks whether the owner has changed. However, if the delegatecall modifies merkleRoot or methods, the attacker can manipulate the Vault contract via the delegatecall. Consider the following scenario, if a module in the Vault contract can modify merkleRoot, an attacker can first call the module to modify the merkleRoot of the Vault contract, and then delegatecall a function that matches the new merkleRoot to destroy the Vault contract or transfer the assets in the Vault contract.

Proof of Concept

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L131-L132 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L15-L21

Tools Used

None

Recommended Mitigation Steps

Consider requiring that the merkleRoot and methods of the Vault contract not be changed after the _execute function calls delegatecall

Lines of code

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

Vulnerability details

Impact

The withdrawContribution and leave functions of the Migration contract use payable.transfer to send eth to the user. This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract. Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Proof of Concept

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172-L173 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L325-L326 https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

None

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

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

Vulnerability details

Impact

A malicious proposer can use 1 ERC1155 token and 0 eth to start a buyout. While other users can buy this ERC1155 token without spending eth, the malicious proposer can prevent others from starting buyouts for 4 days. And the malicious proposer can do this repeatedly to prevent others from starting buyouts

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider setting the minimum number of tokens required to start a buyout in the start function to prevent malicious buyouts.

[Low-01]FERC1155: Manipulations of setRoyalties

Impact

The controller can set royaltyPercent to 100 in the setRoyalties function of the FERC1155 contract. If the royaltyInfo function is used to obtain royalty information when a user purchases an NFT, this may frontrun the selling process, causing the seller to lose assets

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider limiting royaltyPercent in setRoyalties function

[Low-02] Buyout: incorrect comment

Impact

The comment on line 333 should be

// Executes transfer of ERC20 token to caller

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

-    // Executes transfer of ERC721 token to caller
+    // Executes transfer of ERC20 token to caller