Fractional v2 contest - Critical's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L11-L29

Vulnerability details

Impact

The implementation contract of Vault.sol is deployed in the constructor function if VaultFactory, but remains uninitialized.

This makes it possible for an attacker to call init() the implementation contract of Valut.sol and delegatecall to selfdestruct and break all the Vaults.

This attack vector is made well-known by the $10m wormhole bug bounty: https://medium.com/immunefi/wormhole-uninitialized-proxy-bugfix-review-90250c41a43a

Proof of Concept

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/VaultFactory.sol#L11-L22

contract VaultFactory is IVaultFactory {
    /// @dev Use clones library for address types
    using Create2ClonesWithImmutableArgs for address;
    /// @notice Address of Vault proxy contract
    address public implementation;
    /// @dev Internal mapping to track the next seed to be used by an EOA
    mapping(address => bytes32) internal nextSeeds;

    /// @notice Initializes implementation contract
    constructor() {
        implementation = address(new Vault());
    }

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L11-L29

contract Vault is IVault, NFTReceiver {
    /// @notice Address of vault owner
    address public owner;
    /// @notice Merkle root hash of vault permissions
    bytes32 public merkleRoot;
    /// @notice Initializer value
    uint256 public nonce;
    /// @dev Minimum reserve of gas units
    uint256 private constant MIN_GAS_RESERVE = 5_000;
    /// @notice Mapping of function selector to plugin address
    mapping(bytes4 => address) public methods;

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

Recommended Mitigation Steps

Init the implementation contract right after creation:

constructor() {
        implementation = address(new Vault());
	IVault(implementation).init();
}

Lines of code

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

Vulnerability details

Impact

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

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

    /// @notice Transfers ERC-721 tokens
    /// @param _from Source address
    /// @param _to Target address
    /// @param _tokens[] Addresses of token contracts
    /// @param _ids[] IDs of the tokens
    function batchDepositERC721(
        address _from,
        address _to,
        address[] calldata _tokens,
        uint256[] calldata _ids
    ) external {
        for (uint256 i = 0; i < _tokens.length; ) {
            IERC721(_tokens[i]).safeTransferFrom(_from, _to, _ids[i]);
            unchecked {
                ++i;
            }
        }
    }

    /// @notice Transfers ERC-1155 tokens
    /// @param _from Source address
    /// @param _to Target address
    /// @param _tokens[] Addresses of token contracts
    /// @param _ids[] Ids of the token types
    /// @param _amounts[] Transfer amounts
    /// @param _datas[] Additional transaction data
    function batchDepositERC1155(
        address _from,
        address _to,
        address[] calldata _tokens,
        uint256[] calldata _ids,
        uint256[] calldata _amounts,
        bytes[] calldata _datas
    ) external {
        unchecked {
            for (uint256 i = 0; i < _tokens.length; ++i) {
                IERC1155(_tokens[i]).safeTransferFrom(
                    _from,
                    _to,
                    _ids[i],
                    _amounts[i],
                    _datas[i]
                );
            }
        }
    }

There are several public methods in BaseVault.sol with no access control, and it utilizes the approval given by the user to transfer tokens from an arbitrary address (typically the caller's own address) to another arbitrary address (supposed to be the address of a vault?).

But since it's an open method that anyone can call, it puts all the users who approved the BaseVault.sol for whatever reason at risk for the funds in their wallets, because the attacker will call batchDepositERC20() with the _from address specified as the victim's address and transfer funds _to their own address.

Proof of Concept

We failed to find any direct reference for these methods in other contracts, so presumably, these methods will be called directly from the frontend.

Recommended Mitigation Steps

There should be no _from parameter, and the "from" address of the transferFrom should be hardcoded as msg.sender.