CodeHawks Escrow Contract - Competition Details

Impact

The EscrowFactory contract is used to deploy the Escrow contracts. The Escrow contract address is pre-computed using the EscrowFactory.computeEscrowAddress function. Then the buyer (msg.sender) funds are sent to this contract address.

Later the new Escrow contract is created using the new keyword as shown below:

    Escrow escrow = new Escrow{salt: salt}(
        price,
        tokenContract,
        msg.sender, 
        seller,
        arbiter,
        arbiterFee
    );

But the problem here is that neither the computeEscrowAddress nor new Escrow{salt: salt}() operations check the returned contract address against address(0). If the both computeEscrowAddress and new Escrow{salt: salt}() function calls fail (due to an out of gas exception or any other reason) they will return address(0) as the return value.

The only check performed on the pre-computed contract address and actual deployed contract address is as follows:

    if (address(escrow) != computedAddress) { 
        revert EscrowFactory__AddressesDiffer();
    }

But the above condition will not revert if address(escrow) == computedAddress == address(0). Which means the transaction will proceed as if it was successful thus transferring the funds of the buyer to the address(0) and permanently losing them. The transfer to address(0) will be successful if there is no check on address(0) for to address in the transferFrom function of the ERC20 token.

Vulnerability Details

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L28-L38

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L39

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L40-L47

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L48-L50

Tools Used

Manual Review and VSCode

Recommendations

Hence it is recommended to perform the following check to verify that computedAddress is valid before the fund transfer is executed.

require(computedAddress != address(0), "Computed Address can not be Address(0)");