Summary for LOW findings

[L‑01] Some tokens may revert when zero value transfers are made

In spite of the fact that EIP-20 states that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.

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

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

[L‑02] Consider implementing two-step procedure for updating protocol addresses

A copy-paste error or a typo may end up bricking protocol functionality, or sending tokens to an address with no known private key. Consider implementing a two-step procedure for updating protocol addresses, where the recipient is set as pending, and must 'accept' the assignment by making an affirmative call. A straight forward way of doing this would be to have the target contracts implement EIP-165, and to have the 'set' functions ensure that the recipient is of the right interface type.

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

[L‑03] Functions calling contracts/addresses with transfer hooks are missing reentrancy guards

Even if the function follows the best practice of check-effects-interaction, not using a reentrancy guard when there may be transfer hooks will open the users of this protocol up to read-only reentrancies with no way to protect against it, except by block-listing the whole protocol.

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

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

[L-04] Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts. See this for list of available upgradeable contracts

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

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

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

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

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

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

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

[L-05] Constructors contains no validation

In Solidity, when values are being assigned in constructors to unsigned or integer variables, it's crucial to ensure the provided values adhere to the protocol's specific operational boundaries as laid out in the project specifications and documentation. If the constructors lack appropriate validation checks, there's a risk of setting state variables with values that could cause unexpected and potentially detrimental behavior within the contract's operations, violating the intended logic of the protocol. This can compromise the contract's security and impact the maintainability and reliability of the system. In order to avoid such issues, it is recommended to incorporate rigorous validation checks in constructors. These checks should align with the project's defined rules and constraints, making use of Solidity's built-in require function to enforce these conditions. If the validation checks fail, the require function will cause the transaction to revert, ensuring the integrity and adherence to the protocol's expected behavior.

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

[L-06] Emitting storage values instead of the memory one

Emitted values should not be read from storage again. Instead, the existing values from memory should be used.

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

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