diff --git a/docs/detectors/README.md b/docs/detectors/README.md index 3807c54c..80a1333c 100644 --- a/docs/detectors/README.md +++ b/docs/detectors/README.md @@ -48,6 +48,7 @@ detector page and to the relevant [Glossary](../glossary.md) term. | [`tier_boundary_off_by_one`](tier_boundary_off_by_one.md) | [`S022`](../error-codes.md) | logic | Info | `if`/`else if` tier/rank ladder mixes strict and inclusive comparisons on the same variable | | [`wrong_auth_args`](wrong_auth_args.md) | [`S024`](../error-codes.md) | authentication | Medium | Internal function uses `require_auth()` instead of `require_auth_for_args()` | | [`reserve_withdrawal`](reserve_withdrawal.md) | [`S023`](../error-codes.md) | authorization | High | Missing strict authorization guard on reserve or treasury funds withdrawal | +| [`admin_event_missing`](admin_event_missing.md) | [`SANCT_ADMIN_EVENT_MISSING`](../error-codes.md) | events | Warning | Admin/config-change functions that mutate storage without emitting an event | ## Page anatomy diff --git a/docs/detectors/admin_event_missing.md b/docs/detectors/admin_event_missing.md new file mode 100644 index 00000000..435fb22d --- /dev/null +++ b/docs/detectors/admin_event_missing.md @@ -0,0 +1,79 @@ +# `admin_event_missing` — Admin/config function mutates storage without emitting an event + +| | | +| --- | --- | +| **Finding code** | [`SANCT_ADMIN_EVENT_MISSING`](../error-codes.md) | +| **Category** | events | +| **Severity** | Warning | +| **Source rule** | [`rules/admin_event_missing.rs`](../../tooling/sanctifier-core/src/rules/admin_event_missing.rs) | +| **Glossary** | [Event](../glossary.md#event) · [Persistent storage](../glossary.md#persistent-storage) | + +## What it catches + +A `#[contractimpl]` function whose name indicates an admin or configuration-change intent +(e.g. `set_`, `update_`, `change_`, `upgrade`, `pause`, `unpause`, `migrate`, `set_admin`, +`set_owner`, `configure`, `transfer_admin`) that performs a storage mutation — +`.set(…)`, `.update(…)`, or `.remove(…)` — **without** emitting a corresponding on-chain event. + +Off-chain monitors, indexers, dashboards, and governance tools track privileged changes via +events. A silent storage write makes those changes invisible, preventing real-time alerting +and complicating incident response. + +## Vulnerable example + +```rust +#[contractimpl] +impl Token { + pub fn set_admin(env: Env, new_admin: Address) { + // Writes storage but never emits an event — monitors are blind. + env.storage().instance().set(&DataKey::Admin, &new_admin); + } +} +``` + +## The fix + +Emit an event after every admin-level storage mutation: + +```rust +#[contractimpl] +impl Token { + pub fn set_admin(env: Env, new_admin: Address) { + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.events().publish( + (symbol_short!("admin"), symbol_short!("set_adm")), + new_admin, + ); + } +} +``` + +If an event emit is genuinely unnecessary for a specific function, suppress the finding: + +```rust +// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING] +pub fn migrate(env: Env) { + env.storage().instance().set(&DataKey::Version, &2u32); +} +``` + +## How Sanctifier detects it + +The rule uses a `syn::visit::Visit` pass. For every public function in a `#[contractimpl]` +block whose name matches the admin/config heuristic, it runs a body visitor that sets two +flags: `has_mutation` (`.set`/`.update`/`.remove` on a storage receiver) and `has_event` +(`.events()` in any receiver chain, or a call to `publish`/`emit`/`log`). A violation is +emitted only when `has_mutation && !has_event`. + +`#[cfg(test)]` modules and functions annotated with +`// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]` are skipped. + +**Limitations:** detection is name-based, so an admin function with an atypical name is a +false negative. A function that delegates its event emit to an opaque helper may also be +missed. Conversely, a function that genuinely does not need an event (e.g. an internal +migration guard) can be suppressed. + +## References + +- Soroban docs — [Events](https://soroban.stellar.org/docs/fundamentals-and-concepts/events) +- Related: [`auth_gap`](auth_gap.md), [`state_write_in_view`](state_write_in_view.md) diff --git a/tooling/sanctifier-core/src/finding_codes.rs b/tooling/sanctifier-core/src/finding_codes.rs index 0d543f44..b2bd2cfa 100644 --- a/tooling/sanctifier-core/src/finding_codes.rs +++ b/tooling/sanctifier-core/src/finding_codes.rs @@ -30,6 +30,7 @@ pub const UNBOUNDED_STORAGE: &str = "SANCT_UNBOUNDED_STORAGE"; pub const SANCT_VIEW_PANIC: &str = "SANCT_VIEW_PANIC"; pub const ALLOWANCE_RACE: &str = "SANCT_ALLOWANCE_RACE"; pub const STATE_WRITE_IN_VIEW: &str = "SANCT_STATE_WRITE_IN_VIEW"; +pub const SANCT_ADMIN_EVENT_MISSING: &str = "SANCT_ADMIN_EVENT_MISSING"; pub const DIVISION_BY_ZERO: &str = "S018"; pub const TIER_BOUNDARY_OFF_BY_ONE: &str = "S022"; pub const MISSING_RESERVE_AUTH: &str = "S023"; @@ -213,6 +214,11 @@ pub fn all_finding_codes() -> Vec { description: "Getter/view-style function performs a storage write; callers expect it to be read-only", }, + FindingCode { + code: SANCT_ADMIN_EVENT_MISSING, + category: "events", + description: "Admin/config-change function mutates storage without emitting a corresponding on-chain event", + }, FindingCode { code: DIVISION_BY_ZERO, category: "arithmetic", diff --git a/tooling/sanctifier-core/src/rules/admin_event_missing.rs b/tooling/sanctifier-core/src/rules/admin_event_missing.rs new file mode 100644 index 00000000..5bcb7be1 --- /dev/null +++ b/tooling/sanctifier-core/src/rules/admin_event_missing.rs @@ -0,0 +1,339 @@ +use crate::finding_codes::SANCT_ADMIN_EVENT_MISSING; +use crate::rules::{Rule, RuleViolation, Severity}; +#[allow(unused_imports)] +use syn::spanned::Spanned; +use syn::visit::Visit; +use syn::{parse_str, Attribute, File}; + +/// Detects admin/config-change functions that mutate storage without emitting an event. +/// +/// Soroban off-chain observers (indexers, UIs, monitoring systems) rely on events to +/// track privileged state changes. An admin function that silently updates storage +/// without an event emit leaves those consumers blind to the change. +/// +/// A suppression comment `// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]` on or +/// immediately before the function signature opts out explicitly. +pub struct AdminEventMissingRule; + +impl AdminEventMissingRule { + pub fn new() -> Self { + Self + } +} + +impl Default for AdminEventMissingRule { + fn default() -> Self { + Self::new() + } +} + +impl Rule for AdminEventMissingRule { + fn name(&self) -> &str { + "admin_event_missing" + } + + fn description(&self) -> &str { + "Detects admin/config-change functions that mutate storage without emitting a corresponding on-chain event" + } + + fn check(&self, source: &str) -> Vec { + let file = match parse_str::(source) { + Ok(file) => file, + Err(_) => return Vec::new(), + }; + + let mut visitor = AdminEventVisitor { + violations: Vec::new(), + suppressions: suppressions(source), + test_depth: 0, + }; + visitor.visit_file(&file); + visitor.violations + } + + fn as_any(&self) -> &dyn std::any::Any { + self + } +} + +// ── Name heuristic ──────────────────────────────────────────────────────────── + +/// Returns true when the lowercase name contains any admin/config-change keyword. +fn is_admin_fn_name(name: &str) -> bool { + const KEYWORDS: &[&str] = &[ + "set_", + "update_", + "change_", + "configure", + "pause", + "unpause", + "upgrade", + "transfer_admin", + "set_admin", + "set_owner", + "migrate", + ]; + let lower = name.to_lowercase(); + KEYWORDS.iter().any(|kw| lower.contains(kw)) +} + +// ── Storage-receiver detection ──────────────────────────────────────────────── + +/// True when the receiver chain of a method call touches a Soroban storage handle. +fn is_storage_receiver(expr: &syn::Expr) -> bool { + match expr { + syn::Expr::MethodCall(mc) => { + let m = mc.method.to_string(); + m == "persistent" + || m == "instance" + || m == "temporary" + || is_storage_receiver(&mc.receiver) + } + _ => false, + } +} + +// ── Event-chain detection ───────────────────────────────────────────────────── + +/// True when any method call in the receiver chain is named `events`. +fn chain_contains_events(expr: &syn::Expr) -> bool { + match expr { + syn::Expr::MethodCall(mc) => mc.method == "events" || chain_contains_events(&mc.receiver), + _ => false, + } +} + +// ── Function body visitor ───────────────────────────────────────────────────── + +/// Scans a single function body for storage mutations and event emits. +struct FunctionBodyVisitor { + has_mutation: bool, + has_event: bool, +} + +impl<'ast> Visit<'ast> for FunctionBodyVisitor { + fn visit_expr_method_call(&mut self, node: &'ast syn::ExprMethodCall) { + let method = node.method.to_string(); + + // Storage mutation: set/update/remove on a storage receiver chain. + if matches!(method.as_str(), "set" | "update" | "remove") + && is_storage_receiver(&node.receiver) + { + self.has_mutation = true; + } + + // Event emit: direct publish/emit/log call, or any call on an `.events()` chain. + if matches!(method.as_str(), "publish" | "emit" | "log" | "events") + || chain_contains_events(&node.receiver) + { + self.has_event = true; + } + + syn::visit::visit_expr_method_call(self, node); + } +} + +// ── File-level visitor ──────────────────────────────────────────────────────── + +struct AdminEventVisitor { + violations: Vec, + suppressions: Vec, + test_depth: usize, +} + +impl AdminEventVisitor { + fn in_test_module(&self) -> bool { + self.test_depth > 0 + } +} + +impl<'ast> Visit<'ast> for AdminEventVisitor { + fn visit_item_mod(&mut self, node: &'ast syn::ItemMod) { + let was_test = has_cfg_test(&node.attrs); + if was_test { + self.test_depth += 1; + } + + syn::visit::visit_item_mod(self, node); + + if was_test { + self.test_depth -= 1; + } + } + + fn visit_impl_item_fn(&mut self, node: &'ast syn::ImplItemFn) { + if self.in_test_module() { + syn::visit::visit_impl_item_fn(self, node); + return; + } + + let fn_name = node.sig.ident.to_string(); + + if !is_admin_fn_name(&fn_name) { + syn::visit::visit_impl_item_fn(self, node); + return; + } + + let mut body_visitor = FunctionBodyVisitor { + has_mutation: false, + has_event: false, + }; + body_visitor.visit_block(&node.block); + + if !body_visitor.has_mutation || body_visitor.has_event { + syn::visit::visit_impl_item_fn(self, node); + return; + } + + let fn_line = node.sig.ident.span().start().line; + + if is_suppressed(&self.suppressions, fn_line) { + syn::visit::visit_impl_item_fn(self, node); + return; + } + + self.violations.push( + RuleViolation::new( + SANCT_ADMIN_EVENT_MISSING, + Severity::Warning, + format!( + "{SANCT_ADMIN_EVENT_MISSING}: admin/config function `{fn_name}` mutates storage without emitting an event; add env.events().publish(...) to notify off-chain observers of the privileged state change" + ), + format!("{fn_name}:{fn_line}"), + ) + .with_suggestion( + "Add an event emit (e.g. env.events().publish((symbol_short!(\"admin\"), symbol_short!(\"\")), value)) before or after the storage mutation. If this function intentionally omits events, annotate it with `// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]`.".to_string(), + ), + ); + + syn::visit::visit_impl_item_fn(self, node); + } +} + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +fn has_cfg_test(attrs: &[Attribute]) -> bool { + attrs.iter().any(|attr| { + if !attr.path().is_ident("cfg") { + return false; + } + match &attr.meta { + syn::Meta::List(list) => list + .tokens + .to_string() + .split(|ch: char| !ch.is_alphanumeric() && ch != '_') + .any(|part| part == "test"), + _ => false, + } + }) +} + +fn suppressions(source: &str) -> Vec { + source + .lines() + .enumerate() + .filter_map(|(index, line)| { + line.contains("sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]") + .then_some(index + 1) + }) + .collect() +} + +fn is_suppressed(suppressions: &[usize], line: usize) -> bool { + suppressions.iter().any(|s| *s == line || *s + 1 == line) +} + +// ── Unit tests ──────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn flags_admin_fn_with_storage_write_and_no_event() { + let src = r#" + impl Contract { + pub fn set_admin(env: Env, new_admin: Address) { + env.storage().instance().set(&DataKey::Admin, &new_admin); + } + } + "#; + let violations = AdminEventMissingRule::new().check(src); + assert_eq!( + violations.len(), + 1, + "expected 1 violation, got: {violations:#?}" + ); + assert_eq!(violations[0].rule_name, SANCT_ADMIN_EVENT_MISSING); + assert!(violations[0].message.contains("set_admin")); + } + + #[test] + fn does_not_flag_when_event_emit_present() { + let src = r#" + impl Contract { + pub fn set_admin(env: Env, new_admin: Address) { + env.storage().instance().set(&DataKey::Admin, &new_admin); + env.events().publish((symbol_short!("admin"), symbol_short!("set")), new_admin); + } + } + "#; + let violations = AdminEventMissingRule::new().check(src); + assert!( + violations.is_empty(), + "expected no violations, got: {violations:#?}" + ); + } + + #[test] + fn does_not_flag_when_no_storage_mutation() { + let src = r#" + impl Contract { + pub fn set_admin(env: Env) -> Address { + env.storage().instance().get(&DataKey::Admin).unwrap() + } + } + "#; + let violations = AdminEventMissingRule::new().check(src); + assert!( + violations.is_empty(), + "expected no violations, got: {violations:#?}" + ); + } + + #[test] + fn respects_inline_suppression() { + let src = r#" + impl Contract { + // sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING] + pub fn set_admin(env: Env, new_admin: Address) { + env.storage().instance().set(&DataKey::Admin, &new_admin); + } + } + "#; + let violations = AdminEventMissingRule::new().check(src); + assert!( + violations.is_empty(), + "suppression should prevent violation: {violations:#?}" + ); + } + + #[test] + fn skips_cfg_test_modules() { + let src = r#" + #[cfg(test)] + mod tests { + impl Contract { + pub fn set_admin(env: Env, new_admin: Address) { + env.storage().instance().set(&DataKey::Admin, &new_admin); + } + } + } + "#; + let violations = AdminEventMissingRule::new().check(src); + assert!( + violations.is_empty(), + "functions inside #[cfg(test)] should be skipped: {violations:#?}" + ); + } +} diff --git a/tooling/sanctifier-core/src/rules/mod.rs b/tooling/sanctifier-core/src/rules/mod.rs index cb4e4fc5..83b82fb4 100644 --- a/tooling/sanctifier-core/src/rules/mod.rs +++ b/tooling/sanctifier-core/src/rules/mod.rs @@ -1,3 +1,4 @@ +pub mod admin_event_missing; pub mod allowance_race; pub mod arg_dos; pub mod arithmetic_overflow; @@ -177,6 +178,7 @@ impl RuleRegistry { registry.register(view_panic::ViewPanicRule::new()); registry.register(allowance_race::AllowanceRaceRule::new()); registry.register(state_write_in_view::StateWriteInViewRule::new()); + registry.register(admin_event_missing::AdminEventMissingRule::new()); registry.register(division_by_zero::DivisionByZeroRule::new()); registry.register(unsigned_underflow::UnsignedUnderflowRule::new()); registry.register(ledger_seconds::LedgerSecondsRule::new()); diff --git a/tooling/sanctifier-core/tests/detector_snapshots.rs b/tooling/sanctifier-core/tests/detector_snapshots.rs index 0a1e065c..d5c9af33 100644 --- a/tooling/sanctifier-core/tests/detector_snapshots.rs +++ b/tooling/sanctifier-core/tests/detector_snapshots.rs @@ -14,8 +14,8 @@ use sanctifier_core::rules::auth_gap::VisibilityLeakRule; use sanctifier_core::rules::{ - allowance_race::AllowanceRaceRule, arg_dos::ArgDosRule, - arithmetic_overflow::ArithmeticOverflowRule, auth_gap::AuthGapRule, + admin_event_missing::AdminEventMissingRule, allowance_race::AllowanceRaceRule, + arg_dos::ArgDosRule, arithmetic_overflow::ArithmeticOverflowRule, auth_gap::AuthGapRule, balance_equality::BalanceEqualityRule, division_by_zero::DivisionByZeroRule, edge_amount::EdgeAmountRule, error_code_collision::ErrorCodeCollisionRule, excessive_clone::ExcessiveCloneRule, fee_rounding::FeeRoundingRule, @@ -281,6 +281,15 @@ fn snapshot_unsigned_underflow() { ); } +#[test] +fn snapshot_admin_event_missing() { + assert_detector_snapshot( + "admin_event_missing", + &AdminEventMissingRule::new(), + include_str!("fixtures/detectors/admin_event_missing.rs"), + ); +} + #[test] fn unbounded_storage_detector_flags_only_uncapped_persistent_growth() { let findings = RuleRegistry::with_default_rules().run_by_name( diff --git a/tooling/sanctifier-core/tests/fixtures/corpus/differential-corpus.json b/tooling/sanctifier-core/tests/fixtures/corpus/differential-corpus.json index af84fec7..35972bca 100644 --- a/tooling/sanctifier-core/tests/fixtures/corpus/differential-corpus.json +++ b/tooling/sanctifier-core/tests/fixtures/corpus/differential-corpus.json @@ -8,7 +8,7 @@ "sanctifier.observed_codes is the set of finding codes the default RuleRegistry emits on the *vulnerable* fixture today. It is asserted by the harness and kept in lock-step with the gallery snapshots.", "slither.detectors / aderyn.detectors list the closest default detectors each tool ships for the same class (from their published detector catalogs). expected=true means that tool flags the class out of the box." ], - "sanctifier_rule_to_code": { + "sanctifier_rule_to_code": { "auth_gap": "S001", "panic_detection": "S002", "arithmetic_overflow": "S003", @@ -46,8 +46,11 @@ "shift_overflow": "SANCT_SHIFT_OVERFLOW", "SANCT_SHIFT_OVERFLOW": "SANCT_SHIFT_OVERFLOW", "tier_boundary_off_by_one": "S022", + "reserve_withdrawal": "S023", + "admin_event_missing": "SANCT_ADMIN_EVENT_MISSING", + "SANCT_ADMIN_EVENT_MISSING": "SANCT_ADMIN_EVENT_MISSING", "wrong_auth_args": "S024", - "reserve_withdrawal": "S023" + }, "overlap_legend": { "shared-covered": "Class has an EVM analogue and Sanctifier already flags it.", @@ -60,9 +63,16 @@ { "bug_class": "reinit", "title": "Re-initialization (unauthenticated initializer)", - "soroban": { "vulnerable": "reinit_vulnerable.rs", "fixed": "reinit_fixed.rs" }, + "soroban": { + "vulnerable": "reinit_vulnerable.rs", + "fixed": "reinit_fixed.rs" + }, "solidity": null, - "sanctifier": { "ideal_code": "S001", "observed_codes": ["S001"], "status": "flagged" }, + "sanctifier": { + "ideal_code": "S001", + "observed_codes": ["S001"], + "status": "flagged" + }, "slither": { "detectors": [], "expected": false }, "aderyn": { "detectors": ["unprotected-initializer"], "expected": true }, "overlap": "shared-covered", @@ -71,42 +81,98 @@ { "bug_class": "upgrade_auth", "title": "Unchecked upgrade authorization", - "soroban": { "vulnerable": "upgrade_auth_vulnerable.rs", "fixed": "upgrade_auth_fixed.rs" }, - "solidity": { "vulnerable": "unprotected_upgrade_vulnerable.sol", "fixed": "unprotected_upgrade_fixed.sol" }, - "sanctifier": { "ideal_code": "S010", "observed_codes": ["S001"], "status": "surfaced" }, + "soroban": { + "vulnerable": "upgrade_auth_vulnerable.rs", + "fixed": "upgrade_auth_fixed.rs" + }, + "solidity": { + "vulnerable": "unprotected_upgrade_vulnerable.sol", + "fixed": "unprotected_upgrade_fixed.sol" + }, + "sanctifier": { + "ideal_code": "S010", + "observed_codes": ["S001", "SANCT_ADMIN_EVENT_MISSING"], + "status": "surfaced" + }, "slither": { "detectors": ["unprotected-upgrade"], "expected": true }, - "aderyn": { "detectors": ["centralization-risk", "unprotected-initializer"], "expected": true }, + "aderyn": { + "detectors": ["centralization-risk", "unprotected-initializer"], + "expected": true + }, "overlap": "shared-covered", "notes": "No dedicated S010 upgrade detector yet; the underlying unauthenticated admin setter is surfaced via S001 auth_gap. Slither's unprotected-upgrade is the direct analogue." }, { "bug_class": "reentrancy", "title": "CEI violation / reentrancy", - "soroban": { "vulnerable": "reentrancy_vulnerable.rs", "fixed": "reentrancy_fixed.rs" }, - "solidity": { "vulnerable": "reentrancy_vulnerable.sol", "fixed": "reentrancy_fixed.sol" }, - "sanctifier": { "ideal_code": "S006", "observed_codes": [], "status": "planned" }, - "slither": { "detectors": ["reentrancy-eth", "reentrancy-no-eth", "reentrancy-benign"], "expected": true }, - "aderyn": { "detectors": ["state-change-after-external-call"], "expected": true }, + "soroban": { + "vulnerable": "reentrancy_vulnerable.rs", + "fixed": "reentrancy_fixed.rs" + }, + "solidity": { + "vulnerable": "reentrancy_vulnerable.sol", + "fixed": "reentrancy_fixed.sol" + }, + "sanctifier": { + "ideal_code": "S006", + "observed_codes": [], + "status": "planned" + }, + "slither": { + "detectors": [ + "reentrancy-eth", + "reentrancy-no-eth", + "reentrancy-benign" + ], + "expected": true + }, + "aderyn": { + "detectors": ["state-change-after-external-call"], + "expected": true + }, "overlap": "shared-sanctifier-gap", "notes": "Both EVM tools flag interaction-before-effect. Sanctifier has no CEI/reentrancy detector yet; the vulnerable fixture currently produces an empty result (the regression baseline for the planned S006 detector)." }, { "bug_class": "unbounded_loop", "title": "Unbounded loop / DoS by gas exhaustion", - "soroban": { "vulnerable": "unbounded_loop_vulnerable.rs", "fixed": "unbounded_loop_fixed.rs" }, - "solidity": { "vulnerable": "unbounded_loop_vulnerable.sol", "fixed": "unbounded_loop_fixed.sol" }, - "sanctifier": { "ideal_code": "SANCT_ARG_DOS", "observed_codes": ["SANCT_ARG_DOS"], "status": "flagged" }, - "slither": { "detectors": ["calls-loop", "costly-loop"], "expected": true }, - "aderyn": { "detectors": ["costly-operations-inside-loops"], "expected": true }, + "soroban": { + "vulnerable": "unbounded_loop_vulnerable.rs", + "fixed": "unbounded_loop_fixed.rs" + }, + "solidity": { + "vulnerable": "unbounded_loop_vulnerable.sol", + "fixed": "unbounded_loop_fixed.sol" + }, + "sanctifier": { + "ideal_code": "SANCT_ARG_DOS", + "observed_codes": ["SANCT_ARG_DOS"], + "status": "flagged" + }, + "slither": { + "detectors": ["calls-loop", "costly-loop"], + "expected": true + }, + "aderyn": { + "detectors": ["costly-operations-inside-loops"], + "expected": true + }, "overlap": "shared-covered", "notes": "Sanctifier flags caller-controlled Vec/Map argument iteration without a visible length cap via SANCT_ARG_DOS. Slither and Aderyn cover analogous EVM loop-cost patterns." }, { "bug_class": "missing_ttl", "title": "Missing storage TTL bump (state archival)", - "soroban": { "vulnerable": "missing_ttl_vulnerable.rs", "fixed": "missing_ttl_fixed.rs" }, + "soroban": { + "vulnerable": "missing_ttl_vulnerable.rs", + "fixed": "missing_ttl_fixed.rs" + }, "solidity": null, - "sanctifier": { "ideal_code": "S006", "observed_codes": ["S006"], "status": "flagged" }, + "sanctifier": { + "ideal_code": "S006", + "observed_codes": ["S006"], + "status": "flagged" + }, "slither": { "detectors": [], "expected": false }, "aderyn": { "detectors": [], "expected": false }, "overlap": "soroban-specific", @@ -115,9 +181,19 @@ { "bug_class": "weak_randomness", "title": "Weak / predictable randomness", - "soroban": { "vulnerable": "weak_randomness_vulnerable.rs", "fixed": "weak_randomness_fixed.rs" }, - "solidity": { "vulnerable": "weak_randomness_vulnerable.sol", "fixed": "weak_randomness_fixed.sol" }, - "sanctifier": { "ideal_code": "S006", "observed_codes": [], "status": "planned" }, + "soroban": { + "vulnerable": "weak_randomness_vulnerable.rs", + "fixed": "weak_randomness_fixed.rs" + }, + "solidity": { + "vulnerable": "weak_randomness_vulnerable.sol", + "fixed": "weak_randomness_fixed.sol" + }, + "sanctifier": { + "ideal_code": "S006", + "observed_codes": [], + "status": "planned" + }, "slither": { "detectors": ["weak-prng"], "expected": true }, "aderyn": { "detectors": ["weak-randomness"], "expected": true }, "overlap": "shared-sanctifier-gap", @@ -126,9 +202,16 @@ { "bug_class": "integer_overflow", "title": "Unchecked integer overflow", - "soroban": { "vulnerable": "integer_overflow_vulnerable.rs", "fixed": "integer_overflow_fixed.rs" }, + "soroban": { + "vulnerable": "integer_overflow_vulnerable.rs", + "fixed": "integer_overflow_fixed.rs" + }, "solidity": null, - "sanctifier": { "ideal_code": "S003", "observed_codes": ["S003"], "status": "flagged" }, + "sanctifier": { + "ideal_code": "S003", + "observed_codes": ["S003"], + "status": "flagged" + }, "slither": { "detectors": [], "expected": false }, "aderyn": { "detectors": [], "expected": false }, "overlap": "divergent-approach", @@ -137,9 +220,16 @@ { "bug_class": "allowance_race", "title": "Allowance race / approve TOCTOU", - "soroban": { "vulnerable": "allowance_race_vulnerable.rs", "fixed": "allowance_race_fixed.rs" }, + "soroban": { + "vulnerable": "allowance_race_vulnerable.rs", + "fixed": "allowance_race_fixed.rs" + }, "solidity": null, - "sanctifier": { "ideal_code": "SANCT_ALLOWANCE_RACE", "observed_codes": ["SANCT_ALLOWANCE_RACE"], "status": "flagged" }, + "sanctifier": { + "ideal_code": "SANCT_ALLOWANCE_RACE", + "observed_codes": ["SANCT_ALLOWANCE_RACE"], + "status": "flagged" + }, "slither": { "detectors": [], "expected": false }, "aderyn": { "detectors": [], "expected": false }, "overlap": "soroban-specific", @@ -148,9 +238,16 @@ { "bug_class": "oracle_staleness", "title": "Oracle price staleness", - "soroban": { "vulnerable": "oracle_staleness_vulnerable.rs", "fixed": "oracle_staleness_fixed.rs" }, + "soroban": { + "vulnerable": "oracle_staleness_vulnerable.rs", + "fixed": "oracle_staleness_fixed.rs" + }, "solidity": null, - "sanctifier": { "ideal_code": "S006", "observed_codes": [], "status": "planned" }, + "sanctifier": { + "ideal_code": "S006", + "observed_codes": [], + "status": "planned" + }, "slither": { "detectors": [], "expected": false }, "aderyn": { "detectors": [], "expected": false }, "overlap": "mutual-gap", @@ -159,11 +256,27 @@ { "bug_class": "confused_deputy", "title": "Confused-deputy authorization", - "soroban": { "vulnerable": "confused_deputy_vulnerable.rs", "fixed": "confused_deputy_fixed.rs" }, - "solidity": { "vulnerable": "confused_deputy_vulnerable.sol", "fixed": "confused_deputy_fixed.sol" }, - "sanctifier": { "ideal_code": "S001", "observed_codes": [], "status": "planned" }, - "slither": { "detectors": ["arbitrary-send-eth", "tx-origin"], "expected": true }, - "aderyn": { "detectors": ["arbitrary-from-in-transfer-from"], "expected": true }, + "soroban": { + "vulnerable": "confused_deputy_vulnerable.rs", + "fixed": "confused_deputy_fixed.rs" + }, + "solidity": { + "vulnerable": "confused_deputy_vulnerable.sol", + "fixed": "confused_deputy_fixed.sol" + }, + "sanctifier": { + "ideal_code": "S001", + "observed_codes": [], + "status": "planned" + }, + "slither": { + "detectors": ["arbitrary-send-eth", "tx-origin"], + "expected": true + }, + "aderyn": { + "detectors": ["arbitrary-from-in-transfer-from"], + "expected": true + }, "overlap": "shared-sanctifier-gap", "notes": "The vulnerable contract authenticates the wrong party (it calls require_auth, just on an attacker-influenced address), so the presence-only auth_gap detector stays silent (empty result). Refining S001 into a confused-deputy / arbitrary-from check is the action item; Aderyn's arbitrary-from-in-transfer-from is the closest analogue." } diff --git a/tooling/sanctifier-core/tests/fixtures/detectors/admin_event_missing.rs b/tooling/sanctifier-core/tests/fixtures/detectors/admin_event_missing.rs new file mode 100644 index 00000000..6a022dc5 --- /dev/null +++ b/tooling/sanctifier-core/tests/fixtures/detectors/admin_event_missing.rs @@ -0,0 +1,75 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Address, Env, Symbol}; + +// FIXTURE: admin_event_missing detector +// +// Admin/config-change functions must emit an on-chain event when they mutate +// storage. This fixture mixes violating functions with clean cases so the +// golden snapshot pins both true positives and intended exclusions. + +#[contract] +pub struct AdminContract; + +#[contractimpl] +impl AdminContract { + // Violation 1: set_admin writes instance storage, no event. + pub fn set_admin(env: Env, new_admin: Address) { + env.storage() + .instance() + .set(&Symbol::short("admin"), &new_admin); + } + + // Violation 2: pause writes persistent storage, no event. + pub fn pause(env: Env) { + env.storage() + .persistent() + .set(&Symbol::short("paused"), &true); + } + + // Violation 3: upgrade removes instance storage, no event. + pub fn upgrade(env: Env) { + env.storage().instance().remove(&Symbol::short("wasm_hash")); + } + + // Clean 1: update_config writes storage AND emits event via env.events().publish. + pub fn update_config(env: Env, value: u32) { + env.storage().instance().set(&Symbol::short("cfg"), &value); + env.events() + .publish((symbol_short!("admin"), symbol_short!("cfg_upd")), value); + } + + // Clean 2: set_owner writes storage AND emits event via publish. + pub fn set_owner(env: Env, owner: Address) { + env.storage() + .persistent() + .set(&Symbol::short("owner"), &owner); + env.events().publish( + (symbol_short!("admin"), symbol_short!("owner_set")), + owner.clone(), + ); + } + + // Clean 3: configure only reads storage — no storage write, no violation. + pub fn configure(env: Env) -> u32 { + env.storage() + .instance() + .get(&Symbol::short("cfg")) + .unwrap_or(0) + } + + // Suppressed: migrate has a write but is opted out. + // sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING] + pub fn migrate(env: Env) { + env.storage().instance().set(&Symbol::short("v"), &2u32); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + // This admin-named function writes storage inside #[cfg(test)] — must NOT be flagged. + fn set_admin_test_helper(env: &Env) { + env.storage().instance().set(&Symbol::short("admin"), &true); + } +} diff --git a/tooling/sanctifier-core/tests/fixtures/gallery/upgrade_auth_fixed.rs b/tooling/sanctifier-core/tests/fixtures/gallery/upgrade_auth_fixed.rs index d0a1657d..ed9d2d0e 100644 --- a/tooling/sanctifier-core/tests/fixtures/gallery/upgrade_auth_fixed.rs +++ b/tooling/sanctifier-core/tests/fixtures/gallery/upgrade_auth_fixed.rs @@ -11,10 +11,13 @@ pub struct UpgradeAuthFixed; #[contractimpl] impl UpgradeAuthFixed { - // FIX: require the current admin's authorization before reassigning it. + // FIX: require the current admin's authorization before reassigning it, + // and emit an on-chain event so off-chain tooling can track the change. pub fn set_upgrade_admin(env: Env, caller: Address, new_admin: Address) { caller.require_auth(); env.storage().instance().set(&ADMIN, &new_admin); env.storage().instance().extend_ttl(100, 1000); + env.events() + .publish((symbol_short!("adm_chg"),), new_admin.clone()); } } diff --git a/tooling/sanctifier-core/tests/snapshots/detector_snapshots__admin_event_missing.snap b/tooling/sanctifier-core/tests/snapshots/detector_snapshots__admin_event_missing.snap new file mode 100644 index 00000000..1e944765 --- /dev/null +++ b/tooling/sanctifier-core/tests/snapshots/detector_snapshots__admin_event_missing.snap @@ -0,0 +1,19 @@ +--- +source: tooling/sanctifier-core/tests/detector_snapshots.rs +expression: findings +--- +- rule_name: SANCT_ADMIN_EVENT_MISSING + severity: Warning + message: "SANCT_ADMIN_EVENT_MISSING: admin/config function `set_admin` mutates storage without emitting an event; add env.events().publish(...) to notify off-chain observers of the privileged state change" + location: "set_admin:16" + suggestion: "Add an event emit (e.g. env.events().publish((symbol_short!(\"admin\"), symbol_short!(\"\")), value)) before or after the storage mutation. If this function intentionally omits events, annotate it with `// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]`." +- rule_name: SANCT_ADMIN_EVENT_MISSING + severity: Warning + message: "SANCT_ADMIN_EVENT_MISSING: admin/config function `pause` mutates storage without emitting an event; add env.events().publish(...) to notify off-chain observers of the privileged state change" + location: "pause:23" + suggestion: "Add an event emit (e.g. env.events().publish((symbol_short!(\"admin\"), symbol_short!(\"\")), value)) before or after the storage mutation. If this function intentionally omits events, annotate it with `// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]`." +- rule_name: SANCT_ADMIN_EVENT_MISSING + severity: Warning + message: "SANCT_ADMIN_EVENT_MISSING: admin/config function `upgrade` mutates storage without emitting an event; add env.events().publish(...) to notify off-chain observers of the privileged state change" + location: "upgrade:30" + suggestion: "Add an event emit (e.g. env.events().publish((symbol_short!(\"admin\"), symbol_short!(\"\")), value)) before or after the storage mutation. If this function intentionally omits events, annotate it with `// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]`." diff --git a/tooling/sanctifier-core/tests/snapshots/gallery_snapshots__upgrade_auth_vulnerable.snap b/tooling/sanctifier-core/tests/snapshots/gallery_snapshots__upgrade_auth_vulnerable.snap index 76d1ced2..ecbae8d7 100644 --- a/tooling/sanctifier-core/tests/snapshots/gallery_snapshots__upgrade_auth_vulnerable.snap +++ b/tooling/sanctifier-core/tests/snapshots/gallery_snapshots__upgrade_auth_vulnerable.snap @@ -7,3 +7,8 @@ expression: findings message: "Function 'set_upgrade_admin' performs storage mutation without authentication" location: set_upgrade_admin suggestion: Add require_auth() or require_auth_for_args() before storage operations +- rule_name: SANCT_ADMIN_EVENT_MISSING + severity: Warning + message: "SANCT_ADMIN_EVENT_MISSING: admin/config function `set_upgrade_admin` mutates storage without emitting an event; add env.events().publish(...) to notify off-chain observers of the privileged state change" + location: "set_upgrade_admin:17" + suggestion: "Add an event emit (e.g. env.events().publish((symbol_short!(\"admin\"), symbol_short!(\"\")), value)) before or after the storage mutation. If this function intentionally omits events, annotate it with `// sanctifier:ignore[SANCT_ADMIN_EVENT_MISSING]`."