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

[G/I] ReentrancyGuard is redundant and can be removed

BAHOZ

Summary

ReentrancyGuard is redundant and can be removed.

Vulnerability Details

resolveDispute() makes external calls to the tokenContract, although the tokens that will be used in the contract supposed to be trusted and safe by contest docs, resolveDispute() uses nonReentrant. This causes inconsistency and redundancy and it is inefficient.

Since the same function uses inState() modifier, moving the following line up to L110 will have the same effect:
https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L116

Impact

Inconsistency, gas inefficiency.

Tools Used

Manual Review

Recommendations

  • Remove ReentrancyGuard and move https://github.com/Cyfrin/2023-07-escrow/blob/65a60eb0773803fa0be4ba72defaec7d8567bccc/src/Escrow.sol#L116
    to the L:110

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

Support

FAQs

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