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

A permanent revert on transfer among any of the participants in the resolveDispute function could lead to all funds being stuck in the contract.

0xCiphky

Summary

In the resolveDispute function of the Escrow contract, all funds are distributed in the same function among the participants using a push over pull method. One revert could lead to locking all funds in the worst case. To mitigate this, it is recommended to use a pull over push strategy and have a different function for each transfer.

Vulnerability Details

In the resolveDispute function of the Escrow contract, tokens could fail to transfer due to various reasons. For instance, the token of choice could:

  • Implement an admin-controlled address blocklist (e.g., USDC and USDT), which can cause a revert on the resolveDispute function. This could lead to funds getting locked in the contract, or necessitate sending funds to other users first to avoid getting stuck.

  • Have callbacks that allow malicious users to DOS dispute resolutions, as mentioned in the Known Issues section.

Consider the most adverse scenario:

The escrow has moved into the dispute stage, and the arbiter, who has set a fee greater than zero, is either blocked by the token in use or intentionally disrupts the transfer in a DoS attack. This would cause the transfer operation to consistently fail, leading to a situation where the function cannot execute successfully. As a result, all the funds will remain indefinitely locked within the contract.

Impact

Impact: High, as essential functionality in the protocol could fail.

Likelihood: Low, as a specific type of ERC20 token must be used and one of the above conditions must be met.

Tools Used

Manual analysis

Recommendations

To mitigate this issue, consider using a pull over push strategy to send tokens and separate each transfer. This can be implemented as follows:

Set the final values in the resolveDispute function:

//state variables
uint private buyerShare;
uint private sellerShare;
uint private arbiterShare;
​
function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
tokenBalance = i_tokenContract.balanceOf(address(this));
// existing code...
​
if (buyerAward > 0) {
buyerShare = buyerAward;
}
​
if (i_arbiterFee > 0) {
arbiterShare = i_arbiterFee
}
​
uint currTotal = buyerShare + arbiterShare
​
if (tokenBalance - currTotal > 0) {
sellerShare = tokenBalance - currTotal
}
}

Create individual claim functions for each user:

function buyerClaim() external inState(State.resolved) {
if (buyerShare > 0) {
i_tokenContract.safeTransfer(i_buyer, buyerShare);
buyerShare = 0;
}
}
function sellerClaim() external inState(State.resolved) {
if (sellerShare > 0) {
i_tokenContract.safeTransfer(i_seller, sellerShare);
sellerShare = 0
}
}
function arbiterClaim() external inState(State.resolved) {
if (arbiterShare > 0) {
i_tokenContract.safeTransfer(i_arbiter, arbiterShare);
arbiterShare = 0;
}
}

These changes will limit the scope of potential issues. If a transfer fails, it will only affect the single user, rather than all users. Additionally, it can help to mitigate the effects of a known issue where malicious users can DOS dispute resolutions.

Support

FAQs

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