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

Needless `ReentrancyGuard` wastes gas

cuthalion0x

Summary

The Escrow contract's Finite State Machine architecture makes ReentrancyGuard overkill and wastes both gas and bytecode.

Vulnerability Details

The Escrow#resolveDispute() function is marked nonReentrant, but it also utilizes the inState(State.Disputed) modifier and very quickly changes its state before making any state-changing external calls:

function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
// @audit There are no state-modifying external calls in the early part of this function body.
uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
uint256 totalFee = buyerAward + i_arbiterFee; // Reverts on overflow
if (totalFee > tokenBalance) {
revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
}
​
// @audit `s_state` changes here, meaning any reentrancy after this point will fail the check in `inState()`.
s_state = State.Resolved;
emit Resolved(i_buyer, i_seller);

Removing the nonReentrant modifier saves a non-trivial amount of gas and trims bytecode (thereby saving deployment costs) without any adverse consequences.

Impact

Unnecessary expenditure of gas.

Tools Used

Manual review.

Recommendations

Remove the nonReentrant modifier from Escrow#resolveDispute() and remove the import of OpenZeppelin's ReentrancyGuard entirely. This is the only usage.

Support

FAQs

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