Detector: Unused private / internal state variables (#643)

Co-authored-by: Alex Roan <alex@cyfrin.io>

aderyn_core/src/detect/detector.rs

@@ -74,6 +74,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<WeakRandomnessDetector>::default(),
         Box::<PreDeclaredLocalVariableUsageDetector>::default(),
         Box::<DeletionNestedMappingDetector>::default(),
+        Box::<UnusedStateVariablesDetector>::default(),
         Box::<ConstantFunctionContainsAssemblyDetector>::default(),
         Box::<BooleanEqualityDetector>::default(),
         Box::<TxOriginUsedForAuthDetector>::default(),
@@ -149,6 +150,7 @@ pub(crate) enum IssueDetectorNamePool {
     WeakRandomness,
     PreDeclaredLocalVariableUsage,
     DeleteNestedMapping,
+    UnusedStateVariable,
     ConstantFunctionsAssembly,
     BooleanEquality,
     TxOriginUsedForAuth,
@@ -163,6 +165,9 @@ pub fn request_issue_detector_by_name(detector_name: &str) -> Option<Box<dyn Iss
     // Expects a valid detector_name
     let detector_name = IssueDetectorNamePool::from_str(detector_name).ok()?;
     match detector_name {
+        IssueDetectorNamePool::UnusedStateVariable => {
+            Some(Box::<UnusedStateVariablesDetector>::default())
+        }
         IssueDetectorNamePool::DelegateCallInLoop => {
             Some(Box::<DelegateCallInLoopDetector>::default())
         }

aderyn_core/src/detect/low/mod.rs

@@ -20,6 +20,7 @@ pub(crate) mod unindexed_events;
 pub(crate) mod unsafe_erc20_functions;
 pub(crate) mod unsafe_oz_erc721_mint;
 pub(crate) mod unspecific_solidity_pragma;
+pub(crate) mod unused_state_variable;
 pub(crate) mod useless_error;
 pub(crate) mod useless_internal_function;
 pub(crate) mod useless_modifier;
@@ -48,6 +49,7 @@ pub use unindexed_events::UnindexedEventsDetector;
 pub use unsafe_erc20_functions::UnsafeERC20FunctionsDetector;
 pub use unsafe_oz_erc721_mint::UnsafeERC721MintDetector;
 pub use unspecific_solidity_pragma::UnspecificSolidityPragmaDetector;
+pub use unused_state_variable::UnusedStateVariablesDetector;
 pub use useless_error::UselessErrorDetector;
 pub use useless_internal_function::UselessInternalFunctionDetector;
 pub use useless_modifier::UselessModifierDetector;

aderyn_core/src/detect/low/unused_state_variable.rs

@@ -0,0 +1,121 @@
+use std::collections::{BTreeMap, BTreeSet};
+use std::convert::identity;
+use std::error::Error;
+
+use crate::ast::{ASTNode, ContractKind, NodeID, NodeType, Visibility};
+
+use crate::capture;
+use crate::context::browser::{
+    ExtractReferencedDeclarations, ExtractVariableDeclarations, GetClosestAncestorOfTypeX,
+};
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct UnusedStateVariablesDetector {
+    // 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 UnusedStateVariablesDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        // Collect all referencedDeclaration IDs adn StateVariableDeclarationIDs
+        let mut all_referenced_declarations = BTreeSet::new();
+        let mut all_state_variable_declarations = BTreeSet::new();
+
+        for source_unit in context.source_units() {
+            let referenced_declarations =
+                ExtractReferencedDeclarations::from(source_unit).extracted;
+            all_referenced_declarations.extend(referenced_declarations);
+            let variable_declarations = ExtractVariableDeclarations::from(source_unit).extracted;
+            all_state_variable_declarations.extend(
+                variable_declarations
+                    .into_iter()
+                    .filter(|v| {
+                        v.state_variable
+                            && (v.visibility == Visibility::Private
+                                || v.visibility == Visibility::Internal)
+                    })
+                    .map(|v| v.id),
+            )
+        }
+
+        // Now, retain only the ones that have not been referenced
+        all_state_variable_declarations.retain(|v| !all_referenced_declarations.contains(v));
+
+        for unused_state_var_id in all_state_variable_declarations {
+            if let Some(node) = context.nodes.get(&unused_state_var_id) {
+                if let Some(ASTNode::ContractDefinition(contract)) =
+                    node.closest_ancestor_of_type(context, NodeType::ContractDefinition)
+                {
+                    // If this variable is defined inside a contract, make sure it's not an abstract contract before capturing it
+                    if !contract.is_abstract.is_some_and(identity)
+                        && contract.kind == ContractKind::Contract
+                    {
+                        capture!(self, context, node);
+                    }
+                } else {
+                    // Otherwise, just capture it !
+                    capture!(self, context, node);
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Low
+    }
+
+    fn title(&self) -> String {
+        String::from("Potentially unused `private` / `internal` state variables found.")
+    }
+
+    fn description(&self) -> String {
+        String::from("State variable appears to be unused. No analysis has been performed to see if any inilne assembly \
+            references it. So if that's not the case, consider removing this unused variable.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!("{}", IssueDetectorNamePool::UnusedStateVariable)
+    }
+}
+
+#[cfg(test)]
+mod unused_detector_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector, low::unused_state_variable::UnusedStateVariablesDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_unused_state_variables() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/UnusedStateVariables.sol",
+        );
+
+        let mut detector = UnusedStateVariablesDetector::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(), 4);
+        // assert the severity is low
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Low
+        );
+    }
+}

reports/adhoc-sol-files-report.md

@@ -28,6 +28,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
   - [L-14: Contract still has TODOs](#l-14-contract-still-has-todos)
   - [L-15: Inconsistency in declaring uint256/uint (or) int256/int variables within a contract. Use explicit size declarations (uint256 or int256).](#l-15-inconsistency-in-declaring-uint256uint-or-int256int-variables-within-a-contract-use-explicit-size-declarations-uint256-or-int256)
   - [L-16: Unused Custom Error](#l-16-unused-custom-error)
+  - [L-17: Potentially unused `private` / `internal` state variables found.](#l-17-potentially-unused-private--internal-state-variables-found)
 
 
 # Summary
@@ -72,7 +73,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
 | Category | No. of Issues |
 | --- | --- |
 | High | 3 |
-| Low | 16 |
+| Low | 17 |
 
 
 # High Issues
@@ -690,3 +691,56 @@ it is recommended that the definition be removed when custom error is unused
 
 
 
+## L-17: Potentially unused `private` / `internal` state variables found.
+
+State variable appears to be unused. No analysis has been performed to see if any inilne assembly references it. So if that's not the case, consider removing this unused variable.
+
+<details><summary>7 Found Instances</summary>
+
+
+- Found in InconsistentUints.sol [Line: 16](../tests/adhoc-sol-files/InconsistentUints.sol#L16)
+
+	```solidity
+	    mapping(uint256 => uint other) u2uMapping; // 5 3
+	```
+
+- Found in StateVariables.sol [Line: 8](../tests/adhoc-sol-files/StateVariables.sol#L8)
+
+	```solidity
+	    uint256 private staticPrivateNumber;
+	```
+
+- Found in StateVariables.sol [Line: 9](../tests/adhoc-sol-files/StateVariables.sol#L9)
+
+	```solidity
+	    uint256 internal staticInternalNumber;
+	```
+
+- Found in StateVariables.sol [Line: 13](../tests/adhoc-sol-files/StateVariables.sol#L13)
+
+	```solidity
+	    uint256 private staticNonEmptyPrivateNumber = 1;
+	```
+
+- Found in StateVariables.sol [Line: 14](../tests/adhoc-sol-files/StateVariables.sol#L14)
+
+	```solidity
+	    uint256 internal staticNonEmptyInternalNumber = 2;
+	```
+
+- Found in StateVariables.sol [Line: 28](../tests/adhoc-sol-files/StateVariables.sol#L28)
+
+	```solidity
+	    uint256 private constant PRIVATE_CONSTANT = 1;
+	```
+
+- Found in StateVariables.sol [Line: 29](../tests/adhoc-sol-files/StateVariables.sol#L29)
+
+	```solidity
+	    uint256 internal constant INTERNAL_CONSTANT = 2;
+	```
+
+</details>
+
+
+