Remove Non-Reentrant Modifier from Arbitrated Dispute Resolution Function

Severity

Gas Optimization / Informational

Relevant GitHub Links

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

Summary

In the Escrow.sol contract, the resolveDispute function is restricted to be called only by the onlyArbiter modifier, implying that the arbiter is trusted and will not execute reentrancy attacks. As such, the use of the nonReentrant modifier in this context is unnecessary and can be removed to save gas.

Vulnerability Details

The resolveDispute function in the Escrow.sol contract is designed to handle dispute resolution and distribute awards accordingly. It is guarded by the onlyArbiter modifier, which ensures that only an authorized arbiter can execute this function. Given that the arbiter is assumed to be trusted, the need for the nonReentrant modifier can be questioned, as it guards against reentrancy attacks, which are not expected from a trusted arbiter.

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

   function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant 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);

        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);
        }
    }

Cyfrin / 2023-07-escrow