foundry-rs / foundry-rs/foundry
fix(lint): sender_aliases doesn't clear on reassignment (access-control lints can misclassify a broken guard as valid)
- Dominant language
- Rust
- Stars
- 10.6k
- Forks
- 2.6k
- Avg merge
- 16h 38m
- Merged PRs (30d)
- 511
Description
`crates/lint/src/sol/analysis/access_control.rs`'s `sender_aliases` tracks locals holding a `msg.sender`-derived value as an insert-only set: an assignment that reads the sender adds the local, but a later reassignment to something else never removes it. Before the `#16615` consolidation, this same logic (then in `missing_events_access_control.rs`) explicitly removed an alias on reassignment (`update_sender_alias_from_decl`/`update_sender_aliases_from_assignment`, both had an `else { sender_aliases.remove(...) }` branch) — the consolidation silently dropped that half.
Consequence: a modifier/function that reassigns a sender-derived local to an attacker-controlled value *before* comparing it is still recognized as a valid access-control guard:
```solidity
modifier onlyOwnerViaReassignedAlias(address newOwner) {
address caller = msg.sender;
caller = newOwner; // alias should die here - it doesn't
require(caller == owner, "not owner"); // attacker-controlled, wrongly treated as a real check
_;
}
```
Two lints depend on this: `missing-events-access-control` and `missing-events-arithmetic`, both via `is_protected`/`guard_vars`. Confirmed via `git show 3fe1f8f00~1:crates/lint/src/sol/low/missing_events_access_control.rs` that the pre-refactor code had the removal logic; the shared post-refactor version does not.
## Why this isn't a small fix
I attempted the obvious fix (add the missing `remove()` on reassignment) and put it through two rounds of adversarial review. Both rounds found the naive version makes things worse in a different dimension:
- **A single final "aliases" snapshot applied to every dominating statement** breaks the *opposite* ordering: a genuinely valid check *before* a later reassignment gets incorrectly un-recognized, since the reassignment removes the alias from the one shared final set used to evaluate every check.
- **Recomputing the alias set fresh per statement index** (`sender_aliases(hir, stmts[..i])`, "aliases as of just before statement i") fixes that straight-line case, but is still **flow-insensitive** across branches and loops: it visits every statement in the prefix through one shared mutable set regardless of path, so:
- a reassignment inside a branch that always reverts/exits still kills the alias for code after the `if`, even though every path that actually reaches that code still has the original value (new false negative), and
- a reassignment inside a branch that *doesn't* always execute (e.g. `if (flag) caller = msg.sender;`) gets treated as if it always happened (existing false positive, present with or without this fix).
- Loops have the equivalent zero-iteration and loop-carried-state problems.
A correct fix needs real must-alias dataflow: analyze each branch from the same incoming state, intersect the alias sets at fallthrough join points, exclude branches that provably exit (`branch_always_exits` already exists and could feed this), and handle loops as a fixed point (with `do-while`'s at-least-once semantics as a special case). That's a proper rewrite of this helper, not a one-line restoration, so I'm filing this rather than shipping a fix that trades one narrow false-negative for a different, less-narrow one.
## Severity
Low (matches the two lints' own declared severity) and the trigger is narrow — an attacker-facing alias has to be reassigned specifically *before* the comparison that uses it, which is an unusual (though real) pattern. Both affected lints are advisory (missing-events), not fund-safety-critical, but a broken "looks protected" classification is exactly the kind of false confidence a lint shouldn't produce.
## Repro
Any Solidity file with the modifier/function pair above, linted with `--only-lint missing-events-access-control`: `owner = newOwner;` inside `setOwnerViaReassignedAlias` should be flagged (no event, changed under a "guard" that doesn't actually check the caller) but currently isn't.
Contributor guide
Research direction
Start in crates/lint/src/sol/analysis/access_control.rs and trace sender_aliases through is_protected and guard_vars. Compare the pre-refactor removal logic with git show 3fe1f8f00~1:crates/lint/src/sol/low/missing_events_access_control.rs, then study branch_always_exits and the repro described in the issue. Done means reassignment invalidates aliases without misclassifying checks across branches and loops, and the provided Solidity case is flagged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, solidity
- Domain
- security, tooling
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100