fix: non-malleable child op_id derivation and namespaced OpSeen - #95
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe change hardens cross-contract idempotency by hashing events child operation IDs with a profile-contract domain separator and by scoping profile replay markers to domains. New tests cover collision resistance, front-running resistance, and true replay rejection. ChangesIdempotency hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Attacker
participant EventsContract
participant ProfileContract
participant ProfileStorage
EventsContract->>EventsContract: derive hashed child op_ids
Attacker->>ProfileContract: bootstrap_self(attacker, child op_id)
ProfileContract->>ProfileStorage: store under attacker domain
EventsContract->>ProfileContract: claim_prize with child op_ids
ProfileContract->>ProfileStorage: check events domain
ProfileContract-->>EventsContract: apply legitimate profile updates
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
…95) (#96) #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.
…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
Fixes a HIGH severity payout DoS: XOR-derived child
op_ids were malleable, and profileOpSeenlived in a single global namespace that unprivilegedbootstrap_selfcould squat.Closes #68
Problem
derive_child/derive_child_indexedinboundless-eventsXORed a 1-byte tag (and optional sub-index) into the parentop_id. XOR is reversible and collision-friendly.OpSeenwas keyed only byop_id(DataKey::OpSeen(BytesN<32>)), shared by privileged events-originated mutations and unprivilegedbootstrap_self.op_id, recompute deterministic children (BOOTSTRAP/BUMP_REP/REGISTER_EARNINGS), front-runbootstrap_self(attacker, child_id), then the events→profile call hitsOpAlreadySeenand blocks reputation / earnings (and hard-fails grant paths that do not usetry_).Scout
incorrect_exponentiationflags on^=were false positives (intentional XOR). Removing XOR clears those flags by deleting the malleable construction.Fix
Events — domain-separated SHA-256 child ids
callee_contract_idis the configured profile contract address (XDR-encoded domain separator).derive_childusessub_idx = 0;derive_child_indexedkeeps the explicit index.env.crypto().sha256keeps WASM well under the 64 KB CI ceiling.Profile — namespaced
OpSeenbootstrap/bump_reputation/slash_reputation/register_earningsbootstrap_selfadmin_slashUnprivileged
bootstrap_selfcan no longer mark the events domain, so front-running child ids is useless against legitimate orchestration.OpSeenremainstemporary()storage; no persistent migration required.Tests
New module
contracts/events/src/tests/op_id_security.rs:sha256_child_ids_differ_for_xor_colliding_parentsbootstrap_self_cannot_front_run_events_child_op_idsbootstrap_self;claim_prizestill pays and updates reputation + earningsevents_domain_child_op_id_replay_still_rejectedLocal verification
Risk notes
op_idvalues change for any in-flight parent (testnet-acceptable; not a client API break).OpSeenkey shape changes; existing temporary entries simply expire under the old layout.Checklist
Summary by CodeRabbit
Security Improvements
Bug Fixes