Fractional v2 contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L34-L51

Vulnerability details

Buyout's start() now determine fractional token price by dividing native tokens amount by total supply number. Whenever the supply is high enough the precision can be lost, leading to severe losses to buyout proposer as his staked fractional tokens can be valued at zero this way.

Setting the severity to be high as this is principal funds stealing impact with the only precondition of total the fractional tokens supply to be high enough, which isn't restricted in the system.

Proof of Concept

There is no limitation on the total supply number:

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/protoforms/BaseVault.sol#L34-L51

    function deployVault(
        uint256 _fractionSupply,
        address[] calldata _modules,
        address[] calldata _plugins,
        bytes4[] calldata _selectors,
        bytes32[] calldata _mintProof
    ) external returns (address vault) {
        bytes32[] memory leafNodes = generateMerkleTree(_modules);
        bytes32 merkleRoot = getRoot(leafNodes);
        vault = IVaultRegistry(registry).create(
            merkleRoot,
            _plugins,
            _selectors
        );
        emit ActiveModules(vault, _modules);

        _mintFractions(vault, msg.sender, _fractionSupply, _mintProof);
    }

Which is reasonable, as the system being base level functionality allows for various granularity of fractional tokens.

In the same time fractionPrice is now determined without precision multiplier, i.e. with straightforward division:

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

uint256 fractionPrice = buyoutPrice / totalSupply;

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

Once total suypply is big enough this can lead to substantial relative losses for Bob the buyout proposer:

1. Bob owns 90% of totalSupply, which is 1e18, and wants to buyout the rest

2. The only NFT Vault holds is not costly, having market value of 0.98 ETH, Bob's share is valued 0.9 * 0.98 = 0.882 ETH, and he needs to supply 0.1 * 0.98 = 0.098 ETH

3. Bob's buyoutPrice will be (0.098e18 * 100) / (100 - ((0.9e18 * 100) / 1e18)) = 0.98 ETH

4. Bob's fractionPrice = buyoutPrice / totalSupply = 0.98e18 / 1e18 = 0

5. As start() completes, Alice can now run buyFractions() to buy Bob's fractional tokens, paying 0 for the whole Bob's 90% share

6. Now Alice can wait for the auction to end, the buyout will not be successful as Alice took out the tokens from Vault balance with buyFractions()

7. Then Alice can initiate another buyout, running start() with 90% of tokens and 0.1 ETH msg.value to avoid the precision pitfall Bob experienced

8. Alice's buyoutPrice will be (0.1e18 * 100) / (100 - ((0.9e18 * 100) / 1e18)) = 1 ETH

9. Alice's fractionPrice = buyoutPrice / totalSupply = 1e18 / 1e18 = 1

This way Alice buyout will either succeed or someone else would buy her fractional tokens at even higher valuation, yielding at least 0.882 ETH profit. Observing such a possibility Alice can now make a bot that immediately back runs start() transaction similar to Bob's, so initiator will not have the chance to correct the mispricing when observing the buyoutPrice realized.

Recommended Mitigation Steps

Consider adding more precision to the fractionPrice formula and its downstream usage, for example:

+       // @notice Fraction multiplier
+       uint256 public constant FRACTION_MULTI = 1e18;
        ...
+       uint256 fractionPrice = buyoutPrice * FRACTION_MULTI / totalSupply;
-       uint256 fractionPrice = buyoutPrice / totalSupply;

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

+       uint256 ethAmount = fractionPrice * _amount / FRACTION_MULTI;
-       uint256 ethAmount = fractionPrice * _amount;

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

+       if (msg.value != fractionPrice * _amount / FRACTION_MULTI) revert InvalidPayment();
-       if (msg.value != fractionPrice * _amount) revert InvalidPayment();

Lines of code

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

Vulnerability details

withdrawContribution() aims to return the funds to Migration participants. However, it uses initial userProposalFractions[_proposalId][msg.sender] and userProposalEth[_proposalId][msg.sender] records for withdrawal accounting. Real funds structure is different after Buyout as it allows for exchange between tokens and ETH. This way the system becomes insolvent for some users, having excess funds that will be frozen.

Setting the severity to be high as that's principal fund freeze impact.

Proof of Concept

Currently withdrawContribution() does not account for the fact that fractional tokens and ETH composition of the contract balance was changed during the Buyout attempt:

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

    /// @notice Retrieves ether and fractions deposited from an unsuccessful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the failed proposal
    function withdrawContribution(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 caller has no fractional balance to withdraw
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (
            current != State.INACTIVE ||
            migrationInfo[_vault][_proposalId].newVault != address(0)
        ) revert NoContributionToWithdraw();

        // Temporarily store user's fractions for the transfer
        uint256 userFractions = userProposalFractions[_proposalId][msg.sender];
        // Updates fractional balance of caller
        userProposalFractions[_proposalId][msg.sender] = 0;
        // Withdraws fractional tokens from contract back to caller
        IFERC1155(token).safeTransferFrom(
            address(this),
            msg.sender,
            id,
            userFractions,
            ""
        );

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

Recommended Mitigation Steps

Consider introduction of new balance calculations: each fractional tokens bought out during the Buyout attempt should be replaced with ETH funds received and vice versa.

Lines of code

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

Vulnerability details

It is possible to withdraw all the assets after Buyout before settleVault() was run and newVault created as asset transfer functions do not check the address.

Proof of Concept

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

    /// @notice Migrates an ERC-20 token to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-20 token
    /// @param _amount Transfer amount
    /// @param _erc20TransferProof Merkle proof for transferring an ERC-20 token
    function migrateVaultERC20(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256 _amount,
        bytes32[] calldata _erc20TransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Withdraws an ERC-20 token from the old vault and transfers to the new vault
        IBuyout(buyout).withdrawERC20(

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

    /// @notice Migrates an ERC-721 token to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-721 token
    /// @param _tokenId ID of the token
    /// @param _erc721TransferProof Merkle proof for transferring an ERC-721 token
    function migrateVaultERC721(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256 _tokenId,
        bytes32[] calldata _erc721TransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Withdraws an ERC-721 token from the old vault and transfers to the new vault
        IBuyout(buyout).withdrawERC721(

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

    /// @notice Migrates an ERC-1155 token to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-1155 token
    /// @param _id ID of the token
    /// @param _amount amount to be transferred
    /// @param _erc1155TransferProof Merkle proof for transferring an ERC-1155 token
    function migrateVaultERC1155(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256 _id,
        uint256 _amount,
        bytes32[] calldata _erc1155TransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Withdraws an ERC-1155 token from the old vault and transfers to the new vault
        IBuyout(buyout).withdrawERC1155(

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

    /// @notice Batch migrates multiple ERC-1155 tokens to the new vault after a successful migration
    /// @param _vault Address of the vault
    /// @param _proposalId ID of the proposal
    /// @param _token Address of the ERC-1155 token
    /// @param _ids IDs of each token type
    /// @param _amounts Transfer amounts per token type
    /// @param _erc1155BatchTransferProof Merkle proof for batch transferring multiple ERC-1155 tokens
    function batchMigrateVaultERC1155(
        address _vault,
        uint256 _proposalId,
        address _token,
        uint256[] calldata _ids,
        uint256[] calldata _amounts,
        bytes32[] calldata _erc1155BatchTransferProof
    ) external {
        address newVault = migrationInfo[_vault][_proposalId].newVault;
        // Batch withdraws multiple ERC-1155 tokens from the old vault and transfers to the new vault
        IBuyout(buyout).batchWithdrawERC1155(

Recommended Mitigation Steps

Consider checking for the newVault to be set, i.e. add non-zero check.

Lines of code

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

Vulnerability details

As new total supply can be arbitrary, setting it significantly lower than current (say to 100 when it was 1e9 before) can be used to remove current minority shareholders, whose shares will end up being zero on a precision loss due to low new total supply value. This can go unnoticed as the effect is implementation based.

During Buyout the remaining shareholders are left with ETH funds based valuation and can sell the shares, but the minority shareholders that did contributed to the Migration, that could have other details favourable to them, may not realize that new shares will be calculated with the numerical truncation as a result of the new total supply introduction.

Setting the severity to medium as this is a fund loss impact conditional on a user not understanding the particulars of the implementation.

Proof of Concept

Currently migrateFractions() calculates new shares to be transferred for a user as a fraction of her contribution:

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

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

If Bob the msg.sender is a minority shareholder who contributed to Migration with say some technical enhancements of the Vault, not paying attention to the total supply reduction, his share can be lost on commit():

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

            // Starts the buyout process
            IBuyout(buyout).start{value: proposal.totalEth}(_vault);

As commit() starts the Buyout, Bob will not be able to withdraw as both leave() and withdrawContribution() require INACTIVE state:

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

        State required = State.INACTIVE;
        if (current != required) revert IBuyout.InvalidState(required, current);

If Buyout be successful, Bob's share can be calculated as zero given his small initial share and reduction in the Vault total shares.

For example, if Bob's share together with the ETH funds he provided to Migration were cumulatively less than 1%, and new total supply is 100, he will lose all his contribution on commit() as migrateFractions() will send him nothing.

Recommended Mitigation Steps

Consider requiring that the new total supply should be greater than the old one:

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

        proposal.oldFractionSupply = IVaultRegistry(registry).totalSupply(
            _vault
        );
        proposal.newFractionSupply = _newFractionSupply;
+       require(proposal.newFractionSupply > proposal.oldFractionSupply, ""); // reference version

Lines of code

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

Vulnerability details

Migration's leave() and withdrawContribution() transfer out native tokens via payable(to).transfer call. This is unsafe as transfer has hard coded gas budget and can fail when msg.sender is a smart contract. Such transactions will fail for smart contract users which don't fit to 2300 gas stipend transfer have.

Setting severity to be high as accounting is msg.sender based and the users will not be able to workaround the limitation, so their funds will be frozen on unsuccessful Migration proposal. As the affected users are smart contracts it is a programmatic usage failure, i.e. denial of service with funds freeze for downstream systems.

Proof of Concept

Both functions are used to remove the contribution of an arbitrary shareholder before Buyout or after it was denied.

Native funds are attempted to be transferred with the payable.transfer call:

leave():

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

        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(ethAmount);
    }

withdrawContribution():

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

        // Withdraws ether from contract back to caller
        payable(msg.sender).transfer(userEth);
    }

References

The issues with transfer() are outlined here:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

Generally replacing the transfer() should be coupled with the introduction of non-reentrancy feature, but here the transfer calls happen after all the accounting updates, so reentrancy isn't an issue.

This way the recommendation is to replace the call with SafeSend's _sendEthOrWeth() that is used in other parts of the system.

An alternative is using the OpenZeppelin Address.sendValue:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Migration's leave() and withdrawContribution() are effectively the same for end user, but withdrawContribution() performs excessive check that can be safely removed.

Proof of Concept

newVault can be only set with Buyout in State.SUCCESS:

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

    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;

Buyout's end() requires State.LIVE and set either SUCCESS or INACTIVE:

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

        State required = State.LIVE;
        if (current != required) revert InvalidState(required, current);

State.SUCCESS cannot be reset to State.INACTIVE.

This way there cannot be a situation when current == State.INACTIVE and newVault != address(0), as the latter requires State.SUCCESS, which cannot be transformed to the former.

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

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

        // Reverts if caller has no fractional balance to withdraw
        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        if (
            current != State.INACTIVE ||
            migrationInfo[_vault][_proposalId].newVault != address(0)
        ) revert NoContributionToWithdraw();

If it wasn't, i.e. if there be such a situation that newVault != address(0) actually needed, it should be placed to leave() as well, as leave() and withdrawContribution() are effectively the same for end user, which can now avoid the check by calling leave():

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

        (, , State current, , , ) = IBuyout(buyout).buyoutInfo(_vault);
        State required = State.INACTIVE;
        if (current != required) revert IBuyout.InvalidState(required, current);

Recommended Mitigation Steps

Remove migrationInfo[_vault][_proposalId].newVault != address(0) to save gas.