Fractional v2 contest - PwnedNoMore's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L123-L135 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L299-L305 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L308-L325 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L241

Vulnerability details

[PNM-002] proposal.totalEth and proposal.totalFractions may be increased without cost to a user.

Links

• join changes
• withdrawContribution checks
• withdrawContribution changes
• settleVault change

Description

By calling join and withdrawContributions, a user may inflate the values of the current proposal.totalEth and proposal.totalFractions at no cost at all.

This is because withdrawContributions is guarded by revert conditions that satisfy the contract's states before and after a settleVault call begins, even though it is meant to only be used after a settleVault at the end of a Migration life cycle.

withdrawContributions only contains functions to decrease a user's transacted ETH and ERC1155 tokens, but not any that affect the proposal's totalETH and totalFractions, which can be used to replace the leave function that does remove from totalETH and totalFractions, and is meant to be used alongside join.

PoC / Attack Scenario

Consider malicious user Alice is interacting with an existing vault with Migration as one of the active modules.

Once a propose is called, the State of the contract is INACTIVE, so the functions join and leave may be called.

Alice calls the join function with 5ETH and paramter amount = 5, which:

• increases userProposalEth[vault][Alice] by 5,
• increases proposal.totalEth by 5,
• increases userProposalFractions[vault][Alice by 5, and
• increases proposal.totalFractions by 5.

On the other hand, as long as settleVault is not called, Alice can call withdrawContributions[vault], and this call will not revert, as the only different check that this function has from the join and leave revert checks, is migrationInfo[_vault][_proposalId].newVault != address(0)

This passes, as it is only updated within settleVault, which is only called at the very end of a Migration cycle.

This will return all of Alice's 5 ETH and ERC1155's , but will not decrease proposal.totalEth nor 'proposal.totalFractions'.

Hence, as long as settleVault is not called, Alice can repeatedly call join and withdrawCOntributions again and again for only the fee of gas to inflate proposal.totalEth and/or proposal.totalFractions to however high she wants.

One possible consequence is that the proposal may use other proposals' deposited ETH to start a buyout. Note that all proposals' deposited ETH is locked in this Migration contract. Assuming Alice deposits her ETH and withdraws them by withdrawContributions, the proposal.totalEth will increase to any amount Alice wants. After commit, the contract will use other proposals' ETH to start its buyout, rendering users' funds at high risk.

Another possible consequence is a DoS attack, which may arise from commit computing the fraction price based on both proposal.totalEth and proposal.totalFractions, it may be possible to make commit revert every time it is called by making proposal.totalFractions > totalSupply, which is possible, as proposal.totalFractions is not governed by the number of tokens minted, while totalSupply is. Attackers can exploit this vulnerability to reject any proposal they dislike.

Suggested Fix

Apart from the proposer, all other users may withdraw their contributions with leave, so making withdrawContributions only accessible to the proposer should be able to prevent this exploit from occurring.

Lines of code

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

Vulnerability details

Description

The migrateFractions function is used to migrate the caller's fractions from an old vault to a new one after a successful migration. Normally, a user's funds only needs to be migrated once, and CANNOT be migrated twice or more times.

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

However, as shown above, the migrateFractions function does not validate whether the user/caller has migrated his funds before and sends new fraction tokens to him anyway.

This vulnerability lets a malicious user able to claim new tokens multiple times. And other users cannot claim their legitimate funds at the end (i.e., the Migration contract does not have enought new tokens).

PoC / Attack Scenario

After a successful migration, a malicious user Alice invoke the migrateFractions functions multiple time. She will receive much more new tokens than she should do, while some other users cannot retrieve their new tokens (i.e., Alice steals others' funds).

Suggested Fix

Zero out any user's contribution, i.e., userProposalEth[_proposalId][msg.sender] and userProposalFractions[_proposalId][msg.sender], after he migrates his fraction tokens.

Lines of code

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

Vulnerability details

Description

A proposed migration can be processed if and only if the underlying buyout succeeds. However, the settleVault function does not validate whether the current buyout is indeed the one triggered by the given proposal. This lets a malicious proposal be able to process even though on one voted for it.

Specifically, as the following code shows, the settleVault function retrieves the buyout information by querying the IBuyout(buyout).buyoutInfo(_vault), regardless of who triggered this buyout.

function settleVault(address _vault, uint256 _proposalId) external {
    // Reverts if the migration was not proposed
    Proposal storage proposal = migrationInfo[_vault][_proposalId];
    if (!(proposal.isCommited)) revert NotProposed();
    // Reverts if the migration was unsuccessful
    (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
    if (current != State.SUCCESS) revert UnsuccessfulMigration();
    // Reverts if the new vault has already been deployed
    if (proposal.newVault != address(0))
        revert NewVaultAlreadyDeployed(proposal.newVault);

    // Gets the merkle root for the vault and given proposal ID
    bytes32[] memory merkleTree = generateMerkleTree(proposal.modules);
    bytes32 merkleRoot = getRoot(merkleTree);
    // Deploys a new vault with set permissions and plugins
    address newVault = IVaultRegistry(registry).create(
        merkleRoot,
        proposal.plugins,
        proposal.selectors
    );
    // Sets address of the newly deployed vault
    proposal.newVault = newVault;
    // Emits event for settling the new vault
    emit VaultMigrated(
        _vault,
        newVault,
        _proposalId,
        proposal.modules,
        proposal.plugins,
        proposal.selectors
    );
}

As a result, a malicious proposal can be processed as long as there is a successfuly buyout triggered by any other benign proposal. Even though the malicious one has been expired/rejected, it can be re-active.

PoC / Attack Scenario

• The hacker Alice proposes a malicious migration proposal, namely proposal X, which gives Alice permission to mint and burn fraction tokens
  • As such, if the proposal succeeds, Alice can easily get the locked NFTs by burning out any other user's fraction tokens and minting for herself
• Proposal X will definitely be rejected due to its malicious nature
• Another user Bob (or maybe Alice herself) proposes a benign migration proposal, namely proposal Y.
• Proposal Y gets approved as long as it is attractive enough
• Alice observes this, and she front-runs settleVault on proposal X.
  • Note that the settleVault only checks whether the current buyout for the given vault succeeds or not, the malicious proposal X will be processed
  • Alice then invokes settleFractions, migrateFractions, and all other migration functions to prepare the new vault
  • After settling down all the stuffs, Alice uses the malicious plugins to burn others' fraction tokens and mint tokens for herself.
  • Alice starts a buyout to get the locked NFTs, and no one can reject this buyout since they do not hold any fraction tokens

Suggested Fix

For each buyout, record the proposal id if it is triggered by a migration proposal.

Lines of code

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

Vulnerability details

Description

The end function in the Buyout contract uses IERC1155(token).balanceOf(address(this), id) to determine the amount of deposited fraction tokens without distinguishing whether those fraction tokens are depositied by the sellFractions function or by direct transferring. Note that only the sellFractions function is constrained by PROPOSAL_PERIOD.

This vulnerability lets a 51-holder gain the whole batch of NFTs without paying for the rest 49% fractions.

Assume a vault X creates 100 fraction tokens and the market-decided price of a fraction token is 1 ether (i.e., the ideal value of the locked NFTs in vault X is 100 ether). Let's also assume that Alice holds 51 tokens (maybe by paying 51 ether on opensea).

Followings are two scenarios, where the benign one follows the normal workflow and the malicious one exploits the vulnerability.

Benign Scenario

• Alice starts a buyout by depositing her 51 fraction tokens and 49 ether, making the fractionPrice 1 ether
• Other users are satisfied with the provided price, and hence no one buys or sells their fraction tokens
• The buyout succeeds:
  • Alice gets the locked NFTs
  • Other fraction holders can invoke cash to redeem their fraction tokens with a price of 1 ether
• As a result, Alice paid 100 ether in total to get the locked NFTs.

Malicious Scenario

• Alice starts a buyout by depositing 0 fraction tokens and 1 wei, making the fractionPrice 0.01 wei.
  • Note that Alice can create a separated account whose balance for the fraction token is 0, to start the buyout
• No one is satisfied with the price (0.01 wei v/s 1 ether) and hence they will try to buy fraction tokens to reject the buyout
  • Since there is not any fraction tokens locked in the Buyout contract from Alice, other users do not need to do anything
• Alice invokes the end function
  • But before invoking the end function, Alice directly invokes IERC1155(token).safeTransferFrom to send the rest 51 fraction token to the Buyout contract
  • The end function will treat the buyout successful, since the IERC1155(token).balanceOf(address(this), id) is bigger than 50%
  • The above two message calls happen in a single transaction, hence no one can front-run
• As a result
  • Alice only paid 51 ether to get the locked NFTs whose value is 100 ether
  • Other fraction holders get nothing (but they had paid for the fraction token before)

In short, a malicious users can buy any NFT by just paying half of the NFT's market price

Suggested Fix

For each buyout, add a new field to record the amount of fraction tokens deposited by sellFractions. And in the end function, use the newly-added field to determine whether the buyout can be processed or not.

Lines of code

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

Vulnerability details

Description

Vaults use DELEGATECALL to execute plugins/targets. To ensure the owner cannot be modified during the DELEGATECALL, vaults store the owner address in memory and check it after the DELEGATECALL (as shown below).

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

However, it only guards owner but not nonce. Note that in the init function, the owner is assigned as msg.sender as long as nonce == 0 (as shown below).

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

That is, if the DELEGATEDCALL changes the nonce as 0, a malicious user can invoke init function to become the owner.

In other words, the integrity check of owner is insufficient. The project can either remove the check (to save gas) or add another integrity check of nonce.

Lines of code

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

Vulnerability details

Description

The start function starts a buyout, requiring that there is no any live buyout for the same given vault. However, there is no any protocol fee for starting a buyout, rendering malicious users able to front-run to create a meaningless buyout and reject other users' proposals, with a minimal cost. The situtation would become extreme for those layer-2 chains with low gas fees.

Besides, to start a buyout, the proposal does not need to hold any fraction token, make any user able launch this attack.

PoC / Attack Scenario

• Benign user Bob sends a transaction to start a buyout on vault X.
• The malicious attacker Alice observes this transaction in the mempool and front-runs a new start transaction
  • For this new start, Alice only deposits 1 wei and 0 fraction token
• The Alice's buyout will be live and Bob's reverted
• Bob has to wait for the Alice's proposal being rejected, i.e., 4 days, to re-propose
• The attacks can repeat and Alice only needs to pay for the gas fee.

Suggested Fix

Add some (reasonable) protocol fee for a rejected buyout. This would also encourage users to propose buyouts with a reasonable price.

GAS-001

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

OpenZeppelin 4.7.0 supports a gas-friend verifyCalldata.

Use this one to save gas.

GAS-002

Link:

• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L51
• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L110
• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L64
• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L83
• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L107
• https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L122

Put the length function of an array out of a loop to save gas, especially for those in MerkleBase.sol.

GAS-003

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

    if (
        current != State.INACTIVE ||
        migrationInfo[_vault][_proposalId].newVault != address(0)
    ) revert NoContributionToWithdraw();

The migrationInfo[_vault][_proposalId].newVault != address(0) is redundant.