fix(events): namespace OpSeen by authorizing caller (follow-up to #68/#95) - #96
Conversation
…95) #95 namespaced the profile OpSeen but left the events contract's OpSeen global. Permissionless entrypoints (submit, apply_to_bounty, add_funds) mark caller-supplied op_ids in the same namespace as privileged payout paths (select_winners, claim_prize, claim_milestone, start_cancel), so an attacker who observes a pending privileged op_id could front-run a permissionless call with it and revert the payout with OpAlreadySeen — the same cross-flow squat DoS #95 fixed next door. Key OpSeen by (authorizing caller, op_id): DataKey::OpSeen(Address, BytesN<32>). Each entrypoint passes the address it require_auth'd; the two permissionless cranks (process_cancel_batch, finalize_cancel) namespace under the contract's own address. A permissionless call now writes its own domain and can't burn a privileged op_id. OpSeen is temporary() storage, so the key-shape change needs no migration; old-shape entries expire. No public entrypoint signatures change. op_id idempotency is now per-domain: cross-caller reuse of the same op_id is allowed (and safe), true same-domain replay still reverts. Tests: op_id_security::permissionless_apply_cannot_squat_select_winners_op_id (attacker's apply with the owner's op_id no longer blocks the payout); prize_claim::op_id_replay_reverts reworked to a same-domain replay. 225 events + 66 profile green; make build OK (events 56,091 B < 64 KB); fmt clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe events contract now scopes temporary idempotency markers by an address or the contract address. Entry points move checks after authorization or domain resolution, update emitted applicant fields, and add regression coverage for cross-domain operation identifiers. ChangesDomain-scoped event idempotency
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ractmeta (#98) Version stamps had drifted and were internally inconsistent: events contractmeta said 1.2.0 while INITIAL_VERSION said 1.3.0, and neither reflected the public-surface / storage changes merged since (#86 submission cap, #88 manager two-step, #96 OpSeen namespacing). Profile was still 1.1.0 despite #95 namespacing its OpSeen. Bump both contracts coherently — INITIAL_VERSION, contractmeta, and the Cargo package version all set to: events 1.3.0 -> 1.4.0 (submission cap, manager two-step, OpSeen ns) profile 1.1.0 -> 1.2.0 (namespaced OpSeen) version()-asserting admin tests updated to match. 225 events + 66 profile tests green; make build OK (events 56,091 B, profile 15,893 B, both under the 64 KB ceiling); fmt clean.
Summary
Follow-up to #95. That PR namespaced the profile
OpSeenbut left the events contract'sOpSeenin a single global namespace. This closes the matching cross-flow squat DoS on the events side.Problem
Events
OpSeenwas keyed only byop_id(DataKey::OpSeen(BytesN<32>)), shared between permissionless entrypoints (submit,apply_to_bounty,add_funds) and privileged payout paths (select_winners,claim_prize,claim_milestone,start_cancel). An attacker who observes a pending privilegedop_idcan front-run a permissionless call with that sameop_id, mark it seen, and make the manager's payout revertOpAlreadySeen— the same class #95 fixed on the profile side, under the same observability assumption the #68 threat model already accepts.Fix
Namespace events
OpSeenby the authorizing caller:DataKey::OpSeen(Address, BytesN<32>).create_event/add_fundsrequire_auth'd caller)submit/withdraw_submission/apply_to_bounty/withdraw_applicationselect_winners/start_cancelclaim_prizeclaim_milestoneprocess_cancel_batch/finalize_cancel(permissionless cranks)A permissionless call now writes only its own domain and cannot burn a privileged op_id.
require_unseenmoved to just after eachrequire_authso the domain is the authenticated caller.Semantics / risk
OpSeenistemporary()storage → no migration; old-shape entries expire. It is not the lastDataKeyvariant, but changing a temporary-only variant's payload doesn't affect any persistent key's discriminant.Tests
op_id_security::permissionless_apply_cannot_squat_select_winners_op_id: an attacker'sapply_to_bountywith the owner'sselect_winnersop_id no longer blocks the payout (would revert pre-fix).prize_claim::op_id_replay_revertsreworked to a same-domain replay (one recipient, two positions) — the meaningful replay property under namespacing.Verification
cargo test: 225 events + 66 profile greenmake build(stellar-cli 27): events wasm 56,091 bytes (< 64 KB)cargo fmt --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests