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

Unreachable and gas inefficient checks

schnilch

Some of the checks in the constructor of the Escrow Contract are unreachable:

  1. 40: if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress(); can never be reached because there would already have been a revert in the EscrowFactory Contract since tokens from this address are already transferred.

  2. 41: if (buyer == address(0)) revert Escrow__BuyerZeroAddress(); cannot be reached because buyer in EscrowFactory is always passed as msg.sender, and msg.sender cannot be the zero address.

Recommendation:

Move all the checks to the EscrowFactory Contract, as it also saves gas when all the checks are performed before deploying a new Escrow Contract.

File: src/EscrowFactory
20: function newEscrow(
21: uint256 price,
22: IERC20 tokenContract,
23: address seller,
24: address arbiter,
25: uint256 arbiterFee,
26: bytes32 salt
27: ) external returns (IEscrow) {
28: if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress(); //New Checks in EscrowFactory
29: if (seller == address(0)) revert Escrow__SellerZeroAddress();
30: if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
31: address computedAddress = computeEscrowAddress(
32: type(Escrow).creationCode,
33: address(this),
34: uint256(salt),
35: price,
36: tokenContract,
37: msg.sender,
38: seller,
39: arbiter,
40: arbiterFee
41: );
42: tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
43: if (tokenContract.balanceOf(computedAddress) < price) revert Escrow__MustDeployWithTokenBalance(); //Can only be checked after the transfer of tokens
44: Escrow escrow = new Escrow{salt: salt}(
45: price,
46: tokenContract,
47: msg.sender,
48: seller,
49: arbiter,
50: arbiterFee
51: );
52: if (address(escrow) != computedAddress) {
53: revert EscrowFactory__AddressesDiffer();
54: }
55: emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);
56: return escrow;
57: }

The buyer address does not need to be checked as it is always msg.sender.
Please note: The custom errors needed for the checks must be copied from IEscrow.sol to IEscrowFactory.sol.
Required error messages from IEscrow.sol:

8: error Escrow__FeeExceedsPrice(uint256 price, uint256 fee); //Required for new checks
9: error Escrow__OnlyBuyer();
10: error Escrow__OnlyBuyerOrSeller();
11: error Escrow__OnlyArbiter();
12: error Escrow__InWrongState(State currentState, State expectedState);
13: error Escrow__MustDeployWithTokenBalance(); //Required for new checks
14: error Escrow__TotalFeeExceedsBalance(uint256 balance, uint256 totalFee);
15: error Escrow__DisputeRequiresArbiter();
16: error Escrow__TokenZeroAddress(); //Required for new checks
17: error Escrow__BuyerZeroAddress(); //Can be deleted completely because the associated check no longer exists
18: error Escrow__SellerZeroAddress(); //Required for new checks

Gas Savings

Gas saving when attempting to deploy a new Escrow Contract with the seller's address as the zero address.

Gas consumption of the described scenario with the current checks in the constructor:
src/EscrowFactory.sol:EscrowFactory contract
Function Name min avg median max # calls
newEscrow 76396 76396 76396 76396 1
Gas consumption of the described scenario with the proposed checks in EscrowFactory:
src/EscrowFactory.sol:EscrowFactory contract
Function Name min avg median max # calls
newEscrow 410 410 410 410 1

In this scenario, 75986 gas could be saved. Similar amounts of gas can be saved if other checks revert since the checks are performed before executing the code to deploy the Escrow Contracts.

Support

FAQs

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