No zero address check for arbiter in Escrow contract's constructor

[codehawks-bot]

No zero address check for arbiter in Escrow contract's constructor

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/Escrow.sol

Summary

No zero address check for arbiter in Escrow contract's constructor. Also, where is arbiter selected/created if not done during constructor?? I couldnt find it...

Vulnerability Details

n/a

Impact

If no arbiter set during escrow deployment(i.e. address(0)), and if arbiter then required afterwards, then oopsie... DoS of arbitration functionality...

Tools Used

VSC, manual.

Recommendations

Either add a check for zero address in Escrow's constructor as is currently the case for buyer, seller, token address, and/or enable functionality to create/select/add arbiter once off to escrow contract afterwards, like a function which can be called only once?

[PatrickAlphaC]

If you have the example of no arbitor + malicious seller I would have given you this.

[dappconsulting]

Acknowledged.

Two potential issues I mentioned here:

1) no address(0) check for arbiter address during escrow deployment <<< But I see now that this is actually OK but only if arbiter can be selected post escrow deployment, and then can always add checks to ensure arbiter can only be selected once per deployed escrow, unless protocol intends to allow for changing arbiter multiple times?

2) seemingly no functionality to select/add valid arbiter address post escrow deployment.

I still think that not being able to set/select an arbiter post escrow deployment is likely an unwanted issue to sit with, especially if an arbiter is needed at some point.

Scenarios:

• Both seller & buyer could be honest, but a dispute needs resolving.
• Buyer decides to pull out for whatever valid reason, early on.
• Seller for whatever valid reason, cannot commit any longer to providing audit service to buyer
• Malicious buyer or seller chancing it
• Buyer & Seller agreeing to increase/decrease escrowed funds

Summary: Either a valid non-zero arbiter address needs to be added during escrow deployment, or functionality should be implemented to enable adding of arbiter post escrow deployment. Either solution solves the problem. Right now neither is implemented.

[PatrickAlphaC]

Maybe, but as written, this write up does not give nearly enough information to the protocol.

[PatrickAlphaC]

After review, you've got the root issue, but wrong impact, so I'm giving points.