CodeHawks Escrow Contract - Competition Details

Impact

The EscrowFactory.newEscrow function pre-computes the Escrow address and then transfers the funds from the msg.sender to the pre-computed Escrow contract address. During the Escrow contract deployment the contract balance is checked against the passed in price input parameter inside the Escrow.constructor as shown below:

    if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance(); 

Inside the EscrowFactory.newEscrow function the same price variable is used as the parameter for the fund transfer amount and the input parameter for the Escrow contract deployment.

The problem here is that if tokenContract is a fee on transfer then the transaction will revert during the Escrow contract deployment since tokenContract.balanceOf(address(this)) < price condition will be satisfied. As a result the buyer (msg.sender) will lose deployment gas fees. Hence the terms of Escrow will have to be renegotiated with the seller and redeploy a new Escrow contract with a different tokenContract.

Hence it is recommended to inform the participants of the Escrow, that fee on transfer tokens can not be used as tokenContracts (Similar to how it is informed that ERC777 tokens should not be used due to callback exploits). But the down side to this is the usable tokenContracts will be limited now. Since now neither fee on transfer tokens nor ERC777 tokens can be used. Hence this will delay the process of both seller and buyer needing to agree on a suitable tokenContract for the Escrow. This will damage the user experience of the participants.

If the seller and buyer agrees to use a fee on transfer token as tokenContract considering the prevailing market dynamics of a particular token at the time, The EscrowFactory.newEscrow function should be modified to allow that capability.

Vulnerability Details

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/Escrow.sol#L44

Tools Used

Manual Review and VSCode

Recommendations

It is recommended to replace the contract deployment constructor parameter (price) with tokenContract.balanceOf(computedAddress) as follows:

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

This way fee on transfer tokens can be used as tokenContracts since now the Escrow contract deployment will not revert since tokenContract.balanceOf(address(this)) == price will occur inside the Escrow.constructor.