Fractional v2 contest - xiaoming90's results

Lines of code

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

Vulnerability details

Background

The following describes the migration process for a vault.

1. Assume that Alice is the proposer.
2. Alice calls Migration.propose to propose a set of modules and plugins to migrate a vault to
3. Other contributors could join a migration proposal by contributing ether and fractional tokens by calling Migration.join.
4. Alice calls Migration.commit to kick off the buyout process for a migration after the proposal period (7 days)
5. If the buyout is successful, Alice calls the Migration.settleVault to settle a migration. Within this function, a new vault with new set permissions and plugins will be deployed.
6. Alice calls the Migration.settleFractions to mint the fractional tokens for a new vault.
7. Contributors who earlier joined the migration proposal could call the Migration.migrateFractions to migrate their fractional tokens from the old vault to the new vault.
8. Finally, Alice will call Migration.migrateVaultERC20, Migration.migrateVaultERC721, and/or Migration.migrateVaultERC1155 to transfer the ERC20, ERC721 (NFT), and/or ERC1155 tokens from the old vault to the new vault.

Vulnerability Details

It was observed that after a successful vault migration, an attacker could Migration.migrateVaultERC20, Migration.migrateVaultERC721, and/or Migration.migrateVaultERC1155 with an invalid _proposalId parameter, causing the assets within the vault to be burned.

Proof-of-Concept

The PoC for Migration.migrateVaultERC20, Migration.migrateVaultERC721, and/or Migration.migrateVaultERC1155 is the same. Thus, only the PoC for Migration.migrateVaultERC721 is shown below, and the PoC for migrateVaultERC20 and migrateVaultERC1155 are omitted for brevity.

Assume that the following:

• vault A holds only one (1) APE ERC721 NFT

• Alice proposes to migrate vault A to a new vault, and the buyout is successful.

• Alice proceeds to call Migration.settleVault to settle a migration, followed by Migration.settleFractions to mint the fractional tokens for a new vault.

• An attacker calls Migration.migrateVaultERC721(vault A, invalid_proposal_id, ape_nft_address, ape_nft_tokenId, erc721TransferProof) with an invalid proposal ID (proposal ID that does not exist).

  • Within the Migration.migrateVaultERC721 function, the newVault = migrationInfo[_vault][_proposalId].newVault will evaluate to zero. This is because the _proposalId is a non-existent index in the migrationInfo array, so it will point to an address space that has not been initialised yet. Thus, the value zero will be returned, and newVault will be set to address(0).

• Next, the Migration.migrateVaultERC721 function will attempt to transfer the ERC721 NFT from the old vault (_vault) to the new vault (newVault) by calling IBuyout(buyout).withdrawERC721. Since newVault is set to address(0), this will cause the ERC721 NFT to be sent to address(0), which effectively burns the NFT.

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

/// @notice Migrates an ERC-721 token to the new vault after a successful migration
/// @param _vault Address of the vault
/// @param _proposalId ID of the proposal
/// @param _token Address of the ERC-721 token
/// @param _tokenId ID of the token
/// @param _erc721TransferProof Merkle proof for transferring an ERC-721 token
function migrateVaultERC721(
    address _vault,
    uint256 _proposalId,
    address _token,
    uint256 _tokenId,
    bytes32[] calldata _erc721TransferProof
) external {
    address newVault = migrationInfo[_vault][_proposalId].newVault;
    // Withdraws an ERC-721 token from the old vault and transfers to the new vault
    IBuyout(buyout).withdrawERC721(
        _vault,
        _token,
        newVault,
        _tokenId,
        _erc721TransferProof
    );
}

Additional Note #1 - About Buyout.withdrawERC721

When a user proposes a migration, the user will kick off the buyout process after the proposal period. The Migration module will initiate the buyout on behalf of the user. Thus, the proposer of this buyout, in this case, would be the Migration module. Whenever Buyout.withdrawERC721 function is called, it will verify that msg.sender is equal to the proposer to ensure that only the proposer who is the auction winner can migrate the assets from old vault to new vault.

In this example, the attacker has access to Migration.migrateVaultERC20, Migration.migrateVaultERC721, and/or Migration.migrateVaultERC1155 functions that effectively instruct the Migration module to perform the withdrawal. In this case, it will pass the if (msg.sender != proposer) revert NotWinner(); validation within the Buyout.withdrawERC721 because the msg.sender is the Migration contract who initiates the buyout at the start.

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

function IBuyout(buyout).withdrawERC721(
    address _vault,
    address _token,
    address _to,
    uint256 _tokenId,
    bytes32[] calldata _erc721TransferProof
) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if auction state is not successful
    (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
    State required = State.SUCCESS;
    if (current != required) revert InvalidState(required, current);
    // Reverts if caller is not the auction winner
    if (msg.sender != proposer) revert NotWinner();

    // Initializes vault transaction
    bytes memory data = abi.encodeCall(
        ITransfer.ERC721TransferFrom,
        (_token, _vault, _to, _tokenId)
    );
    // Executes transfer of ERC721 token to caller
    ..SNIP..
}

Additional Note #2 - Can we send NFT to address(0)?

Yes, it is possible to send NFT to address(0).

If the ERC721 NFT contract uses Openzeppelin's ERC721 contract or Solmate's ERC721 contract, then the NFT cannot be sent to address(0) because the contracts have implemented validation check to ensure that the to address is not address(0).

However, not all the ERC721 NFT contracts use Openzeppelin or Solmate ERC721 implementation. Therefore, there will be a large number of custom implementations that allow NFT to be transferred to address(0).

The same theory applies to ERC20 and ERC1155 implementations.

Impact

Loss of assets for the users as the assets that they own can be burned by an attacker after a successful migration.

Recommended Mitigation Steps

It is recommended to implement additional validation to ensure that the _proposalId submitted is valid.

Consider checking if newVault points to a valid vault address before transferring the assets from old vault to new vault.

function migrateVaultERC721(
    address _vault,
    uint256 _proposalId,
    address _token,
    uint256 _tokenId,
    bytes32[] calldata _erc721TransferProof
) external {
    address newVault = migrationInfo[_vault][_proposalId].newVault;
+    if (newVault == address(0)) reverts VaultDoesNotExistOrInvalid;
    
    // Withdraws an ERC-721 token from the old vault and transfers to the new vault
    IBuyout(buyout).withdrawERC721(
        _vault,
        _token,
        newVault,
        _tokenId,
        _erc721TransferProof
    );
}

In the above implementation, if anyone attempts to submit an invalid _proposalId, the newVault will be set to address(0). The newly implemented validation will detect the abnormal behavior and revert the transaction.

For defense-in-depth, perform additional validation to ensure that the _to address is not address(0) within the Buyout.withdrawERC721 function.

function withdrawERC721(
    address _vault,
    address _token,
    address _to,
    uint256 _tokenId,
    bytes32[] calldata _erc721TransferProof
) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
+   if (_to == 0) revert ToAddressIsZero(); 
    // Reverts if auction state is not successful
    (, address proposer, State current, , , ) = this.buyoutInfo(_vault);
    State required = State.SUCCESS;
    if (current != required) revert InvalidState(required, current);
    // Reverts if caller is not the auction winner
    if (msg.sender != proposer) revert NotWinner();

    // Initializes vault transaction
    bytes memory data = abi.encodeCall(
        ITransfer.ERC721TransferFrom,
        (_token, _vault, _to, _tokenId)
    );
    // Executes transfer of ERC721 token to caller
    IVault(payable(_vault)).execute(transfer, data, _erc721TransferProof);
}

The same validation checks should be implemented on migrateVaultERC20, migrateVaultERC1155, withdrawERC20 and withdrawERC1155

Lines of code

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

Vulnerability details

Background

Migration.leave is used to leave a proposed migration with contribution amount. This function is intended to be used before the migration proposal is committed and before the buyout process is kicked off. Before the migration is committed, contributors are allowed to leave with their contribution amount.

Migration.withdrawContribution retrieves ether and fractions deposited from an unsuccessful migration. This function is intended to be used after an unsuccessful migration after the buyout process has failed.

Vulnerability Details

After analysing the Migration.leave and Migration.withdrawContribution, it was observed that both functions are almost similar with only the following minor differences:

• Migration.leave will deduct the amount from proposal.totalFractions and ethAmount from proposal.totalEth, while the Migration.withdrawContribution does not.

• Migration.withdrawContribution will check whether the new vault has been deployed, and if the new vault has been deployed, users are not allowed to call this function. Migration.leave does not perform this check.

The issue is that after a malicious user has called Migration.join to contribute his assets, he can call Migration.withdrawContribution function instead of the intended Migration.leave function to leave the proposed migration with his contributed asset.

When Migration.withdrawContribution is called instead, the contributed assets are returned back to the caller, but the proposal.totalFractions and proposal.totalEth are not deducted and will remain the same.

Thus, the malicious could call Migration.join followed by Migration.withdrawContribution multiple times to push up the proposal.totalFractions and proposal.totalEth without locking any asset in the proposal.

Validation within Migration.withdrawContribution function

Following are the validations implemented within this function. The malicious user will be able to pass these validations without any issue:

• if (id == 0) revert NotVault(_vault); - The vault will exists. Thus, the check will pass
• if (current != State.INACTIVE || migrationInfo[_vault][_proposalId].newVault != address(0)) revert NoContributionToWithdraw(); - The first condition will pass because the vault current state is Status.INACTIVE. The second condition will pass because migrationInfo[_vault][_proposalId].newVault has not been initialised yet, thus it will evaluate to zero.

Proof-of-Concept

1. Alice creates a migration proposal. The current proposal state is proposal.totalFractions is 0 and proposal.totalEth is 0.
2. Bob holds 100 ETH and 100 Fractional tokens. He decides to contribute to the migration proposal by calling Migration.join and contribute 100 ETH and 100 Fractional tokens.
3. Bob holds 0 ETH and 0 Fractional tokens at this point. The current proposal state is proposal.totalFractions is 100 and proposal.totalEth is 100.
4. Bob calls Migration.withdrawContribution and he gets back his contributed assets. Bob holds 100 ETH and 100 Fractional tokens at this point. However, the Current proposal state still remains unchanged and proposal.totalFractions is still 100 and proposal.totalEth is still 100.
5. Bob calls Migration.join and contribute 100 ETH and 100 Fractional tokens again.
6. Bob holds 0 ETH and 0 Fractional tokens at this point. The current proposal state is proposal.totalFractions is 200 and proposal.totalEth is 200.
7. Bob calls Migration.withdrawContribution and he gets back his contributed assets. Bob holds 100 ETH and 100 Fractional tokens at this point. However, the Current proposal state still remains unchanged and proposal.totalFractions is still 200 and proposal.totalEth is still 200.
8. Bob can repeat as many times as he likes until the proposal.totalFractions and proposal.totalEth reaches his desired amount and hit his desired price.

Impact

By exploiting this vulnerability, a malicious user can increase the proposal.totalFractions and proposal.totalEth to an arbitrary amount without contributing or locking any assets (Fractional token or ETH) in the proposal.

Assume that after exploiting this vulnerability, proposal.totalFractions is 1000 and proposal.totalEth is 1000, but there are no real assets locked within the proposal by the malicious user. When the proposer calls Migration.commit, it will kick off the buyout process with 1000 fractional tokens and 1000 ETH.

One of the following outcomes will happen:

• If the Migration contract does not have sufficient assets, it will revert; OR

• However, if the Migration contract has sufficient assets, the assets will be sent to the vault to kick start the buyout process. Note that the malicious user has not contributed or locked any asset in the proposal. Thus, the assets sent over to the vault for kicking off the buyout process actually belong to the contributors of other proposals. Assets belonging to other contributors of other proposals, in this case, will be stolen.

Recommended Mitigation Steps

It is recommended to only allow users to call the Migration.withdrawContribution after an unsuccessful migration when the buyout process has ended and is unsuccessful. Users should not be able to call the Migration.withdrawContribution before that.

Consider implementing the following validation check. If a proposal's isCommited is not true, this means that the proposal has not been committed and the buyout process has not been kicked off yet. Therefore, this proposal cannot be used within the Migration.withdrawContribution as it does not meet the requirements.

/// @notice Retrieves ether and fractions deposited from an unsuccessful migration
/// @param _vault Address of the vault
/// @param _proposalId ID of the failed proposal
function withdrawContribution(address _vault, uint256 _proposalId)
    external
{
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);

+    // Gets the migration proposal for the given ID
+    Proposal storage proposal = migrationInfo[_vault][_proposalId];

+    // Reverts if the proposal is not committed
+    if (!proposal.isCommited ) revert ProposalNotCommited;
    
    // Reverts if caller has no fractional balance to withdraw
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    if (
        current != State.INACTIVE ||
        migrationInfo[_vault][_proposalId].newVault != address(0)
    ) revert NoContributionToWithdraw();
    ..SNIP..
}

Lines of code

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

Vulnerability details

Vulnerability Details

A depositor cannot have any residual allowance after depositing to the vault because the tokens can be stolen by anyone.

Proof-of-Concept

Assume that Alice has finished deploying the vault, and she would like to deposit her ERC20, ERC721, and ERC1155 tokens to the vault. She currently holds the following assets in her wallet

• 1000 XYZ ERC20 tokens
• APE #1 ERC721 NFT, APE #2 ERC721 NFT, APE #3 ERC721 NFT,
• 1000 ABC ERC1155 tokens

Thus, she sets up the necessary approval to grant baseVault contract the permission to transfer her tokens to the vault.

erc20.approve(address(baseVault), type(uint256).max);
erc721.setApprovalForAll(address(baseVault), true);
erc1155.setApprovalForAll(address(baseVault), true);

Alice decided to deposit 50 XYZ ERC20 tokens, APE #1 ERC721 NFT, and 50 ABC tokens to the vault by calling baseVault.batchDepositERC20, baseVault.batchDepositERC721, and baseVault.batchDepositERC1155 as shown below:

baseVault.batchDepositERC20(alice.addr, vault, [XYZ.addr], [50])
baseVault.batchDepositERC721(alice.addr, vault, [APE.addr], [#1])
baseVault.batchDepositERC1155(alice.addr, vault, [ABC.addr], [#1], [50], "")

An attacker notices that there is residual allowance left on the baseVault, thus the attacker executes the following transactions to steal Alice's assets and send them to the attacker's wallet address.

baseVault.batchDepositERC20(alice.addr, attacker.addr, [XYZ.addr], [950])
baseVault.batchDepositERC721(alice.addr, attacker.addr, [APE.addr, APE.addr], [#2, #3])
baseVault.batchDepositERC1155(alice.addr, attacker.addr, [ABC.addr], [#1], [950], "")

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

function batchDepositERC20(
    address _from,
    address _to,
    address[] calldata _tokens,
    uint256[] calldata _amounts
) external {
    for (uint256 i = 0; i < _tokens.length; ) {
        IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);
        unchecked {
            ++i;
        }
    }
}

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

function batchDepositERC721(
    address _from,
    address _to,
    address[] calldata _tokens,
    uint256[] calldata _ids
) external {
    for (uint256 i = 0; i < _tokens.length; ) {
        IERC721(_tokens[i]).safeTransferFrom(_from, _to, _ids[i]);
        unchecked {
            ++i;
        }
    }
}

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

function batchDepositERC1155(
    address _from,
    address _to,
    address[] calldata _tokens,
    uint256[] calldata _ids,
    uint256[] calldata _amounts,
    bytes[] calldata _datas
) external {
    unchecked {
        for (uint256 i = 0; i < _tokens.length; ++i) {
            IERC1155(_tokens[i]).safeTransferFrom(
                _from,
                _to,
                _ids[i],
                _amounts[i],
                _datas[i]
            );
        }
    }
}

Impact

Lost of assets for users as a malicious user could utilise the baseVault contract to exploit the user's residual allowance to steal their assets.

Recommended Mitigation Steps

It is recommended to only allow the baseVault.batchDepositERC20, baseVault.batchDepositERC721, and baseVault.batchDepositERC1155 functions to pull tokens from the caller (msg.sender).

Considering updating the affected functions to remove the from parameter, and use msg.sender instead.

function batchDepositERC20(
-   address _from,
    address _to,
    address[] calldata _tokens,
    uint256[] calldata _amounts
) external {
    for (uint256 i = 0; i < _tokens.length; ) {
-       IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);
+       IERC20(_tokens[i]).transferFrom(msg.sender, _to, _amounts[i]);
        unchecked {
            ++i;
        }
    }
}

function batchDepositERC721(
-   address _from,
    address _to,
    address[] calldata _tokens,
    uint256[] calldata _ids
) external {
    for (uint256 i = 0; i < _tokens.length; ) {
-       IERC721(_tokens[i]).safeTransferFrom(_from, _to, _ids[i]);
+       IERC721(_tokens[i]).safeTransferFrom(msg.sender, _to, _ids[i]);
        unchecked {
            ++i;
        }
    }
}

function batchDepositERC1155(
-   address _from,
    address _to,
    address[] calldata _tokens,
    uint256[] calldata _ids,
    uint256[] calldata _amounts,
    bytes[] calldata _datas
) external {
    unchecked {
        for (uint256 i = 0; i < _tokens.length; ++i) {
            IERC1155(_tokens[i]).safeTransferFrom(
-               _from,
+               msg.sender,
                _to,
                _ids[i],
                _amounts[i],
                _datas[i]
            );
        }
    }
}

Lines of code

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

Vulnerability details

Vulnerability Details

Multiple migration proposals can be initiated. However, only one migration proposal can be committed at a time because committing a migration proposal will kick off the buyout process on the vault, and only one buyout can happen on a vault at any point in time.

If one of the proposals is committed and the buyout is successful, the contributors of other proposals have no way to withdraw their contributed assets from the uncommitted proposals. Thus, their assets are stuck in the Migration contract.

Proof-of-Concept

1. Alice calls Migration.propose to create a new migration proposal. We will call this migration proposal Proposal A
2. Bob also call Migration.propose to create a new migration proposal. We will call this migration proposal Proposal B
3. Charles decides to contribute 100 ETH and 100 fractional tokens to Alice's Proposal A by calling Migration.join.
4. David decides to contribute 100 ETH and 100 fractional tokens to Bob's Proposal B by calling Migration.join.
5. After the proposal period (7 days), both Proposal A and Proposal B hit the target price, and therefore both proposals are eligible to be committed by calling Migration.commit to kick off the buyout process.
6. Alice calls Migration.commit on her Proposal A to kick off the buyout process. The vault state is changed to State.LIVE.
7. Bob attempts to call Migration.commit on his Proposal B, but it will fail and revert. This is because the vault state is currently set to State.LIVE, and commit can only be activated if the target vault state is State.INACTIVE.
8. Alice's buyout is successful, thus the vault state changed to State.SUCCESS.
9. Bob attempts to call Migration.commit on his Proposal B again, but it will fail and revert. This is the vault state is currently set to State.SUCCESS, and commit can only be activated if the target vault state is State.INACTIVE. Thus, there is no way for Bob to commit his proposal.
10. At this point, Charles (Contributor of Proposal B) knowing that Bob's Proposal B is unsuccessful, he decides to withdraw the assets that he contributed earlier from the Migration contract.
11. Charles calls Migration.leave which is a function for leaving a proposed migration with contribution amount. However, this function will revert because the vault state is not State.INACTIVE
12. Charles tries calling Migration.withdrawContribution which is a function to retrieve ether and fractions deposited from an unsuccessful migration. However, the function will revert too because the vault state is not State.INACTIVE
13. Charles has no way to withdraw his contribution, thus his assets are stuck in the Migration contract.

The following shows that multiple migration proposals can be created and run at any point in time.

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

function propose(
    address _vault,
    address[] calldata _modules,
    address[] calldata _plugins,
    bytes4[] calldata _selectors,
    uint256 _newFractionSupply,
    uint256 _targetPrice
) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not inactive
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);

    // Initializes migration proposal info
    Proposal storage proposal = migrationInfo[_vault][++nextId]; // @audit not too sure about the nextID.
    proposal.startTime = block.timestamp;
    proposal.targetPrice = _targetPrice;
    proposal.modules = _modules;
    proposal.plugins = _plugins;
    proposal.selectors = _selectors;
    proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(
        _vault
    );
    proposal.newFractionSupply = _newFractionSupply;
}

The following shows that the user could only call Migration.leave if the vault state is State.INACTIVE. Otherwise, it will revert.

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

/// @notice Leaves a proposed migration with contribution amount
/// @param _vault Address of the vault
/// @param _proposalId ID of the proposal being left
function leave(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not inactive
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);
    ..SNIP..
}

The following shows that the user could only call Migration.withdrawContribution if the vault state is State.INACTIVE. Otherwise, it will revert.

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

/// @notice Retrieves ether and fractions deposited from an unsuccessful migration
/// @param _vault Address of the vault
/// @param _proposalId ID of the failed proposal
function withdrawContribution(address _vault, uint256 _proposalId)
    external
{
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
    // Reverts if caller has no fractional balance to withdraw
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    if (
        current != State.INACTIVE ||
        migrationInfo[_vault][_proposalId].newVault != address(0)
    ) revert NoContributionToWithdraw();
	..SNIP..
}

Impact

Loss of assets for contributors as their contributed assets are stuck in the Migration contract.

Recommended Mitigation Steps

It is recommended to update the implementation to allow contributors of an unsuccessful migration proposal to withdraw their contributed assets.

When a migration proposal is committed, the proposal struct's isCommited will be set to true

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

function commit(address _vault, uint256 _proposalId)
    external
    returns (bool started)
{
    ..SNIP..

    // Checks if the current price is greater than target price of the proposal
    if (currentPrice > proposal.targetPrice) {
        // Sets token approval to the buyout contract
        IFERC1155(token).setApprovalFor(address(buyout), id, true);
        // Starts the buyout process
        IBuyout(buyout).start{value: proposal.totalEth}(_vault);
        proposal.isCommited = true;
        started = true;
    }
}

All contributors should be allowed to leave a proposed migration with contribution amount as long as the proposal has not been committed yet, regardless of the vault state.

/// @notice Leaves a proposed migration with contribution amount
/// @param _vault Address of the vault
/// @param _proposalId ID of the proposal being left
function leave(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
-    // Reverts if buyout state is not inactive
-    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
-    State required = State.INACTIVE;
-    if (current != required) revert IBuyout.InvalidState(required, current);
    
+    // Gets the migration proposal for the given ID
+    Proposal storage proposal = migrationInfo[_vault][_proposalId];
+
+	 // Reverts if proposal is already committed
+    if (proposal.isCommited) reverts CannotLeaveWhenProposalIsCommitted();
	..SNIP..
}

Lines of code

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

Vulnerability details

Background

The following describes the migration process for a vault.

1. Assume that Alice is the proposer.
2. Alice calls Migration.propose to propose a set of modules and plugins to migrate a vault to
3. Other contributors could join a migration proposal by contributing ether and fractional tokens by calling Migration.join.
4. Alice calls Migration.commit to kick off the buyout process for a migration after the proposal period (7 days)
5. If the buyout is successful, Alice calls the Migration.settleVault to settle a migration. Within this function, a new vault with new set permissions and plugins will be deployed.
6. Alice calls the Migration.settleFractions to mint the fractional tokens for a new vault.
7. Contributors who earlier joined the migration proposal could call the Migration.migrateFractions to migrate their fractional tokens from the old vault to the new vault.
8. Finally, Alice will call Migration.migrateVaultERC20, Migration.migrateVaultERC721, and/or Migration.migrateVaultERC1155 to transfer the ERC20, ERC721 (NFT), and/or ERC1155 tokens from the old vault to the new vault.

Vulnerability Details

It was observed that after a successful vault migration, a malicious user could call Migration.migrateFractions multiple times to steal new fractional tokens belonging to other users.

Proof-of-Concept

1. Assume that Charles has initiated a proposal to migrate to the new vault by calling Migration.propose function.
2. Assume that the total supply of fractional tokens in the existing vault is 100 fractional tokens
3. Alice decided to join the migration proposal by calling Migration.join to contribute 25 fractional tokens and 50 Ethers.
4. Bob decided to join the migration proposal by calling Migration.join to contribute 25 fractional tokens and 50 Ethers.
5. The buyout process is successful, and Charles proceeds to call Migration.settleVault to settle a migration, follow by Migration.settleFractions to mint the fractional tokens for a new vault. Assume that 100 new fractional tokens are minted.
6. Note that both Alice and Bob have contributed equally during the proposal period. Therefore, each of them is entitled to half of the total new fractional tokens minted. Thus, Alice should be entitled to 50 new fractional tokens, and Bob should be entitled to 50 new fractional tokens.
7. Alice calls Migration.migrateFractions, and receives 50 new fractional tokens.
8. Alice calls Migration.migrateFractions again, and receives 50 new fractional tokens.
9. At this point, all the 100 new fractional tokens have been transferred to Alice.
10. Bob is left with nothing. When Bob tried to call Migration.migrateFractions, the Migration contract will attempt to send Bob 50 new fractional tokens. However, the transfer will revert because there is no new fractional token left in the Migration contract.

Root Cause

The root cause of this issue is that the Migration.migrateFractions does not keep track of the fact that Alice has already migrated/withdraw her new fractional tokens. Thus, Alice could call the function, again and again, to migrate/withdraw more new fractional tokens from the Migration contract.

Using the above example, before Alice calls the Migration.migrateFractions function, her state is as follows:

userProposalEth[_proposalId][alice.address] is 50 ETH

userProposalFractions[_proposalId][alice.address] is 25 Fractional Tokens

After Alice calls the Migration.migrateFractions function, her state remains the same is as follows:

userProposalEth[_proposalId][alice.address] is 50 ETH

userProposalFractions[_proposalId][alice.address] is 25 Fractional Tokens

This is because the Migration.migrateFractions function did not deduct Alice's balances after she has migrated/withdrawn. Thus, she could call this function again, and she will be given the same amount of new fractional tokens.

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

/// @notice Migrates the caller's fractions from an old vault to a new one after a successful migration
/// @param _vault Address of the vault
/// @param _proposalId ID of the proposal
function migrateFractions(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not successful
    (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    State required = State.SUCCESS;
    if (current != required) revert IBuyout.InvalidState(required, current);
    // Reverts if proposer of buyout is not this contract
    if (proposer != address(this)) revert NotProposalBuyout();

    // Gets the last total supply of fractions for the vault
    (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    // Calculates the total ether amount of a successful proposal
    uint256 totalInEth = _calculateTotal(
        1 ether,
        lastTotalSupply,
        migrationInfo[_vault][_proposalId].totalEth,
        migrationInfo[_vault][_proposalId].totalFractions
    );
    // Calculates balance of caller based on ether contribution
    uint256 balanceContributedInEth = _calculateContribution(
        totalInEth,
        lastTotalSupply,
        userProposalEth[_proposalId][msg.sender],
        userProposalFractions[_proposalId][msg.sender]
    );

    // Gets the token and fraction ID of the new vault
    address newVault = migrationInfo[_vault][_proposalId].newVault;
    (address token, uint256 newFractionId) = IVaultRegistry(registry)
        .vaultToToken(newVault);
    // Calculates share amount of fractions for the new vault based on the new total supply
    uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
    uint256 shareAmount = (balanceContributedInEth * newTotalSupply) /
        totalInEth;

    // Transfers fractional tokens to caller based on share amount
    IFERC1155(token).safeTransferFrom(
        address(this),
        msg.sender,
        newFractionId,
        shareAmount,
        ""
    );
}

Impact

Loss of assets for contributors as they could not get the new fractional tokens after a successful migration.

Recommended Mitigation Steps

It is recommended to keep track of the user who has already migrated/withdraw their new fractional tokens and prevent them from doing so more than once.

Considering implementing one of the following solutions:

Solution #1 - Deduct contributor's balance after withdrawing/migrating

If the contributors attempt to call the migrateFractions the second time, they will receive nothing in return since their balance is zero.

function migrateFractions(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not successful
    (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    State required = State.SUCCESS;
    if (current != required) revert IBuyout.InvalidState(required, current);
    // Reverts if proposer of buyout is not this contract
    if (proposer != address(this)) revert NotProposalBuyout();

    // Gets the last total supply of fractions for the vault
    (, , , , , uint256 lastTotalSupply) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    // Calculates the total ether amount of a successful proposal
    uint256 totalInEth = _calculateTotal(
        1 ether,
        lastTotalSupply,
        migrationInfo[_vault][_proposalId].totalEth,
        migrationInfo[_vault][_proposalId].totalFractions
    );
    
+    uint256 userETHContribution = userProposalEth[_proposalId][msg.sender];
+    uint256 userFractionsContribution = userProposalFractions[_proposalId][msg.sender];
    
+    userProposalEth[_proposalId][msg.sender] = 0;
+    userProposalFractions[_proposalId][msg.sender] = 0;
    
    // Calculates balance of caller based on ether contribution
    uint256 balanceContributedInEth = _calculateContribution(
        totalInEth,
        lastTotalSupply,
-       userProposalEth[_proposalId][msg.sender],
-       userProposalFractions[_proposalId][msg.sender]
+       userETHContribution,
+       userFractionsContribution
    );

    // Gets the token and fraction ID of the new vault
    address newVault = migrationInfo[_vault][_proposalId].newVault;
    (address token, uint256 newFractionId) = IVaultRegistry(registry)
        .vaultToToken(newVault);
    // Calculates share amount of fractions for the new vault based on the new total supply
    uint256 newTotalSupply = IVaultRegistry(registry).totalSupply(newVault);
    uint256 shareAmount = (balanceContributedInEth * newTotalSupply) /
        totalInEth;

    // Transfers fractional tokens to caller based on share amount
    IFERC1155(token).safeTransferFrom(
        address(this),
        msg.sender,
        newFractionId,
        shareAmount,
        ""
    );
}

Solution #2 - Keep track of user who has already migrated/withdrew with a state variable

+	mapping (address => bool) hasAlreadyMigrated;

function migrateFractions(address _vault, uint256 _proposalId) external {
+	// Reverts if call has already migrated/withdrawed
+	if (hasAlreadyMigrated[msg.sender]) revert UserAlreadyMigrated();
	
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not successful
    (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    
    ..SNIP..
    
+   hasAlreadyMigrated[msg.sender] = true;
}

Lines of code

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

Vulnerability details

Background

If a vault is created normally via VaultRegistry.create, the owner will be the VaultRegistry address.

If a vault is created via VaultRegistry.createFor, the owner will be the one specified in the owner parameter.

Vulnerability Details

It was observed that the vault owner has the right to execute any function on the vault.

The following code shows that the vault owner is not subjected to any permission access control. Thus, the vault owner could execute a transaction with an arbitrary target and calldata. Since this is a delegatecall, the logic or implementation of the target contract is loaded to the vault contract, and executed as if it were its own code.

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

function execute(
    address _target,
    bytes calldata _data,
    bytes32[] calldata _proof
) external payable returns (bool success, bytes memory response) {
    bytes4 selector;
    assembly {
        selector := calldataload(_data.offset)
    }

    // Generate leaf node by hashing module, target and function selector.
    bytes32 leaf = keccak256(abi.encode(msg.sender, _target, selector));
    // Check that the caller is either a module with permission to call or the owner.
    if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
        if (msg.sender != owner)
            revert NotAuthorized(msg.sender, _target, selector);
    }

    (success, response) = _execute(_target, _data);
}

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

/// @notice Executes plugin transactions through delegatecall
/// @param _target Target address
/// @param _data Transaction data
/// @return success Result status of delegatecall
/// @return response Return data of delegatecall
function _execute(address _target, bytes calldata _data)
    internal
    returns (bool success, bytes memory response)
{
    // Check that the target is a valid contract
    uint256 codeSize;
    assembly {
        codeSize := extcodesize(_target)
    }
    if (codeSize == 0) revert TargetInvalid(_target);
    // Save the owner address in memory to ensure that it cannot be modified during the DELEGATECALL
    address owner_ = owner;
    // Reserve some gas to ensure that the function has enough to finish the execution
    uint256 stipend = gasleft() - MIN_GAS_RESERVE;

    // Delegate call to the target contract
    (success, response) = _target.delegatecall{gas: stipend}(_data);
    if (owner_ != owner) revert OwnerChanged(owner_, owner);

    // Revert if execution was unsuccessful
    if (!success) {
        if (response.length == 0) revert ExecutionReverted();
        _revertedWithReason(response);
    }
}

Impact

A malicious vault owner could steal all the assets within the vault, or destroy the vault by making a delegatecall to a target contract that contains selfdestruct function.

Recommended Mitigation Steps

It is recommended to mitigate the risk by subjecting the vault owner to the built-in permission access control. Vault owners should not be allowed to do whatever they want to the vault.

Consider disallowing the vault owner to execute any function on the vault.

function execute(
    address _target,
    bytes calldata _data,
    bytes32[] calldata _proof
) external payable returns (bool success, bytes memory response) {
    bytes4 selector;
    assembly {
        selector := calldataload(_data.offset)
    }

    // Generate leaf node by hashing module, target and function selector.
    bytes32 leaf = keccak256(abi.encode(msg.sender, _target, selector));
-   // Check that the caller is either a module with permission to call or the owner.
+	// Check that the caller is either a module with permission to call.
    if (!MerkleProof.verify(_proof, merkleRoot, leaf)) {
-        if (msg.sender != owner)
            revert NotAuthorized(msg.sender, _target, selector);
    }

    (success, response) = _execute(_target, _data);
}

If the vault owner wants to make any change to the vault, they must go through the proper procedure and submit a new migration proposal with the necessary changes.

Lines of code

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

Vulnerability details

Proof-of-Concept

The use of .transfer() or .send() to send ether is now considered bad practice as gas costs can change, which would break the code.

The transaction will fail inevitably when:

• Target smart contract does not implement a payable function
• Target smart contract does implement a payable fallback which uses more than 2300 gas unit.
• Target smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

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

function leave(address _vault, uint256 _proposalId) external {
    ..SNIP..
    // Withdraws ether from contract back to caller
    payable(msg.sender).transfer(ethAmount);
}

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

function withdrawContribution(address _vault, uint256 _proposalId)
    external
{
    ..SNIP..
    // Withdraws ether from contract back to caller
    payable(msg.sender).transfer(userEth);
}

Recommended Mitigation Steps

Use call instead of transfer to send ether. And return value must be checked if sending ether is successful or not. make sure to check for reentrancy.

Reference

https://github.com/code-423n4/2022-02-redacted-cartel-findings/issues/51

Lines of code

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

Vulnerability details

Background

The following describes the migration process for a vault with regards to the join and leave functions.

1. Assume that Alice is the proposer.
2. Alice calls Migration.propose to propose a set of modules and plugins to migrate a vault to
3. Other contributors could join a migration proposal by contributing ether and fractional tokens by calling Migration.join.
4. At any point of time before the migration proposal is committed, contributors could leave a migration proposal by withdrawing their contributed assets (ETH or fractional token) by calling Migration.leave.

Vulnerability Details

It was observed that a griefer could deny contributors from leaving a migration proposal, thus preventing them from withdrawing their contributed assets if they change their mind and choose not to support the migration.

The Migration.leave could only be called if the vault state is State.INACTIVE as shown below.

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

/// @notice Leaves a proposed migration with contribution amount
/// @param _vault Address of the vault
/// @param _proposalId ID of the proposal being left
function leave(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not inactive
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);

    ..SNIP..
}

A griefer could call Buyout.start with a dust amount of ethers and fractional tokens to start the auction. The vault state will be set to State.LIVE after the call.

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

/// @notice Starts the auction for a buyout pool
/// @param _vault Address of the vault
function start(address _vault) external payable {
    // Reverts if ether deposit amount is zero
    if (msg.value == 0) revert ZeroDeposit();
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
    // Reverts if auction state is not inactive
    (, , State current, , , ) = this.buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert InvalidState(required, current);

    ..SNIP..

    // Sets info mapping of the vault address to auction struct
    buyoutInfo[_vault] = Auction(
        block.timestamp,
        msg.sender,
        State.LIVE,
        fractionPrice,
        msg.value,
        totalSupply
    );
    ..SNIP..
}

Since the vault state is State.LIVE, anyone who attempts to call Migration.leave function will get a revert.

Proof-of-Concept

1. Alice contributes 100 ETH and 100 fractional tokens to a migration proposal
2. A griefer calls Buyout.start with a dust amount of ethers and fractional tokens to start the auction and the vault state changes to State.LIVE.
3. Immediately after the auction ended, the griefer calls Buyout.start with a dust amount of ethers and fractional tokens to start the auction again to keep the vault state to State.LIVE.
4. Since the vault state is always in State.LIVE, whenever Alice attempts to withdraw her assets by calling Migration.leave, it will revert. Thus, there is no way for Alice to retrieve her contributed assets from the Migration module, and her assets are stuck.

Impact

User's contributed assets are stuck within the Migration contract.

Recommended Mitigation Steps

It is recommended to update the implementation to allow contributors to withdraw their contributed assets regardless of the vault state as long as the proposal that the contributors joined does not kick off the buyout process.

When a migration proposal is committed, the proposal struct's isCommited will be set to true

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

function commit(address _vault, uint256 _proposalId)
    external
    returns (bool started)
{
    ..SNIP..

    // Checks if the current price is greater than target price of the proposal
    if (currentPrice > proposal.targetPrice) {
        // Sets token approval to the buyout contract
        IFERC1155(token).setApprovalFor(address(buyout), id, true);
        // Starts the buyout process
        IBuyout(buyout).start{value: proposal.totalEth}(_vault);
        proposal.isCommited = true;
        started = true;
    }
}

All contributors should be allowed to leave a proposed migration with contribution amount as long as the proposal has not been committed yet regardless of the vault state.

/// @notice Leaves a proposed migration with contribution amount
/// @param _vault Address of the vault
/// @param _proposalId ID of the proposal being left
function leave(address _vault, uint256 _proposalId) external {
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
-    // Reverts if buyout state is not inactive
-    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
-    State required = State.INACTIVE;
-    if (current != required) revert IBuyout.InvalidState(required, current);
    
+    // Gets the migration proposal for the given ID
+    Proposal storage proposal = migrationInfo[_vault][_proposalId];
+
+	 // Reverts if proposal is already committed
+    if (proposal.isCommited) reverts CannotLeaveWhenProposalIsCommitted();
	..SNIP..
}

Lines of code

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

Vulnerability details

Vulnerability Details

A malicious user can always keep the vault in State.LIVE state by starting the auction with dust amount of ether and fractional token, and immediately after the auction ended, the malicious user will start a new auction again. This will effectively prevent genuine users from buying out the NFT or assets in the vault

Per the comment and the source code of Buyout.start, any fraction token owner can start an auction with any amount of ether and fractional tokens. Thus, a user could start the auction with 1 Wei and 1 Fractional token.

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

/// @notice Module contract for vaults to hold buyout pools /// - A fractional owner starts an auction for a vault by depositing any amount of ether and fractional tokens into a pool.

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

function start(address _vault) external payable {
    // Reverts if ether deposit amount is zero
    if (msg.value == 0) revert ZeroDeposit();
    // Reverts if address is not a registered vault
    (address token, uint256 id) = IVaultRegistry(registry).vaultToToken(
        _vault
    );
    if (id == 0) revert NotVault(_vault);
    // Reverts if auction state is not inactive
    (, , State current, , , ) = this.buyoutInfo(_vault);
    State required = State.INACTIVE;
    if (current != required) revert InvalidState(required, current);

    // Gets total supply of fractional tokens for the vault
    uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
    // Gets total balance of fractional tokens owned by caller
    uint256 depositAmount = IERC1155(token).balanceOf(msg.sender, id);

    // Transfers fractional tokens into the buyout pool
    IERC1155(token).safeTransferFrom(
        msg.sender,
        address(this),
        id,
        depositAmount,
        ""
    );

    // Calculates price of buyout and fractions
    // @dev Reverts with division error if called with total supply of tokens
    uint256 buyoutPrice = (msg.value * 100) /
        (100 - ((depositAmount * 100) / totalSupply));
    uint256 fractionPrice = buyoutPrice / totalSupply;

    // Sets info mapping of the vault address to auction struct
    buyoutInfo[_vault] = Auction(
        block.timestamp,
        msg.sender,
        State.LIVE,
        fractionPrice,
        msg.value,
        totalSupply
    );
    ..SNIP..
}

Impact

Denial-of-Service. Genuine users are no longer able to buy out the NFT or assets in the vault. Thus, this affects the key feature of the protocol.

Recommended Mitigation Steps

Consider the following measures to reduce the risk of this attack:

• Implement some sort of algorithm to compute the minimum ethers and fractional tokens that the user needs to provide in order to start the auction. This will prevent a malicious user from starting an auction with a dust amount.
• Only allow the same user account/address to start a new auction once every 3 days
• Force the user who starts the auction to provide a certain amount of assets as a deposit. If the auction is unsuccessful, a penalty will be imposed on the user and the deposit will be slashed.

1. Code Summary

Code Quality and Test Coverage

In summary, the code quality of the Fractional was found to be high. The codes were also found to be well-documented and the team took the efforts to document the NatSpec for all the functions within the contracts. As a result, it is easy for the reader to understand the overall architecture and design of the system. However, some minor errors within the comments were observed. Although it does not cause any technical issues or result in a loss of fund, it is recommended for the team to review them and update them accordingly to ensure that the documentation reflects what the system does accurately.

Further improvement to the code readability can be made by using a modifier, refer to the "Use modifier for better readability and code reuse" below. Another key concern that is the functions within the Supply and Transfer contracts are implemented entirely in assembly. Even though assembly code was used for gas optimization, it reduces the readability (and future updatability) of the code. Consider eliminating all assembly code and re-implement them in Solidity to make the code significantly more clean.

Test coverage was found to be high. All the key features were found to be covered in the test.

2. Key Risks & Improvement Opportunities

Excessive Power Holds By Vault Owner

Fractional allows vault owners to install custom plugins to extend the functionality of the vault during or after deployment. The plugins within the vault could theoretically perform any task such as transferring the asset from the vault to an arbitrary wallet address or minting any amount of new fractional tokens. Therefore, it is critical for the fractional token holders of a vault to be aware of this risk and the token holders must ensure that the vault owner is trustworthy.

Under normal circumstances, the vault owner will be Fractional's VaultRegistry contract, which does not pose much of an issue because VaultRegistry contract is considered a trusted entity within Fractional protocol. However, potential fractional token investors should take note that some vaults can be created via VaultRegistry.createFor, which will transfer the ownership of the vault to an arbitrary address. In such a case, potential investors must ensure that the new vault owner is trustworthy enough not to perform a rug pull or steal the assets in the vault.

Consider documenting this risk if needed so that potential fractional token holders can make an informed decision.

Conflicting Module Might Block Functionality Of Another Module

Both the Buyout and Migration modules depend heavily on the state of the vault (e.g. INACTIVE, LIVE, SUCCESS) to determine if a function can be executed at any point in time. For instance, a buyout can only be started only if the vault state is "INACTIVE", or a migration can only be settled if the vault state is "SUCCESS".

A module changing the vault state might cause unintended behavior in another module. For instance, when a buyer starts an auction within the Buyout module, it will cause the vault state to change to State.LIVE. As a result, it will cause contributors of a proposal within the Migration module to be unable to withdraw their contributed assets from the proposal because the Migration.withdrawContribution function requires the vault state to be State.INACTIVE. Thus, contributor assets are stuck in the Migration contract whenever a buyer starts an auction in the Buyout module.

It is recommended to take extra caution when writing the module to ensure that it does not accidentally block the functionality of another module.

Step In A Process Can Be Bypassed Or Triggered In An Out-of-Order Manner

To ensure that the vault operates in an expected manner, it is important that the contracts prevent users from calling functions in an out-of-order manner or bypassing certain step in a process. It was observed that it is possible for users to call the function in an out-of-order manner or bypass certain step in a process entirely. Following illustrates some of the examples:

• A user can call Migration.settleVault follow by Migration.migrateFractions , thus skipping the Migration.settleFractions
• A contributor should call Migration.leave to leave a proposed migration to get back their asset if the proposal has not been committed yet. However, instead of calling Migration.leave, the contributor can choose to call Migration.withdrawContribution which will succeed without any revert.

Ensure that the sequence in a process (e.g. buyout or migration process) is strictly followed and enforced.

Re-entrancy Risks

The key features of the protocols were found to be following the "Checks Effects Interactions" pattern rigorously, which helps to prevent any possible re-entrancy attack. So far no re-entrancy attack that can lead to loss of asset was observed during the contest. However, further improvements can be made to guard against future re-entrancy attacks in case any attack vector is missed out by C4's wardens during the contest.

A number of key functions within Buyout and Migration modules deal with ERC1155, which contains a hook that will make a callback to the recipient whenever a transfer occurs, thus increasing the risk of a re-entrancy attack. Refer to the "Lack Of Reentrancy Guards" issue for more details.

Thus, it would be prudent to implement additional reentrancy prevention wherever possible by utilizing the nonReentrant modifier from Openzeppelin Library to block possible re-entrancy as a defense-in-depth measure.

Input Validation

Although input validation has been already implemented in the majority of the functions, it can be further strengthened to thwart potential attacks or prevent unexpected behavior in the future. For instance, Vault.transferOwnership does not check if the ownership is being transferred to address(0), which might affect the functionality of the vault.

3. Summary Of Findings

The following is a summary of the low and non-critical findings observed during the contest.

4. Low Risk Issues

4.1 Lack Of Reentrancy Guards

Description

Whenever IERC1155(token).safeTransferFrom is called, the to address can re-enter back to the contracts due to the ERC1155TokenReceiver(to).onERC1155Received(msg.sender, from, id, amount, data) code (hook)

https://github.com/Rari-Capital/solmate/blob/03e425421b24c4f75e4a3209b019b367847b7708/src/tokens/ERC1155.sol#L55

function safeTransferFrom(
    address from,
    address to,
    uint256 id,
    uint256 amount,
    bytes calldata data
) public virtual {
    require(msg.sender == from || isApprovedForAll[from][msg.sender], "NOT_AUTHORIZED");

    balanceOf[from][id] -= amount;
    balanceOf[to][id] += amount;

    emit TransferSingle(msg.sender, from, to, id, amount);

    require(
        to.code.length == 0
            ? to != address(0)
            : ERC1155TokenReceiver(to).onERC1155Received(msg.sender, from, id, amount, data) ==
                ERC1155TokenReceiver.onERC1155Received.selector,
        "UNSAFE_RECIPIENT"
    );
}

The following functions utilise IERC1155(token).safeTransferFrom that allows the caller or proposer to re-enter back to the contracts

• Buyout.buyFractions

• Buyout.end

• Migration.leave

• Migration.withdrawContribution

Recommendation

Apply necessary reentrancy prevention by utilizing the OpenZeppelin's nonReentrant modifier to block possible re-entrancy.

4.2 Migration Sequence Not Enforced

Description

Functions should be called in the following sequence to migrate a vault after a successful buyout.

1. Migration.settleVault - Create new vault

2. Migration.settleFractions - Mint new fractional tokens to new vault

3. Migration.migrateFractions - Give investors the new fractional token

However, a user can call Migration.settleVault follow by Migration.migrateFractions , thus skipping the Migration.settleFractions.

Although it does not result in any loss of asset, allowing users to call the functions pertaining to migration in an out-of-order manner might cause unintended consequence in the future.

Recommendation

After the Migration.settleFractions has been executed, the migrationInfo[_vault][_proposalId].fractionsMigrated will be set to true.

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

function settleFractions(
    address _vault,
    uint256 _proposalId,
    bytes32[] calldata _mintProof
) external {
    ..SNIP..
    migrationInfo[_vault][_proposalId].fractionsMigrated = true;
}

Within the Migration.migrateFractions function, check that migrationInfo[_vault][_proposalId].fractionsMigrated == true to ensure that the Migration.settleFractions has been executed.

function migrateFractions(address _vault, uint256 _proposalId) external {
+	// Fractional tokens must be minted first before migrating
+	require(migrationInfo[_vault][_proposalId].fractionsMigrated, "Fractional token not minted yet");
    // Reverts if address is not a registered vault
    (, uint256 id) = IVaultRegistry(registry).vaultToToken(_vault);
    if (id == 0) revert NotVault(_vault);
    // Reverts if buyout state is not successful
    (, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(
        _vault
    );
    State required = State.SUCCESS;
    if (current != required) revert IBuyout.InvalidState(required, current);
    // Reverts if proposer of buyout is not this contract
    if (proposer != address(this)) revert NotProposalBuyout();

4.3 State Variable Visibility Is Not Set

Description

Visibility is not set for the token state variable.

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/Metadata.sol#L13

/// @title Metadata
/// @author Fractional Art
/// @notice Utility contract for storing metadata of an FERC1155 token
contract Metadata {
    /// @notice Address of FERC1155 token contract
    address immutable token;

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/references/SupplyReference.sol#L12

/// @title Supply
/// @author Fractional Art
/// @notice Reference implementation for the optimized Supply target contract
contract SupplyReference is ISupply {
    /// @notice Address of VaultRegistry contract
    address immutable registry;

Recommendation

It is best practice to set the visibility of state variables explicitly. The default visibility for "token" is internal. Other possible visibility settings are public and private.

4.4 Risk of Plugins

Description

All plugins' functions within the vault can be called by any public user. If the plugins contain any unprotected privileged functions, it can be called by malicious user.

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

/// @dev Callback for handling plugin transactions
/// @param _data Transaction data
/// @return response Return data from executing plugin
// prettier-ignore
fallback(bytes calldata _data) external payable returns (bytes memory response) {
    address plugin = methods[msg.sig]; // @audit-issue what if we have the diff contract with same function name? Collision?
    (,response) = _execute(plugin, _data);
}

Recommendation

Include a warning in the comments or documentation so that the vault owner is aware that any plugin's function added can be called by the public users. Vault owners should ensure that plugin's functions have the necessary access control in place so that only authorised users can trigger the functions.

4.5 Ether Might Stuck In Vault.sol

Description

If a user accidentally sent ether to the Vault contract, the ether will be stuck in the vault with no way to retrieve them.

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

/// @dev Callback for receiving Ether when the calldata is empty
receive() external payable {}

Recommendation

Consider if there is a need for the Vault contract to receive ethers. Otherwise, remove it.

4.6 Ownership May Be Burned

Description

It was observed that the vault owner can transfer the ownership to address(0), which effectively burn the ownership.

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

/// @notice Transfers ownership to given account
/// @param _newOwner Address of new owner
function transferOwnership(address _newOwner) external {
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    owner = _newOwner;
    emit TransferOwnership(msg.sender, _newOwner);
}

Recommendation

It is recommended to implement a validation check to ensure that the ownership is not transferred to address(0).

function transferOwnership(address _newOwner) external {
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
+   require(_newOwner != 0, "Invalid new owner: address(0)");
    owner = _newOwner;
    emit TransferOwnership(msg.sender, _newOwner);
}

4.7 Array Length Not Validated

Description

The Vault.install function did not validate that the length of _selectors and _plugins arrays is the same. If the array length is different, it might cause unexpected behavior.

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

/// @notice Installs plugin by setting function selector to contract address
/// @param _selectors List of function selectors
/// @param _plugins Addresses of plugin contracts
function install(bytes4[] memory _selectors, address[] memory _plugins)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        methods[_selectors[i]] = _plugins[i];
    }
    emit InstallPlugin(_selectors, _plugins);
}

Recommendation

It is recommended to implement validation to ensure that the length of _selectors and _plugins arrays is the same.

function install(bytes4[] memory _selectors, address[] memory _plugins)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
+   require(_selectors.length == _plugins.length, "Length of selectors and plugins is not the same");
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        methods[_selectors[i]] = _plugins[i];
    }
    emit InstallPlugin(_selectors, _plugins);
}

4.8 Consider Two-Phase Ownership Transfer

Description

Owner can calls Vault.transferOwnership function to transfers the ownership to the new address directly. As such, there is a risk that the ownership is transferred to an invalid address, thus causing the contract to be without a owner.

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

/// @notice Transfers ownership to given account
/// @param _newOwner Address of new owner
function transferOwnership(address _newOwner) external {
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    owner = _newOwner;
    emit TransferOwnership(msg.sender, _newOwner);
}

Controller can calls ERC1155.transferController function to transfers the controller role to the new address directly. As such, there is a risk that the ownership is transferred to an invalid address, thus causing the contract to be without a controller.

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

/// @notice Updates the controller address for the FERC1155 token contract
/// @param _newController Address of new controlling entity
function transferController(address _newController)
    external
    onlyController
{
    if (_newController == address(0)) revert ZeroAddress();
    _controller = _newController;
    emit ControllerTransferred(_newController);
}

Recommendation

Consider implementing a two step process where the owner or controller nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of admin to fully succeed. This ensures the nominated EOA account is a valid and active account.

4.9 Migration Proposer Can Hijack Other User's Buyout To Settle A Vault

Description

Migration.settleVault function should only be callable if the buyout initiated by the migration proposal is successful. However, it was observed that it is possible to call Migration.settleVault successfully even though the buyout initiated by the migration proposal has failed.

The following aims to demonstrate the issue:

1. Alice (attacker) creates a migration proposal by calling Migration.propose function. Then, she calls Migration.commit to kick off the buyout process for the migration, and Alice's proposal's isCommited is set to true.
2. Alice's buyout is unsuccessful. At this point in time, note that Alice's proposal's isCommited still remains as true, and the vault state reverts back to State.INACTIVE.
3. In order for the Migration.settleVault function to run successfully, the following three (3) requirements must be met:
   • 1st requirement - Proposal must be committed
   • 2nd requirement - Vault state must be set to status.SUCCESS
   • 3rd requirement - proposal.newVault must not be initialised, which means that new vault has not been deployed yet
4. If Alice attempts to call Migration.settleVault function, it will revert because the vault state is not set to State.SUCCESS due to the failed buyout. In summary, her migration proposal meets all the requirements except for the 2nd requirement.
5. Bob decides to buy out the NFTs in the vault, therefore, he calls the Buyout.start to kick start the auction. After the buyout period (4 days), the vault pool has more than 51% of the total supply, thus the buyout is successful.
6. Bob proceeds to call the Buyout.end to end the auction. Since the buyout is successful, the vault state is set to State.SUCCESS now.
7. Alice decided to hijack Bob's buyout. Therefore, immediately after Bob called the Buyout.end function, Alice calls the Migration.settleVault function.
8. Alice's Migration.settleVault function call will succeed this time because the vault state has been set to status.SUCCESS.

This attack does not lead to loss of asset. Thus, I'm marking this as "Low". Even though the migration proposal has settled the vault successfully, when Alice calls Migration.migrateVaultERC[20|721|1155], it will revert because the Buyout.withdrawERC[20|721|1155] will detect that the caller (Migration module) is not the actual auction winner.

However, Migration.settleVault function could still be called successfully in a situation where it should be failing, thus it is something to be raised.

Recommendation

Ensure that the Migration.settleVault can only be called if the buyout initiated by the migration proposal (within Migration.commit) has succeeded.

5. Non-Critical Issues

5.1 Incorrect Comment

Instance #1 - Buyout

Description

The comment mentioned that if a pool has more than 51% of the total supply after 4 days, the buyout is successful.

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

/// @title Buyout /// @author Fractional Art /// @notice Module contract for vaults to hold buyout pools /// - A fractional owner starts an auction for a vault by depositing any amount of ether and fractional tokens into a pool. /// - During the proposal period (2 days) users can sell their fractional tokens into the pool for ether. /// - During the rejection period (4 days) users can buy fractional tokens from the pool with ether. /// - If a pool has more than 51% of the total supply after 4 days, the buyout is successful and the proposer

However, based on the actual implementation, the buyout will be successful as long as the pool has more than 50% of the total supply.

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

uint256 tokenBalance = IERC1155(token).balanceOf(address(this), id);
// Checks totalSupply of auction pool to determine if buyout is successful or not
if (
    (tokenBalance * 1000) /
        IVaultRegistry(registry).totalSupply(_vault) >
    500
) 

Recommendation

Update the comment to clearly reflect the actual implementation.

/// @title Buyout
/// @author Fractional Art
/// @notice Module contract for vaults to hold buyout pools
/// - A fractional owner starts an auction for a vault by depositing any amount of ether and fractional tokens into a pool.
/// - During the proposal period (2 days) users can sell their fractional tokens into the pool for ether.
/// - During the rejection period (4 days) users can buy fractional tokens from the pool with ether.
-/// - If a pool has more than 51% of the total supply after 4 days, the buyout is successful and the proposer
+/// - If a pool has more than 50% of the total supply after 4 days, the buyout is successful and the proposer

Instance #2 - FERC1155

Description

The comment mentioned that the FERC1155.royaltyInfo function is to set the token royalties. However, the actual implementation is to read the token royalties.

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

/// @notice Sets the token royalties
/// @param _id Token ID royalties are being updated for
/// @param _salePrice Sale price to calculate the royalty for
function royaltyInfo(uint256 _id, uint256 _salePrice)
    external
    view
    returns (address receiver, uint256 royaltyAmount)
{
    receiver = royaltyAddress[_id];
    royaltyAmount = (_salePrice * royaltyPercent[_id]) / 100;
}

Recommendation

Update the comment to clearly reflect the actual implementation.

-/// @notice Sets the token royalties
+/// @notice Reads the token royalties
/// @param _id Token ID royalties are being updated for
/// @param _salePrice Sale price to calculate the royalty for
function royaltyInfo(uint256 _id, uint256 _salePrice)
    external
    view
    returns (address receiver, uint256 royaltyAmount)
{
    receiver = royaltyAddress[_id];
    royaltyAmount = (_salePrice * royaltyPercent[_id]) / 100;
}

5.2 Use Modifier For Better Readability And Code Reuse

Description

To improve readability and code reuse, a onlyOwner modifer can be defined instead of performing a manual conditional check if (owner != msg.sender) revert NotOwner(owner, msg.sender); within the following affected functions:

• Vault.setMerkleRoot
• Vault.transferOwnership
• Vault.uninstall
• Vault.install

Recommendation

It is recommended to define a modifier for access control and use it consistently throughout the codebase.

Following illustrates an example of the changes made to Vault.setMerkleRoot function.

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

modifier modifier onlyOwner { {
  if (owner == msg.sender) {
     _;
  }
}

+ function setMerkleRoot(bytes32 _rootHash) external onlyOwner {
- function setMerkleRoot(bytes32 _rootHash) external {
-    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    merkleRoot = _rootHash;
}

5.3 Assembly Within Supply.sol and Transfer.sol

Description

The following functions were implemented in assembly:

• Supply .mint
• Supply.burn
• Transfer.ERC20Transfer
• Transfer.ERC721TransferFrom
• Transfer.ERC1155TransferFrom
• Transfer.ERC1155BatchTransferFrom

Even though assembly code was used for gas optimization, it reduces the readability (and future updatability) of the code.

Recommendation

Consider eliminating all assembly code and re-implement them in Solidity to make the code significantly more clean.

5.4 Variable Should Be Called isInit Instead Of Nonce

Description

The purpose of the nonce is to ensure that the Vault.init function is only called once. Consider renaming it to isInit for better readability.

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

/// @dev Initializes nonce and proxy owner
function init() external {
    if (nonce != 0) revert Initialized(owner, msg.sender, nonce);
    nonce = 1;
    owner = msg.sender;
    emit TransferOwnership(address(0), msg.sender);
}