Fractional v2 contest - shenwilly's results

Lines of code

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

Vulnerability details

When starting a buyout, it is very easy for sender's assets to get rounded down to zero if totalSupply is larger than 100. This would result in an inaccurate buyout price, leaking value for the sender.

Buyout.sol#L86-88

uint256 buyoutPrice = (msg.value * 100) /
    (100 - ((depositAmount * 100) / totalSupply));

When depositAmount * 100 is lower than totalSupply, the result will be rounded down as if the sender didn't transfer any token. This would result in inaccurate buyoutPrice and respectively, fractionPrice.

Recommended Mitigation Steps

Use a reasonably high number for the scalar:

uint256 buyoutPrice = (msg.value * 10e18) /
    (10e18 - ((depositAmount * 10e18) / totalSupply));

Lines of code

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

Vulnerability details

Starting a buyout via Migration.commit could steal tokens from other pending proposals.

It is possible to have multiple ongoing proposals on the same vault. The issue is that when committing one of the proposal, Buyout will try to transfer all token from the msg.sender, which is Migration.sol. Buyout.sol#L72-82

// 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,
    ""
);

This means that any token contributed to other proposals of the same vault would also be transferred, effectively stealing tokens from other users.

Proof of Concept

• Alice started a migration proposal on vault A, contributing 10 token.
• Bob started a new migration proposal on vault A, contributing 1 token.
• Bob's proposal reached target price. Bob called commit, and all 11 token are sent to Buyout.

Recommended Mitigation Steps

Add _amount parameter to Buyout.start so sender can choose how many tokens to send instead of everything. Implement this new behaviour in Migration.commit.

function start(address _vault, uint256 _depositAmount) external payable {
    ...
    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));
    ...
}

function commit(address _vault, uint256 _proposalId)
    external
    returns (bool started)
{
    ...
    if (currentPrice > proposal.targetPrice) {
        ...
        IBuyout(buyout).start{value: proposal.totalEth}(_vault, proposal.totalFractions);
        ...
    }
}

Lines of code

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

Vulnerability details

An attacker could deploy a malicious FERC1155 to steal all ETH in Buyout.sol.

There are two things that makes this vulnerability possible:

1. VaultRegistry allows anyone to deploy vault with a custom FERC1155. This means anyone can use a token with custom minting behaviour.
2. Buyout.cash doesn't reduce the total ethBalance that could be withdrawn. An attacker that can manipulate token balance can keep calling cash to withdraw funds from the contract.

Specifically, these are the problematic lines: Buyout.sol#L255-270

uint256 tokenBalance = IERC1155(token).balanceOf(msg.sender, id);
if (tokenBalance == 0) revert NoFractions();

// Initializes vault transaction
bytes memory data = abi.encodeCall(
    ISupply.burn,
    (msg.sender, tokenBalance)
);
// Executes burn of fractional tokens from caller
IVault(payable(_vault)).execute(supply, data, _burnProof);

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

Note that even without the custom contract attack, the implementation is still problematic. It would only be correct for the first redeemer, giving later redeemers more fund than they should receive. Assume a 100 supply token divided between Alice and Bob. ethBalance is 100 eth:

• Alice calls cash, burning 50 token and receiving 50 eth. Total supply is now 50.

  uint256 buyoutShare = (tokenBalance * ethBalance) /
          (totalSupply + tokenBalance);
                      = (50 * 100) / (50 + 50);
                      = 50 eth
  

• Bob calls cash, burning 50 token and receiving 100 eth instead of 50 eth.

  uint256 buyoutShare = (tokenBalance * ethBalance) /
          (totalSupply + tokenBalance);
                      = (50 * 100) / (0 + 50);
                      = 100 eth
  

• As there are only 100 eth reserved for this buyout redemption, Bob is effectively stealing an extra 50 eth.

Proof of Concept

• Alice (attacker) deploys a custom FERC1155 with controllable mint/burn capability.
• Alice calls VaultRegistry.createInCollection, using the custom ERC1155 as the token.
• Alice starts a buyout, setting ethBalance of the auction to 10 eth, and end it succesfully.
• Alice burns all supply of the token, and mint herself 1 token. Alice calls cash to redeem 10 eth. Repeat this step until there is no more eth in the contract to steal.

Recommended Mitigation Steps

Reducing the ethBalance at the end of cash would solve the issue:

function cash(address _vault, bytes32[] calldata _burnProof) external {
    ...
    uint256 totalSupply = IVaultRegistry(registry).totalSupply(_vault);
    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#L289-L326

Vulnerability details

A manipulated pending proposal can steal funds in the contract to start a buyout.

The issue here is that there is no check to prevent withdrawContribution from being called when the proposal is still in the contribution phase.

The withdrawContribution function is called to retrieve assets after a proposal has failed to reach target price. Functionality wise, this is similar to the leave function which withdraws assets previously contributed to a proposal. However, it differs in one aspect: withdrawContribution does not update the totalEth and totalFractions of the proposal, while leave does.

A malicious actor could exploit this by contributing to a proposal and subsequently calls withdrawContribution to withdraw their assets without reducing the value of proposal.totalEth. They can then pass the following check to commit the proposal, even though the actual contributed eth doesn't match the proposal.totalEth of the proposal:

Migration.sol#L198-213

uint256 currentPrice = _calculateTotal(
    100,
    IVaultRegistry(registry).totalSupply(_vault),
    proposal.totalEth,
    proposal.totalFractions
);

// Checks if the current price is greater than target price of the proposal
if (currentPrice > proposal.targetPrice) {
    ...
    IBuyout(buyout).start{value: proposal.totalEth}(_vault);
    ...
}

This attack can be executed as long as there is an proposal.totalEth amount of ETH available in the contract.

Proof of Concept

For simplicity, we assume Bob didn't contribute any fraction and total supply of fraction is 100.

• Alice started a migration proposal and contributed 100 eth.
• Bob (attacker) started a new migration proposal with a target price of 99 eth.
• Bob calls join with 100 eth, increasing proposal.totalEth to 100 eth.
• Bob calls withdrawContribution to withdraw his 100 eth. However, proposal.totalEth is still at 100 eth.
• Bob calls commit to start the buyout.

  uint256 currentPrice = _calculateTotal(
      100,
      IVaultRegistry(registry).totalSupply(_vault), / 100
      proposal.totalEth, // 100
      proposal.totalFractions // 0
  );
  
  // given these parameters, currentPrice would return 100.
  

  As currentPrice is 100 and proposal.targetPrice is 99, the currentPrice > proposal.targetPrice condition is satisfied. Alice's 100 eth is then used to call Buyout.start.

Below is the test case to demonstrate the attack:

function testCommitAfterWithdraw() public {
    initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
    (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();

    // Migrate to a vault with no permissions (just to test out migration)
    address[] memory modules = new address[](1);
    modules[0] = address(mockModule);
    // Bob makes the proposal
    bob.migrationModule.propose(
        vault,
        modules,
        nftReceiverPlugins,
        nftReceiverSelectors,
        TOTAL_SUPPLY * 2,
        1 ether
    );
    // Bob joins the proposal
    bob.migrationModule.join{value: 1 ether}(vault, 1, HALF_SUPPLY);

    // Alice made proposal as well
    alice.migrationModule.propose(
        vault,
        modules,
        nftReceiverPlugins,
        nftReceiverSelectors,
        TOTAL_SUPPLY * 2,
        1 ether
    );
    alice.migrationModule.join{value: 1 ether}(vault, 2, HALF_SUPPLY);

    // bob withdraw contribution
    bob.migrationModule.withdrawContribution(vault, 1);

    vm.warp(proposalPeriod + 100);
    // bob is still able to commit the buyout process
    bool started = bob.migrationModule.commit(vault, 1);
    assertTrue(started);
}

Recommended Mitigation Steps

Add a check to prevent withdrawContribution from being called when proposal has not been commited yet:

function withdrawContribution(address _vault, uint256 _proposalId)
    external
{
    if (!migrationInfo[_vault][_proposalId].isCommited) revert ProposalNotCommited();
    ...
}

Lines of code

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

Vulnerability details

Funds in migration proposals could potentially be stuck forever if a buyout auction on the same vault is started by other party.

Most of the functions within Migration.sol can only be executed depending on the state of buyout auction in Buyout.sol. When there is no buyout happening, a migration proposal can be made and anyone can contribute to the proposal. However, it is possible that a buyout auction is started by another party while a pending proposal is not commited yet.

When this scenario happens, there is no action that could be taken to interact with the pending proposal. All funds that have been contributed cannot be withdrawn. This is because the functions only check for the state of the buyout auction, instead of also considering whether the buyout auction's proposer is Migration.sol:

(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);

Proposal contributors have to wait until the buyout failed before they can withdraw their funds. In case the buyout succeeded, their funds will be stuck forever.

Proof of Concept

• Bob made a migration proposal and contributed 0.5 eth.
• Alice individually started a buyout auction. Buyout state is now ACTIVE.
• Bob can't leave the proposal.
• Alice successfully ended the buyout auction. Buyout state is now SUCCESS.
• Bob can't withdraw the funds.

Below are the test cases that show the scenarios described above.

function testLeaveBuyoutStarted() public {
    initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
    (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
    // Migrate to a vault with no permissions (just to test out migration)
    address[] memory modules = new address[](1);
    modules[0] = address(mockModule);
    // Bob makes the proposal
    bob.migrationModule.propose(
        vault,
        modules,
        nftReceiverPlugins,
        nftReceiverSelectors,
        TOTAL_SUPPLY * 2,
        1 ether
    );
    // Bob joins the proposal
    bob.migrationModule.join{value: 0.5 ether}(vault, 1, HALF_SUPPLY);

    // Alice started buyout
    alice.buyoutModule.start{value: 1 ether}(vault);
    (, , State current, , , ) = alice.buyoutModule.buyoutInfo(vault);
    assert(current == State.LIVE);

    vm.expectRevert(
        abi.encodeWithSelector(IBuyout.InvalidState.selector, 0, 1)
    );
    // Bob cannot leave
    bob.migrationModule.leave(vault, 1);
}

function testLeaveBuyoutSuccess() public {
    // Send Bob a smaller amount so Alice can win the auction
    initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY/2, true);
    (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
    // Migrate to a vault with no permissions (just to test out migration)
    address[] memory modules = new address[](1);
    modules[0] = address(mockModule);
    // Bob makes the proposal
    bob.migrationModule.propose(
        vault,
        modules,
        nftReceiverPlugins,
        nftReceiverSelectors,
        TOTAL_SUPPLY * 2,
        1 ether
    );
    // Bob joins the proposal
    bob.migrationModule.join{value: 0.5 ether}(vault, 1, HALF_SUPPLY/2);

    // Alice did a buyout
    alice.buyoutModule.start{value: 1 ether}(vault);
    vm.warp(rejectionPeriod + 1);
    alice.buyoutModule.end(vault, burnProof);

    (, , State current, , , ) = alice.buyoutModule.buyoutInfo(vault);
    assert(current == State.SUCCESS);

    vm.expectRevert(
        abi.encodeWithSelector(IBuyout.InvalidState.selector, 0, 2)
    );
    // Bob cannot leave
    bob.migrationModule.leave(vault, 1);
}

Recommended Mitigation Steps

Modify the checks for the following functions:

• leave
• withdrawContribution

so users can withdraw their funds from the proposal when the buyout auction proposer is not Migration.sol.

In addition, it's also possible that there are multiple ongoing proposals on the same vault and the buyout is started by one of them. To allow other proposals' contributors to withdraw their fund, consider tracking the latest proposalId that started the buyout on a vault:

mapping(address => uint256) public latestCommit;

function commit(address _vault, uint256 _proposalId) {
    ...
    if (currentPrice > proposal.targetPrice) {
        ...
        latestCommit[_vault] = _proposalId;
    }
}

For leave:

(, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);

// if buyout is started by this proposal, check that state is inactive. Else allow leaving.
if (proposer == address(this) && latestCommit[_vault] == _proposalId) {
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);
}

For withdrawContribution:

(, address proposer, State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);

// if buyout is started by this proposal, check that state is inactive. Else allow withdrawing.
if (proposer == address(this) && latestCommit[_vault] == _proposalId) {
    State required = State.INACTIVE;
    if (current != required) revert IBuyout.InvalidState(required, current);
}
if (
    migrationInfo[_vault][_proposalId].newVault != address(0)
) revert NoContributionToWithdraw();

Lines of code

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

Vulnerability details

A malicious actor can grief the Buyout.start function to deny vaults from ever starting a proper buyout.

There is practically no cost to start a buyout auction for a vault. Once a buyout is started, one has to wait until the buyout is ended before they are able to start a new one.

An actor that wants to prevent a vault from getting bought out could frontran a proper start transaction by starting a buyout with unreasonable parameter such as a buyoutPrice of 1 wei. They can keep repeating this forever, preventing a proper buyout auction from happening.

Proof of Concept

• Alice wants to start a buyout auction on vault A.
• Bob doesn't want vault A to be bought out, so he frontran Alice's start transaction with 1 wei as the msg.value, setting the fractionPrice to 0.
• As the fractionPrice is unreasonable, no one wants to sell their fraction and the auction fails. Bob can keep doing this whenever anyone tried to start a buyout on vault A.

Recommended Mitigation Steps

Consider allowing multiple buyout auctions on a same vault to happen at the same time. This could be done in a similar way to how proposal works in Migration.sol.

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.