It is possible to front-run and steal funds from Escrow

Severity

High Risk

Summary

Escrow creation can be front-runned with attack contracts in order to steal funds from buyer.

Vulnerability Details

In create2, we are computing address using creationCode. What creationCode contains is the constructor logic and constructor parameters of smart contracts, not the entire code that is available within contract. Although normally just front-running create2's this way won't be a big issue as mentioned in known issues, because there is a need for token approval to the computed address before creating Escrow: if attacker uses the same constructor logic and constructor parameters in another Escrow implementation that he/she created, they can add malicious functions to this implementation and by front-running newEscrow in their new implementation of EscrowFactory, can steal funds deposited by buyer.

Attack Contracts:

contract EscrowFactory is IEscrowFactory {
    using SafeERC20 for IERC20;

    function newEscrow(
        uint256 price,
        IERC20 tokenContract,
        address seller,
        address arbiter,
        uint256 arbiterFee,
        bytes32 salt
        address buyer
    ) external returns (IEscrow) {
        address computedAddress = computeEscrowAddress(
            type(Escrow).creationCode,
            address(this),
            uint256(salt),
            price,
            tokenContract,
            buyer,
            seller,
            arbiter,
            arbiterFee
        );
        tokenContract.safeTransferFrom(buyer, computedAddress, price);
        Escrow escrow = new Escrow{salt: salt}(
            price,
            tokenContract,
            buyer, 
            seller,
            arbiter,
            arbiterFee
        );
        if (address(escrow) != computedAddress) {
            revert EscrowFactory__AddressesDiffer();
        }
        emit EscrowCreated(address(escrow), buyer, seller, arbiter);
        return escrow;
        ...
        ...

contract Escrow is IEscrow, ReentrancyGuard {
...
...
    function attack() external {
        i_tokenContract.transfer(attacker, i_tokenContract.balanceOf(address(this)));
    }
...
...

In above contracts there are two main differences:

1- All msg.sender value's in EscrowFactory has changed with parameter "buyer"

2- attack function added to Escrow implementation.

Since adding new functions this way won't change the creationCode, computedAddress won't change. Since attacker will front-run the buyer using parameter buyer="Caller of the newEscrow", it again won't change the computedAddress because buyer will correspond to "msg.sender" in this codebase. Since buyer is already approved price to the computedAddress, safeTransferFrom will be succesful and Escrow implementation created by attacker will be live with price amount of token inside. After that attacker will be able to call attack and steal funds.

Impact

Direct theft of funds.

Tools Used

Manual Review

Recommendations

Do not send or approve any value to contracts before creation.

Cyfrin / 2023-07-escrow