diff --git a/contracts/shadowing-example/Cargo.toml b/contracts/shadowing-example/Cargo.toml new file mode 100644 index 0000000..a476a6c --- /dev/null +++ b/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 diff --git a/contracts/shadowing-example/src/lib.rs b/contracts/shadowing-example/src/lib.rs new file mode 100644 index 0000000..40b19e6 --- /dev/null +++ b/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 { + 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! + } +} diff --git a/tooling/sanctifier-cli/src/commands/analyze.rs b/tooling/sanctifier-cli/src/commands/analyze.rs index 6d45a91..50ace7a 100644 --- a/tooling/sanctifier-cli/src/commands/analyze.rs +++ b/tooling/sanctifier-cli/src/commands/analyze.rs @@ -98,6 +98,7 @@ pub(crate) struct FileAnalysisResult { pub(crate) truncation_bounds_issues: Vec, pub(crate) sep41_checked_contracts: Vec, pub(crate) sep41_issues: Vec, + pub(crate) variable_shadowing_violations: Vec, 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 = 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); diff --git a/tooling/sanctifier-core/src/rules/mod.rs b/tooling/sanctifier-core/src/rules/mod.rs index e11fed2..ba82b5c 100644 --- a/tooling/sanctifier-core/src/rules/mod.rs +++ b/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 } } diff --git a/tooling/sanctifier-core/src/rules/variable_shadowing.rs b/tooling/sanctifier-core/src/rules/variable_shadowing.rs new file mode 100644 index 0000000..5be6dcd --- /dev/null +++ b/tooling/sanctifier-core/src/rules/variable_shadowing.rs @@ -0,0 +1,453 @@ +use crate::rules::{Rule, RuleViolation, Severity}; +use std::collections::HashMap; +use syn::visit::{self, Visit}; +use syn::{parse_str, File, Local, Pat}; + +/// Rule that detects variable shadowing in nested scopes. +/// +/// Variable shadowing occurs when a variable declared in an inner scope +/// has the same name as a variable in an outer scope. This can lead to +/// logic bugs where the wrong variable is updated or accessed. +pub struct VariableShadowingRule; + +impl VariableShadowingRule { + /// Create a new instance. + pub fn new() -> Self { + Self + } +} + +impl Default for VariableShadowingRule { + fn default() -> Self { + Self::new() + } +} + +impl Rule for VariableShadowingRule { + fn name(&self) -> &str { + "variable_shadowing" + } + + fn description(&self) -> &str { + "Detects variable shadowing in nested scopes that can lead to logic bugs" + } + + fn check(&self, source: &str) -> Vec { + let file = match parse_str::(source) { + Ok(f) => f, + Err(_) => return vec![], + }; + + let mut visitor = ShadowingVisitor::new(); + visitor.visit_file(&file); + + visitor + .shadowing_violations + .into_iter() + .map(|(name, inner_span, outer_span)| { + let inner_line = inner_span.start().line; + let inner_col = inner_span.start().column; + let outer_line = outer_span.start().line; + + RuleViolation::new( + self.name(), + Severity::Warning, + format!( + "Variable '{}' shadows an outer variable declared at line {}", + name, outer_line + ), + format!("{}:{}", inner_line, inner_col), + ) + .with_suggestion(format!( + "Consider renaming the inner variable to avoid shadowing (e.g., '{}_inner', '{}_2')", + name, name + )) + }) + .collect() + } + + fn as_any(&self) -> &dyn std::any::Any { + self + } +} + +/// Visitor that tracks variable declarations across nested scopes. +struct ShadowingVisitor { + /// Stack of scopes, each containing a map of variable names to their spans. + /// The last element is the current (innermost) scope. + scope_stack: Vec>, + /// Detected shadowing violations: (name, inner_span, outer_span) + shadowing_violations: Vec<(String, proc_macro2::Span, proc_macro2::Span)>, + /// Current scope depth (for debugging) + depth: usize, +} + +impl ShadowingVisitor { + fn new() -> Self { + Self { + scope_stack: vec![HashMap::new()], + shadowing_violations: Vec::new(), + depth: 0, + } + } + + /// Enter a new scope. + fn enter_scope(&mut self) { + self.scope_stack.push(HashMap::new()); + self.depth += 1; + } + + /// Exit the current scope. + fn exit_scope(&mut self) { + self.scope_stack.pop(); + if self.depth > 0 { + self.depth -= 1; + } + } + + /// Add a variable declaration to the current scope. + /// If the variable shadows an outer variable, record a violation. + fn add_variable(&mut self, name: String, span: proc_macro2::Span) { + // Skip underscore-prefixed variables (intentionally unused) + if name.starts_with('_') { + return; + } + + // Skip common parameter names that are often reused + if matches!(name.as_str(), "env" | "e" | "self") { + return; + } + + // Check if this variable shadows an outer variable + for outer_scope in self.scope_stack.iter().rev().skip(1) { + if let Some(&outer_span) = outer_scope.get(&name) { + self.shadowing_violations + .push((name.clone(), span, outer_span)); + break; + } + } + + // Add to current scope + if let Some(current_scope) = self.scope_stack.last_mut() { + current_scope.insert(name, span); + } + } +} + +impl<'ast> Visit<'ast> for ShadowingVisitor { + fn visit_item_fn(&mut self, node: &'ast syn::ItemFn) { + self.enter_scope(); + + // Add function parameters to the scope + for input in &node.sig.inputs { + if let syn::FnArg::Typed(pat_type) = input { + if let Pat::Ident(pat_ident) = &*pat_type.pat { + let name = pat_ident.ident.to_string(); + if let Some(current_scope) = self.scope_stack.last_mut() { + current_scope.insert(name, pat_ident.ident.span()); + } + } + } + } + + visit::visit_item_fn(self, node); + self.exit_scope(); + } + + fn visit_impl_item_fn(&mut self, node: &'ast syn::ImplItemFn) { + self.enter_scope(); + + // Add function parameters to the scope + for input in &node.sig.inputs { + if let syn::FnArg::Typed(pat_type) = input { + if let Pat::Ident(pat_ident) = &*pat_type.pat { + let name = pat_ident.ident.to_string(); + if let Some(current_scope) = self.scope_stack.last_mut() { + current_scope.insert(name, pat_ident.ident.span()); + } + } + } + } + + visit::visit_impl_item_fn(self, node); + self.exit_scope(); + } + + fn visit_block(&mut self, node: &'ast syn::Block) { + self.enter_scope(); + visit::visit_block(self, node); + self.exit_scope(); + } + + fn visit_local(&mut self, node: &'ast Local) { + // Extract variable name from pattern + match &node.pat { + Pat::Ident(pat_ident) => { + let name = pat_ident.ident.to_string(); + self.add_variable(name, pat_ident.ident.span()); + } + Pat::Tuple(pat_tuple) => { + // Handle tuple destructuring: let (a, b) = ... + for elem in &pat_tuple.elems { + if let Pat::Ident(pat_ident) = elem { + let name = pat_ident.ident.to_string(); + self.add_variable(name, pat_ident.ident.span()); + } + } + } + _ => {} + } + + visit::visit_local(self, node); + } + + fn visit_expr_for_loop(&mut self, node: &'ast syn::ExprForLoop) { + self.enter_scope(); + + // Add loop variable to scope + if let Pat::Ident(pat_ident) = &*node.pat { + let name = pat_ident.ident.to_string(); + self.add_variable(name, pat_ident.ident.span()); + } + + visit::visit_expr_for_loop(self, node); + self.exit_scope(); + } + + fn visit_expr_closure(&mut self, node: &'ast syn::ExprClosure) { + self.enter_scope(); + + // Add closure parameters to scope + for input in &node.inputs { + if let Pat::Ident(pat_ident) = input { + let name = pat_ident.ident.to_string(); + self.add_variable(name, pat_ident.ident.span()); + } + } + + visit::visit_expr_closure(self, node); + self.exit_scope(); + } + + fn visit_expr_match(&mut self, node: &'ast syn::ExprMatch) { + // Visit the match expression first + visit::visit_expr(self, &node.expr); + + // Visit each arm - each arm gets its own scope for pattern bindings + for arm in &node.arms { + self.enter_scope(); + + // Extract pattern bindings and check for shadowing + extract_pattern_bindings(&arm.pat, self); + + // Visit the guard if present + if let Some((_, guard)) = &arm.guard { + visit::visit_expr(self, guard); + } + + // Visit the arm body + visit::visit_expr(self, &arm.body); + + self.exit_scope(); + } + } + + fn visit_arm(&mut self, node: &'ast syn::Arm) { + // Don't do anything here - we handle arms in visit_expr_match + // to ensure proper scoping + visit::visit_arm(self, node); + } +} + +/// Extract variable bindings from a pattern (used in match arms). +fn extract_pattern_bindings(pat: &Pat, visitor: &mut ShadowingVisitor) { + match pat { + Pat::Ident(pat_ident) => { + let name = pat_ident.ident.to_string(); + visitor.add_variable(name, pat_ident.ident.span()); + } + Pat::Tuple(pat_tuple) => { + for elem in &pat_tuple.elems { + extract_pattern_bindings(elem, visitor); + } + } + Pat::Struct(pat_struct) => { + for field in &pat_struct.fields { + extract_pattern_bindings(&field.pat, visitor); + } + } + Pat::TupleStruct(pat_tuple_struct) => { + for elem in &pat_tuple_struct.elems { + extract_pattern_bindings(elem, visitor); + } + } + _ => {} + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_detects_simple_shadowing() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn compute() -> u32 { + let x = 10; + { + let x = 20; // shadows outer x + x + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 1, "should detect one shadowing violation"); + assert!(violations[0].message.contains("'x'")); + assert!(violations[0].message.contains("shadows")); + } + + #[test] + fn test_detects_shadowing_in_nested_blocks() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn process() { + let value = 100; + if true { + let value = 200; // shadows outer value + println!("{}", value); + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 1); + assert!(violations[0].message.contains("'value'")); + } + + #[test] + fn test_detects_shadowing_in_for_loop() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn iterate() { + let i = 0; + for i in 0..10 { // shadows outer i + println!("{}", i); + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 1); + assert!(violations[0].message.contains("'i'")); + } + + #[test] + fn test_no_violation_for_different_scopes() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn compute() { + { + let x = 10; + } + { + let x = 20; // different scope, not shadowing + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 0, "sibling scopes should not trigger"); + } + + #[test] + fn test_no_violation_for_underscore_prefix() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn compute() { + let _x = 10; + { + let _x = 20; // underscore prefix, intentional + } + } + "#; + let violations = rule.check(source); + assert_eq!( + violations.len(), + 0, + "underscore-prefixed variables should be ignored" + ); + } + + #[test] + fn test_detects_shadowing_in_match_arm() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn handle(opt: Option) { + let value = 100; + match opt { + Some(value) => println!("{}", value), // shadows outer value + None => {} + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 1); + assert!(violations[0].message.contains("'value'")); + } + + #[test] + fn test_detects_shadowing_in_closure() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn process() { + let x = 10; + let closure = |x| { // shadows outer x + x + 1 + }; + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 1); + assert!(violations[0].message.contains("'x'")); + } + + #[test] + fn test_no_violation_for_env_parameter() { + let rule = VariableShadowingRule::new(); + let source = r#" + impl Contract { + pub fn transfer(env: Env) { + let env = env.clone(); // common pattern, should be ignored + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 0, "env parameter should be ignored"); + } + + #[test] + fn test_multiple_shadowing_violations() { + let rule = VariableShadowingRule::new(); + let source = r#" + fn complex() { + let x = 1; + let y = 2; + { + let x = 10; // shadows x + let y = 20; // shadows y + } + } + "#; + let violations = rule.check(source); + assert_eq!(violations.len(), 2, "should detect both shadowing cases"); + } + + #[test] + fn test_invalid_source_produces_no_panic() { + let rule = VariableShadowingRule::new(); + let violations = rule.check("not valid rust }{{{"); + assert_eq!( + violations.len(), + 0, + "parse error must return empty, not panic" + ); + } +}