40,000 USDC
View results
Submission Details
Severity: gas
Valid

Check price != 0 before interacting with IERC20

Summary

When deploying a new Escrow via the factory, we can check if the set price is not 0 before interacting with the IERC20 via external calls. This ultimately saves the user some gas.

Vulnerability Details

As it stands, the factory contract performs the token transfer regardless of whether the price is 0 or not. There are currently no checks to ensure that price is always greater than 0, so it can be assumed that a price of 0 is a legal choice (please see my other finding that states a logic flaw resulting in a DoS for the user).

If price is set to 0, the IERC20 interaction will still occur and ultimately waste some gas for the caller. There are 2 interactions during escrow creation that can be tweaked to reduce the overall cost:

EscrowFactory.sol:newEscrow()

tokenContract.safeTransferFrom(msg.sender, computedAddress, price);

Escrow.sol:constructor()

if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();

A quick 'forge test --gas-report' shows the following (extracted from the overall results) for the code before the patch:

| src/EscrowFactory.sol:EscrowFactory contract | | | | | |
|----------------------------------------------|-----------------|--------------------|--------|---------------------|---------|
| Deployment Cost | Deployment Size | | | | |
| 1720216 | 8620 | | | | |
| Function Name | min | avg | median | max | # calls |
| computeEscrowAddress | 11014 | 11014 | 11014 | 11014 | 2 |
| newEscrow | 12324 | 288303014855889986 | 627054 | 8937393460515961103 | 31 |

Impact

Additional gas costs occur even if the price is set to 0. These can be avoided.

Tools Used

  • VS Code

  • Foundry

  • Manually reading

Recommendations

This can be addressed by simply wrapping the two identified called with a "not zero" check. In some cases, using != instead of > for the 0 check can also be more efficient. These have been used in this remediation example:

EscrowFactory.sol:newEscrow()

if (price != 0) {
tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
}

Escrow.sol:constructor()

if (price != 0){
if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
}

The following shows the same gas report after the patch, as can be seen, the majority of results are reduced:

| src/EscrowFactory.sol:EscrowFactory contract | | | | | |
|----------------------------------------------|-----------------|--------------------|--------|---------------------|---------|
| Deployment Cost | Deployment Size | | | | |
| 1726423 | 8651 | | | | |
| Function Name | min | avg | median | max | # calls |
| computeEscrowAddress | 11014 | 11014 | 11014 | 11014 | 2 |
| newEscrow | 12349 | 288303014855889527 | 627114 | 8937393460515961046 | 31 |

Support

FAQs

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