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

Move construction checks from Escrow to EscrowFactory

hasanza

Summary

Better to catch and revert early in the EscrowFactory than to first do address computation of Escrow and then fail in Escrow constructor.

Vulnerability Details

The following tests are preformed in Escrow constructor:

if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
if (seller == address(0)) revert Escrow__SellerZeroAddress();
if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();

The values checked are all available in EscrowFactory.newEscrow() as well. As such, it is more gas efficient to check them in EscrowFactory.newEscrow() before an Escrow address is even computed.

Impact

Gas

Tools Used

Forge, Foundry Toolkit (gas report, gas snapshots)

Recommendation

Move the checks on Escrow constructor arguments to EscrowFactory.newEscrow(). This change saved a significant 135729 across all tests. As an example, the tests testRevertIfFeeGreaterThanPrice andtestSellerZeroReverts saved ~55000 gas each. The deployment cost for Escrow fell by 1158 gas. All of this will add up to notable gas savings in the long term.

Note that to achieve this, I moved all errors and the enum to a new IErrors interface file, and inherited that in the other two interfaces.

interface IErrors {
​
error Escrow__FeeExceedsPrice(uint256 price, uint256 fee);
error Escrow__OnlyBuyer();
error Escrow__OnlyBuyerOrSeller();
error Escrow__OnlyArbiter();
error Escrow__InWrongState(State currentState, State expectedState);
error Escrow__MustDeployWithTokenBalance();
error Escrow__TotalFeeExceedsBalance(uint256 balance, uint256 totalFee);
error Escrow__DisputeRequiresArbiter();
error Escrow__TokenZeroAddress();
error Escrow__BuyerZeroAddress();
error Escrow__SellerZeroAddress();
error EscrowFactory__AddressesDiffer();
​
enum State {
Created,
Confirmed,
Disputed,
Resolved
}
}

Changes also need to be made to tests so they access the enum and errors from IErrors directly.

Support

FAQs

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

Give us feedback!