CodeHawks Escrow Contract - Competition Details

Summary

An Escrow can be initiated with zero ariberFee and the checks in the constructor will still pass

Vulnerability Details

The check on line 44 in Escrow.sol constructor is the following :

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

This means that if the balance of the escrow is equal to the price the check will pass.The balance of the contract should be the price + arbiterFee.Therefore it is not possible for the price to be lower than the balance of the escrow contract and that is what the if statement on line 44 is checking.But if the buyer does not set arbiterFee then balanceOf(address(this)) = price is true and the above mentioned if statement passes , leaving the contract with 0 arbiterFee. Because of this a dispute can never be resolved because the arbiter gets no profit of it and is not incentivised to resolve the dispute at all. Another issue with comparison operators is observed in Escrow.sol.resoveDispute() . This function can fail to transfer tokens to seller because of the buyerAward input provided by the arbiter. There is the following check in Escrow.sol.resoveDispute() if (totalFee > tokenBalance){ revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);}.This means that if (totalFee == tokenBalance) should pass. The resolveDispute() function first transfers the buyerAward to the buyer (which is an input provided by the arbiter) , then transfers the arbiterFee to the arbiter and after that caches the remaining balance of the escrow in the tokenBalance local variable.If totalFee is equal to tokenBalance there will be no funds left for the seller to receive.Not sure if this is intended behaviour or not.

Impact

Disputes become irresolvable due to deployer mistake.
Seller can be left without payment.

Tools Used

Manual Review

Recommendations

In my opinion the if statement on line 44 of Escrow.sol should be rewritten to:

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

As for the resolveDispute() function - Rewrite the if statement to be if (totalFee >= tokenBalance){ revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);} or set a reasonable cap to the buyerAward.This is a known issue in terms of the
arbiter fees being too large as stated in the documentation, but a large buyerAward can lead to the same thing and buyerAward is not a state variable which can be seen by the seller beforehand , it is provided by the arbiter upon calling resolveDispute().