Cyfrin / 2023-07-escrow

17 stars 12 forks source link

Ensuring Fair Dispute Resolution: Modifying resolveDispute for Automated Award Calculation and Recipient Flexibility #845

Closed codehawks-bot closed 11 months ago

codehawks-bot commented 11 months ago

Ensuring Fair Dispute Resolution: Modifying resolveDispute for Automated Award Calculation and Recipient Flexibility

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L109

Summary

The existing resolveDispute function only provides the option to return funds to the buyer, which could potentially be unfair to the seller. A more balanced approach would be to allow the arbiter to decide who should receive the funds and to automatically calculate the amount based on the contract balance and the predefined arbiter fee.

Vulnerability Details

In the resolveDispute function, the arbiter has the option to return funds to the buyer but not the seller. This could potentially lead to unfair outcomes.

function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
    ...
}

This design could potentially exclude valid scenarios where the seller should receive the funds. It also places the responsibility of calculating the correct award on the arbiter, which could lead to potential errors or misuse.

Impact

The current implementation could lead to unfair dispute resolution outcomes and could potentially result in loss of funds for the seller.

Tools Used

This potential vulnerability was found using manual code review methods.

Recommendations

I recommend modifying the resolveDispute function to allow the arbiter to decide who the recipient of the funds should be, either the buyer or the seller, and to automatically calculate the amount to be awarded.

Here is a potential implementation:

modifier onlyBuyerOrSeller() {
    require(msg.sender == i_buyer || msg.sender == i_seller, "Sender must be either the buyer or seller");
    _;
}

function resolveDispute(address recipient) external onlyArbiter onlyBuyerOrSeller nonReentrant inState(State.Disputed) {
    uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
    uint256 totalFee = tokenBalance - i_arbiterFee; // Reverts on underflow

    s_state = State.Resolved;
    emit Resolved(i_buyer, i_seller);

    if (i_arbiterFee > 0) {
        i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
    }
    if (totalFee > 0) {
        i_tokenContract.safeTransfer(recipient, totalFee);
    }
}

In this version, the function uses a onlyBuyerOrSeller modifier to ensure that the recipient of the funds is either the buyer or the seller. This ensures a fairer dispute resolution process and reduces the chance of errors in the distribution of funds.