CodeHawks Escrow Contract - Competition Details

Impact

The Escrow.constructor assigns the buyer, seller and arbiter addresses as follows:

    i_buyer = buyer;
    i_seller = seller;
    i_arbiter = arbiter;

But there is no check to verify buyer != arbiter. As a result a malicious buyer can set arbiter as himself. The Escrow documentation provides the following explanation about this scenario.

Before the buyer deploys a new Escrow, the buyer and seller should agree to the terms for the Escrow. If the buyer accidentally or maliciously deploys an Escrow with incorrect arbiter details, then the seller could refuse to provide their services. Given that the buyer is the actor deploying the new Escrow and locking the funds, it's in their best interest to deploy this correctly.

According to the above description, it says that buyer is the actor who is locking his funds in the Escrow contract and hence it is in his best interest for him to deploy the contract correctly.

But the problem here is that if the buyer sets himself as the arbiter he can not lose his funds since he can call Escrow.initiateDispute and then call Escrow.resolveDispute to transfer the entire deposited funds to his address via using the buyerAward and i_arbiterFee amounts.

Hence there is no disadvantage for the buyer in setting the arbiter address to his own address maliciously. Because if it is detected by the seller and denies supplying the service, the buyer can withdraw the funds. But if the seller forgets to re-verify the arbiter address to be the correct one, it could be loss of funds to the seller. Because the buyer after receiving the service from the seller can call the Escrow.initiateDispute instead of calling the Escrow.confirmReceipt. Since buyer is the arbiter himself, he can call the Escrow.resolveDispute withdraw the entire contract balance to his account address. This way the seller will lose the payment for his rendered service.

Vulnerability Details

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

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L119-L124

Tools Used

Manual Review and VSCode

Recommendations

Hence it is recommended to add the following check inside the Escrow.constructor function.

require( buyer != arbiter, "buyer can not be the arbiter");