Fractional v2 contest - sseefried's results

Lines of code

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

Vulnerability details

Note: This submission contains a link to a private fork of the contest repo. Please ask warden @sseefried for GitHub collaborator access.

Impact

A user can start a perpetual buyout that cannot be stopped except by making the buyout succeed. This can be done by creating a malicious contract that will call back to start when it receives ETH via its receive function. The user then starts the perpetual buyout by calling start from the malicious contract.

Assume the rejection period has passed and the auction pool is not large enough (i.e. < 50%). If end is called then the method _sendEthOrWeth will attempt to send ETH to the malicious contract. The contract will simply call back to start sending the ETH it has just received.

The impact is that end can never be called on this buyout proposal if the buyout auction has failed. Worse, no new buyout proposal can be made since the current one is still live, and it is never in a state where it is not live.

The others users will either need to accept that assets are locked inside the vault, or that they will need to sellFractions in order to make the buyout succeed.

Proof of Concept

• Each vault can only have one buyoutInfo associated with it as can be seen on line 39.
• A new buyout proposal cannot be made unless the buyoutInfo state is State.INACTIVE as can be seen in lines 66-68
• A proposer makes a proposal by calling start. They do this from a smart contract that simply calls start again when its receive function is called.
• If the proposer fails to get over 50% then, when end is called, _sendEthOrWeth is called using the proposer value which is the smart contract that re-enters. See line 235. _sendETHOrWeth is cleverly written so that if receive were to revert the reversion would not "bubble up". However, it does not protect against re-entrancy.
• This means that buyoutInfo[vault] can never be overwritten. It is permanently stuck in state State.LIVE meaning that start can never be called for vault by anyone else.
• The only way out of this conundrum is for the other users of the vault to sellFractions to make the auction succeed or to accept that assets are locked in the vault forever.

A foundry test exhibiting this attack has been written in a private fork of the contest repo.

Note that onERC1155Received needs to be implemented in the malicious contract.

Tools Used

Manual inspection + foundry

Recommended Mitigation Steps

Prevent re-entrancy in the start function by using the nonReentrant modifier provided by OpenZeppelin's ReentrancyGuard contract, or use an equivalent custom solution.

$ forge test -m testPerpetualBuyoutBug

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L126 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

Note: This submission contains a link to a private fork of the contest repo. Please ask warden @sseefried for GitHub collaborator access.

Impact

In the event that a malicious target is proposed and migrated to, there is code on line 127 and line 132 of the Vault contract that checks whether the owner has been changed by target of delegatecall.

While this offers some protection this check is not sufficient. A malicious target (which should normally be stateless) could have 3 or more storage slots. If so, it can update the nonce to 0 via a storage slot collision via delegatecall which will then allow init to be called on the vault, thus updating the owner to the message sender.

Proof of Concept

• Assume that a malicious target has been proposed and migrated to. Also assume that it has at least 3 storage slots, and that a function f exists that updates the storage slot 3 with a user specified value.
• Malicious user now calls a module function which indirectly calls f in the malicious target with the value 0.
• A storage slot collision occurs and the nonce field of the vault is set to 0.
• The malicious user can call init() on the vault and transfer ownership. This is because line 25 will not revert when nonce == 0

A foundry test has been written that exhibits this attack.

Tools used

Manual inspection + foundry

Recommended Mitigation Steps

Consider protecting all the important state variables in the Vault contract, not just owner, by saving them before the call to delegatecall and reverting if any of them have changed after the call.

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

Impact

The .transfer method is used to transfer ETH in both the withdrawContribution and leave functions of the Migration. This is not recommended because if the recipient is a contract then it will call the fallback or receive function but it is only given 2300 gas. This can cause the call to revert, thus locking the user's funds in the Migration contract for when the recipient contract

• does not implement a payable function.
• does implement a payable fallback/receive function but it uses more than 2300 gas.
• does implement a payable fallback/receive function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Proof of Concept

• User contributes ETH to the Migration contract by calling the join function. They do this from a smart contract with a payable receive (or fallback) function that uses more than 2300 gas.
• Later they call leave to leave the proposal and get their ETH back. The call reverts since .transfer only provides 2300 gas to the call to receive

A similar PoC is possible for withdrawContribution.

Tools Used

Manual inspection

Recommended Mitigation Steps

Replace payable(msg.sender).transfer(ethAmount) with payable(msg.sender).call{value: ethAmount}("") in the relevant functions.

Lines of code

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

Vulnerability details

Note: This submission contains a link to a private fork of the contest repo. Please ask warden @sseefried for GitHub collaborator access.

Impact

A serious proposal will usually require substantial amounts of ETH and fractions to be added via calls to join. However, a malicious user can always submit a proposal with a low targetPrice call join with more ETH than the target price and then quickly call commit. This will have the effect of starting a 4 day buyout on the vault. Since there can only be one buyout per vault at any one time, these "indecent proposals" can DOS serious ones indefinitely. On each occasion 4 days are lost to the buyout process.

Proof of Concept

• Bob makes a serious proposal by calling propose for a target price of 1ETH
• Alice makes an "indecent" proposal for a target price of 1 wei
• Alice quickly calls join with a value of 2 wei
• Alice then calls commit successfully committing and starting the buyout process.
• Later, once Bob has amassed enough support he tries to call commit but the function reverts with InvalidState(0, 1) because line 210 is called but a buyout already exists
• Now everyone must wait until the buyout process is over for another migration proposal to be made.

A foundry test exhibiting this bug has been written in a private fork of the contest repo.

Tools Used

Manual inspection + foundry

Recommended Mitigation Steps

The logic of _calculateTotal needs to be changed so that a user cannot easily propose a migration, then join and commit it if they do not have not submitted a substantial amount of the fractions of the total supply.

$ forge test -m testGriefCommit

QA Report

Documentation errors.

Some of the documentation mentions that Buyouts need 51% support but the code shows that, in reality, it is anything strictly greater than 50%. e.g. 50.0001%

See Buyout.sol:208-211

Non-critical: Use permissions.size to intialize nodes in Buyout.getLeafNodes

Instead of using the magic number 5 on line 451 why not simply use permissions.length to allocate the correct number of array indices?

Like so:

function getLeafNodes() external view returns (bytes32[] memory nodes) {
    Permission[] memory permissions = getPermissions();
    nodes = new bytes32[](permissions.length);
    for (uint256 i; i < permissions.length; ) {
        // Hashes permission into leaf node
        nodes[i] = keccak256(abi.encode(permissions[i]));
        // Can't overflow since loop is a fixed size
        unchecked {
            ++i;
        }
    }
}

Low Risk: BaseVault.generateMerkleTree will not work with new targets when total leaf length is greater than 6

Impact

Function BaseVault.deployVault takes a modules parameter which allows for any set of modules to be used with the contract.

If those modules have targets which have a total number of leaf nodes greater than 6 then generateMerkleTree will revert, which in turn causes deployVault to revert.

The impact is that one can successfully create a BaseVault but not call deployVault.

Proof of Concept

• Create a new instance of BaseVault called baseVault.
• Call baseVault.deployVault with modules parameter which, collectively, have more than 6 leaf nodes.
• Observe a revert with "Index out of bounds"

Tools used

Manual Inspection

Recommended Mitigation Steps

If the intention of BaseVault is to allow arbitrary modules then one could rewrite generateMerkleTree as follows.

function generateMerkleTree(address[] calldata _modules)
    public
    view
    returns (bytes32[] memory hashes)
{
    uint256 numLeaves;
    uint256 counter;
    bytes32[][] memory leavesList = new bytes32[][](_modules.length);

    // Get leaf nodes

    unchecked {
        for (uint256 i; i < _modules.length; ++i) {
            bytes32[] memory leaves = IModule(_modules[i]).getLeafNodes();
            leavesList[i] = leaves;
            numLeaves += leaves.length;
        }
    }

    hashes = new bytes32[](numLeaves);
    unchecked {
        for (uint256 i; i < leavesList.length; ++i) {
            bytes32[] memory leaves = leavesList[i];
            for (uint256 j; j < leaves.length; ++j) {
                hashes[counter++] = leaves[j];
            }
        }
    }
}

If the intention is for it only to allow Supply and Buyout modules then simply add some checks with require statements.

Low Risk: There is no way to get ETH out of Vault when accidentally sent to it

There is no function to retrieve ETH accidentally sent to the Vault contract.

Low Risk: BaseVault batch deposit functions do not check arrays are of the same length

The functions batchDepositERC20, batchDepositERC721, and batchDepositERC1155 all fail to check that the length of the their array arguments have the same length.

Although this only results in a revert, it will result in increased gas use.