From 13b7c7834bc5758127df2dc4948bd39cb511b87f Mon Sep 17 00:00:00 2001 From: Maverick Date: Sun, 31 May 2026 10:47:57 +0100 Subject: [PATCH] feat: Add S029 rule to detect require_auth instead of require_auth_for_args Implements detection for replay/scope-confusion attacks on multi-arg admin operations. - Add S029 finding code for require_auth_for_args violations - Implement RequireAuthForArgsRule with comprehensive detection logic - Add rule documentation with attack scenarios and mitigation strategies - Add test fixtures with vulnerable and safe code examples - Add 6 unit tests covering all edge cases - Add 2 CLI integration tests for JSON and NDJSON output - Update fixtures README to include S029 (and missing S025-S027) The rule flags public functions with 2+ Address parameters that use require_auth() instead of require_auth_for_args() when performing sensitive operations (storage mutations or external calls). Closes #753 --- contracts/fixtures/finding-codes/README.md | 50 ++-- .../s030_require_auth_for_args.rs | 40 +++ docs/rules/require-auth-for-args.md | 258 ++++++++++++++++ tooling/sanctifier-cli/tests/cli_tests.rs | 137 +++++++++ tooling/sanctifier-core/src/finding_codes.rs | 12 + tooling/sanctifier-core/src/rules/mod.rs | 3 + .../src/rules/require_auth_for_args.rs | 281 ++++++++++++++++++ 7 files changed, 758 insertions(+), 23 deletions(-) create mode 100644 contracts/fixtures/finding-codes/s030_require_auth_for_args.rs create mode 100644 docs/rules/require-auth-for-args.md create mode 100644 tooling/sanctifier-core/src/rules/require_auth_for_args.rs diff --git a/contracts/fixtures/finding-codes/README.md b/contracts/fixtures/finding-codes/README.md index 217b453b..d5bbc5f2 100644 --- a/contracts/fixtures/finding-codes/README.md +++ b/contracts/fixtures/finding-codes/README.md @@ -10,29 +10,33 @@ This directory contains fixture source files used as deterministic scan inputs f ## Fixture index -| Finding code | Fixture file | -| --- | --- | -| `S001` | `s001_authentication.rs` | -| `S002` | `s002_panic_handling.rs` | -| `S003` | `s003_arithmetic.rs` | -| `S004` | `s004_storage_limits.rs` | -| `S005` | `s005_storage_keys.rs` | -| `S006` | `s006_unsafe_patterns.rs` | -| `S007` | `s007_custom_rule.rs` | -| `S008` | `s008_events.rs` | -| `S009` | `s009_logic_result_handling.rs` | -| `S010` | `s010_upgrade_admin.rs` | -| `S011` | `s011_formal_verification.rs` | -| `S012` | `s012_token_interface.rs` | -| `S013` | `s013_reentrancy.rs` | -| `S014` | `s014_admin_trust.rs` | -| `S015` | `s015_secrets.rs` | -| `S016` | `s016_truncation.rs` | -| `S018` | `s018_unsafe_prng.rs` | -| `S019` | `s019_unchecked_calls.rs` | -| `S020` | `s020_missing_events.rs` | -| `S021` | `s021_storage_misuse.rs` | -| `S022` | `s022_raw_invoke_contract.rs` | +| Finding code | Fixture file | +| ------------ | ------------------------------- | +| `S001` | `s001_authentication.rs` | +| `S002` | `s002_panic_handling.rs` | +| `S003` | `s003_arithmetic.rs` | +| `S004` | `s004_storage_limits.rs` | +| `S005` | `s005_storage_keys.rs` | +| `S006` | `s006_unsafe_patterns.rs` | +| `S007` | `s007_custom_rule.rs` | +| `S008` | `s008_events.rs` | +| `S009` | `s009_logic_result_handling.rs` | +| `S010` | `s010_upgrade_admin.rs` | +| `S011` | `s011_formal_verification.rs` | +| `S012` | `s012_token_interface.rs` | +| `S013` | `s013_reentrancy.rs` | +| `S014` | `s014_admin_trust.rs` | +| `S015` | `s015_secrets.rs` | +| `S016` | `s016_truncation.rs` | +| `S018` | `s018_unsafe_prng.rs` | +| `S019` | `s019_unchecked_calls.rs` | +| `S020` | `s020_missing_events.rs` | +| `S021` | `s021_storage_misuse.rs` | +| `S022` | `s022_raw_invoke_contract.rs` | +| `S025` | `s025_missing_ttl_bump.rs` | +| `S026` | `s026_taint_propagation.rs` | +| `S027` | `s027_static_reentrancy.rs` | +| `S030` | `s030_require_auth_for_args.rs` | ## Usage diff --git a/contracts/fixtures/finding-codes/s030_require_auth_for_args.rs b/contracts/fixtures/finding-codes/s030_require_auth_for_args.rs new file mode 100644 index 00000000..e7893af6 --- /dev/null +++ b/contracts/fixtures/finding-codes/s030_require_auth_for_args.rs @@ -0,0 +1,40 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Address}; + +#[contract] +pub struct RequireAuthForArgsFixture; + +#[contractimpl] +impl RequireAuthForArgsFixture { + /// VULNERABLE: Multi-arg admin function using require_auth instead of require_auth_for_args + /// This allows replay attacks where a signature for one (caller, new_admin) pair + /// can be replayed with different new_admin values. + pub fn set_admin(env: Env, caller: Address, new_admin: Address) { + caller.require_auth(); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + + /// VULNERABLE: Three Address parameters with require_auth + /// Attacker can replay a transfer_from signature with different from/to addresses + pub fn transfer_from(env: Env, spender: Address, from: Address, to: Address, amount: i128) { + spender.require_auth(); + env.storage().instance().set(&symbol_short!("balance"), &amount); + } + + /// SAFE: Uses require_auth_for_args to bind auth to exact arguments + pub fn set_admin_safe(env: Env, caller: Address, new_admin: Address) { + caller.require_auth_for_args((new_admin.clone(),).into_val(&env)); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + + /// SAFE: Single Address parameter - require_auth is appropriate + pub fn set_owner(env: Env, owner: Address) { + owner.require_auth(); + env.storage().instance().set(&symbol_short!("owner"), &owner); + } + + /// SAFE: Read-only function with multiple Address params - no auth needed + pub fn check_permission(_env: Env, _user: Address, _admin: Address) -> bool { + true + } +} diff --git a/docs/rules/require-auth-for-args.md b/docs/rules/require-auth-for-args.md new file mode 100644 index 00000000..788be2c8 --- /dev/null +++ b/docs/rules/require-auth-for-args.md @@ -0,0 +1,258 @@ +# require_auth_for_args Rule (S030) + +## Overview + +The `require_auth_for_args` rule detects functions with multiple `Address` parameters that use `require_auth()` instead of `require_auth_for_args()`. This vulnerability enables replay and scope-confusion attacks on multi-argument admin operations. + +## Severity + +**High** - This is a critical security vulnerability that can lead to unauthorized operations and privilege escalation. + +## Description + +In Soroban smart contracts, authentication is crucial for privileged operations. While `require_auth()` verifies that a caller has authorized the transaction, it does **not** bind the authorization to specific function arguments. This creates a vulnerability in functions with multiple `Address` parameters. + +### The Problem + +When a function has multiple `Address` parameters (e.g., `set_admin(caller: Address, new_admin: Address)`), using `require_auth()` only verifies that `caller` signed _something_, but not _what_ they signed. An attacker can: + +1. **Replay Attack**: Capture a valid signature for one set of arguments and replay it with different arguments +2. **Scope Confusion**: Use a signature intended for one operation in a different context with different parameters + +### Why It Matters + +This vulnerability is particularly dangerous for: + +- **Admin operations**: `set_admin(caller, new_admin)` - attacker can change admin to any address +- **Multi-party transfers**: `transfer_from(spender, from, to, amount)` - attacker can redirect funds +- **Permission grants**: `grant_role(admin, user, role)` - attacker can grant arbitrary roles +- **Delegation**: `set_proposer(admin, proposer)` - attacker can change proposers + +## Detection Logic + +The rule flags functions that meet **all** of the following criteria: + +1. ✅ Function is `pub` (public) +2. ✅ Function has **2 or more** `Address` parameters (excluding `Env`) +3. ✅ Function performs sensitive operations: + - Storage mutations (`.set()`, `.update()`, `.remove()`, `.extend_ttl()`, `.bump()`) + - External contract calls (`.invoke_contract()`, `.try_invoke_contract()`, `.call()`) +4. ✅ Function uses `require_auth()` +5. ❌ Function does **NOT** use `require_auth_for_args()` + +## Examples + +### ❌ Vulnerable Code + +```rust +#[contractimpl] +impl AdminContract { + /// VULNERABLE: Attacker can replay signature with different new_admin + pub fn set_admin(env: Env, caller: Address, new_admin: Address) { + caller.require_auth(); // ❌ Only verifies caller signed, not what they signed + env.storage() + .instance() + .set(&symbol_short!("admin"), &new_admin); + } + + /// VULNERABLE: Attacker can redirect transfer to different addresses + pub fn transfer_from( + env: Env, + spender: Address, + from: Address, + to: Address, + amount: i128 + ) { + spender.require_auth(); // ❌ Doesn't bind to from/to/amount + // ... transfer logic + } +} +``` + +**Attack Scenario for `set_admin`:** + +1. Alice (legitimate admin) calls `set_admin(alice, bob)` to make Bob the new admin +2. Alice's signature authorizes the transaction +3. Attacker intercepts the transaction +4. Attacker replays Alice's signature but changes `new_admin` to attacker's address +5. Contract accepts it because `alice.require_auth()` only checks Alice signed, not what she signed +6. Attacker is now admin + +### ✅ Safe Code + +```rust +#[contractimpl] +impl AdminContract { + /// SAFE: Authorization bound to exact arguments + pub fn set_admin(env: Env, caller: Address, new_admin: Address) { + // Bind authorization to the new_admin argument + caller.require_auth_for_args( + (new_admin.clone(),).into_val(&env) + ); + env.storage() + .instance() + .set(&symbol_short!("admin"), &new_admin); + } + + /// SAFE: Authorization bound to all critical parameters + pub fn transfer_from( + env: Env, + spender: Address, + from: Address, + to: Address, + amount: i128 + ) { + // Bind authorization to from, to, and amount + spender.require_auth_for_args( + (from.clone(), to.clone(), amount).into_val(&env) + ); + // ... transfer logic + } +} +``` + +### ✅ Safe Code - Single Address Parameter + +```rust +#[contractimpl] +impl SimpleContract { + /// SAFE: Single Address parameter - require_auth is appropriate + pub fn set_owner(env: Env, owner: Address) { + owner.require_auth(); // ✅ OK for single-address functions + env.storage() + .instance() + .set(&symbol_short!("owner"), &owner); + } +} +``` + +**Why this is safe:** With only one `Address` parameter, there's no ambiguity about what's being authorized. The signature is inherently bound to the transaction context. + +## Mitigation Strategies + +### 1. Use `require_auth_for_args()` for Multi-Address Functions + +Replace `require_auth()` with `require_auth_for_args()` and pass all critical parameters: + +```rust +// Before (vulnerable) +caller.require_auth(); + +// After (safe) +caller.require_auth_for_args( + (new_admin.clone(), role.clone()).into_val(&env) +); +``` + +### 2. Include All Security-Critical Parameters + +Bind authorization to **all** parameters that affect the operation's security: + +```rust +pub fn approve_transfer( + env: Env, + owner: Address, + spender: Address, + amount: i128, + expiration: u64 +) { + // Include ALL critical parameters + owner.require_auth_for_args( + (spender.clone(), amount, expiration).into_val(&env) + ); + // ... approval logic +} +``` + +### 3. Understand the `.into_val(&env)` Conversion + +The arguments must be converted to Soroban's value representation: + +```rust +// Single argument - use tuple with trailing comma +caller.require_auth_for_args((arg1.clone(),).into_val(&env)); + +// Multiple arguments - use tuple +caller.require_auth_for_args((arg1.clone(), arg2, arg3).into_val(&env)); +``` + +## When `require_auth()` is Acceptable + +Use `require_auth()` (without args) when: + +1. **Single Address parameter**: No ambiguity about what's being authorized +2. **Read-only operations**: No state changes or external calls +3. **Private functions**: Not exposed as contract entry points + +## Related Rules + +- **S001 (AUTH_GAP)**: Detects missing authentication entirely +- **S024 (TRANSFER_FROM_NO_ALLOWANCE)**: Detects missing allowance checks in transfer_from +- **S013 (REENTRANCY)**: Detects reentrancy vulnerabilities + +## References + +- [Soroban Address Documentation](https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Address.html) +- [Soroban Authorization Guide](https://soroban.stellar.org/docs/learn/authorization) +- [require_auth_for_args API](https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Address.html#method.require_auth_for_args) + +## Testing + +Test fixtures demonstrating this vulnerability are available at: + +- `contracts/fixtures/finding-codes/s030_require_auth_for_args.rs` + +### Example Test + +```rust +#[test] +fn test_replay_attack_prevented() { + let env = Env::default(); + let contract = TestContractClient::new(&env, &env.register_contract(None, TestContract)); + + let alice = Address::generate(&env); + let bob = Address::generate(&env); + let attacker = Address::generate(&env); + + // Alice authorizes setting Bob as admin + contract.set_admin(&alice, &bob); + + // Attacker tries to replay with different address + // This should FAIL if require_auth_for_args is used correctly + let result = contract.try_set_admin(&alice, &attacker); + assert!(result.is_err()); +} +``` + +## Configuration + +This rule is enabled by default with **High** severity. To customize in `.sanctify.toml`: + +```toml +[rules.require_auth_for_args] +enabled = true +severity = "error" # or "warning", "info" +``` + +## False Positives + +This rule may flag functions where: + +- Multiple addresses serve different roles and replay is not a concern +- Additional validation logic prevents replay attacks + +In such cases, you can suppress the warning with: + +```rust +// sanctifier: ignore[require_auth_for_args] +pub fn special_case(env: Env, addr1: Address, addr2: Address) { + addr1.require_auth(); + // ... special validation logic +} +``` + +However, carefully review whether `require_auth_for_args()` would be more secure. + +## Summary + +Always use `require_auth_for_args()` for functions with multiple `Address` parameters that perform state changes or external calls. This binds authorization to the exact call payload, preventing replay and scope-confusion attacks that could lead to unauthorized operations and privilege escalation. diff --git a/tooling/sanctifier-cli/tests/cli_tests.rs b/tooling/sanctifier-cli/tests/cli_tests.rs index b4cad049..7a771bc0 100644 --- a/tooling/sanctifier-cli/tests/cli_tests.rs +++ b/tooling/sanctifier-cli/tests/cli_tests.rs @@ -813,3 +813,140 @@ fn test_ndjson_clean_file_emits_done() { assert_eq!(done["event"], "done"); assert_eq!(done["total_findings"], 0); } + +/// Test S029: require_auth_for_args rule detection +#[test] +fn test_s029_require_auth_for_args_detection() { + let dir = tempdir().unwrap(); + let contract = dir.path().join("s029_test.rs"); + + // Write a contract with vulnerable require_auth usage + fs::write( + &contract, + r#" +use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + +#[contract] +pub struct TestContract; + +#[contractimpl] +impl TestContract { + /// VULNERABLE: Multi-arg function using require_auth instead of require_auth_for_args + pub fn set_admin(env: Env, caller: Address, new_admin: Address) { + caller.require_auth(); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + + /// SAFE: Uses require_auth_for_args + pub fn set_admin_safe(env: Env, caller: Address, new_admin: Address) { + caller.require_auth_for_args((new_admin.clone(),).into_val(&env)); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + + /// SAFE: Single Address parameter + pub fn set_owner(env: Env, owner: Address) { + owner.require_auth(); + env.storage().instance().set(&symbol_short!("owner"), &owner); + } +} +"#, + ) + .unwrap(); + + let output = Command::cargo_bin("sanctifier") + .unwrap() + .args(["analyze", "--format", "json"]) + .arg(&contract) + .output() + .unwrap(); + + let stdout = String::from_utf8(output.stdout).unwrap(); + let json: Value = serde_json::from_str(&stdout).unwrap(); + + // Check that we found the require_auth_for_args violation + let violations = json["rule_violations"].as_array().unwrap(); + let s029_violations: Vec<&Value> = violations + .iter() + .filter(|v| v["rule_name"] == "require_auth_for_args") + .collect(); + + assert_eq!( + s029_violations.len(), + 1, + "Expected exactly 1 require_auth_for_args violation" + ); + + let violation = s029_violations[0]; + assert_eq!(violation["severity"], "Error"); + assert!( + violation["message"] + .as_str() + .unwrap() + .contains("require_auth_for_args"), + "Message should mention require_auth_for_args" + ); + assert!( + violation["location"].as_str().unwrap().contains("set_admin"), + "Violation should be in set_admin function" + ); +} + +/// Test S029 with NDJSON format +#[test] +fn test_s029_ndjson_format() { + let dir = tempdir().unwrap(); + let contract = dir.path().join("s029_ndjson.rs"); + + fs::write( + &contract, + r#" +use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + +#[contract] +pub struct VulnerableContract; + +#[contractimpl] +impl VulnerableContract { + pub fn transfer_from(env: Env, spender: Address, from: Address, to: Address, amount: i128) { + spender.require_auth(); + env.storage().instance().set(&symbol_short!("balance"), &amount); + } +} +"#, + ) + .unwrap(); + + let output = Command::cargo_bin("sanctifier") + .unwrap() + .args(["analyze", "--format", "ndjson"]) + .arg(&contract) + .output() + .unwrap(); + + let stdout = String::from_utf8(output.stdout).unwrap(); + let finding_lines: Vec = stdout + .lines() + .map(|l| serde_json::from_str(l).unwrap()) + .filter(|v: &Value| v["event"] == "finding") + .collect(); + + let s029_findings: Vec<&Value> = finding_lines + .iter() + .filter(|v| v["rule"] == "require_auth_for_args") + .collect(); + + assert!( + !s029_findings.is_empty(), + "Should detect require_auth_for_args violation in transfer_from" + ); + + let finding = s029_findings[0]; + assert_eq!(finding["severity"], "Error"); + assert!( + finding["message"] + .as_str() + .unwrap() + .contains("3 Address parameters"), + "Should mention 3 Address parameters" + ); +} diff --git a/tooling/sanctifier-core/src/finding_codes.rs b/tooling/sanctifier-core/src/finding_codes.rs index e5afdb85..f1e0c56a 100644 --- a/tooling/sanctifier-core/src/finding_codes.rs +++ b/tooling/sanctifier-core/src/finding_codes.rs @@ -81,6 +81,8 @@ pub const STATIC_REENTRANCY: &str = "S027"; pub const DEPRECATED_SDK_USAGE: &str = "S028"; /// Use of env.ledger().timestamp() as entropy for randomness. pub const TIMESTAMP_RANDOMNESS: &str = "S029"; +/// require_auth used instead of require_auth_for_args in multi-arg admin operations, enabling replay/scope-confusion attacks. +pub const REQUIRE_AUTH_FOR_ARGS: &str = "S030"; /// A single finding-code entry with machine-readable code, category, and /// human-readable description. @@ -376,6 +378,15 @@ pub fn all_finding_codes() -> Vec { remediation: "Never use env.ledger().timestamp() as a sole source of randomness. Use a VRF oracle or combine multiple unpredictable entropy sources", doc_url: "https://github.com/HyperSafeD/Sanctifier/blob/main/docs/rules/unsafe-prng.md", }, + FindingCode { + code: REQUIRE_AUTH_FOR_ARGS, + category: "authentication", + description: "Function with multiple Address parameters uses require_auth instead of require_auth_for_args, enabling replay/scope-confusion attacks on multi-arg admin operations", + title: "Missing require_auth_for_args", + severity: FindingSeverity::High, + remediation: "Replace require_auth() with require_auth_for_args() to bind authorization to the exact call payload, preventing replay attacks across different argument combinations", + doc_url: "https://github.com/HyperSafeD/Sanctifier/blob/main/docs/rules/require-auth-for-args.md", + }, ] } @@ -418,5 +429,6 @@ mod tests { assert!(codes.iter().any(|c| c.code == STATIC_REENTRANCY)); assert!(codes.iter().any(|c| c.code == DEPRECATED_SDK_USAGE)); assert!(codes.iter().any(|c| c.code == TIMESTAMP_RANDOMNESS)); + assert!(codes.iter().any(|c| c.code == REQUIRE_AUTH_FOR_ARGS)); } } diff --git a/tooling/sanctifier-core/src/rules/mod.rs b/tooling/sanctifier-core/src/rules/mod.rs index 7d2d015e..8f703320 100644 --- a/tooling/sanctifier-core/src/rules/mod.rs +++ b/tooling/sanctifier-core/src/rules/mod.rs @@ -52,6 +52,8 @@ pub mod static_reentrancy; pub mod deprecated_sdk_usage; /// Detect env.ledger().timestamp() used as entropy for randomness. pub mod timestamp_randomness; +/// require_auth used instead of require_auth_for_args in multi-arg admin operations. +pub mod require_auth_for_args; use serde::Serialize; use std::any::Any; @@ -221,6 +223,7 @@ impl RuleRegistry { registry.register(static_reentrancy::StaticReentrancyRule::new()); registry.register(deprecated_sdk_usage::DeprecatedSdkUsageRule::new()); registry.register(timestamp_randomness::TimestampRandomnessRule::new()); + registry.register(require_auth_for_args::RequireAuthForArgsRule::new()); registry } } diff --git a/tooling/sanctifier-core/src/rules/require_auth_for_args.rs b/tooling/sanctifier-core/src/rules/require_auth_for_args.rs new file mode 100644 index 00000000..051d88ee --- /dev/null +++ b/tooling/sanctifier-core/src/rules/require_auth_for_args.rs @@ -0,0 +1,281 @@ +use crate::rules::{Rule, RuleViolation, Severity}; +use syn::{parse_str, File, Item}; + +/// Rule S030 — detects functions with multiple Address parameters that use +/// require_auth instead of require_auth_for_args, enabling replay/scope-confusion +/// attacks on multi-arg admin operations. +pub struct RequireAuthForArgsRule; + +impl RequireAuthForArgsRule { + pub fn new() -> Self { + Self + } +} + +impl Default for RequireAuthForArgsRule { + fn default() -> Self { + Self::new() + } +} + +impl Rule for RequireAuthForArgsRule { + fn name(&self) -> &str { + "require_auth_for_args" + } + + fn description(&self) -> &str { + "Detects functions with multiple Address parameters that use require_auth instead of require_auth_for_args, enabling replay/scope-confusion attacks" + } + + fn check(&self, source: &str) -> Vec { + let file = match parse_str::(source) { + Ok(f) => f, + Err(_) => return vec![], + }; + + let mut violations = Vec::new(); + + for item in &file.items { + if let Item::Impl(impl_block) = item { + for impl_item in &impl_block.items { + if let syn::ImplItem::Fn(method) = impl_item { + // Only check public functions + if !matches!(method.vis, syn::Visibility::Public(_)) { + continue; + } + + let fn_name = method.sig.ident.to_string(); + + // Skip reserved Soroban entrypoints + if is_reserved_soroban_entrypoint(&fn_name) { + continue; + } + + // Count Address parameters (excluding Env) + let address_param_count = count_address_params(&method.sig); + + // Only flag functions with 2+ Address parameters + if address_param_count < 2 { + continue; + } + + let body = quote::quote!(#method.block).to_string(); + + // Check if function has state mutations or external calls + if !has_sensitive_operations(&body) { + continue; + } + + // Check if it uses require_auth but not require_auth_for_args + let has_require_auth = body.contains("require_auth()") + || body.contains("require_auth ("); + let has_require_auth_for_args = body.contains("require_auth_for_args"); + + if has_require_auth && !has_require_auth_for_args { + violations.push( + RuleViolation::new( + self.name(), + Severity::Error, + format!( + "Function '{}' has {} Address parameters and uses require_auth() instead of require_auth_for_args() — vulnerable to replay/scope-confusion attacks", + fn_name, address_param_count + ), + fn_name.clone(), + ) + .with_suggestion( + format!( + "Replace require_auth() with require_auth_for_args() to bind authorization to the exact call payload. \ + Example: address.require_auth_for_args((arg1, arg2, ...).into_val(&env)); \ + This prevents an attacker from replaying a signature with different argument combinations." + ) + ), + ); + } + } + } + } + } + + violations + } + + fn as_any(&self) -> &dyn std::any::Any { + self + } +} + +fn is_reserved_soroban_entrypoint(fn_name: &str) -> bool { + matches!(fn_name, "__constructor" | "__check_auth") +} + +fn count_address_params(sig: &syn::Signature) -> usize { + sig.inputs + .iter() + .filter(|arg| { + if let syn::FnArg::Typed(pt) = arg { + let ty_str = quote::quote!(#pt.ty).to_string(); + // Match Address type (with or without whitespace) + ty_str.contains("Address") && !ty_str.contains("Env") + } else { + false + } + }) + .count() +} + +fn has_sensitive_operations(body: &str) -> bool { + // Check for storage mutations + let has_storage_mutation = body.contains(".set(") + || body.contains(".update(") + || body.contains(".remove(") + || body.contains(".extend_ttl(") + || body.contains(".bump("); + + // Check for external contract calls + let has_external_call = body.contains("invoke_contract") + || body.contains("try_invoke_contract") + || body.contains(".call(") + || body.contains(".try_call("); + + has_storage_mutation || has_external_call +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_flags_multi_address_with_require_auth() { + let source = r#" + use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + + #[contract] + pub struct TestContract; + + #[contractimpl] + impl TestContract { + pub fn set_admin(env: Env, caller: Address, new_admin: Address) { + caller.require_auth(); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + } + "#; + + let rule = RequireAuthForArgsRule::new(); + let violations = rule.check(source); + assert_eq!(violations.len(), 1); + assert!(violations[0].message.contains("require_auth_for_args")); + } + + #[test] + fn test_allows_require_auth_for_args() { + let source = r#" + use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + + #[contract] + pub struct TestContract; + + #[contractimpl] + impl TestContract { + pub fn set_admin(env: Env, caller: Address, new_admin: Address) { + caller.require_auth_for_args((new_admin.clone(),).into_val(&env)); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + } + "#; + + let rule = RequireAuthForArgsRule::new(); + let violations = rule.check(source); + assert_eq!(violations.len(), 0); + } + + #[test] + fn test_ignores_single_address_param() { + let source = r#" + use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + + #[contract] + pub struct TestContract; + + #[contractimpl] + impl TestContract { + pub fn set_owner(env: Env, owner: Address) { + owner.require_auth(); + env.storage().instance().set(&symbol_short!("owner"), &owner); + } + } + "#; + + let rule = RequireAuthForArgsRule::new(); + let violations = rule.check(source); + assert_eq!(violations.len(), 0); + } + + #[test] + fn test_ignores_read_only_functions() { + let source = r#" + use soroban_sdk::{contract, contractimpl, Env, Address}; + + #[contract] + pub struct TestContract; + + #[contractimpl] + impl TestContract { + pub fn check_permission(env: Env, user: Address, admin: Address) -> bool { + user.require_auth(); + // No storage mutation, just reading + true + } + } + "#; + + let rule = RequireAuthForArgsRule::new(); + let violations = rule.check(source); + assert_eq!(violations.len(), 0); + } + + #[test] + fn test_flags_three_address_params() { + let source = r#" + use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + + #[contract] + pub struct TestContract; + + #[contractimpl] + impl TestContract { + pub fn transfer_from(env: Env, spender: Address, from: Address, to: Address, amount: i128) { + spender.require_auth(); + env.storage().instance().set(&symbol_short!("balance"), &amount); + } + } + "#; + + let rule = RequireAuthForArgsRule::new(); + let violations = rule.check(source); + assert_eq!(violations.len(), 1); + assert!(violations[0].message.contains("3 Address parameters")); + } + + #[test] + fn test_ignores_private_functions() { + let source = r#" + use soroban_sdk::{contract, contractimpl, Env, Address, symbol_short}; + + #[contract] + pub struct TestContract; + + #[contractimpl] + impl TestContract { + fn internal_set_admin(env: Env, caller: Address, new_admin: Address) { + caller.require_auth(); + env.storage().instance().set(&symbol_short!("admin"), &new_admin); + } + } + "#; + + let rule = RequireAuthForArgsRule::new(); + let violations = rule.check(source); + assert_eq!(violations.len(), 0); + } +}