QA Report

[code423n4]

Low Risk Vulnerabilities

1. settleVault and settleFractions can be called even if proposal failed

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

It is possible to call settleVault and settleFractions even if the proposal failed to pass the priceTarget, as long as there is another party that succesfully did a buyout on the same vault.

While there is no damage from calling these functions, it would lead to incorrect state and misleading event emissions.

Proof of Concept

• Alice started a migration proposal and failed.
• Bob started a buyout on the same vault and succeeded.
• Alice can now call settleVault on the failed proposal to create a new vault, and settleFractions to mint new tokens and mark the vault as migrated.

Recommended Mitigation Steps

Add a check in settleVault and settleFractions to ensure that Migrator is the proposer of the successful buyout:

(, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
if (address(this) != proposer) revert UnsuccessfulMigration();

2. Function selectors might clash during install

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

When installing new plugins in Vault, there is no check to ensure that new selectors did not clash with previously installed selectors. Unaware users might inadvertently replaced it and misused the function later.

Recommended Mitigation Steps

Consider adding toggleable check in install to prevent accidental replacement of clashing selectors:

function install(bytes4[] memory _selectors, address[] memory _plugins, bool _force)
    external
{
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        if (!_force) {
            if (methods[_selectors[i]] != address(0)) revert SelectorAlreadyInstalled();
        }
        methods[_selectors[i]] = _plugins[i];
    }
    emit InstallPlugin(_selectors, _plugins);
}

3. Two-step ownership change

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

It is best practice to use a two-step pattern when updating critical variables such as contract ownership. An input mistake could cause vault ownership to be lost.

Recommended Mitigation Steps

Implement two-step pattern when transferring vault ownership:

• transferOwnership to set a pending new owner.
• acceptOwnership to be called by the new owner to assume ownership.

4. FERC1155 royalty sanity check

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

There is no sanity check when setting royalties. It is possible for a compromised or malicious controller to frontran a trade transaction by setting royalties to an unexpectedly high value, effectively stealing funds intended for the seller.

Recommended Mitigation Steps

Add a sanity check to ensure royalties cannot be set to an unreasonable amount:

function setRoyalties(
    uint256 _id,
    address _receiver,
    uint256 _percentage
) external onlyController {
    if (_percentage > 30) revert RoyaltyTooHigh();
    royaltyAddress[_id] = _receiver;
    royaltyPercent[_id] = _percentage;
    emit SetRoyalty(_receiver, _id, _percentage);
}

Non-Critical Risk Vulnerabilities

1. Use inclusive operator when checking targetPrice

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

When committing a migration proposal, the code checks that currentPrice must be higher than proposal.targetPrice.
Using an inclusive operator >= might be more intuitive here.

function commit(address _vault, uint256 _proposalId) {
    ...
    if (currentPrice > proposal.targetPrice) {
        ...
    }
    ...
}

2. Typo

IMigration.sol#L25

// Boolean status to check if the propoal is active

propoal should be proposal.