fix(events): require acceptance for manager delegation (#70) - #88
Conversation
Manager delegation previously granted privileged authority (select_winners, cancel, manager rotation) to an address that never authorized. A typo, wrong-network address, or a swapped params.manager at signing could lock the owner out permanently, or hand escrow-draining select_winners authority to an attacker with no further owner sign-off. Replace the immediate assignment with a two-step propose/accept flow mirroring the existing admin rotation: - propose_manager: current authority (manager, else owner) records a pending proposal with a short expiry; no authority transfers. - accept_manager: the proposed address must require_auth to accept before the role transfers. - cancel_pending_manager: current authority vetoes a pending proposal. - create_event now only proposes params.manager; the owner stays in control until acceptance. Emit ManagerProposed and ManagerChanged (the latter also closes the missing set_manager storage-change event). Remove set_manager. Storage layout extended append-only (DataKey::PendingManager, PendingManager).
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe events contract replaces direct manager assignment with an expiring two-step proposal and acceptance workflow. It adds pending-manager storage, lifecycle events, cancellation, public APIs, authorization checks, expiry handling, tests, and updated pause documentation. ChangesManager rotation workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant EventContract
participant Storage
participant PendingManager
Caller->>EventContract: propose_manager(event_id, target)
EventContract->>Storage: store pending target and expiry
Storage-->>EventContract: pending manager saved
PendingManager->>EventContract: accept_manager(event_id)
EventContract->>Storage: read pending manager
EventContract->>Storage: activate target and clear pending state
EventContract-->>PendingManager: ManagerChanged
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/events/src/event_ops.rs`:
- Around line 216-219: Remove the storage::clear_pending_manager call from the
expiry branch in the pending-manager validation flow, since returning
Error::PendingManagerMismatch rolls back that write. Preserve the existing
expiry error behavior and avoid adding cleanup on the failing path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c53ef2f6-33ea-4df0-a761-d3fe512bac4c
📒 Files selected for processing (9)
contracts/events/src/errors.rscontracts/events/src/event_ops.rscontracts/events/src/events.rscontracts/events/src/lib.rscontracts/events/src/storage.rscontracts/events/src/tests/cancel_refund.rscontracts/events/src/tests/cross_contract.rscontracts/events/src/types.rsdocs/mainnet-deploy-runbook.md
…rror-enum cap Brings the two-step manager delegation up to current testnet (boundlessfi#90 pull-model claims, boundlessfi#84 snapshots) and makes it build and pass there. Error enum: testnet is at the 50-case contracterror cap, so a new PendingManagerMismatch variant would not compile. Instead generalize the two admin-rotation variants — PendingAdminMismatch/PendingAdminExpired -> PendingRotationMismatch/PendingRotationExpired (discriminants 12/13 unchanged, so no on-chain code change) — and share them across both two-step rotations. accept_manager now distinguishes no-pending (Mismatch) from expired (Expired), which is more precise than the original single-variant usage. Conflict resolution: - types.rs / storage.rs / event_ops.rs: PendingManager keys and helpers ordered after the 1.3.0 prize keys; append-only, no reorder. - cross_contract.rs: kept the new manager suite; the accepted-manager test now claims the prize before asserting Completed, since under the pull model select_winners records rather than pays. Verified: cargo test 216 events + 66 profile green; make build OK (events wasm 56,202 bytes, < 64 KB); fmt + clippy clean.
Quota reachedYour plan allows 300 CI/CD file units per month. You've used 280 and this scan would add 23 more (total: 303). |
…n) (#97) The queries merged in #80 return zero rows against Dune: they filtered on JSON_EXTRACT_SCALAR(topics_decoded,'$[0]') (an ScVal object, never the event name) and read data_decoded as flat $.field (it's a type-wrapped ScVal map). Both silently yield null, so every dashboard panel is empty. Verified the real stellar.history_contract_events shapes on live Dune data and rewrote all 10 dune-queries/*.sql to: - filter the event name via topics_decoded '$[0].symbol' - decode fields by rebuilding data_decoded '$.map' into MAP(field -> ScVal JSON) with map_from_entries(), then reading each by ScVal type ($.u64, $.i128, $.address, $.string, $.vec[0].symbol) - add the closed_at_date partition filter (avoids full-table scans) - to_hex(transaction_hash) (it is varbinary) Every primitive was executed against live Soroban events on Dune. The one field that can't be checked without a real Boundless event — pillar's unit- enum encoding — is emitted as pillar_raw in the decode test for confirmation. Doc fixes: rewrote the §1 decoding reference; corrected the fee accounting (escrow holds the full budget, fee charged on top; fee revenue is not in the events); added the ManagerProposed/ManagerChanged/PendingManagerCancelled events (#88); pointed §4 at the canonical .sql files instead of duplicating now-corrected SQL inline.
…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.
Closes #70
Problem
create_event(params.manager)andset_managergranted privileged authority (select_winners, cancel, manager rotation) to an address that never authorized. A typo, wrong-network address, or a malicious frontend swappingparams.managerat signing time could:select_winnersauthority to an attacker with no further owner sign-off (non-crowdfunding pillars).Root cause: transferring a privileged role to an address that never proved it controls the key.
Fix
Two-step propose/accept, mirroring the existing
set_admin/accept_adminrotation:propose_manager(event_id, new_manager)— current authority (the manager, or the owner when none is set) records a pending proposal with a short expiry. No authority transfers. EmitsManagerProposed.accept_manager(event_id)— the proposed address mustrequire_auth()to accept before the role transfers. EmitsManagerChanged(which also closes the missingset_managerstorage-change event flagged by Scout).cancel_pending_manager(event_id)— current authority vetoes a pending proposal.create_eventnow only proposesparams.manager; authority resolves to the owner (viaresolve_manager) until acceptance, so the owner is never locked out.get_pending_manager(event_id)read added.set_managerremoved (its instant-transfer semantics were the vulnerability).The only path that grants manager authority (
accept_manager) callspending.target.require_auth()immediately before writing the manager, so a never-accepting address gains nothing and the owner is never displaced.Storage
Extended append-only (no reorder): new
DataKey::PendingManager(u64)andPendingManager { target, expires_at_ledger }.Tests
Rewrote the manager suite in
cross_contract.rs:env.auths())select_winnersauthoritycancel_pending_managervetoes a proposalUpdated the one prior
set_managercall incancel_refund.rs, and the pause-guarded-function list indocs/mainnet-deploy-runbook.md.Verification
cargo test --all— 269 passed (203 events + 66 profile)cargo build --release --target wasm32v1-none— buildscargo fmt --checkandcargo clippy -p boundless-events— cleanSummary by CodeRabbit
New Features
Breaking Changes
Documentation