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

[G‑06] Emit after the transfer has been made in case it fails

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‑06] Emit after the transfer has been made in case it fails

In confirmReceipt() and resolveDispute() an event is emitted right before a transfer has been made. It would be better to emit the event after the transfer because the transfer can easily fail and you would have to pay for everything before the REVERT opcode was hit so you would have to pay for emitting that event.

Gas Savings for confirmReceipt() obtained via protocol's tests: avg 803 gas in the revert case where the transfer fails

AVG MED MAX
Before 18329 26017 26237
After 17526 24874 25094

Gas Savings for resolveDispute() obtained via protocol's tests: avg 578 gas in the revert case where the transfer fails

AVG MED MAX
Before 8249 3915 45127
After 7671 3146 45118

There are 2 instances of this issue:

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

File: Escrow.sol
​
94: function confirmReceipt() external onlyBuyer inState(State.Created) {
95: s_state = State.Confirmed;
96: emit Confirmed(i_seller);
97:
98: i_tokenContract.safeTransfer(i_seller, i_tokenContract.balanceOf(address(this)));
99: }
​
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: }
​

The events are emitted before the transfer has been made, however if the transfer fails then you would have to pay for emitting that event so its better to emit on the end in case the tx reverts

Support

FAQs

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