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

If token has hooks `buyer` or `seller` could trigger a DOS on `resolveDispute`

LaScaloneta

Summary

The current implementation of the Escrow contract is vulnerable to a Denial of Service (DoS) attack. This potential issue arises from the fact that if the token being used is an ERC-777 (or another token standard with hooks), the seller or buyer could implement a hook to make a contract call that reverts. This would prevent the resolveDispute function from transferring the tokens and the state from being updated to Resolved, leaving the contract in a stuck Disputed state.

Vulnerability Details

This issue occurs in the resolveDispute function. After the state has been updated to Resolved and the Resolved event emitted, the contract attempts to transfer the buyerAward and the arbiterFee, and then the remainder of the balance to the seller.

If the token being used has hooks (like an ERC-777 token), the buyer or seller could implement a hook that reverts when i_tokenContract.safeTransfer is called. This would revert the entire resolveDispute call, preventing the contract's state from being updated and leaving the contract in the Disputed state.

function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
...
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);
}
}

Impact

A malicious buyer or seller could intentionally block the resolution of disputes. This prevents funds from being distributed correctly and leaves the contract in a permanently disputed state.

Tools Used

This analysis was performed using manual review of the provided code snippet.

Recommendations

Consider using a pull payment system for the resolution of disputes. This would separate the resolution of the dispute from the transfer of funds. When a dispute is resolved, the contract would record how much each party is entitled to. Each party would then be able to withdraw their funds using a separate function. This would prevent a malicious party from blocking the resolution of disputes, as the transfer of funds would no longer be part of the dispute resolution transaction.

Here is a sample implementation of such a pattern:

mapping(address => uint256) public balances;
​
function resolveDispute(uint256 buyerAward) external onlyArbiter inState(State.Disputed) {
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);
​
balances[i_buyer] = buyerAward;
balances[i_arbiter] = i_arbiterFee;
balances[i_seller] = tokenBalance - totalFee;
}
​
function withdraw() external {
uint256 amount = balances[msg.sender];
require(amount > 0, "Nothing to withdraw");
balances[msg.sender] = 0;
i_tokenContract.safeTransfer(msg.sender, amount);
}

This implementation mitigates the DoS vulnerability by separating dispute resolution from the transfer of tokens. It makes the resolveDispute function only responsible for updating the internal balances state, and introduces a new withdraw function that users can call to retrieve their awarded funds.

Support

FAQs

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