Report output improvements

Cyfrin · Oct 26, 2023 · 794755e · 794755e
1 parent 4db17c1
commit 794755e
Show file tree

Hide file tree

Showing 3 changed files with 278 additions and 56 deletions.
diff --git a/report.md b/report.md
@@ -1,49 +1,170 @@
-# Critical Issues
+# Table of Contents
+
+- [High Issues](#high-issues)
+  - [H-1: Using `delegatecall` in loop](#H-1)
+- [Medium Issues](#medium-issues)
+  - [M-1: Centralization Risk for trusted owners](#M-1)
+  - [M-2: Solmate's SafeTransferLib does not check for token contract's existence](#M-2)
+- [Low Issues](#low-issues)
+  - [L-1: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`](#L-1)
+  - [L-2: `ecrecover` is susceptible to signature malleability](#L-2)
+  - [L-3: Deprecated OpenZeppelin functions should not be used](#L-3)
+  - [L-4: Unsafe ERC20 Operations should not be used](#L-4)
+  - [L-5: Solidity pragma should be specific, not wide](#L-5)
+- [NC Issues](#nc-issues)
+  - [NC-1: Missing checks for `address(0)` when assigning values to address state variables](#NC-1)
+  - [NC-2: Functions not used internally could be marked external](#NC-2)
+  - [NC-3: Constants should be defined and used instead of literals](#NC-3)
+  - [NC-4: Event is missing `indexed` fields](#NC-4)
+  - [NC-5: `require()` / `revert()` statements should have descriptive reason strings or custom errors](#NC-5)
+  - [NC-6: The `nonReentrant` `modifier` should occur before all other modifiers](#NC-6)
+
+
 # High Issues
-## Using `delegatecall` in loop
+
+<a name="H-1"></a>
+## H-1: Using `delegatecall` in loop
+
 When calling `delegatecall` the same `msg.value` amount will be accredited multiple times.
-- Found in contracts/ExtendedInheritance.sol: 488:19:1
+
+- Found in src/ExtendedInheritance.sol: 488:19:39
+
+
 # Medium Issues
+
+<a name="M-1"></a>
+## M-1: Centralization Risk for trusted owners
+
+Contracts have owners with privileged rights to perform admin tasks and need to be trusted to not perform malicious updates or drain funds.
+
+- Found in src/AdminContract.sol: unknown
+- Found in src/AdminContract.sol: 377:9:36
+- Found in src/AdminContract.sol: 506:9:36
+- Found in src/DeprecatedOZFunctions.sol: unknown
+
+
+<a name="M-2"></a>
+## M-2: Solmate's SafeTransferLib does not check for token contract's existence
+
+There is a subtle difference between the implementation of solmate's SafeTransferLib and OZ's SafeERC20: OZ's SafeERC20 checks if the token is a contract or not, solmate's SafeTransferLib does not.
+https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9 
+`@dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller`
+
+
+- Found in src/DeprecatedOZFunctions.sol: 898:17:38
+- Found in src/DeprecatedOZFunctions.sol: 579:22:38
+- Found in src/T11sTranferer.sol: 294:18:44
+
+
 # Low Issues
-## `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`
+
+<a name="L-1"></a>
+## L-1: `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`
+
 Use `abi.encode()` instead which will pad items to 32 bytes, which will [prevent hash collisions](https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#non-standard-packed-mode) (e.g. `abi.encodePacked(0x123,0x456)` => `0x123456` => `abi.encodePacked(0x1,0x23456)`, but `abi.encode(0x123,0x456)` => `0x0...1230...456`). Unless there is a compelling reason, `abi.encode` should be preferred. If there is only one argument to `abi.encodePacked()` it can often be cast to `bytes()` or `bytes32()` [instead](https://ethereum.stackexchange.com/questions/30912/how-to-compare-strings-in-solidity#answer-82739).
 If all arguments are strings and or bytes, `bytes.concat()` should be used instead.
-- Found in contracts/KeccakContract.sol: 878:16:4
-- Found in contracts/KeccakContract.sol: 584:16:4
-- Found in contracts/KeccakContract.sol: 731:16:4
-## `ecrecover` is susceptible to signature malleability
+
+- Found in src/KeccakContract.sol: 584:16:42
+- Found in src/KeccakContract.sol: 731:16:42
+- Found in src/KeccakContract.sol: 878:16:42
+
+
+<a name="L-2"></a>
+## L-2: `ecrecover` is susceptible to signature malleability
+
 The `ecrecover` function is susceptible to signature malleability. This means that the same message can be signed in multiple ways, allowing an attacker to change the message signature without invalidating it. This can lead to unexpected behavior in smart contracts, such as the loss of funds or the ability to bypass access control. Consider using OpenZeppelin's ECDSA library instead of the built-in function.
-- Found in contracts/ExtendedInheritance.sol: 705:9:1
-## Unsafe ERC20 Operations should not be used
+
+- Found in src/ExtendedInheritance.sol: 705:9:39
+
+
+<a name="L-3"></a>
+## L-3: Deprecated OpenZeppelin functions should not be used
+
+Openzeppelin has deprecated several functions and replaced with newer versions. Please consult https://docs.openzeppelin.com/
+
+- Found in src/DeprecatedOZFunctions.sol: 737:10:38
+- Found in src/DeprecatedOZFunctions.sol: 898:17:38
+
+
+<a name="L-4"></a>
+## L-4: Unsafe ERC20 Operations should not be used
+
 ERC20 functions may not behave as expected. For example: return values are not always meaningful. It is recommended to use OpenZeppelin's SafeERC20 library.
-- Found in contracts/Lock.sol: 962:14:5
-## Solidity pragma should be specific, not wide
+
+- Found in src/DeprecatedOZFunctions.sol: 1062:13:38
+- Found in src/DeprecatedOZFunctions.sol: 1598:18:38
+- Found in src/DeprecatedOZFunctions.sol: 1322:13:38
+- Found in src/DeprecatedOZFunctions.sol: 1424:13:38
+- Found in src/DeprecatedOZFunctions.sol: 1272:13:38
+
+
+<a name="L-5"></a>
+## L-5: Solidity pragma should be specific, not wide
+
 Consider using a specific version of Solidity in your contracts instead of a wide version. For example, instead of `pragma solidity ^0.8.0;`, use `pragma solidity 0.8.0;`
-- Found in contracts/InheritanceBase.sol: 32:23:3
-- Found in contracts/IContractInheritance.sol: 32:24:2
-- Found in contracts/Lock.sol: 39:24:5
-- Found in contracts/Counter.sol: 39:24:0
+
+- Found in src/InheritanceBase.sol: 32:23:41
+- Found in src/Counter.sol: 39:24:37
+- Found in src/IContractInheritance.sol: 32:24:40
+
+
 # NC Issues
-## Missing checks for `address(0)` when assigning values to address state variables
+
+<a name="NC-1"></a>
+## NC-1: Missing checks for `address(0)` when assigning values to address state variables
+
 Assigning values to address state variables without checking for `address(0)`.
-- Found in contracts/StateVariables.sol: 2121:14:6
-## Functions not used internally could be marked external
-
-- Found in contracts/Counter.sol: 120:80:0
-- Found in contracts/StateVariables.sol: 1755:145:6
-- Found in contracts/Lock.sol: 516:490:5
-- Found in contracts/Lock.sol: 271:239:5
-- Found in contracts/StateVariables.sol: 1426:292:6
-- Found in contracts/StateVariables.sol: 1906:151:6
-- Found in contracts/StateVariables.sol: 2500:376:6
-- Found in contracts/StateVariables.sol: 2148:346:6
-- Found in contracts/StateVariables.sol: 2063:79:6
-## Constants should be defined and used instead of literals
-
-- Found in contracts/Counter.sol: 434:1:0
-- Found in contracts/ExtendedInheritance.sol: 466:1:1
-## Event is missing `indexed` fields
+
+- Found in src/StateVariables.sol: 2121:14:43
+
+
+<a name="NC-2"></a>
+## NC-2: Functions not used internally could be marked external
+
+
+
+- Found in src/StateVariables.sol: 2063:79:43
+- Found in src/StateVariables.sol: 2148:346:43
+- Found in src/StateVariables.sol: 1426:292:43
+- Found in src/StateVariables.sol: 2500:376:43
+- Found in src/Counter.sol: 120:80:37
+- Found in src/AdminContract.sol: 302:26:36
+- Found in src/StateVariables.sol: 1906:151:43
+- Found in src/StateVariables.sol: 1755:145:43
+
+
+<a name="NC-3"></a>
+## NC-3: Constants should be defined and used instead of literals
+
+
+
+- Found in src/ExtendedInheritance.sol: 466:1:39
+- Found in src/Counter.sol: 434:1:37
+
+
+<a name="NC-4"></a>
+## NC-4: Event is missing `indexed` fields
+
 Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
-- Found in contracts/InheritanceBase.sol: 150:28:3
-- Found in contracts/Lock.sol: 224:41:5
-- Found in contracts/ExtendedInheritance.sol: 144:45:1
+
+- Found in src/ExtendedInheritance.sol: 144:45:39
+- Found in src/InheritanceBase.sol: 150:28:41
+
+
+<a name="NC-5"></a>
+## NC-5: `require()` / `revert()` statements should have descriptive reason strings or custom errors
+
+
+
+- Found in src/DeprecatedOZFunctions.sol: 1389:6:38
+- Found in src/DeprecatedOZFunctions.sol: 1264:7:38
+
+
+<a name="NC-6"></a>
+## NC-6: The `nonReentrant` `modifier` should occur before all other modifiers
+
+This is a best-practice to protect against reentrancy in other modifiers
+
+- Found in src/AdminContract.sol: 334:109:36
+
+
diff --git a/src/framework/hardhat.rs b/src/framework/hardhat.rs
@@ -25,7 +25,7 @@ pub fn load_hardhat(hardhat_root: PathBuf) -> Result<HardhatOutput, Box<dyn Erro
     let config_path = hardhat_root.join("artifacts/build-info");
     let json_build_files = collect_json_files(config_path).unwrap_or_else(|err| {
         // Exit with a non-zero exit code
-        eprintln!("Error getting Hardhat build-info files");
+        eprintln!("Error getting Hardhat build-info files. Try compiling your contracts with Hardhat before running aderyn.");
         // print err
         eprintln!("{:?}", err);
         std::process::exit(1);

diff --git a/src/report/printer.rs b/src/report/printer.rs
@@ -11,8 +11,15 @@ pub trait ReportPrinter {
         report: &Report,
         loader: &ContextLoader,
     ) -> Result<()>;
-    fn print_issue<W: Write>(&self, writer: W, issue: &Issue, loader: &ContextLoader)
-        -> Result<()>;
+    fn print_table_of_contents<W: Write>(&self, writer: W, report: &Report) -> Result<()>;
+    fn print_issue<W: Write>(
+        &self,
+        writer: W,
+        issue: &Issue,
+        loader: &ContextLoader,
+        severity: &str,
+        number: i32,
+    ) -> Result<()>;
 }
 
 pub struct MarkdownReportPrinter;
@@ -24,26 +31,113 @@ impl ReportPrinter for MarkdownReportPrinter {
         report: &Report,
         loader: &ContextLoader,
     ) -> Result<()> {
-        writeln!(writer, "# Critical Issues")?;
-        for issue in &report.criticals {
-            self.print_issue(&mut writer, issue, loader)?;
+        self.print_table_of_contents(&mut writer, report)?;
+        let mut counter = 0;
+        if !report.criticals.is_empty() {
+            writeln!(writer, "# Critical Issues\n")?;
+            for issue in &report.criticals {
+                counter += 1;
+                self.print_issue(&mut writer, issue, loader, "C", counter)?;
+            }
+        }
+        if !report.highs.is_empty() {
+            writeln!(writer, "# High Issues\n")?;
+            counter = 0;
+            for issue in &report.highs {
+                counter += 1;
+                self.print_issue(&mut writer, issue, loader, "H", counter)?;
+            }
+        }
+        if !report.mediums.is_empty() {
+            writeln!(writer, "# Medium Issues\n")?;
+            counter = 0;
+            for issue in &report.mediums {
+                counter += 1;
+                self.print_issue(&mut writer, issue, loader, "M", counter)?;
+            }
+        }
+        if !report.lows.is_empty() {
+            writeln!(writer, "# Low Issues\n")?;
+            counter = 0;
+            for issue in &report.lows {
+                counter += 1;
+                self.print_issue(&mut writer, issue, loader, "L", counter)?;
+            }
+        }
+        if !report.ncs.is_empty() {
+            writeln!(writer, "# NC Issues\n")?;
+            counter = 0;
+            for issue in &report.ncs {
+                counter += 1;
+                self.print_issue(&mut writer, issue, loader, "NC", counter)?;
+            }
+        }
+        Ok(())
+    }
+
+    fn print_table_of_contents<W: Write>(&self, mut writer: W, report: &Report) -> Result<()> {
+        writeln!(writer, "# Table of Contents\n")?;
+        if !report.criticals.is_empty() {
+            writeln!(writer, "- [Critical Issues](#critical-issues)")?;
+            for (index, issue) in report.criticals.iter().enumerate() {
+                writeln!(
+                    writer,
+                    "  - [C-{}: {}](#C-{})",
+                    index + 1,
+                    issue.title,
+                    index + 1
+                )?;
+            }
         }
-        writeln!(writer, "# High Issues")?;
-        for issue in &report.highs {
-            self.print_issue(&mut writer, issue, loader)?;
+        if !report.highs.is_empty() {
+            writeln!(writer, "- [High Issues](#high-issues)")?;
+            for (index, issue) in report.highs.iter().enumerate() {
+                writeln!(
+                    writer,
+                    "  - [H-{}: {}](#H-{})",
+                    index + 1,
+                    issue.title,
+                    index + 1
+                )?;
+            }
         }
-        writeln!(writer, "# Medium Issues")?;
-        for issue in &report.mediums {
-            self.print_issue(&mut writer, issue, loader)?;
+        if !report.mediums.is_empty() {
+            writeln!(writer, "- [Medium Issues](#medium-issues)")?;
+            for (index, issue) in report.mediums.iter().enumerate() {
+                writeln!(
+                    writer,
+                    "  - [M-{}: {}](#M-{})",
+                    index + 1,
+                    issue.title,
+                    index + 1
+                )?;
+            }
         }
-        writeln!(writer, "# Low Issues")?;
-        for issue in &report.lows {
-            self.print_issue(&mut writer, issue, loader)?;
+        if !report.lows.is_empty() {
+            writeln!(writer, "- [Low Issues](#low-issues)")?;
+            for (index, issue) in report.lows.iter().enumerate() {
+                writeln!(
+                    writer,
+                    "  - [L-{}: {}](#L-{})",
+                    index + 1,
+                    issue.title,
+                    index + 1
+                )?;
+            }
         }
-        writeln!(writer, "# NC Issues")?;
-        for issue in &report.ncs {
-            self.print_issue(&mut writer, issue, loader)?;
+        if !report.ncs.is_empty() {
+            writeln!(writer, "- [NC Issues](#nc-issues)")?;
+            for (index, issue) in report.ncs.iter().enumerate() {
+                writeln!(
+                    writer,
+                    "  - [NC-{}: {}](#NC-{})",
+                    index + 1,
+                    issue.title,
+                    index + 1
+                )?;
+            }
         }
+        writeln!(writer, "\n")?; // Add an extra newline for spacing
         Ok(())
     }
 
@@ -52,8 +146,14 @@ impl ReportPrinter for MarkdownReportPrinter {
         mut writer: W,
         issue: &Issue,
         loader: &ContextLoader,
+        severity: &str,
+        number: i32,
     ) -> Result<()> {
-        writeln!(writer, "## {}\n{}", issue.title, issue.description)?;
+        writeln!(
+            writer,
+            "<a name=\"{}-{}\"></a>\n## {}-{}: {}\n\n{}\n", // <a name> is the anchor for the issue title
+            severity, number, severity, number, issue.title, issue.description
+        )?;
         for instance in &issue.instances {
             if let Some(node) = instance {
                 let mut contract_path = "unknown";
@@ -69,6 +169,7 @@ impl ReportPrinter for MarkdownReportPrinter {
                 writeln!(writer, "- Found in {}: {}", contract_path, source_location)?;
             }
         }
+        writeln!(writer, "\n")?; // Add an extra newline for spacing
         Ok(())
     }
 }