search

Cyfrin / 2023-07-escrow

16 stars 12 forks source link

[LOW]Escrow#Constructor - Doesn’t check that arbiter is not a zero address and risks locking the funds. #58

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

[LOW]Escrow#Constructor - Doesn’t check that arbiter is not a zero address and risks locking the funds.

Severity

Low Risk

Relevant GitHub Links

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

Summary

The constructor function in the Escrow contract has an oversight where it does not verify if the arbiter address is a non-zero address. This could lead to potential issues where funds may be inadvertently locked.

Instead, the verification happens inside initiateDispute(), which is too late in terms of the funds being locked in the contract.

Vulnerability Details

The function does not verify whether the arbiter address is non-zero. This creates a possibility for the arbiter address to be accidentally or intentionally set as the zero address, which could lock funds in the contract as the dispute resolution process requires the presence of an arbiter.

Instead, the verification happens inside **initiateDispute()**, at which point it is too late to mitigate the arbiter to a correct address since the Escrow contract has already been created and funds sent to it.

Impact

If the arbiter address is set as a zero address, it means there is no actual address that can arbitrate disputes. Therefore, if a dispute arises, the contract's funds may remain locked indefinitely since no one can call the resolveDispute function.

Tools Used

Manual Review.

Recommendations

Move the code line:

**if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();**

to the constructor to revert in case the **i_arbiter** is a zero-address, just like **i_buyer** and i_seller

PatrickAlphaC commented 1 year ago

it can be 0

0xvangrim commented 1 year ago

[ESCALATE]

Escalating this issue as it addresses the same points as the findings related to the following label: https://github.com/Cyfrin/2023-07-escrow/labels/finding-no%2Fdead-arbitor-%2B-malicious%2Fdead-buyer

This issue also mentions:

See a similar issue that has been rewarded the aforementioned vulnerability: https://github.com/Cyfrin/2023-07-escrow/issues/846

Also see the selected issue (although the recommendation is different): https://github.com/Cyfrin/2023-07-escrow/issues/150

P.S. The initial severity level was based on the understanding that the Escrow contract would allow Escrows without arbiters. I understand that this has been revised (See @PatrickAlphaC comment https://github.com/Cyfrin/2023-07-escrow/issues/846). The new severity level should be high (impact: high, likelihood: high).