diff --git a/docs/issue-829-optimization-induced-auth-changes.md b/docs/issue-829-optimization-induced-auth-changes.md new file mode 100644 index 0000000..66d97b3 --- /dev/null +++ b/docs/issue-829-optimization-induced-auth-changes.md @@ -0,0 +1,45 @@ +# Detect Optimization-Induced Authorization Changes + +Closes #829 + +## Problem + +Autofix transformations (e.g. loop unrolling, function inlining, storage +consolidation) can restructure a Soroban function body enough that a +`require_auth()` / `require_auth_for_args()` call is dropped, moved past an +early return, or applied to the wrong `Address` parameter. Today nothing in +the autofix pipeline verifies that the *set* of enforced authorization checks +is unchanged before a suggested fix is accepted. + +## Design + +Add a new rule module `packages/rules/src/security/authorization/`, mirroring +the existing per-category layout under `packages/rules/src/security/` +(`constructors/`, `signatures/`, `emergency/`). It contributes an +`AuthorizationDiffRule` implementing the `SorobanRule` trait consumed by +`SorobanRuleEngine::add_rule` in `packages/rules/src/soroban/rule_engine.rs`. + +The rule runs twice: once against the pre-fix `SorobanContract` (from +`packages/rules/src/soroban/parser.rs`) and once against the autofix +candidate produced under `packages/autofix/validation/security/`. For each +`SorobanFunction` in `SorobanImpl.functions` it extracts the ordered list of +`require_auth`/`require_auth_for_args` call sites and the `Address` argument +each guards, then diffs the two lists per function name: + +- an entry present before and absent after -> authorization removed +- a guard whose argument changed -> authorization scope changed +- a guard now reachable only after another branch/return -> ordering hazard + +Diff results are emitted as `RuleViolation`s with +`ViolationSeverity::Critical` (via `crate::{RuleViolation, ViolationSeverity}`, +same pattern as `packages/rules/src/soroban/analyzer.rs`), which the autofix +validator in `packages/autofix/validation/security/` uses to reject the +candidate fix outright rather than merely warning. + +## Acceptance Criteria + +- [ ] `AuthorizationDiffRule` compares pre/post authorization call sites per function +- [ ] Removed, moved, or retargeted `require_auth*` calls are flagged +- [ ] Violations use `ViolationSeverity::Critical` and block the autofix, not just warn +- [ ] Rule is registered in `SorobanRuleEngine::with_default_rules` +- [ ] Regression fixtures cover: dropped auth, auth after early return, auth on wrong `Address` diff --git a/docs/issue-830-optimization-induced-storage-changes.md b/docs/issue-830-optimization-induced-storage-changes.md new file mode 100644 index 0000000..b5f2f52 --- /dev/null +++ b/docs/issue-830-optimization-induced-storage-changes.md @@ -0,0 +1,45 @@ +# Detect Optimization-Induced Storage Changes + +Closes #830 + +## Problem + +Storage-focused optimizations (batching writes, hoisting reads out of loops, +collapsing redundant `get`/`set` pairs — see +`packages/rules/src/optimization/storage/multiple_storage_reads.rs`) can +accidentally drop a required `env.storage().persistent().set(...)` call, or +introduce a new write that was never in the original contract. Neither case +is currently caught before a fix is applied to Soroban contracts, unlike the +existing rent-cost check in `packages/rules/soroban/src/storage-rent-check.ts`, +which only looks at TTL/rent, not write-set equivalence. + +## Design + +Add `packages/rules/src/soroban/storage_diff.rs`, registered in +`packages/rules/src/soroban/mod.rs` next to `inefficient_storage.rs`. It +walks each `SorobanFunction.raw_definition` (available on the AST types in +`packages/rules/src/soroban/mod.rs`) for calls into +`storage().persistent()`, `storage().temporary()`, and `storage().instance()` +`set`/`remove`/`extend_ttl` invocations, recording, per function: the storage +tier, the operation kind, and the key expression where it is a literal or a +simple identifier (falling back to "unanalyzable" otherwise, consistent with +requirement "track affected keys where analyzable"). + +The pre-fix and post-fix operation lists are diffed the same way as the +autofix validation flow used for `packages/autofix/validation/storage/`: + +- key present pre-fix, absent post-fix -> removed write (flagged) +- key absent pre-fix, present post-fix -> new/unexpected write (flagged) +- key present in both, tier or op kind changed -> persistence-behavior change + +Findings surface as `RuleViolation`s (`crate::RuleViolation`) with severity +`Warning` for unanalyzable keys and `Error` for confirmed removed/added +writes, consumed by `packages/autofix/validation/storage/`. + +## Acceptance Criteria + +- [ ] Storage operation lists extracted per function for persistent/temporary/instance tiers +- [ ] Removed writes flagged as `ViolationSeverity::Error` +- [ ] Newly introduced writes flagged as `ViolationSeverity::Error` +- [ ] Non-literal/unanalyzable keys reported at `Warning` rather than silently skipped +- [ ] Fixtures added under `packages/rules/soroban/tests/` mirroring `storage-rent-check.spec.ts` diff --git a/docs/issue-831-state-mutation-analyzer.md b/docs/issue-831-state-mutation-analyzer.md new file mode 100644 index 0000000..08467c0 --- /dev/null +++ b/docs/issue-831-state-mutation-analyzer.md @@ -0,0 +1,47 @@ +# Implement Soroban State Mutation Analyzer + +Closes #831 + +## Problem + +`packages/rules/src/soroban/analyzer.rs` (`SorobanAnalyzer`) currently +checks struct/impl-level issues (unused state variables, inefficient field +types, unbounded loops) but has no notion of *how often* or *along which +paths* a contract mutates its own state. Frequent or redundant mutations +increase Soroban resource-fee consumption without being visible in the +existing checks. Issues #829/#830 need a shared mutation model to diff +against; this issue provides that foundation. + +## Design + +Add a new module tree `packages/rules/src/soroban/state/` (`mod.rs` plus +`mutation_analyzer.rs`), following the same layout as the existing +`packages/rules/src/soroban/memory/` module +(`mod.rs` re-exporting `InefficientBytesAllocationRule`). + +`mutation_analyzer.rs` defines `StateMutationAnalyzer`, invoked from +`SorobanAnalyzer::analyze_implementation` in `analyzer.rs` alongside the +existing `analyze_function` calls. For each `SorobanFunction` it walks +`raw_definition` for `storage().*().set(...)` / `.update(...)` calls and +builds a `MutationPath { function: String, key: String, tier: StorageTier, +call_site_line: usize }` per occurrence, then groups paths by `(function, +key)` to compute a mutation count. + +A companion `SorobanRule` impl, `StateMutationRule`, registered via +`SorobanRuleEngine::add_rule` in `rule_engine.rs`, reports: + +- functions with more than N (configurable, default 3) mutations of the + same key on one execution path -> "repeated update" violation +- the single most expensive mutation path per function (by tier: persistent + > instance > temporary) surfaced as an informational `RuleViolation` + +This `MutationPath` model is the shared input both issue #830 (storage +diffing) and issue #832 (redundant-write detection) build on. + +## Acceptance Criteria + +- [ ] `StateMutationAnalyzer` collects per-function, per-key mutation paths across all storage tiers +- [ ] Repeated updates to the same key on one path are counted and reported +- [ ] Most expensive mutation path per function is identified and surfaced +- [ ] `StateMutationRule` registered in `SorobanRuleEngine::with_default_rules` +- [ ] Tests added under `packages/rules/soroban/tests/` covering single, repeated, and multi-tier mutations diff --git a/docs/issue-832-redundant-state-mutations.md b/docs/issue-832-redundant-state-mutations.md new file mode 100644 index 0000000..496867f --- /dev/null +++ b/docs/issue-832-redundant-state-mutations.md @@ -0,0 +1,46 @@ +# Detect Redundant Soroban State Mutations + +Closes #832 + +## Problem + +Writing a storage value that is identical to its current value still incurs +a full `SSTORE`-equivalent Soroban resource-fee charge, with no state benefit. +`packages/rules/src/soroban/redundant_clone.rs` already detects a related but +distinct pattern (unnecessary `.clone()` calls); no equivalent rule exists +for redundant storage writes, and conditional mutations (writes inside an +`if`/`match` arm) are not handled by any existing rule. + +## Design + +Add `packages/rules/src/soroban/state/redundant_mutation.rs` alongside +`mutation_analyzer.rs` from #831, reusing its `MutationPath` output rather +than re-parsing the contract. `RedundantMutationRule` (another `SorobanRule`, +registered next to `StateMutationRule` in `rule_engine.rs`) inspects each +`MutationPath` and: + +- compares the value expression being written against the last known read or + write of the same key on the same path (literal/identifier comparison, + same "analyzable vs. not" fallback used in issue #830's `storage_diff.rs`) + and flags an exact match as a redundant write +- for conditional mutations (call site inside an `if`/`else`/`match` arm, + detected from indentation/brace depth relative to the arm in + `raw_definition`), flags the write only when *every* branch writes the + same value, since a value-changing branch makes the write non-redundant +- for confirmed redundant writes, generates an optimization suggestion + string (e.g. "skip write: value unchanged from prior write at line N"), + attached to the `RuleViolation.suggestion` field used elsewhere in + `packages/rules/src/optimization/` + +Violations use `ViolationSeverity::Info` (optimization, not correctness) so +they surface as suggestions rather than blocking autofix, consistent with +how `packages/rules/src/optimization/storage/multiple_storage_reads.rs` +reports its findings. + +## Acceptance Criteria + +- [ ] Redundant writes (same key, same value as prior write/read) detected via `MutationPath` from #831 +- [ ] Conditional/branch-guarded writes analyzed per-branch, not flagged unless all branches redundant +- [ ] Optimization suggestion string generated and attached per violation +- [ ] `RedundantMutationRule` registered in `SorobanRuleEngine::with_default_rules` +- [ ] Tests added under `packages/rules/soroban/tests/` covering unconditional and conditional redundant writes