codehawks-bot commented 1 year ago

Consider allowing the buyer to refund their tokens after a certain amount of time has passed

Severity

Low Risk

Relevant GitHub Links

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

Summary

initiateDispute() checks that i_arbiter is not the zero address, meaning the use of an arbiter is optional. In the case where no arbiter exists, tokens can effectively become locked in the contract indefinitely. While it could be argued that this is an inherent risk that must be taken buy the buyer and seller, it can be avoided by allowing the buyer to withdraw the tokens after a predetermined amount of time has passed.

Impact

In the event that no arbiter is used, and the seller refuses to adhere to the agreement, the tokens would be stranded in the Escrow contract indefinitely, as the buyer would have no incentive to release the tokens to the seller, and likely wouldn't want to.

Tools Used

Manual review

Recommendations

+   uint256 private immutable i_deadline;

    constructor(
        uint256 price,
        IERC20 tokenContract,
        address buyer,
        address seller,
+       uint256 deadline,
        address arbiter,
        uint256 arbiterFee
    ) {
        if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
        if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
        if (seller == address(0)) revert Escrow__SellerZeroAddress();
        if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
        if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
+       if (deadline < block.timestamp) revert Escrow__DeadlineInPast();
        i_price = price;
        i_tokenContract = tokenContract;
        i_buyer = buyer;
        i_seller = seller;
        i_arbiter = arbiter;
        i_arbiterFee = arbiterFee;
+       i_deadline = deadline;
    }

+   function withdraw() external onlyBuyer inState(State.Created) {
+       require(block.timestamp > i_deadline, "deadline not met");
+       s_state = State.Expired;
+
+       i_tokenContract.safeTransfer(i_buyer, i_tokenContract.balanceOf(address(this)));
+   }

PatrickAlphaC commented 1 year ago

great suggestion

PatrickAlphaC commented 1 year ago

Actually, no, a buyer is now incentivized to just wait out till the end and never confirm receipt to get their money back. They could wait till the last minute so it's difficult for an arbitor to get involved.

Madalad commented 1 year ago

I think that this issue is a duplicate of H-01 Lack of emergency withdraw function when no arbiter is set.

Both issues describe the possibility that, if no arbiter is set, funds may be trapped in the contract indefinitely
Both reports suggest mitigating this by setting a deadline after which the buyer may retract funds

Concerning Patricks comment above:

Actually, no, a buyer is now incentivized to just wait out till the end and never confirm receipt to get their money back. They could wait till the last minute so it's difficult for an arbiter to get involved.

This concern is also applicable to H-01 as the recommended mitigation is practically the same, and so if this is being used to invalidate this issue then it should also invalidate H-01.

If there is an arbiter then the deadline would realistically be set far enough into the future to allow the seller to dispute before the buyer gets the opportunity to emergency withdraw. If there is no arbiter then a disagreement negatively affecting either the buyer or seller is unavoidable.

Cyfrin / 2023-07-escrow