CodeHawks Escrow Contract - Competition Details

1. STATE VARIABLES WHICH ARE READ MULTIPLE TIMES WITHIN A FUNCTION CAN BE CACHED AND READ FROM MEMORY

In the Escrow.resolveDispute contract there are multiple occasions in which state variables are used more than once. These state variables can be cached and used from the memory.

For example in the Escrow.resolveDispute the immutable state variable i_tokenContract is used five times inside the function scope. This requires five SLOAD operations separately. Where as i_tokenContract can be cached into a local variable and read from the memory. This will require one SLOAD and five MLOAD operations thus saving gas.

Similarly i_buyer, i_seller and i_arbiterFee are used 2 times each inside the function scope. Hence they can be cached and read from the memory to save gas as well.

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L120
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L123
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L125
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L127

2. > 0 CAN BE REPLACED WITH != 0

In the Escrow.resolveDispute function, the token amounts are checked within if statements as follows:

    if (buyerAward > 0) {

This can be replaced as follows to save gas:

    if (buyerAward != 0) {

There are two more instances of this issue inside the Escrow.resolveDispute function.

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

3. REDUNDANT nonReentrant MODIFIER CAN BE OMITTED

The Escrow.resolveDispute function is called by the arbiter to resolve the disputes. Once the dispute is settled the arbiter can transfer the funds to the respective addresses. To avoid reentrancy which could occur as a result of moving the execution context to an external contract nonreentrant modifier is used in the resolveDispute. But this modifier is a redundant check and can be omitted due to following two reasons.

1. Before the fund transfer occurs the `s_state` is changed to `State.Resolved` state. If an attacker tries to reenter the function the transaction will revert due to the `inState(State.Disputed)` modifier in the `resolveDispute` function.

2. It is clearly mentioned that `ERC777` should not be used as a `tokenContract`. Hence there is no possibility for `execution context` to be moved to an `external contract`. Hence reentrancy is not a possibility here.

Hence it is recommended to remove the nonreentrant modifier in the resolveDispute function to save gas.

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