add security review

security-review/lightchaser/v0.1.0-response-04-20-2024.md

@@ -0,0 +1,67 @@
+# Light Chaser V3 Security Review Response
+
+## Low
+
+### 1. Solidity version 0.8.20 won't work on all chains due to PUSH0
+
+Acknowledged. ERC721 uses `mcopy`, so 0.8.25 is required, as is EVM Cancun.
+
+## NonCritical
+
+### 1. Floating pragma should be avoided
+
+This is a library rather than a final contract, so floating pragma is preferred.
+
+### 2. Explicitly define visibility of functions to prevent misconceptions on what can access the function
+
+These are only free functions, so visibility is disallowed by the compiler.
+
+### 3. Assembly block creates dirty bits
+
+The lack of 'memory-safe' modifiers to assembly blocks is due to overwriting the upper bits of the free memory pointer.
+
+The upper bits are restored to zero but truth be told, I have no idea if this techincally violates the memory-safe ruleset because the documentation does not specify this edge case and memory-safe violations is undefined behavior. So in abundance of caution, memory-safe will only be used where it is guaranteed to not violate the ruleset.
+
+### 4. .call bypasses function existence check, type checking and argument packing
+
+There are nonzero returndatasize checks on calls to validate the contract's existence (except erc20 due to USDT). There is some indirection here by deferring checks for gas optimization though.
+
+In either case, acknowledged.
+
+### 5. Missing events in sensitive functions
+
+This only triggered once and most functions are technically stateful in that an external call is made, though the types never mutate local storage. This may be a false positive.
+
+### 6. All verbatim blocks are considered identical by deduplicator and can incorrectly be unified
+
+There is no verbatim block I'm aware of.
+
+### 7. Avoid using 'owner' or '\_owner' as a parameter name
+
+Good point, though `owner` in most cases is a reference to the specification. Will review.
+
+### 8. Memory-safe annotation missing
+
+These hits appear to be on assembly blocks with no memory access. Is this still required without memory accesses? The documentation doesn't clearly say one way or the other.
+
+## Gas
+
+### 1. Assembly let var only used on once
+
+There are a few cases of this that went unnoticed, though the optimizer inlines these variables.
+
+However, it's worth noting that the hit this tool generated points to a free memory pointer access before overwriting it. ie the following procedures _must_ be in this exact order and the variable _must_ be assigned if only once for the sake of restoring the free memory pointer. It may be worth adding a check to handle cases where the expressions between the variable assignment and variable usage affect the data location from which the variable originates.
+
+```solidity
+let fmp := mload(0x40)
+
+// -- snip
+
+mstore(0x24, receiver)
+
+mstore(0x44, tokenId)
+
+// -- snip
+
+mstore(0x40, fmp)
+```