Summary

Since the following is a known issue, I've decided to include it here anyways because I've added some useful information which might be useful to know, so here goes:

Known issue: "Tokens with callbacks allow malicious sellers to DOS dispute resolutions"

Summary of Reentrancy Attack Vector in the Escrow Contract:

This attack vector is ONLY possible in two scenarios:

• where native token(ETH) is sent to seller as payment token(IF escrow system allows for this?), and/or

• where ERC20 tokens, that enable callback, is sent as payment token

In both scenarios, the rogue seller/attacker, needs to use an attack contract as their payment receiving address, and implement custom code/functionality for carrying out the reentrancy attack.

Vulnerability Details

Decription:
The reentrancy attack vector in the Escrow contract arises from the confirmReceipt function, where the seller's address (i_seller) is used as the recipient for the token transfer via safeTransfer. If a rogue seller is using a malicious contract as their address, they can trigger a reentrant call into the Escrow contract during the token transfer. The attacker's contract can then invoke the initiateDispute function, changing the s_state state variable to State.Disputed, which could lead to unexpected behavior.

PoC:

• The confirmReceipt() function is called by buyer and sets the value of s_state to State.Confirmed and initiates a token transfer to the seller's address (i_seller).

• The rogue seller/attacker's malicious smart contract receives the tokens and then attacker contract triggers a callback into the Escrow contract, calling the initiateDispute() function.

• The initiateDispute() function changes the state value of s_state to State.Disputed, which could lead to unexpected behavior in the contract.

• EVM Behavior: During the reentrant call, the initiateDispute function will successfully update the s_state to State.Disputed without causing a revert, even though the confirmReceipt function initially set s_state to State.Confirmed. The latest value (State.Disputed) will be persisted.

Impact

RISK/DAMAGE:
(seems no funds are at risk)

• Worst (DoS dispute resolution):
  If the below mentioned conditions (totalFee == tokenBalance == 0 and buyerAward & i_arbiterFee == 0) are not met, then same reentrancy attack will DoS at least part of functionality of this escrow contract instance.
  It will definitely cause the resolveDispute() to revert permanently(DoS attack) until below conditions are met, somehow...

• Least:
  In resolveDispute() function, ONLY if totalFee == tokenBalance == 0 in case of both buyerAward & i_arbiterFee == 0, will this function pass and make s_state = State.Resolved, and then all should be fine in theory post-attack...

Tools Used

VSC, manual.

Recommendations

Mitigation Steps/Recommendation:

a. Mutex Lock: to prevent access to initiateDispute() function DURING execution of confirmReceipt() function.

b. Validate State: Carefully validate the state/value of s_state before performing critical operations to ensure that unexpected state changes do not occur due to reentrant calls. One method could be to place a check AFTER the transfer line inside confirmReceipt() function to ensure that s_state is still equal to State.Confirmed, as follows:
if (s_state != State.Confirmed) revert Escrow__UnexpectedStateChange();

c. alternatively, implement checks to ensure sellers cannot use contracts as their payment receiving addresses.