search

Cyfrin / 2023-07-escrow

17 stars 12 forks source link

[H-01] Lack of emergency withdraw function when no arbiter is set #150

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

[H-01] Lack of emergency withdraw function when no arbiter is set

Severity

High Risk

Relevant GitHub Links

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

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

Impact

If there is no arbiter, buyer can never retract the funds sent to escrow, causing tokens to be lost forever.

In Escrow.initiateDispute(), if no arbiter is set by buyer, dispute can never be initiated.

Escrow.sol#L103

function initiateDispute() external onlyBuyerOrSeller inState(State.Created) {
    /// @audit if no arbiter set, dispute cannot be initiated
->  if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();
    s_state = State.Disputed;
    emit Disputed(msg.sender);
}

The only way to retrieve back the funds is through buyer/seller first calling initiateDispute() then arbiter calling resolveDispute(). However, Escrow.resolveDispute() will always revert due to the inState modifier because initiateDispute() cannot be called to set state of escrow to Dispute.

Escrow.sol#L109

/// @inheritdoc IEscrow
function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
    /// @audit this function will always revert unless initiateDispute is called by buyer or seller
    uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
    uint256 totalFee = buyerAward + i_arbiterFee; // Reverts on overflow
    if (totalFee > tokenBalance) {
        revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
    }

    s_state = State.Resolved;
    emit Resolved(i_buyer, i_seller);

    if (buyerAward > 0) {
        i_tokenContract.safeTransfer(i_buyer, buyerAward);
    }
    if (i_arbiterFee > 0) {
        i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
    }
    tokenBalance = i_tokenContract.balanceOf(address(this));
    if (tokenBalance > 0) {
        i_tokenContract.safeTransfer(i_seller, tokenBalance);
    }
}

In the above scenario, the only way to transfer funds out of escrow is for buyer to call confirmReceipt() and send funds to seller.

Proof of Concept

Consider the scenario where there is no arbiter, and buyer is dissatisfied with seller delivery, but escrow contract is created with no arbiter.

In this case, there is no way for buyer to retrieve their funds sent to Escrow contract since both functions initiateDispute() and resolveDispute() cannot be called if there are no arbiter set as their tokens are locked forever in Escrow contract, unless they are willing to go ahead with payment via confirmReceipt().

Tools Used

Manual Analysis

Recommendation

Consider adding an additional onlyBuyer function where withdrawal of escrowed funds by buyer is allowed when there is no arbiter set. However, set a delay so that buyer cannot immediately pull escrowed tokens to grief sellers payment.

In general start of audit to end of audit will normally not take more than 3 months.

PatrickAlphaC commented 1 year ago

This is the one that convinced me this was an issue.

PatrickAlphaC commented 1 year ago

Idk what In general start of audit to end of audit will normally not take more than 3 months. is about and if this is an AI-generated report I'm going to be upset.

nevillehuang commented 1 year ago

Hi @PatrickAlphaC I have alot of respect for your judging but here are my escalations, I have aggregated all comments here for ease of reading, let me know if you want to comment on each individual unique submission.

The root cause of #150 is no arbiter (address(0)) set by buyer, which can in turn grief buyer if seller breaks trust/provides a dissatisfactory audit. Alot of the issues duplicated do not have the same root cause, and so should be reconsidered to prevent judging inconsistencies.

My opinion is that there should be no high issues in this contest. #154 and #150 both have a low likelihood of happening, and so should be downgraded to Medium

Duplicates of #150 (Valid Medium)

52, #128, #150, #245, #293, #298, #339, #344, #415, #462, #484, #493, #511, #534, #619, #643, #730, #746, #749, #834

Valid duplicates of #150, with correct root cause describing lack of arbiters to resolve disputes

Additional Unique escalations that is related to #150:

inallhonesty commented 1 year ago

I'd like to escalate this issue. In my opinion the presence of a withdraw function defeats the purpose of the escrow concept because it provides a way for a malicious buyer to contract and receive an audit and not pay for it. Let's analyze all possible no-arbiter scenarios:

  1. Happy scenario (having a withdraw function or not doesn't influence this)
  2. Malicious seller, good buyer: In the end seller won't receive any payment and will probably face exclusion from the platform. The buyer will lose his money because he didn't want to pay to use an arbiter which would have completely mitigated this problem. The withdraw function would help in this case, but would render useless the services of an arbiter.
  3. Malicious buyer, good seller: The seller won't receive any payment. In absence of a withdraw function the buyer will lose his money and probably face exclusion from the platform. The withdraw function will help the malicious buyer giving him a way to contract an audit, never call the confirmReceipt function, wait any cooldown (if implemented) and withdraw his money, defeating the purpose of an escrow because the malicious buyer won't lose anything.
  4. Not worth to talk about the case when they are both malicious.

TLDR: If you think your funds will get stuck, use an arbiter. Don't make the conditions worse for the honest seller. Having a supervising 3rd party is the heart of an escrow.

nevillehuang commented 1 year ago

I'd like to escalate this issue. In my opinion the presence of a withdraw function defeats the purpose of the escrow concept because it provides a way for a malicious buyer to contract and receive an audit and not pay for it. Let's analyze all possible no-arbiter scenarios:

  1. Happy scenario (having a withdraw function or not doesn't influence this)
  2. Malicious seller, good buyer: In the end seller won't receive any payment and will probably face exclusion from the platform. The buyer will lose his money because he didn't want to pay to use an arbiter which would have completely mitigated this problem. The withdraw function would help in this case, but would render useless the services of an arbiter.
  3. Malicious buyer, good seller: The seller won't receive any payment. In absence of a withdraw function the buyer will lose his money and probably face exclusion from the platform. The withdraw function will help the malicious buyer giving him a way to contract an audit, never call the confirmReceipt function, wait any cooldown (if implemented) and withdraw his money, defeating the purpose of an escrow because the malicious buyer won't lose anything.
  4. Not worth to talk about the case when they are both malicious.

TLDR: If you think your funds will get stuck, use an arbiter. Don't make the conditions worse for the honest seller. Having a supervising 3rd party is the heart of an escrow.

My friend u described the correct recommendation right there, use an arbiter! My recommendation is not the best, but thats exactly the issue here, the contract SHOULD enforce the use of an arbiter to prevent the possibility of this vulnerability occuring

inallhonesty commented 1 year ago

My recommendation is not the best, but thats exactly the issue here, the contract SHOULD enforce the use of an arbiter to prevent the possibility of this vulnerability occurring

100%. A withdraw method will only make the platform more vulnerable!

pinalikefruit commented 1 year ago

Another case related to the dispute https://github.com/Cyfrin/2023-07-escrow/issues/231

alymurtazamemon commented 1 year ago

@PatrickAlphaC This is a known issue;

Addresses other than the zero address (for example 0xdead) could prevent disputes from being resolved.

Before the buyer deploys a new Escrow, the buyer and seller should agree to the terms for the Escrow. If the buyer accidentally or maliciously deploys an Escrow with incorrect arbiter details, then the seller could refuse to provide their services. Given that the buyer is the actor deploying the new Escrow and locking the funds, it's in their best interest to deploy this correctly.

Here we can see it is a mutual understanding between buyer and seller to not select the arbiter.

This can be possible that the buyer trusts the seller (due to previous service or whatever reason) and does not want to give a fee to an arbitrator.

This is a known issue that the protocol team will either work on it or intentionally allow the buyer to transfer funds to the seller when no arbitrator is selected while deploying.

0x4ka5h commented 1 year ago

@PatrickAlphaC #571 is same as above listed issues, but closed due to insufficient explanation.

nevillehuang commented 1 year ago

@PatrickAlphaC This is a known issue;

Addresses other than the zero address (for example 0xdead) could prevent disputes from being resolved.

Before the buyer deploys a new Escrow, the buyer and seller should agree to the terms for the Escrow. If the buyer accidentally or maliciously deploys an Escrow with incorrect arbiter details, then the seller could refuse to provide their services. Given that the buyer is the actor deploying the new Escrow and locking the funds, it's in their best interest to deploy this correctly.

Here we can see it is a mutual understanding between buyer and seller to not select the arbiter.

This can be possible that the buyer trusts the seller (due to previous service or whatever reason) and does not want to give a fee to an arbitrator.

This is a known issue that the protocol team will either work on it or intentionally allow the buyer to transfer funds to the seller when no arbitrator is selected while deploying.

This issue has nothing to do with the known issues. Originally, it is intended that escrow contract can be deployed with no arbiter aka address(0)

Here, the buyer puts trust on the seller by deploying a contract with no arbiter, but the seller griefs the buyer by say providing a dissatisfactory audit. Seller here is not a trusted role, and as such should not be allowed to grief buyer. This is why this issue is valid and a trusted arbiter must be enforced.

alymurtazamemon commented 1 year ago

@PatrickAlphaC This is a known issue; Addresses other than the zero address (for example 0xdead) could prevent disputes from being resolved. Before the buyer deploys a new Escrow, the buyer and seller should agree to the terms for the Escrow. If the buyer accidentally or maliciously deploys an Escrow with incorrect arbiter details, then the seller could refuse to provide their services. Given that the buyer is the actor deploying the new Escrow and locking the funds, it's in their best interest to deploy this correctly. Here we can see it is a mutual understanding between buyer and seller to not select the arbiter. This can be possible that the buyer trusts the seller (due to previous service or whatever reason) and does not want to give a fee to an arbitrator. This is a known issue that the protocol team will either work on it or intentionally allow the buyer to transfer funds to the seller when no arbitrator is selected while deploying.

This issue has nothing to do with the known issues. Originally, it is intended that escrow contract can be deployed with no arbiter aka address(0)

Here, the buyer puts trust on the seller by deploying a contract with no arbiter, but the seller griefs the buyer by say providing a dissatisfactory audit. Seller here is not a trusted role, and as such should not be allowed to grief buyer. This is why this issue is valid and a trusted arbiter must be enforced.

See I had debated on this issue several times in the discord channel. Where trust is involved there smart contracts cannot do anything.

CodeHawks has given the option to the buyer to select the arbitrator and the seller to confirm before providing service. Now if they both agree then this is not a smart contract issue.

Now let's say a buyer is not satisfied with the service then it means he trusted the wrong person and the seller's reputation is affected.

And here in the discord, I asked Patrick about user errors Will user errors (which happen due to design decisions) not be considered platform flaws?

Everybody (including the platform) knew that this could happen but it allowed it.

nevillehuang commented 1 year ago

@PatrickAlphaC This is a known issue; Addresses other than the zero address (for example 0xdead) could prevent disputes from being resolved. Before the buyer deploys a new Escrow, the buyer and seller should agree to the terms for the Escrow. If the buyer accidentally or maliciously deploys an Escrow with incorrect arbiter details, then the seller could refuse to provide their services. Given that the buyer is the actor deploying the new Escrow and locking the funds, it's in their best interest to deploy this correctly. Here we can see it is a mutual understanding between buyer and seller to not select the arbiter. This can be possible that the buyer trusts the seller (due to previous service or whatever reason) and does not want to give a fee to an arbitrator. This is a known issue that the protocol team will either work on it or intentionally allow the buyer to transfer funds to the seller when no arbitrator is selected while deploying.

This issue has nothing to do with the known issues. Originally, it is intended that escrow contract can be deployed with no arbiter aka address(0) Here, the buyer puts trust on the seller by deploying a contract with no arbiter, but the seller griefs the buyer by say providing a dissatisfactory audit. Seller here is not a trusted role, and as such should not be allowed to grief buyer. This is why this issue is valid and a trusted arbiter must be enforced.

See I had debated on this issue several times in the discord channel. Where trust is involved there smart contracts cannot do anything.

CodeHawks has given the option to the buyer to select the arbitrator and the seller to confirm before providing service. Now if they both agree then this is not a smart contract issue.

Now let's say a buyer is not satisfied with the service then it means he trusted the wrong person and the seller's reputation is affected.

And here in the discord, I asked Patrick about user errors Will user errors (which happen due to design decisions) not be considered platform flaws?

Everybody (including the platform) knew that this could happen but it allowed it.

I really don’t see where the user error is. Seller griefing buyer can happen anytime (easily in fact) and is out of buyers control. But if the contract enforces a trusted impartial arbiter (especially if its codehawks themselves) , funds can always be sent back to the correct disputer (in this case buyer)

Besides, are discord information part of the known issues? Will let @PatrickAlphaC correct me on this since afaik, they are not, unless changes are directly made to the github repo

8ahoz commented 1 year ago

I agree with @nevillehuang and @inallhonesty I submitted this as an issue which was overruled #758

inallhonesty commented 1 year ago

I see that I've been misunderstood a bit.

In my opinion, whether the buyer and seller decide to use an arbiter or not is strictly their problem. If the buyer considers the arbiter fee > risk of a malicious seller is strictly the buyer's problem. The protocol allows deals without arbiter and that's a feature of the protocol that I'm not discussing in my escalation. I just consider this contract shouldn't have a withdraw function because that will give too much power to the buyer in detriment of the seller. If the buyer feels they are about to get scammed, then they should 100% use an arbiter, not rely of a withdraw method, which could be used to steal the just payment of the seller.

nevillehuang commented 1 year ago

All the above comments are related to when the arbiter is 0, however the main issue here is lack of withdrawal mechanism when disputes cannot be solved. You can see my issue as a reference #30

Agreed, different root cause, should be separate issues. #30 is a duplicate of #8 (see comments there)

PatrickAlphaC commented 1 year ago

checking

PatrickAlphaC commented 1 year ago

I've made a number of comments and changes. Yes. The root cause of this is an issue, and we are keeping it as such. Thank you all for the comments on these, it's been great to read.

Especially after all this, the biggest feedback we've received is that the judging guidelines need to be stricter. We will be asking for feedback in the discord shortly. Thank you all!!

alexroan commented 1 year ago

I'm really appreciative of the diligence and depth of analysis that's gone into this. Thanks all @PatrickAlphaC @nevillehuang @inallhonesty @alymurtazamemon @8ahoz @0x4ka5h @pinalikefruit @0kage-eth