Summary

Zero arbiter address can cause funds to be stuck forever

Vulnerability Details

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

When deploying the Escrow contract, the address of the buyer and seller are checked to ensure it's not the zero address, but, the check is not done for the arbiter as it is labelled optional.

Impact

Since the Escrow contract can be deployed with the zero arbiter address, this means the buyer can have his funds stuck forever.

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

In the initiateDispute, the arbiter zero address check would prevent any dispute from being created, thus, funds are locked.

Due to the importance of this parameter, I know it should be checked thoroughly to ensure this doesn't happen, and a wrong address would achieve the same impact. But I'm leaving it as a vulnerability due to the severity of the impact, and the fact that this could be easily prevented either by checking for address(0) at the constructor and preventing it altogether OR giving the option to set an arbiter after the escrow contract is created - This can be a 2 step implementation where the buyer and seller have to accept it.

Tools Used

Manual review

Recommendations

Add the zero address check in the Escrow.sol constructor: