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

[G‑02] ReentrancyGuard can be safely removed

Arz

All optimizations were benchmarked using the protocol's tests using the following config: solc version 0.8.18, optimizer on, 200 runs, viaIR = true. In most cases forge test --gas-report was used

Each optimization was submitted individually.

Gas Optimizations

Issue Instances Total Gas Saved
[G‑01] Consider using clones * -70% cheaper deployment gas
[G‑02] ReentrancyGuard can be safely removed 1 42725
[G‑03] computeEscrowAddress() can be internal instead of public 1 55863 + 193
[G‑04] Redundant zero address checks 2 237
[G‑05] Input validation should be done in the beginning 2 110649(in the revert case)
[G‑06] Emit after the transfer has been made in case it fails 2 1381(in the revert case)
[G‑07] The bytecode can be removed from the function params 1 27
[G‑08] Nested ifs are cheaper than && 1 19

[G‑02] ReentrancyGuard can be safely removed

The resolveDispute() has a reentrancy guard just in case a token had a callback function and wanted to reenter the contract. However, because CEI is followed the nonReentrant and the reentrancy guard only wastes gas here(both deployment and runtime).

Deployment Gas Savings for Escrow.sol obtained via protocol's tests(not using newEscrow): 40525 gas
Before 591900
After 551375

The runtime gas savings for resolveDispute() couldnt be calculated with foundry because it warms up the slots in the tests but when using Remix the gas savings were about 2200 gas

There is 1 instance of this issue:

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

File: Escrow.sol
​
109: function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {
110: uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
111: uint256 totalFee = buyerAward + i_arbiterFee; // Reverts on overflow
112: if (totalFee > tokenBalance) {
113: revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
114: }
115:
116: s_state = State.Resolved;
117: emit Resolved(i_buyer, i_seller);
118:
119: if (buyerAward > 0) {
120: i_tokenContract.safeTransfer(i_buyer, buyerAward);
121: }
122: if (i_arbiterFee > 0) {
123: i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
124: }
125: tokenBalance = i_tokenContract.balanceOf(address(this));
126: if (tokenBalance > 0) {
127: i_tokenContract.safeTransfer(i_seller, tokenBalance);
128: }
129: }
​

As you can see here the state is set to State.Resolved before the transfer is made so if the transfer wanted to reenter then the inState() modifier would revert because the state is no longer State.Disputed. The reentrancy guard can be safely removed here.

Support

FAQs

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