Detector: x.sub(y) without if checks to avoid unexpected reverts

aderyn_core/src/detect/detector.rs

@@ -36,9 +36,12 @@ use std::{
     str::FromStr,
 };
 
-use super::high::{
-    DelegateCallOnUncheckedAddressDetector, SelfdestructIdentifierDetector,
-    SendEtherNoChecksDetector,
+use super::{
+    high::{
+        DelegateCallOnUncheckedAddressDetector, SelfdestructIdentifierDetector,
+        SendEtherNoChecksDetector,
+    },
+    low::SubWithoutIfDetector,
 };
 
 pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
@@ -86,6 +89,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<StateVariableShadowingDetector>::default(),
         Box::<SendEtherNoChecksDetector>::default(),
         Box::<DelegateCallOnUncheckedAddressDetector>::default(),
+        Box::<SubWithoutIfDetector>::default(),
     ]
 }
 
@@ -140,6 +144,7 @@ pub(crate) enum IssueDetectorNamePool {
     StateVariableShadowing,
     SendEtherNoChecks,
     DelegateCallUncheckedAddress,
+    SubWithoutIf,
     // NOTE: `Undecided` will be the default name (for new bots).
     // If it's accepted, a new variant will be added to this enum before normalizing it in aderyn
     Undecided,
@@ -262,6 +267,7 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
         IssueDetectorNamePool::DelegateCallUncheckedAddress => {
             Some(Box::<DelegateCallOnUncheckedAddressDetector>::default())
         }
+        IssueDetectorNamePool::SubWithoutIf => Some(Box::<SubWithoutIfDetector>::default()),
         IssueDetectorNamePool::Undecided => None,
     }
 }

aderyn_core/src/detect/low/mod.rs

@@ -12,6 +12,7 @@ pub(crate) mod push_0_opcode;
 pub(crate) mod require_with_string;
 pub(crate) mod reverts_and_requries_in_loops;
 pub(crate) mod solmate_safe_transfer_lib;
+pub(crate) mod sub_without_if;
 pub(crate) mod unindexed_events;
 pub(crate) mod unsafe_erc20_functions;
 pub(crate) mod unsafe_oz_erc721_mint;
@@ -36,6 +37,7 @@ pub use push_0_opcode::PushZeroOpcodeDetector;
 pub use require_with_string::RequireWithStringDetector;
 pub use reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector;
 pub use solmate_safe_transfer_lib::SolmateSafeTransferLibDetector;
+pub use sub_without_if::SubWithoutIfDetector;
 pub use unindexed_events::UnindexedEventsDetector;
 pub use unsafe_erc20_functions::UnsafeERC20FunctionsDetector;
 pub use unsafe_oz_erc721_mint::UnsafeERC721MintDetector;

aderyn_core/src/detect/low/sub_without_if.rs

@@ -0,0 +1,150 @@
+use std::collections::{BTreeMap, HashSet};
+use std::error::Error;
+
+use crate::ast::NodeID;
+
+use crate::capture;
+use crate::context::browser::ExtractMemberAccesses;
+use crate::context::investigator::{
+    StandardInvestigationStyle, StandardInvestigator, StandardInvestigatorVisitor,
+};
+
+use crate::context::workspace_context::ASTNode;
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct SubWithoutIfDetector {
+    // Keys are: [0] source file name, [1] line number, [2] character location of node.
+    // Do not add items manually, use `capture!` to add nodes to this BTreeMap.
+    found_instances: BTreeMap<(String, usize, String), NodeID>,
+}
+
+impl IssueDetector for SubWithoutIfDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        // Plan:
+        // First, collect the if statements and investigate them for any `X.sub(Y)` patterns
+        // and capture all the IDs ... Set A (protected_subs)
+        // Then, collect all the IDS of X.sub(Y) pattern ... Set B (all_subs)
+        // Answer = SetB - SetA
+        // (We're left with unchecked sub method calls)
+
+        let mut all_subs = context
+            .member_accesses()
+            .into_iter()
+            .filter(|member| member.member_name == "sub")
+            .map(|x| x.id)
+            .collect::<HashSet<_>>();
+
+        let mut protected_subs: HashSet<NodeID> = HashSet::new();
+
+        let if_statements = context
+            .if_statements()
+            .into_iter()
+            .map(|if_statement| if_statement.into())
+            .collect::<Vec<ASTNode>>();
+
+        let mut tracker = SubTracker {
+            sub_pattern_ids: vec![],
+        };
+        let investigator = StandardInvestigator::new(
+            context,
+            &if_statements.iter().collect::<Vec<_>>(),
+            StandardInvestigationStyle::Downstream,
+        )?;
+        investigator.investigate(context, &mut tracker)?;
+        protected_subs.extend(tracker.sub_pattern_ids.iter());
+
+        for protected_sub in protected_subs {
+            let _ = all_subs.remove(&protected_sub);
+        }
+
+        // Now, we have the unprotected subs remaining in all_subs
+        for sub in all_subs {
+            context
+                .nodes
+                .get(&sub)
+                .inspect(|&node| capture!(self, context, node));
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Low
+    }
+
+    fn title(&self) -> String {
+        String::from("Potential unchecked call made to `x.sub(y)`.")
+    }
+
+    fn description(&self) -> String {
+        String::from("Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        IssueDetectorNamePool::SubWithoutIf.to_string()
+    }
+}
+
+struct SubTracker {
+    sub_pattern_ids: Vec<NodeID>,
+}
+
+impl StandardInvestigatorVisitor for SubTracker {
+    fn visit_fallback(
+        &mut self,
+        node: &crate::context::workspace_context::ASTNode,
+    ) -> eyre::Result<()> {
+        let member_accesses = ExtractMemberAccesses::from(node).extracted;
+        self.sub_pattern_ids.extend(
+            member_accesses
+                .into_iter()
+                .filter(|member| member.member_name == "sub")
+                .map(|x| x.id),
+        );
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod sub_without_if_tests {
+    use crate::detect::{detector::IssueDetector, low::sub_without_if::SubWithoutIfDetector};
+
+    #[test]
+    fn test_sub_without_if() {
+        let context = crate::detect::test_utils::load_solidity_source_unit_with_callgraphs(
+            "../tests/contract-playground/src/SubWithoutIf.sol",
+        );
+
+        let mut detector = SubWithoutIfDetector::default();
+        let found = detector.detect(&context).unwrap();
+        // assert that the detector found an issue
+        assert!(found);
+        // assert that the detector found the correct number of instances
+        assert_eq!(detector.instances().len(), 1);
+        // assert the severity is low
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Low
+        );
+        // assert the title is correct
+        assert_eq!(
+            detector.title(),
+            String::from("Potential unchecked call made to `x.sub(y)`.")
+        );
+        // assert the description is correct
+        assert_eq!(
+            detector.description(),
+            String::from("Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts"),
+        );
+    }
+}

reports/report.json

@@ -241,7 +241,7 @@
   },
   "issue_count": {
     "high": 20,
-    "low": 23
+    "low": 24
   },
   "high_issues": {
     "issues": [
@@ -2967,6 +2967,19 @@
             "src_char": "541:5"
           }
         ]
+      },
+      {
+        "title": "Potential unchecked call made to `x.sub(y)`.",
+        "description": "Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts",
+        "detector_name": "sub-without-if",
+        "instances": [
+          {
+            "contract_path": "src/SubWithoutIf.sol",
+            "line_no": 17,
+            "src": "549:23",
+            "src_char": "549:23"
+          }
+        ]
       }
     ]
   },
@@ -3013,6 +3026,7 @@
     "yul-return",
     "state-variable-shadowing",
     "send-ether-no-checks",
-    "delegate-call-unchecked-address"
+    "delegate-call-unchecked-address",
+    "sub-without-if"
   ]
 }

reports/report.md

@@ -52,6 +52,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
   - [L-21: Unused Custom Error](#l-21-unused-custom-error)
   - [L-22: Loop contains `require`/`revert` statements](#l-22-loop-contains-requirerevert-statements)
   - [L-23: Incorrect Order of Division and Multiplication](#l-23-incorrect-order-of-division-and-multiplication)
+  - [L-24: Potential unchecked call made to `x.sub(y)`.](#l-24-potential-unchecked-call-made-to-xsuby)
 
 
 # Summary
@@ -134,7 +135,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
 | Category | No. of Issues |
 | --- | --- |
 | High | 20 |
-| Low | 23 |
+| Low | 24 |
 
 
 # High Issues
@@ -3007,3 +3008,20 @@ Division operations followed directly by multiplication operations can lead to p
 
 
 
+## L-24: Potential unchecked call made to `x.sub(y)`.
+
+Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts
+
+<details><summary>1 Found Instances</summary>
+
+
+- Found in src/SubWithoutIf.sol [Line: 17](../tests/contract-playground/src/SubWithoutIf.sol#L17)
+
+	```solidity
+	        uint256 magicNumber = subtractionContract.sub(a, b) + 4 ** 7;
+	```
+
+</details>
+
+
+

reports/report.sarif

@@ -4823,6 +4823,26 @@
             "text": "Division operations followed directly by multiplication operations can lead to precision loss due to the way integer arithmetic is handled in Solidity."
           },
           "ruleId": "division-before-multiplication"
+        },
+        {
+          "level": "note",
+          "locations": [
+            {
+              "physicalLocation": {
+                "artifactLocation": {
+                  "uri": "src/SubWithoutIf.sol"
+                },
+                "region": {
+                  "byteLength": 23,
+                  "byteOffset": 549
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "Use an if-statement to see that `x` is bigger than `y` to protect from unexpected reverts"
+          },
+          "ruleId": "sub-without-if"
         }
       ],
       "tool": {