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

Poorly configured Access control verification can lead to wrong Escrow workflow.

talfao

Summary

When a buyer is creating an escrow, he can insert the buyer or seller's address to the Arbiter, which will lead to the improper workflow of the contract.

Vulnerability Details

No mechanism of verification of arbiter address results in the buyer can insert his (buyer) or seller's address as an arbiter. We can summarise the details in a few points.

  1. The workflow of the Escrow contract says that all these three roles, Arbiter/Seller/Buyer, should be independent and assigned to three different addresses/people. If they are not, it can lead to improper behaviour.

  2. If the Buyer or Seller is the Arbiter, it can cause confusion when one of them initiates a dispute within initiateDispute() function where the buyerOrSeller modifier takes place. When the dispute is initiated, the Arbiter can resolve the dispute. But as we know, when Seller or Buyer is Arbiter, it destroys the workflow.

  3. Even though the Arbiter is a trusted role, and we also know this info from README, If the buyeraccidentally or maliciously deploys anEscrowwith incorrectarbiterdetails, then thesellercould refuse to provide their services. Given that thebuyeris the actor deploying the newEscrow and locking the funds, it's in their best interest to deploy this correctly. This kind of verification can be done directly in the code, which prevents off-chain activities which are not necessary and can lead to proper workflow and correct distribution of funds.

Impact

The control of Funds and most of the functionality will be in the hands of the Seller or Buyer, which disturbs the workflow.

Tools Used

Manual Review

Proof of concept

A simple POC as a foundry test which showed improper workflow if seller is Arbiter.

​
function test_destroy_WorkFlow_SellerIsArbiter() public {
vm.startPrank(BUYER);
address arbiter = SELLER;
ERC20Mock(address(i_tokenContract)).mint(BUYER, PRICE);
ERC20Mock(address(i_tokenContract)).approve(
address(escrowFactory),
PRICE
);
escrow = escrowFactory.newEscrow(
PRICE,
i_tokenContract,
SELLER,
arbiter,
ARBITER_FEE,
SALT1
);
vm.stopPrank();
​
vm.startPrank(SELLER);
escrow.initiateDispute();
vm.stopPrank();
vm.startPrank(SELLER); // THE SELLER == ARBITER
escrow.resolveDispute(1);
vm.stopPrank();
}

Recommendations

Implement verification in Escrow.sol that SELLER OR BUYER != ARBITER.

Support

FAQs

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