search

Cyfrin / 2023-07-escrow

17 stars 12 forks source link

`Escrow` should not be deployed without an `arbiter` to avoid possible scenario where funds are stuck in the contract #43

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

Escrow should not be deployed without an arbiter to avoid possible scenario where funds are stuck in the contract

Severity

Low Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L32-L51

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

Summary

If no arbiter is configured, initiateDispute will always revert and funds will be stuck in the Escrow contract.

Vulnerability Details

When the Escrow contract is deployed via the EscrowFactory, price tokens will be transferred from the buyer address to the Escrow contract.

Those funds will be stored in the contract until one of these events happens:

1) seller correctly provides its service off-chain and the buyer executes confirmReceipt by transferring the funds from the Escrow contract to the seller account 2) buyer or seller are in dispute and one of them call initiateDispute. At this point, the arbiter considers the situation off-chain and execute resolveDispute to split the Escrow funds between the buyer and seller (minus the arbiterFee)

The second option is only available if the arbiter address is correctly configured, otherwise the call to initiateDispute will always revert.

In a scenario where a dispute is needed and the arbiter has not been configured, those funds will always be stuck in the Escrow contract.

Scenario 1) The seller does not provide the service. In this case, the buyer wants his tokens back, but the only possible option available (because no arbiter is configured) to move funds is confirmReceipt that would send the funds to the seller who did not provide the service. Scenario 2) The seller has correctly provided the service, but the buyer refuses to pay and will not execute confirmReceipt. In this case, the buyer has received the off-chain work, but the seller will not get paid, and the funds will remain in the contract.

Impact

Funds remain stuck in the contracts or go to a seller that have not provided the service.

Tools Used

Manual

Recommendations

The Escrow contract should always be fully configured to avoid the two scenarios explained above. If the arbiter or arbiterFee are not correctly configured, the creation of the Escrow should revert.

The Escrow.constructor should perform the following checks: 1) arbiter != address(0) 2) arbiterFee should have a lower bound (proportional to the price) to incentivize the arbiter to fulfil the role

PatrickAlphaC commented 1 year ago

The arbitor can be 0

StErMi commented 1 year ago

The following issue has been accepted as a high severity issue: https://github.com/Cyfrin/2023-07-escrow/issues/621

The issue I have submitted is identical to the one that I previously mentioned. I think that at this point also this issue should be accepted and marked as high.