code423n4 commented 2 years ago

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

Ferret-san commented 2 years ago

Duplicate of #68

HardlyDifficult commented 2 years ago

Lowering these to Low risk and converting this into a QA report for the warden. See https://github.com/code-423n4/2022-07-fractional-findings/issues/501 for context.