From 608515d01b0e9d06a0de382d4c0f8c2afd64a0e2 Mon Sep 17 00:00:00 2001 From: Shadow-MMN Date: Fri, 24 Jul 2026 00:21:48 +0100 Subject: [PATCH 1/3] fix(profile,events): replace saturating_add with checked_add + typed errors (#72) --- contracts/events/src/errors.rs | 1 + contracts/events/src/event_ops.rs | 2 +- contracts/events/src/idempotency.rs | 10 +++--- contracts/events/src/tests/op_id_security.rs | 36 ++++++++++++++++++++ contracts/profile/src/earnings.rs | 2 +- contracts/profile/src/errors.rs | 1 + contracts/profile/src/tests/earnings.rs | 18 +++++++--- 7 files changed, 59 insertions(+), 11 deletions(-) diff --git a/contracts/events/src/errors.rs b/contracts/events/src/errors.rs index fd8959c..62d38bf 100644 --- a/contracts/events/src/errors.rs +++ b/contracts/events/src/errors.rs @@ -70,6 +70,7 @@ pub enum Error { MigrationAlreadyApplied = 69, Paused = 70, + EventIdOverflow = 71, ProfileCallFailed = 80, diff --git a/contracts/events/src/event_ops.rs b/contracts/events/src/event_ops.rs index 814cf08..8e002cb 100644 --- a/contracts/events/src/event_ops.rs +++ b/contracts/events/src/event_ops.rs @@ -128,7 +128,7 @@ pub fn create_event(env: &Env, params: CreateEventParams, op_id: BytesN<32>) -> ); } - let id = idempotency::next_event_id(env); + let id = idempotency::next_event_id(env)?; let record = EventRecord { id, ..provisional }; storage::set_event(env, id, &record); storage::set_non_owner_contribution_total(env, id, 0); diff --git a/contracts/events/src/idempotency.rs b/contracts/events/src/idempotency.rs index 65187b7..c052cc5 100644 --- a/contracts/events/src/idempotency.rs +++ b/contracts/events/src/idempotency.rs @@ -25,11 +25,13 @@ pub fn id_base(env: &Env) -> u64 { (seq as u64) << 32 } -pub fn next_event_id(env: &Env) -> u64 { +pub fn next_event_id(env: &Env) -> Result { let base = id_base(env); - let id = storage::get_next_event_id(env, base.saturating_add(1)); - storage::set_next_event_id(env, id.saturating_add(1)); - id + let fallback = base.checked_add(1).ok_or(Error::EventIdOverflow)?; + let id = storage::get_next_event_id(env, fallback); + let next = id.checked_add(1).ok_or(Error::EventIdOverflow)?; + storage::set_next_event_id(env, next); + Ok(id) } pub mod tag { diff --git a/contracts/events/src/tests/op_id_security.rs b/contracts/events/src/tests/op_id_security.rs index 8c7931b..9703ead 100644 --- a/contracts/events/src/tests/op_id_security.rs +++ b/contracts/events/src/tests/op_id_security.rs @@ -10,6 +10,7 @@ use soroban_sdk::{ }; use crate::idempotency::{self, tag}; +use crate::storage; use crate::types::{CreateEventParams, Pillar, ReleaseKind, WinnerSpec}; use crate::{EventsContract, EventsContractClient}; @@ -209,6 +210,41 @@ fn events_domain_child_op_id_replay_still_rejected() { ); } +/// Calling create_event when the stored next_event_id is at u64::MAX must revert +/// with EventIdOverflow rather than silently returning the same id forever. +#[test] +fn event_id_overflow_reverts() { + let ctx = setup(); + let env = &ctx.env; + + // Set the stored next_event_id to u64::MAX so the increment overflows. + env.as_contract(&ctx.events_id, || { + storage::set_next_event_id(env, u64::MAX); + }); + + let params = CreateEventParams { + pillar: Pillar::Bounty, + owner: ctx.owner.clone(), + token: ctx.token_addr.clone(), + total_budget: TOTAL_BUDGET, + release_kind: ReleaseKind::Single, + content_uri: String::from_str(env, "https://api.boundless.fi/events/overflow"), + title: String::from_str(env, "Overflow"), + deadline: Some(env.ledger().timestamp() + 86_400), + winner_distribution: dist_100(env), + fee_bps_override: None, + manager: None, + }; + + let err = ctx + .events + .try_create_event(¶ms, &BytesN::random(env)) + .err() + .expect("event creation should fail when next_event_id overflows") + .unwrap(); + assert_eq!(err, crate::errors::Error::EventIdOverflow); +} + /// Events-side OpSeen is namespaced by the authorizing caller: a permissionless /// entrypoint (apply) cannot pre-mark an op_id and block a privileged one /// (select_winners) that reuses it. Before namespacing, the shared global diff --git a/contracts/profile/src/earnings.rs b/contracts/profile/src/earnings.rs index 04d59f3..c05c605 100644 --- a/contracts/profile/src/earnings.rs +++ b/contracts/profile/src/earnings.rs @@ -23,7 +23,7 @@ pub fn register( } let current = storage::get_earnings(env, &user, &token); - let new = current.saturating_add(amount); + let new = current.checked_add(amount).ok_or(Error::EarningsOverflow)?; storage::set_earnings(env, &user, &token, new); evt::EarningsRegistered { diff --git a/contracts/profile/src/errors.rs b/contracts/profile/src/errors.rs index 12884d2..662b378 100644 --- a/contracts/profile/src/errors.rs +++ b/contracts/profile/src/errors.rs @@ -22,6 +22,7 @@ pub enum Error { ReasonRequired = 13, OpAlreadySeen = 20, + EarningsOverflow = 21, Paused = 30, UpgradeNotProposed = 40, diff --git a/contracts/profile/src/tests/earnings.rs b/contracts/profile/src/tests/earnings.rs index b413729..5835138 100644 --- a/contracts/profile/src/tests/earnings.rs +++ b/contracts/profile/src/tests/earnings.rs @@ -179,7 +179,7 @@ fn register_earnings_rejects_duplicate_op_id() { } #[test] -fn register_earnings_saturating_add() { +fn register_earnings_overflow_reverts() { let ctx = setup(); ctx.client.set_events_contract(&events_addr(&ctx.env)); @@ -187,11 +187,19 @@ fn register_earnings_saturating_add() { let t = token(&ctx.env); ctx.client - .register_earnings(&u, &t, &(i128::MAX - 1), &BytesN::random(&ctx.env)); - assert_eq!(ctx.client.get_earnings(&u, &t), i128::MAX - 1); + .register_earnings(&u, &t, &i128::MAX, &BytesN::random(&ctx.env)); + assert_eq!(ctx.client.get_earnings(&u, &t), i128::MAX); - ctx.client - .register_earnings(&u, &t, &100_i128, &BytesN::random(&ctx.env)); + // A second registration must overflow and revert. + let err = ctx + .client + .try_register_earnings(&u, &t, &1_i128, &BytesN::random(&ctx.env)) + .err() + .expect("overflow should revert") + .unwrap(); + assert_eq!(err, Error::EarningsOverflow); + + // State unchanged: still at i128::MAX. assert_eq!(ctx.client.get_earnings(&u, &t), i128::MAX); } From 32bab1c4983121f2f4499e76e1ce1edc9b20395e Mon Sep 17 00:00:00 2001 From: Shadow-MMN Date: Fri, 24 Jul 2026 00:33:49 +0100 Subject: [PATCH 2/3] =?UTF-8?q?chore:=20address=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20extend=20rollback=20assertions,=20remove=20narratio?= =?UTF-8?q?n=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/events/src/tests/op_id_security.rs | 26 +++++++++++++++++++- contracts/profile/src/tests/earnings.rs | 3 --- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/contracts/events/src/tests/op_id_security.rs b/contracts/events/src/tests/op_id_security.rs index 9703ead..bf372db 100644 --- a/contracts/events/src/tests/op_id_security.rs +++ b/contracts/events/src/tests/op_id_security.rs @@ -25,6 +25,7 @@ struct Ctx<'a> { events_id: Address, profile: ProfileContractClient<'a>, owner: Address, + fee_account: Address, applicant: Address, token_addr: Address, } @@ -69,6 +70,7 @@ fn setup<'a>() -> Ctx<'a> { events_id, profile, owner, + fee_account, applicant, token_addr, } @@ -217,7 +219,10 @@ fn event_id_overflow_reverts() { let ctx = setup(); let env = &ctx.env; - // Set the stored next_event_id to u64::MAX so the increment overflows. + let token = token::Client::new(env, &ctx.token_addr); + let owner_balance_before = token.balance(&ctx.owner); + let fee_balance_before = token.balance(&ctx.fee_account); + env.as_contract(&ctx.events_id, || { storage::set_next_event_id(env, u64::MAX); }); @@ -243,6 +248,25 @@ fn event_id_overflow_reverts() { .expect("event creation should fail when next_event_id overflows") .unwrap(); assert_eq!(err, crate::errors::Error::EventIdOverflow); + + // Verify transaction rollback: no funds moved, no event persisted. + assert_eq!( + token.balance(&ctx.owner), + owner_balance_before, + "owner balance unchanged after failed create_event" + ); + assert_eq!( + token.balance(&ctx.fee_account), + fee_balance_before, + "fee account balance unchanged after failed create_event" + ); + env.as_contract(&ctx.events_id, || { + let next_id = storage::get_next_event_id(env, 0); + assert_eq!( + next_id, u64::MAX, + "next_event_id unchanged after failed create_event" + ); + }); } /// Events-side OpSeen is namespaced by the authorizing caller: a permissionless diff --git a/contracts/profile/src/tests/earnings.rs b/contracts/profile/src/tests/earnings.rs index 5835138..4ee4c25 100644 --- a/contracts/profile/src/tests/earnings.rs +++ b/contracts/profile/src/tests/earnings.rs @@ -190,7 +190,6 @@ fn register_earnings_overflow_reverts() { .register_earnings(&u, &t, &i128::MAX, &BytesN::random(&ctx.env)); assert_eq!(ctx.client.get_earnings(&u, &t), i128::MAX); - // A second registration must overflow and revert. let err = ctx .client .try_register_earnings(&u, &t, &1_i128, &BytesN::random(&ctx.env)) @@ -198,8 +197,6 @@ fn register_earnings_overflow_reverts() { .expect("overflow should revert") .unwrap(); assert_eq!(err, Error::EarningsOverflow); - - // State unchanged: still at i128::MAX. assert_eq!(ctx.client.get_earnings(&u, &t), i128::MAX); } From 5f13fe19ee7b6512c723ef9cad117215ac83f5aa Mon Sep 17 00:00:00 2001 From: Collins Ikechukwu Date: Mon, 27 Jul 2026 20:55:24 +0100 Subject: [PATCH 3/3] style(events): fix rustfmt in op_id_security test; correct stale enum-cap comments - rustfmt (rust 1.93) wants the assert_eq! args in event_id_overflow_reverts split one-per-line; apply it (CI rustfmt was failing on this). - The errors enum is at 48/50 after #99 removed the three deadline variants, so the three 'enum is at the 50-case cap / consolidate before adding' comments were stale. Reword them: state the real reuse rationale, and the true current count. --- contracts/events/src/errors.rs | 12 ++++++------ contracts/events/src/tests/op_id_security.rs | 3 ++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/contracts/events/src/errors.rs b/contracts/events/src/errors.rs index 62d38bf..40eb478 100644 --- a/contracts/events/src/errors.rs +++ b/contracts/events/src/errors.rs @@ -15,8 +15,8 @@ pub enum Error { NotAdmin = 11, // Shared by both two-step rotations (admin and event manager): no pending // proposal / target mismatch (12) and pending proposal expired (13). The - // enum is at the 50-case XDR cap, so the manager flow reuses these rather - // than adding variants. + // manager flow reuses these because the two flows are structurally + // identical, not to duplicate a variant per flow. PendingRotationMismatch = 12, PendingRotationExpired = 13, @@ -54,9 +54,8 @@ pub enum Error { OpAlreadySeen = 60, - // Also returned by append_submission's cap check — the enum is at - // the 50-case XDR cap, so the hackathon submission cap reuses this - // rather than adding a variant. + // Also returned by append_submission's cap check: the hackathon submission + // cap reuses this rather than adding a near-duplicate "TooManySubmissions". TooManyContributors = 61, CancellationNotStarted = 62, @@ -74,6 +73,7 @@ pub enum Error { ProfileCallFailed = 80, - // Enum is at the 50-case XDR cap; consolidate before adding another. + // contracterror caps at 50 cases (48 used). Discriminants are not dense — + // 91 is a numeric label, not the case count. PrizeAlreadyClaimed = 91, } diff --git a/contracts/events/src/tests/op_id_security.rs b/contracts/events/src/tests/op_id_security.rs index bf372db..87fe2be 100644 --- a/contracts/events/src/tests/op_id_security.rs +++ b/contracts/events/src/tests/op_id_security.rs @@ -263,7 +263,8 @@ fn event_id_overflow_reverts() { env.as_contract(&ctx.events_id, || { let next_id = storage::get_next_event_id(env, 0); assert_eq!( - next_id, u64::MAX, + next_id, + u64::MAX, "next_event_id unchanged after failed create_event" ); });