CodeHawks Escrow Contract - Competition Details

Cyfrin

Foundry

40,000 USDC

View results

Previous Next

Submission Details

Severity: gas

Valid

Gas

sma4

Summary for GAS findings

Number	Details	Instances
[G-01]	Functions guaranteed to revert when called by normal users can be marked payable	3
[G-02]	A modifier used only once and not being inherited should be inlined to save gas	3
[G-03]	Use hardcode address instead address(this)	5
[G-04]	Using unchecked blocks to save gas	1
[G-05]	Use functions instead of modifiers	1
[G-06]	Use assembly to emit events	4
[G-07]	If statements that use && can be refactored into nested if statements	1
[G-08]	Constructors can be marked payable	1
[G-09]	Use != 0 instead of > 0 for unsigned integer comparison	3
[G-10]	use assembly for address zero to save gas	4
[G-11]	Amounts should be checked for 0 before calling a transfer	5
[G-12]	abi.encode() is less efficient than abi.encodePacked()	1
[G-13]	Use solidity version 0.8.19 to gain some gas boost	4

[G-01] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier or require such as onlyOwner/onlyX is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

file: main/src/Escrow.sol

94 function confirmReceipt() external onlyBuyer inState(State.Created) {

102 function initiateDispute() external onlyBuyerOrSeller inState(State.Created) {

109 function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L94

[G-02] A modifier used only once and not being inherited should be inlined to save gas

file: main/src/Escrow.sol

58 modifier onlyBuyer() {

66 modifier onlyBuyerOrSeller() {

74 modifier onlyArbiter() {

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L58

[G-03] Use hardcode address instead address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry’s script.sol and solmate’s LibRlp.sol contracts can help achieve this.

References:

https://book.getfoundry.sh/reference/forge-std/compute-create-address
https://twitter.com/transmissions11/status/1518507047943245824

file: main/src/Escrow.sol

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

98 i_tokenContract.safeTransfer(i_seller, i_tokenContract.balanceOf(address(this)));

110 uint256 tokenBalance = i_tokenContract.balanceOf(address(this));

125 tokenBalance = i_tokenContract.balanceOf(address(this));

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L44

file: main/src/EscrowFactory.sol

30 address(this),

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L30

[G-04] Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isnâ€™t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

file: main/src/Escrow.sol

111 uint256 totalFee = buyerAward + i_arbiterFee;

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L111

[G-05] Use functions instead of modifiers

Modifiers:
Modifiers are used to add pre or post-conditions to functions. They are typically applied using the modifier keyword and can be used to enforce access control, check conditions, or perform other checks before or after a function's execution. Modifiers are often used to reduce code duplication and enforce certain rules consistently across multiple functions.

However, using modifiers can lead to increased gas costs, especially when multiple function are applied to a single modifier. Each modifier adds overhead in terms of gas consumption because its code is effectively inserted into the function at the point of the modifier keyword. This results in additional gas usage for each modifier, which can accumulate if multiple modifiers are involved.

Functions:
Functions, on the other hand, are more efficient in terms of gas consumption. When you define a function, its code is written only once and can be called from multiple places within the contract. This reduces gas costs compared to using modifiers since the function's code is not duplicated every time it's called.

By using functions instead of modifiers, you can avoid the gas overhead associated with multiple modifiers and create more gas-efficient smart contracts. If you find that certain logic needs to be applied to multiple functions, you can write a separate function to encapsulate that logic and call it from the functions that require it. This way, you achieve code reusability without incurring unnecessary gas costs.

Let's illustrate the difference between using modifiers and functions with a simple example.

Using Modifiers:

pragma solidity ^0.8.0;

contract BankAccountUsingModifiers {

address public owner;

uint public balance;

modifier onlyOwner() {

require(msg.sender == owner, "Only the owner can perform this action.");

}

constructor() {

owner = msg.sender;

}

function withdraw(uint amount) public onlyOwner {

require(amount <= balance, "Insufficient balance.");

balance -= amount;

// Perform withdrawal logic

}

function transfer(address recipient, uint amount) public onlyOwner {

require(amount <= balance, "Insufficient balance.");

balance -= amount;

// Perform transfer logic

}

Using Functions:

pragma solidity ^0.8.0;

contract BankAccountUsingFunctions {

address public owner;

uint public balance;

constructor() {

owner = msg.sender;

}

function onlyOwner() private view returns (bool) {

return msg.sender == owner;

}

function withdraw(uint amount) public {

require(onlyOwner(), "Only the owner can perform this action.");

require(amount <= balance, "Insufficient balance.");

balance -= amount;

// Perform withdrawal logic

}

function transfer(address recipient, uint amount) public {

require(onlyOwner(), "Only the owner can perform this action.");

require(amount <= balance, "Insufficient balance.");

balance -= amount;

// Perform transfer logic

}

In the first contract (BankAccountUsingModifiers), we used a modifier called onlyOwner to check if the caller is the owner of the bank account. The onlyOwner modifier is applied to both the withdraw and transfer functions to ensure that only the owner can perform these actions. However, using the modifier will result in additional gas costs since the modifier's code is duplicated in both functions.

In the second contract (BankAccountUsingFunctions), we replaced the modifier with a private function called onlyOwner, which checks if the caller is the owner. We then call this function at the beginning of both the withdraw and transfer functions. By doing this, we avoid the gas overhead associated with using modifiers, making the contract more gas-efficient.

file: main/src/Escrow.sol

82 modifier inState(State expectedState)

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L82

[G-06] Use assembly to emit events

We can use assembly to emit events efficiently by utilizing scratch space and the free memory pointer. This will allow us to potentially avoid memory expansion costs.

Note: In order to do this optimization safely, we will need to cache and restore the free memory pointer.

file: main/src/Escrow.sol

96 emit Confirmed(i_seller);

105 emit Disputed(msg.sender);

117 emit Resolved(i_buyer, i_seller);

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L96

file: main/src/EscrowFactory.sol

51 emit EscrowCreated(address(escrow), msg.sender, seller, arbiter);

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L51

[G-07] If statements that use && can be refactored into nested if statements

file: main/src/Escrow.sol

67 if (msg.sender != i_buyer && msg.sender != i_seller) {

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L67

[G-08] Constructors can be marked payable

saved gas 21

file: main/src/Escrow.sol

32 constructor(

uint256 price,

IERC20 tokenContract,

address buyer,

address seller,

address arbiter,

uint256 arbiterFee

) {

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();

i_price = price;

i_tokenContract = tokenContract;

i_buyer = buyer;

i_seller = seller;

i_arbiter = arbiter;

i_arbiterFee = arbiterFee;

}

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L32-L51

[G-09] Use != 0 instead of > 0 for unsigned integer comparison

file: main/src/Escrow.sol

119 if (buyerAward > 0)

122 if (i_arbiterFee > 0)

126 if (tokenBalance > 0)

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L112

[G-10] use assembly for address zero to save gas

file: main/src/Escrow.sol

40 if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();

41 if (buyer == address(0)) revert Escrow__BuyerZeroAddress();

42 if (seller == address(0)) revert Escrow__SellerZeroAddress();

103 if (i_arbiter == address(0)) revert Escrow__DisputeRequiresArbiter();

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L40

[G-11] Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.
While this is done at some places, it’s not consistently done in the solution.
I suggest adding a non-zero-value check here:

file: main/src/Escrow.sol

98 i_tokenContract.safeTransfer(i_seller, i_tokenContract.balanceOf(address(this)));

120 i_tokenContract.safeTransfer(i_buyer, buyerAward);

123 i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);

127 i_tokenContract.safeTransfer(i_seller, tokenBalance);

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L98

file: main/src/EscrowFactory.sol

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

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/EscrowFactory.sol#L39