Fractional v2 contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

VaultFactory is Factory contract for deploying fractional vaults. in the constructor() it creates a Vault implementation contract but don't initialize it. then it uses this implementation address to creates clone of Vault contract. attacker can call init() for that Vault implementation address then it can use delegatecall functionality in Vault to perform selfdestruct in the implementation contract which would case other deployed contract or new deployments to fail.

Proof of Concept

This is constructor() code in VaultFactory:

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

As you can see it creates new Vault contract and set the implementation variable. but it doesn't call init() function of that Vault. in deployFor() code uses implementation to clone and create new vaults. This is init() code in Vault():

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

As you can see if anyone who calls this function become the owner of the Vault and can perform authorized action in Vault like installing plugins. because VaultFactory don't call init() for implementation contract, so attacker can call init() on implementation contract and become the owner of that contract and install plugins and then call them and preform selfdestruct on the implementation contract. this would cause implementation contract to be destroyed and all the Vault contract based on proxy pattern to fail and users funds would be lost, also new deployment of Vault would fail too and protocol would be in broken stage.

Tools Used

VIM

Recommended Mitigation Steps

call init() for implementation contract.

Lines of code

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

Vulnerability details

Impact

Function _execute() Executes plugin transactions through delegatecall, but code don't have any mechanism to prevent delegatecall from executing selfdestruct. if selfdestruct get executed in delegatecall, then Vault storage would be removed and users funds would be lost.

Proof of Concept

This is _execute() function code:

    function _execute(address _target, bytes calldata _data)
        internal
        returns (bool success, bytes memory response)
    {
        // Check that the target is a valid contract
        uint256 codeSize;
        assembly {
            codeSize := extcodesize(_target)
        }
        if (codeSize == 0) revert TargetInvalid(_target);
        // Save the owner address in memory to ensure that it cannot be modified during the DELEGATECALL
        address owner_ = owner;
        // Reserve some gas to ensure that the function has enough to finish the execution
        uint256 stipend = gasleft() - MIN_GAS_RESERVE;

        // Delegate call to the target contract
        (success, response) = _target.delegatecall{gas: stipend}(_data);
        if (owner_ != owner) revert OwnerChanged(owner_, owner);

        // Revert if execution was unsuccessful
        if (!success) {
            if (response.length == 0) revert ExecutionReverted();
            _revertedWithReason(response);
        }
    }

As you can see code calls _target contract with delegatecall, if that contract executes selfdestruct by mistake or intentionally then Vault state would be removed and users funds would be lost.

Tools Used

VIM

Recommended Mitigation Steps

add some mechanism to detect and prevent selfdestruct execution in delegatecall.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L74-L87 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L52-L63

Vulnerability details

Impact

Function mint() mints new fractions for an ID and is only callable by VaultRegistry. code mints tokens then updates totalSupply value. when minting contract may make external call to target address, in that external call contract state is wrong, tokens are minted but totalSupply don't show correct value. totalSupply is used in Buyout and Migration modules for calculating price and other important stuff, attacker can call those logics and exploit the issue and steal funds(because totalSupply don't show correct number, the calculations of Buyout and Migration would be wrong)

Proof of Concept

This is mint() and burn() code in FERC1155:

    function mint(
        address _to,
        uint256 _id,
        uint256 _amount,
        bytes memory _data
    ) external onlyRegistry {
        _mint(_to, _id, _amount, _data);
        totalSupply[_id] += _amount;
    }

    function burn(
        address _from,
        uint256 _id,
        uint256 _amount
    ) external onlyRegistry {
        _burn(_from, _id, _amount);
        totalSupply[_id] -= _amount;
    }

As you can see it doesn't follow check-effect-interact pattern, _mint() may call the target address, int that external call contract state is wrong and totalSupply is not showing the correct number. attacker can use this issue to steal other users funds, because other contracts uses totalSupply in their logics for calculating important stuff like price and ... For example in Buyout contract start() function:

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

totalSupply is used for calculating price.

Tools Used

VIM

Recommended Mitigation Steps

follow check-effect-intract pattern

Lines of code

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

Vulnerability details

Impact

deployFor() function in VaultFactory creates vault for tx.origin and transfer ownership to specified address in the call. attacker can redirect others transactions to deployFor() and creates a vault for user address but set the owner as attacker.

Proof of Concept

This is deployFor() code:

    function deployFor(address _owner) public returns (address payable vault) {
        bytes32 seed = nextSeeds[tx.origin];

        // Prevent front-running the salt by hashing the concatenation of tx.origin and the user-provided seed.
        bytes32 salt = keccak256(abi.encode(tx.origin, seed));

        bytes memory data = abi.encodePacked();
        vault = implementation.clone(salt, data);
        Vault(vault).init();

        // Transfer the ownership from this factory contract to the specified owner.
        Vault(vault).transferOwnership(_owner);

        // Increment the seed.
        unchecked {
            nextSeeds[tx.origin] = bytes32(uint256(seed) + 1);
        }

        // Log the vault via en event.
        emit DeployVault(
            tx.origin,
            msg.sender,
            _owner,
            seed,
            salt,
            address(vault)
        );
    }

because it uses tx.origin it's possible to perform this attack. attacker only need to create some smart contracts and make users to call attacker controlled address. in general attacker don't need to create a smart contract and ask users to create a transaction and call attacker contract, for any user transaction that reach to attacker controlled address, attacker can perform this action (for example in ERC721 transfer).

Tools Used

VIM

Recommended Mitigation Steps

don't use tx.origin