Detector: State variable that could be declared constant

aderyn_core/src/context/browser/mod.rs

@@ -10,6 +10,7 @@ mod peek_over;
 mod peek_under;
 mod siblings;
 mod sort_nodes;
+mod storage_vars;
 pub use ancestral_line::*;
 pub use closest_ancestor::*;
 pub use extractor::*;
@@ -21,3 +22,4 @@ pub use peek_over::*;
 pub use peek_under::*;
 pub use siblings::*;
 pub use sort_nodes::*;
+pub use storage_vars::*;

aderyn_core/src/detect/detector.rs

@@ -87,6 +87,7 @@ pub fn get_all_issue_detectors() -> Vec<Box<dyn IssueDetector>> {
         Box::<OutOfOrderRetryableDetector>::default(),
         Box::<FunctionInitializingStateDetector>::default(),
         Box::<BuiltinSymbolShadowDetector>::default(),
+        Box::<StateVariableCouldBeConstantDetector>::default(),
     ]
 }
 
@@ -169,6 +170,7 @@ pub(crate) enum IssueDetectorNamePool {
     IncorrectERC20Interface,
     ReturnBomb,
     OutOfOrderRetryable,
+    StateVariableCouldBeDeclaredConstant,
     // 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,
@@ -178,6 +180,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::StateVariableCouldBeDeclaredConstant => {
+            Some(Box::<StateVariableCouldBeConstantDetector>::default())
+        }
         IssueDetectorNamePool::BuiltinSymbolShadow => {
             Some(Box::<BuiltinSymbolShadowDetector>::default())
         }

aderyn_core/src/detect/low/mod.rs

@@ -19,6 +19,7 @@ pub(crate) mod require_with_string;
 pub(crate) mod return_bomb;
 pub(crate) mod reverts_and_requries_in_loops;
 pub(crate) mod solmate_safe_transfer_lib;
+pub(crate) mod state_variable_could_be_constant;
 pub(crate) mod unindexed_events;
 pub(crate) mod unsafe_erc20_functions;
 pub(crate) mod unsafe_oz_erc721_mint;
@@ -51,6 +52,7 @@ pub use require_with_string::RequireWithStringDetector;
 pub use return_bomb::ReturnBombDetector;
 pub use reverts_and_requries_in_loops::RevertsAndRequiresInLoopsDetector;
 pub use solmate_safe_transfer_lib::SolmateSafeTransferLibDetector;
+pub use state_variable_could_be_constant::StateVariableCouldBeConstantDetector;
 pub use unindexed_events::UnindexedEventsDetector;
 pub use unsafe_erc20_functions::UnsafeERC20FunctionsDetector;
 pub use unsafe_oz_erc721_mint::UnsafeERC721MintDetector;

aderyn_core/src/detect/low/state_variable_could_be_constant.rs

@@ -0,0 +1,193 @@
+use std::collections::{BTreeMap, HashSet};
+use std::error::Error;
+
+use crate::ast::{Mutability, NodeID};
+
+use crate::capture;
+use crate::detect::detector::IssueDetectorNamePool;
+use crate::{
+    context::workspace_context::WorkspaceContext,
+    detect::detector::{IssueDetector, IssueSeverity},
+};
+use eyre::Result;
+
+#[derive(Default)]
+pub struct StateVariableCouldBeConstantDetector {
+    // 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 StateVariableCouldBeConstantDetector {
+    fn detect(&mut self, context: &WorkspaceContext) -> Result<bool, Box<dyn Error>> {
+        // PLAN
+        // 1. Collect all state variables that are not marked constant or immutable and are also not structs/mappings/contracts (collection A)
+        // 2. Investigate every function and collect all the state variables that could change (collection B)
+        // 3. Result = collection A - collection B
+
+        let mut collection_a = Vec::new();
+
+        for variable in context.variable_declarations() {
+            // If we're not able to set the value upfront, then it cannot be constant
+            if variable.value.is_none() {
+                continue;
+            }
+
+            if variable.mutability() == Some(&Mutability::Immutable) {
+                continue;
+            }
+
+            // Do not report it if it's a struct / mapping / contract type
+            if variable
+                .type_descriptions
+                .type_string
+                .as_ref()
+                .is_some_and(|type_string| {
+                    type_string.starts_with("mapping")
+                        || type_string.starts_with("struct")
+                        || type_string.starts_with("contract")
+                })
+            {
+                continue;
+            }
+
+            if variable.overrides.is_some() {
+                continue;
+            }
+
+            if variable.state_variable && !variable.constant {
+                collection_a.push(variable);
+            }
+        }
+
+        let mut all_state_changes = None;
+        for func in context.function_definitions() {
+            if let Some(changes) = func.state_variable_changes(context) {
+                if all_state_changes.is_none() {
+                    all_state_changes = Some(changes);
+                } else if let Some(existing_changes) = all_state_changes {
+                    let new_changes = existing_changes + changes;
+                    all_state_changes = Some(new_changes);
+                }
+            }
+        }
+
+        if let Some(all_state_changes) = all_state_changes {
+            let collection_b = all_state_changes.fetch_non_exhaustive_manipulated_state_variables();
+            let collection_b_ids: HashSet<_> = collection_b.into_iter().map(|v| v.id).collect();
+
+            // RESULT =  collection A - collection B
+            for variable in collection_a {
+                if !collection_b_ids.contains(&variable.id) {
+                    capture!(self, context, variable);
+                }
+            }
+        }
+
+        Ok(!self.found_instances.is_empty())
+    }
+
+    fn severity(&self) -> IssueSeverity {
+        IssueSeverity::Low
+    }
+
+    fn title(&self) -> String {
+        String::from("State variable could be declared constant")
+    }
+
+    fn description(&self) -> String {
+        String::from("State variables that are not updated following deployment should be declared constant to save gas. Add the `constant` attribute to state variables that never change.")
+    }
+
+    fn instances(&self) -> BTreeMap<(String, usize, String), NodeID> {
+        self.found_instances.clone()
+    }
+
+    fn name(&self) -> String {
+        format!(
+            "{}",
+            IssueDetectorNamePool::StateVariableCouldBeDeclaredConstant
+        )
+    }
+}
+
+mod function_state_changes_finder_helper {
+    use crate::{
+        ast::{ASTNode, FunctionDefinition},
+        context::{
+            browser::ApproximateStorageChangeFinder,
+            graph::{CallGraph, CallGraphDirection, CallGraphVisitor},
+            workspace_context::WorkspaceContext,
+        },
+    };
+
+    impl FunctionDefinition {
+        /// Investigates the function with the help callgraph and accumulates all the state variables
+        /// that have been changed.
+        pub fn state_variable_changes<'a>(
+            &self,
+            context: &'a WorkspaceContext,
+        ) -> Option<ApproximateStorageChangeFinder<'a>> {
+            let mut tracker = StateVariableChangeTracker {
+                changes: None,
+                context,
+            };
+
+            let investigator =
+                CallGraph::new(context, &[&(self.into())], CallGraphDirection::Inward).ok()?;
+            investigator.accept(context, &mut tracker).ok()?;
+
+            tracker.changes.take()
+        }
+    }
+
+    struct StateVariableChangeTracker<'a> {
+        context: &'a WorkspaceContext,
+        changes: Option<ApproximateStorageChangeFinder<'a>>,
+    }
+
+    impl<'a> CallGraphVisitor for StateVariableChangeTracker<'a> {
+        fn visit_any(&mut self, node: &ASTNode) -> eyre::Result<()> {
+            let changes = ApproximateStorageChangeFinder::from(self.context, node);
+            if self.changes.is_none() {
+                self.changes = Some(changes);
+            } else if let Some(existing_changes) = self.changes.take() {
+                let new_changes = existing_changes + changes;
+                self.changes = Some(new_changes);
+            }
+            Ok(())
+        }
+    }
+}
+
+#[cfg(test)]
+mod state_variable_could_be_constant_tests {
+    use serial_test::serial;
+
+    use crate::detect::{
+        detector::IssueDetector,
+        low::state_variable_could_be_constant::StateVariableCouldBeConstantDetector,
+    };
+
+    #[test]
+    #[serial]
+    fn test_state_variable_could_be_declared_constant() {
+        let context = crate::detect::test_utils::load_solidity_source_unit(
+            "../tests/contract-playground/src/StateVariableCouldBeDeclaredConstant.sol",
+        );
+
+        let mut detector = StateVariableCouldBeConstantDetector::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);
+
+        println!("{:?}", detector.instances());
+        // assert the severity is low
+        assert_eq!(
+            detector.severity(),
+            crate::detect::detector::IssueSeverity::Low
+        );
+    }
+}

reports/adhoc-sol-files-report.md

@@ -29,6 +29,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
   - [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)
+  - [L-18: State variable could be declared constant](#l-18-state-variable-could-be-declared-constant)
 
 
 # Summary
@@ -73,7 +74,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
 | Category | No. of Issues |
 | --- | --- |
 | High | 3 |
-| Low | 17 |
+| Low | 18 |
 
 
 # High Issues
@@ -744,3 +745,56 @@ State variable appears to be unused. No analysis has been performed to see if an
 
 
 
+## L-18: State variable could be declared constant
+
+State variables that are not updated following deployment should be declared constant to save gas. Add the `constant` attribute to state variables that never change.
+
+<details><summary>7 Found Instances</summary>
+
+
+- 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: 15](../tests/adhoc-sol-files/StateVariables.sol#L15)
+
+	```solidity
+	    uint256 public staticNonEmptyPublicNumber = 3;
+	```
+
+- Found in multiple-versions/0.5/B.sol [Line: 5](../tests/adhoc-sol-files/multiple-versions/0.5/B.sol#L5)
+
+	```solidity
+	    address public MY_ADDRESS = address(0);
+	```
+
+- Found in multiple-versions/0.5/B.sol [Line: 6](../tests/adhoc-sol-files/multiple-versions/0.5/B.sol#L6)
+
+	```solidity
+	    uint256 public MY_UINT = 134131;
+	```
+
+- Found in multiple-versions/0.8/B.sol [Line: 5](../tests/adhoc-sol-files/multiple-versions/0.8/B.sol#L5)
+
+	```solidity
+	    address public MY_ADDRESS = address(0);
+	```
+
+- Found in multiple-versions/0.8/B.sol [Line: 6](../tests/adhoc-sol-files/multiple-versions/0.8/B.sol#L6)
+
+	```solidity
+	    uint256 public MY_UINT = 134131;
+	```
+
+</details>
+
+
+

reports/nft-report.md

@@ -11,6 +11,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
   - [L-1: Solidity pragma should be specific, not wide](#l-1-solidity-pragma-should-be-specific-not-wide)
   - [L-2: `public` functions not used internally could be marked `external`](#l-2-public-functions-not-used-internally-could-be-marked-external)
   - [L-3: PUSH0 is not supported by all chains](#l-3-push0-is-not-supported-by-all-chains)
+  - [L-4: State variable could be declared constant](#l-4-state-variable-could-be-declared-constant)
 
 
 # Summary
@@ -40,7 +41,7 @@ This report was generated by [Aderyn](https://github.com/Cyfrin/aderyn), a stati
 | Category | No. of Issues |
 | --- | --- |
 | High | 0 |
-| Low | 3 |
+| Low | 4 |
 
 
 # Low Issues
@@ -102,3 +103,26 @@ Solc compiler version 0.8.20 switches the default target EVM version to Shanghai
 
 
 
+## L-4: State variable could be declared constant
+
+State variables that are not updated following deployment should be declared constant to save gas. Add the `constant` attribute to state variables that never change.
+
+<details><summary>2 Found Instances</summary>
+
+
+- Found in src/inner-core-modules/ICM.sol [Line: 6](../tests/foundry-nft-f23/src/inner-core-modules/ICM.sol#L6)
+
+	```solidity
+	    string public PROJECT_NAME = "Dogie";
+	```
+
+- Found in src/inner-core-modules/ICM.sol [Line: 7](../tests/foundry-nft-f23/src/inner-core-modules/ICM.sol#L7)
+
+	```solidity
+	    string public PROJECT_SYMBOL = "DOG";
+	```
+
+</details>
+
+
+