diff --git a/packages/rules/src/lib.rs b/packages/rules/src/lib.rs index 01c845d..4cccbe5 100644 --- a/packages/rules/src/lib.rs +++ b/packages/rules/src/lib.rs @@ -11,11 +11,11 @@ pub mod vyper; // Explicitly export core types to avoid ambiguity pub use optimization::deployment::{estimate_bytecode_size, ExcessiveContractSizeRule}; pub use optimization::storage::{ - detect_packing_opportunities, find_consecutive_packable_groups, get_type_size, - is_packable_type, PackingOpportunity, VariableInfo, -}; -pub use rule_engine::{ - extract_struct_fields, find_variable_usage, Rule, RuleEngine, RuleViolation, ViolationSeverity, + detect_packing_opportunities, + find_consecutive_packable_groups, + get_type_size, + PackingOpportunity, + VariableInfo, }; pub use security::{HardcodedAddressesRule, MissingDomainSeparationRule}; pub use solidity::{StateVariablePackingRule, MappingIterationRule}; diff --git a/packages/rules/src/stellar/linting/gas_optimization_rules.rs b/packages/rules/src/stellar/linting/gas_optimization_rules.rs index 1a830bc..39ca843 100644 --- a/packages/rules/src/stellar/linting/gas_optimization_rules.rs +++ b/packages/rules/src/stellar/linting/gas_optimization_rules.rs @@ -34,14 +34,8 @@ impl SorobanLintRule for StorageReadRule { // Check for multiple .get() calls on same storage if line.contains(".get(") { // Look for repeated patterns - let func_lines = lines - .iter() - .skip(i.saturating_sub(20)) - .take(40) - .copied() - .collect::>() - .join("\n"); - + let func_lines = lines.iter().skip(i.saturating_sub(20)).take(40).copied().collect::>().join("\n"); + let get_count = func_lines.matches(".get(").count(); if get_count > 2 { violations.push(RuleViolation { @@ -95,20 +89,9 @@ impl SorobanLintRule for MapIterationRule { let lines: Vec<&str> = source.lines().collect(); for (i, line) in lines.iter().enumerate() { - if (line.contains("for ") || line.contains("while ")) - && (line.contains(".iter()") - || line.contains(".keys(") - || line.contains(".values(") - || line.contains(".entries(") - || line.contains(".range(")) - { - let window = lines - .iter() - .skip(i.saturating_sub(8)) - .take(20) - .copied() - .collect::>() - .join("\n"); + if (line.contains("for ") || line.contains("while ")) && + (line.contains(".iter()") || line.contains(".keys(") || line.contains(".values(") || line.contains(".entries(") || line.contains(".range(")) { + let window = lines.iter().skip(i.saturating_sub(8)).take(20).copied().collect::>().join("\n"); if window.contains("Map<") || window.contains("Map::") || window.contains(": Map") { violations.push(RuleViolation { rule_name: self.id().to_string(), @@ -191,17 +174,9 @@ impl SorobanLintRule for EventEmissionRule { let lines: Vec<&str> = source.lines().collect(); for (i, line) in lines.iter().enumerate() { - if line.contains("pub fn") - && (line.contains("transfer") || line.contains("mint") || line.contains("burn")) - { - let func_lines = lines - .iter() - .skip(i) - .take(20) - .copied() - .collect::>() - .join("\n"); - + if line.contains("pub fn") && (line.contains("transfer") || line.contains("mint") || line.contains("burn")) { + let func_lines = lines.iter().skip(i).take(20).copied().collect::>().join("\n"); + if !func_lines.contains("events().publish(") { violations.push(RuleViolation { rule_name: self.id().to_string(), @@ -247,7 +222,8 @@ impl MyContract { } "#; - let violations = MapIterationRule.check(source, "test.rs"); + let rule = MapIterationRule; + let violations = rule.check(source, "test.rs"); assert!(violations.is_some()); let violations = violations.unwrap(); assert!(violations @@ -270,7 +246,8 @@ impl MyContract { } "#; - let violations = MapIterationRule.check(source, "test.rs"); + let rule = MapIterationRule; + let violations = rule.check(source, "test.rs"); assert!(violations.is_none()); } } diff --git a/packages/rules/src/stellar/linting/mod.rs b/packages/rules/src/stellar/linting/mod.rs index ef4b947..f9f30e1 100644 --- a/packages/rules/src/stellar/linting/mod.rs +++ b/packages/rules/src/stellar/linting/mod.rs @@ -14,6 +14,7 @@ pub use soroban_rules::*; pub use stellar_sdk_rules::*; use crate::{RuleViolation, ViolationSeverity}; +use crate::stellar::unsafe_operations::UnsafeOperationsRule; /// Main linting engine for Soroban contracts pub struct SorobanLinter { @@ -35,7 +36,8 @@ impl SorobanLinter { rules.push(Box::new(gas_optimization_rules::MapIterationRule)); rules.push(Box::new(gas_optimization_rules::EventEmissionRule)); rules.push(Box::new(networking::NetworkValidationRule)); - + rules.push(Box::new(UnsafeOperationsRule)); + Self { rules } } diff --git a/packages/rules/src/stellar/linting/soroban_rules.rs b/packages/rules/src/stellar/linting/soroban_rules.rs index 2f2f997..248ea91 100644 --- a/packages/rules/src/stellar/linting/soroban_rules.rs +++ b/packages/rules/src/stellar/linting/soroban_rules.rs @@ -112,13 +112,7 @@ impl SorobanLintRule for EnvParameterRule { if line.contains("pub fn") && !line.contains("Env") { // Check if function body has storage operations if i + 1 < lines.len() { - let next_lines = lines - .iter() - .skip(i) - .take(10) - .copied() - .collect::>() - .join("\n"); + let next_lines = lines.iter().skip(i).take(10).copied().collect::>().join("\n"); if next_lines.contains(".set(") || next_lines.contains(".put(") { violations.push(RuleViolation { rule_name: self.id().to_string(), diff --git a/packages/rules/src/stellar/linting/stellar_sdk_rules.rs b/packages/rules/src/stellar/linting/stellar_sdk_rules.rs index 31908aa..3368039 100644 --- a/packages/rules/src/stellar/linting/stellar_sdk_rules.rs +++ b/packages/rules/src/stellar/linting/stellar_sdk_rules.rs @@ -114,18 +114,11 @@ impl SorobanLintRule for AddressValidationRule { // Check for functions with Address parameters that might need validation if line.contains("Address") && (line.contains("pub fn") || line.contains("fn ")) { // Look ahead to see if there's validation - let next_lines = lines - .iter() - .skip(i) - .take(15) - .copied() - .collect::>() - .join("\n"); - - if !next_lines.contains("require_auth") - && !next_lines.contains("require_auth_for_args") - && !next_lines.contains("try_from_xdr") - { + let next_lines = lines.iter().skip(i).take(15).copied().collect::>().join("\n"); + + if !next_lines.contains("require_auth") && + !next_lines.contains("require_auth_for_args") && + !next_lines.contains("try_from_xdr") { violations.push(RuleViolation { rule_name: self.id().to_string(), description: diff --git a/packages/rules/src/stellar/mod.rs b/packages/rules/src/stellar/mod.rs index ea6bfc2..d894866 100644 --- a/packages/rules/src/stellar/mod.rs +++ b/packages/rules/src/stellar/mod.rs @@ -3,7 +3,9 @@ //! This module provides Stellar and Soroban-specific analysis capabilities pub mod linting; +pub mod unsafe_operations; pub mod upgradeability; pub use linting::*; +pub use unsafe_operations::*; pub use upgradeability::*; diff --git a/packages/rules/src/stellar/unsafe_operations/mod.rs b/packages/rules/src/stellar/unsafe_operations/mod.rs new file mode 100644 index 0000000..ca66243 --- /dev/null +++ b/packages/rules/src/stellar/unsafe_operations/mod.rs @@ -0,0 +1,417 @@ +//! Unsafe Operations Detection Rule +//! +//! Detects potentially unsafe operations in Soroban contracts that may +//! introduce security vulnerabilities or unexpected behavior. + +use crate::stellar::linting::SorobanLintRule; +use crate::{RuleViolation, ViolationSeverity}; + +/// Rule to detect unsafe operations in Soroban contracts +pub struct UnsafeOperationsRule; + +impl UnsafeOperationsRule { + fn detect_panic_operations(source: &str, file_path: &str) -> Vec { + let mut violations = Vec::new(); + let lines: Vec<&str> = source.lines().collect(); + + for (i, line) in lines.iter().enumerate() { + if line.contains(".unwrap(") && !line.contains("//") + { + violations.push(RuleViolation { + rule_name: "unsafe-unwrap".to_string(), + description: "Use of `.unwrap()` may cause contract panic on error".to_string(), + severity: ViolationSeverity::High, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Replace `.unwrap()` with pattern matching or `.unwrap_or()` / `.unwrap_or_else()` with a safe fallback. Example: `value.unwrap_or(Default::default())`".to_string(), + }); + } + + if line.contains(".expect(") && !line.contains("//") + { + violations.push(RuleViolation { + rule_name: "unsafe-expect".to_string(), + description: "Use of `.expect()` may cause contract panic with custom message".to_string(), + severity: ViolationSeverity::High, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Replace `.expect()` with pattern matching. Example: `match value { Some(v) => v, None => return Err(Error::NotFound) }`".to_string(), + }); + } + + let panic_patterns = ["panic!()", "unreachable!()", "todo!()", "unimplemented!()"]; + for pattern in &panic_patterns { + if line.contains(pattern) && !line.contains("//") + { + violations.push(RuleViolation { + rule_name: "unsafe-panic".to_string(), + description: format!("Use of `{}` will cause contract panic", pattern), + severity: ViolationSeverity::Critical, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Avoid panic in contract code. Return proper error types instead. Example: `return Err(Error::Unexpected)`".to_string(), + }); + } + } + } + + violations + } + + fn detect_unsafe_blocks(source: &str, file_path: &str) -> Vec { + let mut violations = Vec::new(); + let lines: Vec<&str> = source.lines().collect(); + + for (i, line) in lines.iter().enumerate() { + let trimmed = line.trim(); + if trimmed == "unsafe {" || trimmed.starts_with("unsafe {") { + violations.push(RuleViolation { + rule_name: "unsafe-block".to_string(), + description: "Use of `unsafe` block bypasses Rust safety guarantees".to_string(), + severity: ViolationSeverity::Critical, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Avoid unsafe blocks in Soroban contracts. If absolutely necessary, isolate in a minimal helper and document safety invariants thoroughly.".to_string(), + }); + } + } + + violations + } + + fn detect_unprotected_invoke(source: &str, file_path: &str) -> Vec { + let mut violations = Vec::new(); + let lines: Vec<&str> = source.lines().collect(); + + for (i, line) in lines.iter().enumerate() { + if line.contains("invoke_contract") || line.contains("env.invoke()") { + let context_start = i.saturating_sub(15); + let context: Vec<&str> = lines.iter().skip(context_start).take(20).copied().collect(); + let context_str = context.join("\n"); + + let has_auth = context_str.contains("require_auth") + || context_str.contains("require_auth_for") + || context_str.contains("check_auth") + || context_str.contains("authorize"); + + if !has_auth { + violations.push(RuleViolation { + rule_name: "unprotected-invoke".to_string(), + description: "Contract invocation without prior authorization check".to_string(), + severity: ViolationSeverity::Critical, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Add `require_auth()` or `require_auth_for()` before invoking external contracts to ensure proper authorization.".to_string(), + }); + } + } + } + + violations + } + + fn detect_unchecked_arithmetic(source: &str, file_path: &str) -> Vec { + let mut violations = Vec::new(); + let lines: Vec<&str> = source.lines().collect(); + + for (i, line) in lines.iter().enumerate() { + if line.contains("pub fn") { + let func_lines: Vec<&str> = lines.iter().skip(i).take(30).copied().collect(); + let func_str = func_lines.join("\n"); + + let has_arithmetic = func_str.contains(" + ") || func_str.contains(" - ") + || func_str.contains(" * ") || func_str.contains(" / "); + + if has_arithmetic { + let has_checked = func_str.contains("checked_") + || func_str.contains("overflowing_") + || func_str.contains("saturating_") + || func_str.contains(".wrapping_"); + + let has_u256 = func_str.contains("u256") || func_str.contains("i256") + || func_str.contains("BigInt"); + + if !has_checked && !has_u256 { + violations.push(RuleViolation { + rule_name: "unchecked-arithmetic".to_string(), + description: "Function uses raw arithmetic operations that may overflow".to_string(), + severity: ViolationSeverity::High, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Use checked arithmetic methods: `a.checked_add(b).ok_or(Error::Overflow)?` or `a.saturating_add(b)` to prevent overflow panics.".to_string(), + }); + } + } + } + } + + violations + } + + fn detect_unbounded_loops(source: &str, file_path: &str) -> Vec { + let mut violations = Vec::new(); + let lines: Vec<&str> = source.lines().collect(); + + for (i, line) in lines.iter().enumerate() { + let trimmed = line.trim(); + if trimmed == "loop {" || trimmed.starts_with("loop ") { + let context: Vec<&str> = lines.iter().skip(i).take(20).copied().collect(); + let context_str = context.join("\n"); + + let has_break = context_str.contains("break") || context_str.contains("return"); + + if !has_break { + violations.push(RuleViolation { + rule_name: "unbounded-loop".to_string(), + description: "Unbounded `loop` without break condition may cause gas exhaustion".to_string(), + severity: ViolationSeverity::High, + line_number: i + 1, + column_number: 0, + variable_name: file_path.to_string(), + suggestion: "Add a `break` condition or use bounded iteration (e.g., `for i in 0..max_iterations`). Consider Soroban's CPU instruction limit.".to_string(), + }); + } + } + } + + violations + } +} + +impl SorobanLintRule for UnsafeOperationsRule { + fn id(&self) -> &'static str { + "soroban-unsafe-operations" + } + + fn name(&self) -> &'static str { + "Soroban Unsafe Operations" + } + + fn description(&self) -> &'static str { + "Detects potentially unsafe operations including panic-prone code, unchecked arithmetic, unsafe blocks, unauthorized contract invocations, and unbounded loops." + } + + fn severity(&self) -> ViolationSeverity { + ViolationSeverity::Critical + } + + fn check(&self, source: &str, file_path: &str) -> Option> { + let mut violations = Vec::new(); + + violations.extend(Self::detect_panic_operations(source, file_path)); + violations.extend(Self::detect_unsafe_blocks(source, file_path)); + violations.extend(Self::detect_unprotected_invoke(source, file_path)); + violations.extend(Self::detect_unchecked_arithmetic(source, file_path)); + violations.extend(Self::detect_unbounded_loops(source, file_path)); + + if violations.is_empty() { + None + } else { + Some(violations) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_detects_unwrap() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn do_something(env: Env) { + let value = storage.get(&env, &key).unwrap(); + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unsafe-unwrap")); + } + + #[test] + fn test_detects_expect() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn do_something(env: Env) { + let value = some_result.expect("should exist"); + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unsafe-expect")); + } + + #[test] + fn test_detects_panic() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn do_something(env: Env) { + panic!("unexpected"); + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unsafe-panic")); + } + + #[test] + fn test_detects_unreachable() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn do_something(env: Env) { + unreachable!(); + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unsafe-panic")); + } + + #[test] + fn test_detects_unsafe_block() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn do_something(env: Env) { + unsafe { + let ptr = &raw_val as *const u64; + } + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unsafe-block")); + } + + #[test] + fn test_detects_unprotected_invoke() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env, Address}; + +#[contractimpl] +impl MyContract { + pub fn call_other(env: Env, contract: Address, amount: u64) { + env.invoke_contract(&contract, "transfer", (amount,)); + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unprotected-invoke")); + } + + #[test] + fn test_does_not_flag_protected_invoke() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env, Address}; + +#[contractimpl] +impl MyContract { + pub fn call_other(env: Env, contract: Address, amount: u64) { + contract.require_auth(); + env.invoke_contract(&contract, "transfer", (amount,)); + } +} +"#; + let violations = rule.check(source, "test.rs"); + if let Some(viols) = violations { + let invoke_violations: Vec<_> = viols.iter().filter(|v| v.rule_name == "unprotected-invoke").collect(); + assert!(invoke_violations.is_empty()); + } + } + + #[test] + fn test_detects_unchecked_arithmetic() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn add_values(env: Env, a: u64, b: u64) -> u64 { + a + b + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_some()); + let violations = violations.unwrap(); + assert!(violations.iter().any(|v| v.rule_name == "unchecked-arithmetic")); + } + + #[test] + fn test_does_not_flag_checked_arithmetic() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env}; + +#[contractimpl] +impl MyContract { + pub fn add_values(env: Env, a: u64, b: u64) -> u64 { + a.checked_add(b).unwrap_or(0) + } +} +"#; + let violations = rule.check(source, "test.rs"); + if let Some(viols) = violations { + let arith_violations: Vec<_> = viols.iter().filter(|v| v.rule_name == "unchecked-arithmetic").collect(); + assert!(arith_violations.is_empty()); + } + } + + #[test] + fn test_safe_contract_no_violations() { + let rule = UnsafeOperationsRule; + let source = r#" +use soroban_sdk::{contract, contractimpl, Env, Address}; + +#[contractimpl] +impl MyContract { + pub fn safe_op(env: Env, a: u64, b: u64) -> u64 { + a.checked_add(b).unwrap_or(0) + } +} +"#; + let violations = rule.check(source, "test.rs"); + assert!(violations.is_none()); + } +} diff --git a/packages/rules/src/stellar/upgradeability/mod.rs b/packages/rules/src/stellar/upgradeability/mod.rs index 3395bce..f39a068 100644 --- a/packages/rules/src/stellar/upgradeability/mod.rs +++ b/packages/rules/src/stellar/upgradeability/mod.rs @@ -41,16 +41,17 @@ impl UpgradeCompatibilityChecker for DefaultUpgradeChecker { let old_schemas = SchemaAnalyzer::extract_schemas(old_code); let new_schemas = SchemaAnalyzer::extract_schemas(new_code); - let new_schema_map: std::collections::HashMap<&str, _> = new_schemas + let new_schema_map: std::collections::HashMap = new_schemas .iter() - .map(|s| (s.struct_name.as_str(), s)) + .enumerate() + .map(|(i, s)| (s.struct_name.clone(), i)) .collect(); let mut all_issues = Vec::new(); - for old_schema in old_schemas { - if let Some(new_schema) = new_schema_map.get(old_schema.struct_name.as_str()) { - let issues = SchemaAnalyzer::detect_incompatibilities(&old_schema, new_schema); + for old_schema in &old_schemas { + if let Some(&idx) = new_schema_map.get(&old_schema.struct_name) { + let issues = SchemaAnalyzer::detect_incompatibilities(old_schema, &new_schemas[idx]); all_issues.extend(issues); } } diff --git a/packages/rules/src/stellar/upgradeability/serialization_rules.rs b/packages/rules/src/stellar/upgradeability/serialization_rules.rs index 80946ee..97d4c25 100644 --- a/packages/rules/src/stellar/upgradeability/serialization_rules.rs +++ b/packages/rules/src/stellar/upgradeability/serialization_rules.rs @@ -29,9 +29,9 @@ impl SerializationUpgradeCompatibilityRule { .collect(); // Check each old schema for compatibility with new version - for old_schema in old_schemas { + for old_schema in &old_schemas { if let Some(new_schema) = new_schema_map.get(old_schema.struct_name.as_str()) { - let issues = SchemaAnalyzer::detect_incompatibilities(&old_schema, new_schema); + let issues = SchemaAnalyzer::detect_incompatibilities(old_schema, *new_schema); for issue in issues { let violation = self.issue_to_violation(&issue, file_path);