Fractional v2 contest - horsefacts's results

Lines of code

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

Vulnerability details

If a user starts a buyout auction with a very small initial deposit, like 1 wei, the fractionPrice on L#88 of Buyout may round down to zero.

Buyout#start

        // 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));
        uint256 fractionPrice = buyoutPrice / totalSupply;

Impact: If fractionPrice is zero, fractions may be accidentally sold for 0 ETH, and these accidentally sold fractions may be purchased by an attacker for 0 ETH.

This is a weird attack that would require tricking users into selling for 0 ETH before buying back their fractions, but it's probably a good idea to validate that fractionPrice is nonzero.

Suggestion: Add a new error and revert if fractionPrice is zero:

        // 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));
        uint256 fractionPrice = buyoutPrice / totalSupply;
        if(fractionPrice == 0) revert ZeroFractionPrice();

Test case:

    function testSellFractionsZeroPrice() public {
        initializeBuyout(alice, bob, TOTAL_SUPPLY, HALF_SUPPLY, true);

        bob.buyoutModule.start{value: 1}(vault);

        assertEq(getFractionBalance(alice.addr), HALF_SUPPLY);
        assertEq(getFractionBalance(buyout), HALF_SUPPLY);
        assertEq(getETHBalance(alice.addr), INITIAL_BALANCE);
        assertEq(getETHBalance(buyout), 1);

        // Alice sells her Fractions for 0 ETH
        alice.buyoutModule.sellFractions(vault, 1000);

        assertEq(getFractionBalance(alice.addr), 4000);
        assertEq(getFractionBalance(buyout), 6000);
        assertEq(getETHBalance(alice.addr), INITIAL_BALANCE);
        assertEq(getETHBalance(buyout), 1);

        (,,,uint256 fractionPrice,,) = bob.buyoutModule.buyoutInfo(address(vault));
        assertEq(fractionPrice, 0);

        // Bob buys all Fractions for 0 ETH
        bob.buyoutModule.buyFractions{value: 0}(vault, 6000);
    }

Lines of code

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

Vulnerability details

Migration#withdrawContribution and Migration#leave both use payable(address).transfer to send native ETH. It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed gas stipend that may be insufficient for smart contract recipients, and could potentially revert in the future if gas costs change. (See the Consensys Diligence article here).

Migration#withdrawContribution:

    function withdrawContribution(address _vault, uint256 _proposalId)
        external
    {
        // Body omitted...

        // Temporarily store user's eth for the transfer
        uint256 userEth = userProposalEth[_proposalId][msg.sender];
        // Udpates ether balance of caller
        userProposalEth[_proposalId][msg.sender] = 0;
        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(userEth);
    }

Migration#leave

function leave(address _vault, uint256 _proposalId) external {
        // Reverts if address is not a registered vault
        (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);

        // Gets the migration proposal for the given ID
        Proposal storage proposal = migrationInfo[_vault][_proposalId];
        // Updates fraction balances of the proposal and caller
        uint256 amount = userProposalFractions[_proposalId][msg.sender];
        proposal.totalFractions -= amount;
        userProposalFractions[_proposalId][msg.sender] = 0;
        // Updates ether balances of the proposal and caller
        uint256 ethAmount = userProposalEth[_proposalId][msg.sender];
        proposal.totalEth -= ethAmount;
        userProposalEth[_proposalId][msg.sender] = 0;

        // Withdraws fractions from contract back to caller
        IFERC1155(token).safeTransferFrom(
            address(this),
            msg.sender,
            id,
            amount,
            ""
        );
        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(ethAmount);
    }

Impact: Currently, some users and contracts may be unable to withdraw ETH contributions from migration modules. If gas costs increase in the future, all users may be impacted.

Recommendation: Consider using payable(address).call, OpenZeppelin Address.sendValue, or your own SafeSend helper, but note that any use of call() introduces an opportunity for re-entrancy. Since these transfers are the final interaction in the functions above, they are OK, but make sure you consider re-entrancy if you use call() in any other context.

Low

Prefer two step ownership transfers

Vault owners may transfer ownership of a vault contract in a single step by calling transferOwnership:

Vault#transferOwnership

    function transferOwnership(address _newOwner) external {
        if (owner != msg.sender) revert NotOwner(owner, msg.sender);
        owner = _newOwner;
        emit TransferOwnership(msg.sender, _newOwner);
    }

Similarly, the FERC1155 controller may be transferred in a single step:

FERC1155#transferController

    function transferController(address _newController)
        external
        onlyController
    {
        if (_newController == address(0)) revert ZeroAddress();
        _controller = _newController;
        emit ControllerTransferred(_newController);
    }

If the owner/controller is accidentally transferred to an incorrect address, ownership of these contracts may be permanently lost.

Consider introducing a mechanism for two-step ownership transfers.

Missing array length validations

Vault#install assumes that the _selectors and _plugins arrays are of equal length, but does not validate that they are equal.

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

Unlike batch token transfers, there is a more limited impact here—in the worst case, the user can call this function again to add the accidentally omitted plugin.

Gas reserve cannot be increased

Vaults define a MIN_GAS_RESERVE constant used to calculate the gas stipend used in Vault#_execute. If future gas costs increase, this stipend may be insufficient. Consider allowing the contract owner to increase the value of this parameter.

Informational

Solidity optimizer bugs in versions 0.8.13 and 0.8.14

Solidity versions 0.8.13 and 0.8.14 are vulnerable to an optimizer bug related to inline assembly. Solidity 0.8.15 has been released with a fix.

This bug only occurs under very specific conditions: the legacy optimizer must be enabled rather than the IR pipeline (true for this project's current project configuration), and the affected assembly blocks must not refer to any local Solidity variables. Inline assembly used here, in Solmate, and in OpenZeppelin does not appear vulnerable. However, it's worth being aware of this vulnerability. Consider upgrading to Solidity 0.8.15.

QA

Your project README is a great high level intro to the core concepts in Fractional V2, but I'd encourage you to expand on this documentation. Including some worked examples of how components are intended to fit together would be really helpful, especially since Fractional intends to be an open, extensible hyperstructure.

I think it would be especially helpful to clarify how protocol components fit together in two broad scenarios: 1) "normal" users creating new vaults through the Fractional UI and 2) "power" users configuring vaults at a low level and creating new modules to extend the protocol.

For example, I was quite surprised to see that a vault owner can execute arbitrary calls that might bypass modules until I understood that vaults are not usually configured with EOA owners. This wasn't clear from reading the docs and my mental model of the protocol was wrong.

There are some very cool patterns in place in this codebase that others may want to adopt or build on!

Use address.code.length

Vault#_execute uses inline assembly to check that the target is a valid contract:

        // Check that the target is a valid contract
        uint256 codeSize;
        assembly {
            codeSize := extcodesize(_target)
        }
        if (codeSize == 0) revert TargetInvalid(_target);

In Solidity version 0.8.1 and later, address.code.length can be used to access code size. this is equivalent to the assembly version above, but more concise.

Suggestion:

        if (_target.code.length == 0) revert TargetInvalid(_target);

Missing events

A number of state changing functions do not emit corresponding events. Consider emitting events from these functions.

• Migration#propose
• Migration#join
• Migration#leave
• Migration#commit
• Migration#withdrawContribution
• FERC1155#royaltyInfo

Misnamed constants

There are a handful of typos, misnamings, and inconsistencies in the constants used for functions implemented in assembly.

Transfer#ERC1155_SAFE_TRANSFER_FROM_signature has a trailing lowercase word, unlike other constants.

uint256 constant ERC1155_SAFE_TRANSFER_FROM_signature = (
    0xf242432a00000000000000000000000000000000000000000000000000000000
);

The Transfer#TOKEN_TRANSFER_GENERTIC_ constants should be TOKEN_TRANSFER_GENERIC.

uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_SIGNATURE = (
    0xf486bc8700000000000000000000000000000000000000000000000000000000
);
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_SIG_PTR = 0x00;
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_TOKEN_PTR = 0x04;
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_FROM_PTR = 0x24;
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_TO_PTR = 0x44;
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_ID_PTR = 0x64;
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_AMOUNT_PTR = 0x84;
uint256 constant TOKEN_TRANSFER_GENERTIC_FAILURE_ERROR_LENGTH = 0xa4; // 4 + 32 * 5 == 164

Finally, Memory#EIGHT_BYTES is actually FOUR_BYTES.

uint256 constant EIGHT_BYTES = 0x04;

This is pretty confusing when used in Transfer#ERC1155BatchTransferFrom, since the implementation is correct, but the offsets appear to be wrong:

    function ERC1155BatchTransferFrom(
        address, /*_token*/
        address, /*_from*/
        address, /*_to*/
        uint256[] calldata, /*_ids*/
        uint256[] calldata /*_amounts*/
    ) external {
        // Utilize assembly to perform an optimized ERC1155 batch transfer.
        assembly {
            // Write the function selector
            // safeBatchTransferFrom(address,address,uint256[],uint256[],bytes)
            mstore(
                ERC1155_BATCH_TRANSFER_FROM_OFFSET,
                ERC1155_SAFE_BATCH_TRANSFER_FROM_SIGNATURE
            )

            // Retrieve the token from calldata.
            let token := calldataload(EIGHT_BYTES)

            // If the token has no code, revert.
            if iszero(extcodesize(token)) {
                mstore(NO_CONTRACT_ERROR_SIG_PTR, NO_CONTRACT_ERROR_SIGNATURE)
                mstore(NO_CONTRACT_ERROR_TOKEN_PTR, token)
                revert(NO_CONTRACT_ERROR_SIG_PTR, NO_CONTRACT_ERROR_LENGTH)
            }

            // Get the total number of supplied ids.
            let idsLength := calldataload(
                add(EIGHT_BYTES, ERC1155_BATCH_TRANSFER_IDS_LENGTH_OFFSET)
            )


            // Rest of function omitted here...
    }

Incorrect comments

The transfer in Buyout.sol#withdrawERC20 is an ERC20 transfer, not an ERC721:

        // Executes transfer of ERC721 token to caller
        IVault(payable(_vault)).execute(transfer, data, _erc20TransferProof);

Unused imports

• IERC20 in Migration.sol

Apply updates from upstream Murky library

The upstream Murky library has added a few gas optimization changes you may want to implement yourself. Two are minor optimizations, one is more significant.

Caching _proof.length in MerkleBase#verifyProof:

    function verifyProof(
        bytes32 _root,
        bytes32[] memory _proof,
        bytes32 _valueToProve
    ) public pure returns (bool) {
        // proof length must be less than max array size
        bytes32 rollingHash = _valueToProve;
        unchecked {
            for (uint256 i = 0; i < _proof.length; ++i) {
                rollingHash = hashLeafPairs(rollingHash, _proof[i]);
            }
        }
        return _root == rollingHash;
    }

Suggestion (link to Murky):

    function verifyProof(
        bytes32 _root,
        bytes32[] memory _proof,
        bytes32 _valueToProve
    ) public pure returns (bool) {
        // proof length must be less than max array size
        bytes32 rollingHash = _valueToProve;
        uint256 length = _proof.length;
        unchecked {
            for (uint256 i = 0; i < length; ++i) {
                rollingHash = hashLeafPairs(rollingHash, _proof[i]);
            }
        }
        return _root == rollingHash;
    }

Using a bitwise and in place of the modulo operator in MerkleBase#getProof:

        while (_data.length > 1) {
            unchecked {
                if (_node % 2 == 1) {
                    result[pos] = _data[_node - 1];
                } else if (_node + 1 == _data.length) {
                    result[pos] = bytes32(0);
                    ++counter;
                } else {
                    result[pos] = _data[_node + 1];
                }
                ++pos;
                _node = _node / 2;
            }
            _data = hashLevel(_data);
        }

Suggestion (link to Murky):

        while (_data.length > 1) {
            unchecked {
                if (_node & 0x1 == 1) {
                    result[pos] = _data[_node - 1];
                } else if (_node + 1 == _data.length) {
                    result[pos] = bytes32(0);
                    ++counter;
                } else {
                    result[pos] = _data[_node + 1];
                }
                ++pos;
                _node = _node / 2;
            }
            _data = hashLevel(_data);
        }

Replacing log2ceil_naive with a more efficient log2ceilBitMagic function:

MerkleBase#getProof

        // The size of the proof is equal to the ceiling of log2(numLeaves)
        uint256 size = log2ceil_naive(_data.length);

Suggestion:

        // The size of the proof is equal to the ceiling of log2(numLeaves)
        uint256 size = log2ceilBitMagic(_data.length);

Link to change in upstream Murky.

Consider reviewing these recent changes to Murky and upgrading your forked implementation.