Josiah data for issue #209

data/Josiah-Q.md

@@ -0,0 +1,75 @@
+Report contents changed: ## USE OF BLOCK.TIMESTAMP
+Some contract code uses the block.timestamp as part of the calculations and time checks. Nevertheless, timestamps can be slightly altered by miners/validators to favor them in contracts that have logic that depends strongly on them.
+
+Consider taking into account this issue and warning the users that such a scenario could happen. If the alteration of timestamps cannot affect the protocol in any way, consider documenting the reasoning and writing tests enforcing that these guarantees will be preserved even if the code changes in the future. Here are the two instances found.
+
+[Exchange.sol#L377-L378](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L377-L378)
+
+```
+            (order.listingTime < block.timestamp) &&
+            (block.timestamp < order.expirationTime)
+```
+## COMMENT AND CODE LINE LENGTH
+Try limit the length of comments and/or code lines to 80 - 100 character long for readability sake. Here is one instance found.
+
+[Exchange.sol#L230](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L230)
+
+## TIMELOCK FOR CRITICAL PARAMETER CHANGE
+It is a good practice giving time to users to react and adjust to critical changes with a mandatory time window between the changes. The first step is simply broadcasting to users with a specific change that is coming whilst the second step commits that change after an appropriate period of waiting. This would allow time for users opposing to the change to withdraw within the set time frame. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of the owner making a malicious act). Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes.
+
+It is recommended extending the timelock feature to all business critical functions instead of just the contract ownership management. Here are the four instances found.
+
+[Exchange.sol#L323-L356](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L323-L356)
+
+```
+    function setExecutionDelegate(IExecutionDelegate _executionDelegate)
+        external
+        onlyOwner
+    {
+        require(address(_executionDelegate) != address(0), "Address cannot be zero");
+        executionDelegate = _executionDelegate;
+        emit NewExecutionDelegate(executionDelegate);
+    }
+
+    function setPolicyManager(IPolicyManager _policyManager)
+        external
+        onlyOwner
+    {
+        require(address(_policyManager) != address(0), "Address cannot be zero");
+        policyManager = _policyManager;
+        emit NewPolicyManager(policyManager);
+    }
+
+    function setOracle(address _oracle)
+        external
+        onlyOwner
+    {
+        require(_oracle != address(0), "Address cannot be zero");
+        oracle = _oracle;
+        emit NewOracle(oracle);
+    }
+
+    function setBlockRange(uint256 _blockRange)
+        external
+        onlyOwner
+    {
+        blockRange = _blockRange;
+        emit NewBlockRange(blockRange);
+    }
+```
+## MISSING NATSPEC
+As documented in the link below:
+
+https://docs.soliditylang.org/en/v0.8.16/natspec-format.html
+
+It is recommended using a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more.
+
+Here are some of the instances found.
+
+Missing Natspec in its entirety:
+
+[Exchange.sol#L323-L356](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L323-L356)
+
+Missing `* @param amount`:
+
+[Exchange.sol#L645-L667](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L645-L667)