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

Unnecessary comparison leading to waste of gas

Omsify

Summary

Unnecessary comparison leading to waste of gas

Vulnerability Details

In EscrowFactory::newEscrow(), in lines 48-50 the code performs a comparsion between the deployed Escrow address and precomputed Escrow address. The thing is, the addresses will never differ. This can be checked with a fuzz-test. This issue leads to both higher EscrowFactory contract deployment cost and EscrowFactory::newEscrow() function interaction cost.
POC (foundry fuzz test):

// EscrowFactoryComparsion.t.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.18;
​
import {Test, console} from "forge-std/Test.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {EscrowFactory} from "src/EscrowFactory.sol";
​
contract EscrowFactoryComparsion is Test {
ERC20Mock public immutable mockErc20;
EscrowFactory public immutable ef;
​
constructor() {
mockErc20 = new ERC20Mock();
ef = new EscrowFactory();
}
​
function testAddressCalculation(address buyer,
uint256 price,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt) public {
vm.assume(buyer!=address(0));
vm.assume(seller!=address(0));
vm.assume(arbiterFee<price);
mockErc20.mint(buyer, price);
vm.prank(buyer);
mockErc20.approve(address(ef), price);
vm.prank(buyer);
ef.newEscrow(price, mockErc20, seller, arbiter, arbiterFee, salt);
}
}

And in the foundry.toml file:

[fuzz]
runs = 50_000
fail_on_revert = true

I tested this with 50000 fuzz runs.

Impact

About 34 wasted gas units on EscrowFactory::newEscrow() interaction.

Tools Used

Foundry, fuzz-tests

Recommendations

Remove the unnecessary comparsion (lines 48-50 in EscrowFactory.sol)

Support

FAQs

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