Funds may be locked in `Escrow` contract beacuse Arbiter zero address check done late

[codehawks-bot]

Funds may be locked in Escrow contract beacuse Arbiter zero address check done late

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L103

Summary

Escrow contract constructor checks if the addresses passed are different than the zero address. Even though this does not solve the issue that non-controlled addresses can be passed (like 0xdead), it is a good practice to check for zero addresses.

This is not the case for the arbiter, where this check is performed in initiateDispute function. This method may not be called at all but in case of the need for a dispute. If the arbiter is set to zero address, it is impossible to initiate dispute, which may affect both parties (buyer and seller).

Vulnerability Details

If the arbiter is set to zero address, it is impossible to initiate a dispute by either relevant party (buyer or seller), which will affect both of them (in case the seller does not do the job according to the agreement, the buyer cannot withdraw funds. In case of buyer does not want to pay even though the seller did the job, the seller cannot withdraw funds).

Impact

The impact may be marked as high, since it may affect one of the parties of interest with no possibility to withdraw funds, which would be lost forever in case of the arbiter being set to zero address.

Tools Used

Manual review.

Recommendations

Check if the arbiter has an address different from the zero address should be done in the Escrow contract constructor instead of in initiateDispute function.

[0kage-eth]

Arbiter can be a zero address

[PatrickAlphaC]

You mentioned the attack vector that convinced me this was an issue