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

QA Report (Low & Informational Severity Findings)

Rolezn

QA Summary

Low Risk Issues

Issue Contexts
LOW‑1 Large transfers may not work with some ERC20 tokens 1
LOW‑2 Contracts are not using their OZ Upgradeable counterparts 5
LOW‑3 Use SafeCast to safely downcast variables 1
LOW‑4 Lack of input validation in constructor 1

Total: 7 contexts over 3 issues

Non-critical Issues

Issue Contexts
NC‑1 Empty bytes check is missing 2
NC‑2 Generate perfect code headers for better solidity code layout and readability 2
NC‑3 Lines are too long 6
NC‑4 NatSpec comments should be increased in contracts 1
NC‑5 NatSpec @param is missing 1
NC‑6 NatSpec @return argument is missing 1
NC‑7 The nonReentrant modifier should occur before all other modifiers 1
NC‑8 Use the latest solidity (prior to 0.8.20 if on L2s) for deployment 2
NC‑9 Use @inheritdoc rather than using a non-standard annotation 1

Total: 17 contexts over 9 issues

Low Risk Issues

[LOW‑1] Large transfers may not work with some ERC20 tokens

Some IERC20 implementations (e.g UNI, COMP) may fail if the valued transferred is larger than uint96. Source
The documentation does not mention if there are any specific limits to specific ERC20 tokens, as it stands, it seems to support all types of ERC20 tokens

Proof Of Concept

File: EscrowFactory.sol
​
39: tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
​

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

[LOW‑2] 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.

Proof of Concept

File: Escrow.sol
​
7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
8: import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
9: import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​

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

File: EscrowFactory.sol
​
7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
8: import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​

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

Recommended Mitigation Steps

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for list of available upgradeable contracts

[LOW‑3] Use SafeCast to safely downcast variables

Downcasting int/uints in Solidity can be unsafe due to the potential for data loss and unintended behavior. When downcasting a larger integer type to a smaller one (e.g., uint256 to uint128), the value may exceed the range of the target type, leading to truncation and loss of significant digits. This data loss can result in unexpected state changes, incorrect calculations, or other contract vulnerabilities, ultimately compromising the contracts functionality and reliability. To prevent these risks, developers should carefully consider the range of values their variables may hold and ensure that proper checks are in place to prevent out-of-range values before performing downcasting. Also consider using OZ SafeCast functionality.

Proof Of Concept

File: EscrowFactory.sol
​
68: uint160(
​

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

[LOW‑4] Lack of input validation in constructor

The constructor is missing input validation for: arbiter and arbiterFee

The current constructor already checks for the following inputs but fails to check on the missing ones:

if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
if (seller == address(0)) revert Escrow__SellerZeroAddress();

Proof Of Concept

File: 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/tree/main/src/Escrow.sol#L32

Non Critical Issues

[NC‑1] Empty bytes check is missing

To avoid mistakenly accepting empty bytes as a valid value, consider to check for empty bytes. This helps ensure that empty bytes are not treated as valid inputs, preventing potential issues.

Proof Of Concept

File: EscrowFactory.sol
​
21: function newEscrow(
uint256 price,
IERC20 tokenContract,
address seller,
address arbiter,
uint256 arbiterFee,
bytes32 salt
) external returns (IEscrow) {

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

File: EscrowFactory.sol
​
57: function computeEscrowAddress(
bytes memory byteCode,
address deployer,
uint256 salt,
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) public pure returns (address) {

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

[NC‑2] Generate perfect code headers for better solidity code layout and readability

It is recommended to use pre-made headers for Solidity code layout and readability:
https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
TESTING 123
//////////////////////////////////////////////////////////////*/

Proof Of Concept

File: Escrow.sol
​
6: Escrow.sol
​

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

File: EscrowFactory.sol
​
4: EscrowFactory.sol
​

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

[NC‑3] Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Proof Of Concept

File: Escrow.sol
​
4: // Inspired by `BillOfSaleERC20` contract: https://github.com/open-esq/Digital-Organization-Designs/blob/master/Finance/BillofSaleERC20.sol

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

File: Escrow.sol
​
28: /// @dev Sets the Escrow transaction values for `price`, `tokenContract`, `buyer`, `seller`, `arbiter`, `arbiterFee`. All of

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

File: Escrow.sol
​
29: /// these values are immutable: they can only be set once during construction and reflect essential deal terms.

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

File: Escrow.sol
​
30: /// @dev Funds should be sent to this address prior to its deployment, via create2. The constructor checks that the tokens have

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

File: EscrowFactory.sol
​
17: /// @dev msg.sender must approve the token contract to spend the price amount before calling this function.

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

File: EscrowFactory.sol
​
55: /// @dev See https://docs.soliditylang.org/en/latest/control-structures.html#salted-contract-creations-create2

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

[NC‑4] NatSpec comments should be increased in contracts

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability. https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

Proof Of Concept

All in-scope contracts

Recommended Mitigation Steps

NatSpec comments should be increased in contracts

[NC‑5] NatSpec @param is missing

Proof Of Concept

File: EscrowFactory.sol
​
55: /// @dev See https://docs.soliditylang.org/en/latest/control-structures.html#salted-contract-creations-create2
function computeEscrowAddress(
bytes memory byteCode,
address deployer,
uint256 salt,
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) public pure returns (address) {
​

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

[NC‑6] NatSpec @return argument is missing

Proof Of Concept

File: EscrowFactory.sol
​
55: /// @dev See https://docs.soliditylang.org/en/latest/control-structures.html#salted-contract-creations-create2
function computeEscrowAddress(
bytes memory byteCode,
address deployer,
uint256 salt,
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) public pure returns (address) {
​

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

[NC‑7] The nonReentrant modifier should occur before all other modifiers

Currently the nonReentrant modifier is not the first to occur, it should occur before all other modifiers.

Proof Of Concept

File: Escrow.sol
​
109: function resolveDispute(uint256 buyerAward) external onlyArbiter nonReentrant inState(State.Disputed) {

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

Recommended Mitigation Steps

Re-sort modifiers so that the nonReentrant modifier occurs first.

[NC‑8] Use the latest solidity (prior to 0.8.20 if on L2s) for deployment

When deploying contracts, you should use the latest released version of Solidity. Apart from exceptional cases, only the latest version receives security fixes.

0.8.19:
SMTChecker: New trusted mode that assumes that any compile-time available code is the actual used code, even in external calls.
Bug Fixes:

  • Assembler: Avoid duplicating subassembly bytecode where possible.

  • Code Generator: Avoid including references to the deployed label of referenced functions if they are called right away.

  • ContractLevelChecker: Properly distinguish the case of missing base constructor arguments from having an unimplemented base function.

  • SMTChecker: Fix internal error caused by unhandled z3 expressions that come from the solver when bitwise operators are used.

  • SMTChecker: Fix internal error when using the custom NatSpec annotation to abstract free functions.

  • TypeChecker: Also allow external library functions in using for.

0.8.20:

  • Assembler: Use push0 for placing 0 on the stack for EVM versions starting from “Shanghai”. This decreases the deployment and runtime costs.

  • EVM: Set default EVM version to “Shanghai”.

  • EVM: Support for the EVM Version “Shanghai”.

  • Optimizer: Re-implement simplified version of UnusedAssignEliminator and UnusedStoreEliminator. It can correctly remove some unused assignments in deeply nested loops that were ignored by the old version.

  • Parser: Unary plus is no longer recognized as a unary operator in the AST and triggers an error at the parsing stage (rather than later during the analysis).

  • SMTChecker: Group all messages about unsupported language features in a single warning. The CLI option --model-checker-show-unsupported and the JSON option settings.modelChecker.showUnsupported can be enabled to show the full list.

  • SMTChecker: Properties that are proved safe are now reported explicitly at the end of analysis. By default, only the number of safe properties is shown. The CLI option --model-checker-show-proved-safe and the JSON option settings.modelChecker.showProvedSafe can be enabled to show the full list of safe properties.

  • Standard JSON Interface: Add experimental support for importing ASTs via Standard JSON.

  • Yul EVM Code Transform: If available, use push0 instead of codesize to produce an arbitrary value on stack in order to create equal stack heights between branches.

Proof Of Concept

File: Escrow.sol
​
pragma solidity 0.8.18;

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

File: EscrowFactory.sol
​
pragma solidity 0.8.18;

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

Recommended Mitigation Steps

Consider updating to a more recent solidity version.

[NC‑9] Use @inheritdoc rather than using a non-standard annotation

Proof Of Concept

File: EscrowFactory.sol
​
55: /// @dev See https://docs.soliditylang.org/en/latest/control-structures.html#salted-contract-creations-create2
function computeEscrowAddress(
bytes memory byteCode,
address deployer,
uint256 salt,
uint256 price,
IERC20 tokenContract,
address buyer,
address seller,
address arbiter,
uint256 arbiterFee
) public pure returns (address) {
​

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

Support

FAQs

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