codehawks-bot commented 1 year ago

Funds Locked in Contract as Escrow Contracts can be created without a Valid Arbiter

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/EscrowFactory.sol#L20-L26

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

Summary

Escrow contracts do not validate if the arbiter is a valid address, this means if a user creates an escrow contract with arbiter address as address(0) and the user wishes to dispute the escrow contract it will perpetually revert leading to funds being locked in the contract.

Vulnerability Details

The contract EscrowFactory.sol allows msg.sender to create a contract without validating the arbiter address to be a non 0 address. (Bear in mind this cannot be changed once the contract has been created. ) Therefore, if there is a dispute and the user wants to call the function initiateDispute(), they will be unable to do so due to this line of code reverting the function if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();

To prove this concept:

Go to test/EscrowTestBase.t.sol and set address public constant ARBITER = address(0);
You should be able to create a new Escrow contract
Then call forge test --match-test "testInitiateDisputeChangesState" -vvvv
This test should fail as the arbiter address is 0

Impact

Sellers and Buyers will be unable to dispute escrow contracts created by the buyer since i_arbiter == address(0). Therefore, there will be no way of resolving the dispute as it cannot be initiated in the first place. This will lead to loss of funds as a result of token locking.

Tools Used

Manual Review

Recommendations

Create a validation check in EscrowFactory.sol which will ensure arbiter addresses are non-zero addresses. Like so:

if (address(arbiter) == address(0)) {
            revert EscrowFactory__ArbiterAddressNotValid();
        }

0kage-eth commented 1 year ago

It is possible to have no arbiter contracts (incases where there is past history between buyers and sellers and they trust each other enough not to assign arbiter). Since seller needs to agree upfront not to have an arbiter, seller will only agree to this if he trusts buyer's creditworthiness.

PatrickAlphaC commented 1 year ago

After review, it'll never make sense to not have an arbitor. I have been convinced

nevillehuang commented 1 year ago

Duplicates of #40 (Partially Valid M, Since there is no partial credit in codehawks, could be Invalid)

168, #173, #231, #346, #621, #751, #846

Partially valid, correctly describe that dispute cannot be created, but does not explain why (e.g. buyer dissatisfied with seller performance). In addition, buyer griefing seller is not a valid issue as buyers are trusted. Only seller griefing buyer is a valid issue.

PatrickAlphaC commented 1 year ago

👍

PatrickAlphaC commented 1 year ago

@nevillehuang the root cause seems to be the same though. I agree. We don't have partial credit, so going to give points for this one.

Cyfrin / 2023-07-escrow