Fractional v2 contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

These three transfer functions allow an attacker to supply an arbitrary from and to to transfer ERC20s, ERC721s, and ERC1155s. The moment that a user sets approval for the contract to spend their tokens, an attacker can front-run the next call and steal the tokens for themselves.

Proof of Concept

This function allows anyone to transfer anyone's ERC20s given that the owner has already provided approval to the contract.

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

The same is true for the ERC721 and ERC1155 deposit functions.

Tools Used

Manual review.

Recommended Mitigation Steps

Do not allow users to supply arbitrary to and from parameters to the token transfer functions.

Lines of code

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

Vulnerability details

Impact

All migration actions such as propose, join, leave, and commit require that the pool's buyout state is INACTIVE. At any point, a user can call buyout.Start() to start the buyout process which will change the buyout state to LIVE.

Proof of Concept

migration.join() for example looks as follows:

function join(
        address _vault,
        uint256 _proposalId,
        uint256 _amount
    ) external payable nonReentrant {
        // 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 ether balances of the proposal and caller
        proposal.totalEth += msg.value;
        userProposalEth[_proposalId][msg.sender] += msg.value;
        // Deposits fractional tokens into contract
        IFERC1155(token).safeTransferFrom(
            msg.sender,
            address(this),
            id,
            _amount,
            ""
        );
        // Updates fraction balances of the proposal and caller
        proposal.totalFractions += _amount;
        userProposalFractions[_proposalId][msg.sender] += _amount;
    }

After the proposal has been created, this function allows users to transfer funds in support of the proposal. At any time, another user can start the buyout directly via Buyout.start() to change the buyout state to LIVE, therefore calls to propose, join, leave, and commit will revert.

Tools Used

Manual review.

Recommended Mitigation Steps

Restrict Buyout.start() to be called from either the migrations contract or internally if used within a proxy structure.

QA Report

[L-01] Ownership transfer should be a two-step process

It is recommended that ownership transfer be completed in two steps, firstly by setting a pending admin and finally by accepting the change from the pending admin account.

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

[L-02] CEI Pattern not followed

I was not able to determine a valid attack vector arising from the ERC1155 transfer prior to updating ethBalance, though it would be wise to change the order.

https://github.com/code-423n4/2022-07-fractional/blob/e2c5a962a94106f9495eb96769d7f60f7d5b14c9/src/modules/Buyout.sol#L168-L176

[L-03] Use of transfer over call

It is recommended to use call in order to avoid potential future op code gas inflations.

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