feat: Add variable shadowing detection in nested scopes

contracts/shadowing-example/Cargo.toml

@@ -0,0 +1,27 @@
+[package]
+name = "shadowing-example"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+crate-type = ["cdylib"]
+
+[dependencies]
+soroban-sdk = "21.7.0"
+
+[dev-dependencies]
+soroban-sdk = { version = "21.7.0", features = ["testutils"] }
+
+[profile.release]
+opt-level = "z"
+overflow-checks = true
+debug = 0
+strip = "symbols"
+debug-assertions = false
+panic = "abort"
+codegen-units = 1
+lto = true
+
+[profile.release-with-logs]
+inherits = "release"
+debug-assertions = true

contracts/shadowing-example/src/lib.rs

@@ -0,0 +1,147 @@
+#![no_std]
+use soroban_sdk::{contract, contractimpl, Env, Symbol, symbol_short};
+
+/// Example contract demonstrating variable shadowing issues.
+///
+/// This contract intentionally contains variable shadowing patterns
+/// that the Sanctifier tool should detect.
+#[contract]
+pub struct ShadowingExample;
+
+#[contractimpl]
+impl ShadowingExample {
+    /// Example 1: Simple shadowing in nested block
+    /// BUG: The inner `balance` shadows the outer one, potentially causing confusion
+    pub fn update_balance_buggy(env: Env, user: Symbol, amount: i128) -> i128 {
+        let balance = Self::get_balance(env.clone(), user.clone());
+
+        // This block shadows the outer balance variable
+        {
+            let balance = amount;  // SHADOWING: This shadows the outer balance!
+            env.storage().persistent().set(&user, &balance);
+        }
+
+        // Developer might think they're returning the updated balance,
+        // but they're actually returning the original balance
+        balance  // BUG: Returns the OLD balance, not the new one!
+    }
+
+    /// Example 2: Shadowing in conditional blocks
+    /// BUG: Different branches shadow the same variable
+    pub fn process_transaction_buggy(env: Env, amount: i128, fee: i128) -> i128 {
+        let total = amount;
+
+        if fee > 0 {
+            let total = amount + fee;  // SHADOWING: Shadows outer total
+            env.storage().instance().set(&symbol_short!("last_tx"), &total);
+        } else {
+            let total = amount;  // SHADOWING: Also shadows outer total
+            env.storage().instance().set(&symbol_short!("last_tx"), &total);
+        }
+
+        // Returns the original total, not the calculated one
+        total  // BUG: Returns amount, not amount + fee
+    }
+
+    /// Example 3: Shadowing in for loop
+    /// BUG: Loop variable shadows outer variable
+    pub fn calculate_sum_buggy(env: Env, count: u32) -> u32 {
+        let i = 100;  // Some important value
+        let mut sum = 0;
+
+        for i in 0..count {  // SHADOWING: Loop variable shadows outer i
+            sum += i;
+        }
+
+        // Developer might expect i to still be 100 here
+        env.storage().instance().set(&symbol_short!("last_i"), &i);
+        sum
+    }
+
+    /// Example 4: Shadowing in match arms
+    /// BUG: Match arm pattern shadows outer variable
+    pub fn handle_option_buggy(env: Env, opt: Option<i128>) -> i128 {
+        let value = 42;  // Default value
+
+        match opt {
+            Some(value) => {  // SHADOWING: Pattern binding shadows outer value
+                env.storage().instance().set(&symbol_short!("opt_val"), &value);
+                value
+            }
+            None => value,  // Uses outer value
+        }
+        // The outer value is never modified, which might be unexpected
+    }
+
+    /// Example 5: Correct implementation without shadowing
+    pub fn update_balance_correct(env: Env, user: Symbol, amount: i128) -> i128 {
+        let old_balance = Self::get_balance(env.clone(), user.clone());
+
+        {
+            let new_balance = amount;  // Different name, no shadowing
+            env.storage().persistent().set(&user, &new_balance);
+        }
+
+        amount  // Correctly returns the new balance
+    }
+
+    /// Example 6: Correct implementation with explicit naming
+    pub fn process_transaction_correct(env: Env, amount: i128, fee: i128) -> i128 {
+        let base_amount = amount;
+
+        let final_total = if fee > 0 {
+            let total_with_fee = amount + fee;  // Clear, distinct name
+            env.storage().instance().set(&symbol_short!("last_tx"), &total_with_fee);
+            total_with_fee
+        } else {
+            let total_no_fee = amount;  // Clear, distinct name
+            env.storage().instance().set(&symbol_short!("last_tx"), &total_no_fee);
+            total_no_fee
+        };
+
+        final_total
+    }
+
+    // Helper function
+    fn get_balance(env: Env, user: Symbol) -> i128 {
+        env.storage().persistent().get(&user).unwrap_or(0)
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use soroban_sdk::{testutils::Address as _, Address, Env};
+
+    #[test]
+    fn test_shadowing_bug_demonstration() {
+        let env = Env::default();
+        let contract_id = env.register_contract(None, ShadowingExample);
+        let client = ShadowingExampleClient::new(&env, &contract_id);
+
+        let user = symbol_short!("alice");
+
+        // The buggy version returns the OLD balance (0), not the new one (100)
+        let result = client.update_balance_buggy(&user, &100);
+        assert_eq!(result, 0);  // BUG: Should be 100, but returns 0!
+
+        // The correct version returns the new balance
+        let result = client.update_balance_correct(&user, &100);
+        assert_eq!(result, 100);  // Correct!
+    }
+
+    #[test]
+    fn test_transaction_shadowing_bug() {
+        let env = Env::default();
+        let contract_id = env.register_contract(None, ShadowingExample);
+        let client = ShadowingExampleClient::new(&env, &contract_id);
+
+        // Buggy version returns amount, not amount + fee
+        let result = client.process_transaction_buggy(&100, &10);
+        assert_eq!(result, 100);  // BUG: Should be 110, but returns 100!
+
+        // Correct version returns the right total
+        let result = client.process_transaction_correct(&100, &10);
+        assert_eq!(result, 110);  // Correct!
+    }
+}

tooling/sanctifier-cli/src/commands/analyze.rs

@@ -98,6 +98,7 @@ pub(crate) struct FileAnalysisResult {
     pub(crate) truncation_bounds_issues: Vec<sanctifier_core::TruncationBoundsIssue>,
     pub(crate) sep41_checked_contracts: Vec<String>,
     pub(crate) sep41_issues: Vec<sanctifier_core::Sep41Issue>,
+    pub(crate) variable_shadowing_violations: Vec<sanctifier_core::RuleViolation>,
     pub(crate) timed_out: bool,
 }
 
@@ -214,6 +215,7 @@ pub fn exec(args: AnalyzeArgs) -> anyhow::Result<()> {
     let mut truncation_bounds_issues = Vec::new();
     let mut sep41_checked_contracts = Vec::new();
     let mut sep41_issues = Vec::new();
+    let mut variable_shadowing_violations = Vec::new();
     let mut timed_out_files: Vec<String> = Vec::new();
 
     for r in results {
@@ -232,6 +234,7 @@ pub fn exec(args: AnalyzeArgs) -> anyhow::Result<()> {
         truncation_bounds_issues.extend(r.truncation_bounds_issues);
         sep41_checked_contracts.extend(r.sep41_checked_contracts);
         sep41_issues.extend(r.sep41_issues);
+        variable_shadowing_violations.extend(r.variable_shadowing_violations);
         if r.timed_out {
             timed_out_files.push(r.file_path);
         }
@@ -253,6 +256,7 @@ pub fn exec(args: AnalyzeArgs) -> anyhow::Result<()> {
         + smt_issues.len()
         + truncation_bounds_issues.len()
         + sep41_issues.len()
+        + variable_shadowing_violations.len()
         + timed_out_files.len();
 
     let has_critical = auth_gaps
@@ -544,6 +548,23 @@ pub fn exec(args: AnalyzeArgs) -> anyhow::Result<()> {
             );
             println!("      Call: {}", issue.call_expression);
             println!("      Location: {}", issue.location);
+            println!("      Message: {}", issue.message);
+        }
+    }
+    if variable_shadowing_violations.is_empty() {
+        println!("{} No variable shadowing detected.", "✅".green());
+    } else {
+        println!("\n{} Found Variable Shadowing issues!", "⚠️".yellow());
+        for violation in &variable_shadowing_violations {
+            println!(
+                "   {} [S006] {}",
+                "->".red(),
+                violation.message.bold()
+            );
+            println!("      Location: {}", violation.location);
+            if let Some(suggestion) = &violation.suggestion {
+                println!("      Suggestion: {}", suggestion);
+            }
         }
     }
     let total_upgrade_findings: usize = upgrade_reports.iter().map(|r| r.findings.len()).sum();
@@ -716,6 +737,13 @@ pub(crate) fn analyze_single_file(
     }
     res.unhandled_results = r;
 
+    // Scan for variable shadowing using the rule system
+    let mut vs = analyzer.run_rule(content, "variable_shadowing");
+    for v in &mut vs {
+        v.location = format!("{}:{}", file_name, v.location);
+    }
+    res.variable_shadowing_violations = vs;
+
     let mut up = analyzer.analyze_upgrade_patterns(content);
     for f in &mut up.findings {
         f.location = format!("{}:{}", file_name, f.location);

tooling/sanctifier-core/src/rules/mod.rs

@@ -21,6 +21,8 @@ pub mod truncation_bounds;
 pub mod unhandled_result;
 /// Unsafe PRNG usage in state-critical code.
 pub mod unsafe_prng;
+/// Variable shadowing in nested scopes.
+pub mod variable_shadowing;
 /// Unused local variables.
 pub mod unused_variable;
 use serde::Serialize;
@@ -178,6 +180,7 @@ impl RuleRegistry {
         registry.register(reentrancy::ReentrancyRule::new());
         registry.register(truncation_bounds::TruncationBoundsRule::new());
         registry.register(unsafe_prng::UnsafePrngRule::new());
+        registry.register(variable_shadowing::VariableShadowingRule::new());
         registry
     }
 }