QA Report

[code423n4]

QA Report

Non-Critical Issues

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/lib/OrderCombiner.sol:
 238:        uint256 elapsed = block.timestamp - startTime;

contracts/lib/Verifiers.sol:
  43:        if (startTime > block.timestamp || endTime <= block.timestamp) {

contracts/lib/OrderFulfiller.sol:
 164:        uint256 elapsed = block.timestamp - orderParameters.startTime;

Floating compiler versions

Non-library/interface files should use fixed compiler versions, not floating ones:

contracts/conduit/ConduitController.sol:
   2:        pragma solidity >=0.8.7;

contracts/conduit/Conduit.sol:
   2:        pragma solidity >=0.8.7;

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

contracts/interfaces/ConsiderationEventsAndErrors.sol:
  25:        event OrderFulfilled(
  26:                bytes32 orderHash,
  27:                address indexed offerer,
  28:                address indexed zone,
  29:                address fulfiller,
  30:                SpentItem[] offer,
  31:                ReceivedItem[] consideration
  32:            );

  41:        event OrderCancelled(
  42:                bytes32 orderHash,
  43:                address indexed offerer,
  44:                address indexed zone
  45:            );

  56:        event OrderValidated(
  57:                bytes32 orderHash,
  58:                address indexed offerer,
  59:                address indexed zone
  60:            );

[HardlyDifficult]

Use of block.timestamp

Using time is appropriate for this project. I don't see a viable alternative.

Floating compiler versions

Their config pins the version used, so this is a minor style preference. I often using floating personally so that when I upgrade versions the diff is just the config and therefore easy to review.

event is missing indexed fields

There is a cost to using indexed, so it should only be added where it will add value. It's not indicated which fields would be appropriate to index here.

Since none of these are clear wins, closing as invalid.

[GalloDaSballo]

Agree with judge in that the submission looks like a low effort automated tool output with no clear actionable advice

[IllIllI000]

Can you point to a specific tool that was used to generate this? If it is automated, it looks custom, and is therefore not low-effort - tools take a significant amount of time to write.

[GalloDaSballo]

Can you point to a specific tool that was used to generate this? If it is automated, it looks custom, and is therefore not low-effort - tools take a significant amount of time to write.

Finding 1 is copy paste from https://www.bookstack.cn/read/ethereumbook-en/spilt.14.c2a6b48ca6e1e33c.md#:~:text=Block%20timestamps%20have%20historically%20been,statements%20that%20are%20time%2Ddependent.

There is no nuance, no suggested remediation, no value to be gained except "eheh you used block.timestamp"

Finding 2 I've disagreed with per #67

Finding 3 I've historically disagreed, just check your own QAs, I always disputed without a reasoning behind it

The code can be copy pasted and setup via GithubLinker

[GalloDaSballo]

1 NC per discussion on #67