CodeHawks Escrow Contract - Competition Details
Cyfrin
Foundry
40,000 USDC
View results
Previous Next
Submission Details
Severity: high

Design Issue Gives the `buyer` Optionality to Conduct the Audit or Not

No12Samurai

Summary

I discuss the impact of malicious behavior from each party, where the arbiter can favor one side during dispute resolution, and the seller can exploit a scenario not previously considered. Specifically, when the seller has the option not to complete a task after the buyer funds the Escrow contract, the buyer may lose a significant amount of funds.

The Proof of Concept demonstrates a situation where the seller chooses not to perform the task, leaving the buyer's funds locked in the contract. The buyer can initiate a dispute, and the arbiter can resolve it, but the buyer still loses a substantial amount.

The recommended mitigation involves modifying the contract to require the seller to lock some funds in the Escrow contract. Until the seller locks the funds, the buyer can retrieve their funds. After that, if the buyer confirms receipt or a dispute is initiated, the locked funds will be transferred back to the seller. This modification aims to prevent situations where the seller can exploit the system to lock the buyer's funds without completing the task.

In summary, implementing the proposed changes and adding a multi-signature setup for dispute resolution can help mitigate the vulnerabilities associated with the arbiter, buyer, and seller roles in the Escrow contract.

Vulnerability Details

When a buyer creates an Escrow transaction for a task to be done, the seller has the option to complete the task or just ignore it.

If the seller completes the task without any problem, the buyer will call confirmReceipt() and the money will be transferred to the seller. If any disputes happen, the buyer or seller will call initiateDispute(), and again the task will be completed with almost no problem. However, each of the parties, buyer, seller, and arbiter can be malicious and cause issues.

Three cases can happen here with respect to each party:

  1. The buyer can be malicious and never call the confirmReceipt() function. However, there is no incentive in doing that cause the buyer will not gain any money, this should be considered because it is not impossible to happen. This issue is mentioned in Known Issues section, so I do not explore it:

    buyer never calls confirmReceipt - The terms of the Escrow are agreed upon by the buyer and seller before deploying it. The onus is on the seller to perform due diligence on the buyer and their off-chain identity/reputation before deciding to supply the buyer with their services.

  2. The arbiter can be malicious and call the resolveDispute() function in favor of the buyer or seller. For example, the arbiter can set the buyerAwared such that all the funds get back to the buyer. Also, this case is mentioned:

    arbiter is a trusted role

  3. The case that the seller can be malicious is the case that is not considered. Consider a case when buyer and seller decides to do a project. The buyer creates an Escrow contract and funds it with specific amount. From now on, the seller has the option to complete the project or not. Considering the case that the arbiter is honest, the buyer still loses the i_arbiterFee amount of their funds.

Impact

Consider Alice as the buyer who is requesting an audit. Bob is a seller who is an auditor. Alice and Bob decide on a $10,000 USDC to audit the project and adds Charlie as the arbiter who takes $1,000 in case of a dispute.

Alice creates an Escrow and funds it with $10,000 USDC. Bob finds a better project and does not perform Alice's audit. Now, Alice's funds are locked in the contract without having the option to take back the money. She only can call the initiateDispute() function, and Bob will call the resolveDispute() function and sends $9,000 as the buyerAward, and $9,000 will be sent back to Alice. In this case, Alice loses $9,000 dollars.

Tools Used

Manual Review

Recommendations

I suggest the following method. The seller must lock some funds in the Escrow contract. Until the time the seller has not locked any funds in the contract, the buyer can receive their funds. After that, the buyer cannot take back the funds. When the buyer calls the confirmReceipt() function or the arbiter calls the resolveDispute() function, the seller-locked funds will be sent back to the seller.

First, we add a state to the State called SellerParticipated:

enum State {
Created,
SellerParticipated,
Confirmed,
Disputed,
Resolved,
Cancelled
}

The contract takes a new input called i_sellerLockAmount that the seller must lock for progress.

uint256 private immutable i_sellerLockAmount;
​
// Set this value in the constructor()
// I do not add the implementation of the constructor() function for the sake of simplicity.
// Note that the constructor() should change accordingly.

Also, we add the following event to be emitted when the seller locked the funds, or the buyer cancels the execution:

event ParticipatedSeller();
event Cancelled();

Also, we add the following modifier to make sure only seller calls the function; however, it may not be necessary:

error Escrow__OnlySeller();
​
modifier onlySeller() {
if (msg.sender != i_seller) {
revert Escrow__OnlySeller();
}
_;
}

Then, we add another function that the seller should lock some funds so that the state will be changed from Created to SellerParticipated:

function participateSeller() public onlySeller inState(State.Created) {
s_state = State.SellerParticipated;
emit ParticipatedSeller();
i_tokenContract.safeTransferFrom(i_seller, address(this), i_sellerLockAmount);
}

Also, we change the confirmReceipt() and initiateDispute() to be in the SellerParticipated to be executed:

function confirmReceipt() external onlyBuyer inState(State.SellerParticipated) { ... }
function initiateDispute() external onlyBuyerOrSeller inState(State.SellerParticipated) { ... }

Finally, we add a function so that if the contract in the Created state, the buyer can get back the funds:

function getBackFunds() external onlyBuyer inState(State.Created) {
s_state = State.Cancelled;
emit Canclled();
i_tokenContract.safeTransfer(i_buyer, i_tokenContract.balanceOf(address(this)));
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.