From af40923545ae4f4cd9a9bb3a1651419ac8662186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Salazar?= Date: Tue, 21 Jul 2026 11:14:33 -0600 Subject: [PATCH 1/3] fix(events): replace xor child op_id derivation with domain-separated sha256 --- contracts/events/src/idempotency.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/contracts/events/src/idempotency.rs b/contracts/events/src/idempotency.rs index 7c25305..63a1383 100644 --- a/contracts/events/src/idempotency.rs +++ b/contracts/events/src/idempotency.rs @@ -1,6 +1,6 @@ #![allow(dead_code)] -use soroban_sdk::{BytesN, Env}; +use soroban_sdk::{xdr::ToXdr, Bytes, BytesN, Env}; use crate::errors::Error; use crate::storage; @@ -35,15 +35,22 @@ pub mod tag { pub const REGISTER_EARNINGS: u8 = 0xE1; } +/// Collision-resistant child op_id. +/// +/// Invariant: `sha256(parent ‖ op_tag ‖ sub_idx ‖ callee_contract_id)`. +/// XOR into parent bytes is malleable (reversible, squat-friendly); hashing with +/// the profile contract id as domain separator is not. pub fn derive_child(env: &Env, parent: &BytesN<32>, op_tag: u8) -> BytesN<32> { - let mut payload = parent.to_array(); - payload[0] ^= op_tag; - BytesN::from_array(env, &payload) + derive_child_indexed(env, parent, op_tag, 0) } pub fn derive_child_indexed(env: &Env, parent: &BytesN<32>, op_tag: u8, sub_idx: u8) -> BytesN<32> { - let mut payload = parent.to_array(); - payload[0] ^= op_tag; - payload[1] ^= sub_idx; - BytesN::from_array(env, &payload) + let callee = storage::get_profile_contract(env); + let mut payload = Bytes::new(env); + payload.append(&Bytes::from_array(env, &parent.to_array())); + payload.push_back(op_tag); + payload.push_back(sub_idx); + payload.append(&callee.to_xdr(env)); + let digest = env.crypto().sha256(&payload); + BytesN::from_array(env, &digest.to_array()) } From eed1ad14c548cfcc229075a7d1f0ea651a849e80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Salazar?= Date: Tue, 21 Jul 2026 11:14:33 -0600 Subject: [PATCH 2/3] fix(profile): namespace opseen by caller domain to block bootstrap squat --- contracts/profile/src/bootstrap.rs | 12 +++++++----- contracts/profile/src/earnings.rs | 5 +++-- contracts/profile/src/idempotency.rs | 15 ++++++++++----- contracts/profile/src/reputation.rs | 15 +++++++++------ contracts/profile/src/storage.rs | 8 ++++---- contracts/profile/src/types.rs | 4 +++- 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/contracts/profile/src/bootstrap.rs b/contracts/profile/src/bootstrap.rs index 2f669eb..255a2c8 100644 --- a/contracts/profile/src/bootstrap.rs +++ b/contracts/profile/src/bootstrap.rs @@ -10,7 +10,8 @@ use crate::types::Profile; pub fn bootstrap(env: &Env, user: Address, op_id: BytesN<32>) -> Result<(), Error> { admin::require_events_contract(env)?; admin::require_not_paused(env)?; - idempotency::require_unseen(env, &op_id)?; + let domain = idempotency::events_domain(env)?; + idempotency::require_unseen(env, &domain, &op_id)?; if storage::get_profile(env, &user).is_none() { let profile = Profile::new(env.ledger().timestamp()); @@ -18,21 +19,22 @@ pub fn bootstrap(env: &Env, user: Address, op_id: BytesN<32>) -> Result<(), Erro evt::ProfileBootstrapped { user }.publish(env); } - idempotency::mark_seen(env, &op_id); + idempotency::mark_seen(env, &domain, &op_id); Ok(()) } pub fn bootstrap_self(env: &Env, user: Address, op_id: BytesN<32>) -> Result<(), Error> { user.require_auth(); admin::require_not_paused(env)?; - idempotency::require_unseen(env, &op_id)?; + // Domain = user so unprivileged self-bootstrap cannot squat events-domain op_ids. + idempotency::require_unseen(env, &user, &op_id)?; if storage::get_profile(env, &user).is_none() { let profile = Profile::new(env.ledger().timestamp()); storage::set_profile(env, &user, &profile); - evt::ProfileBootstrapped { user }.publish(env); + evt::ProfileBootstrapped { user: user.clone() }.publish(env); } - idempotency::mark_seen(env, &op_id); + idempotency::mark_seen(env, &user, &op_id); Ok(()) } diff --git a/contracts/profile/src/earnings.rs b/contracts/profile/src/earnings.rs index 8c5ca33..04d59f3 100644 --- a/contracts/profile/src/earnings.rs +++ b/contracts/profile/src/earnings.rs @@ -15,7 +15,8 @@ pub fn register( ) -> Result<(), Error> { admin::require_events_contract(env)?; admin::require_not_paused(env)?; - idempotency::require_unseen(env, &op_id)?; + let domain = idempotency::events_domain(env)?; + idempotency::require_unseen(env, &domain, &op_id)?; if amount <= 0 { return Err(Error::InvalidAmount); @@ -31,6 +32,6 @@ pub fn register( amount, } .publish(env); - idempotency::mark_seen(env, &op_id); + idempotency::mark_seen(env, &domain, &op_id); Ok(()) } diff --git a/contracts/profile/src/idempotency.rs b/contracts/profile/src/idempotency.rs index 1d4fd26..d5a6259 100644 --- a/contracts/profile/src/idempotency.rs +++ b/contracts/profile/src/idempotency.rs @@ -1,15 +1,20 @@ -use soroban_sdk::{BytesN, Env}; +use soroban_sdk::{Address, BytesN, Env}; use crate::errors::Error; use crate::storage; -pub fn require_unseen(env: &Env, op_id: &BytesN<32>) -> Result<(), Error> { - if storage::is_op_seen(env, op_id) { +pub fn require_unseen(env: &Env, domain: &Address, op_id: &BytesN<32>) -> Result<(), Error> { + if storage::is_op_seen(env, domain, op_id) { return Err(Error::OpAlreadySeen); } Ok(()) } -pub fn mark_seen(env: &Env, op_id: &BytesN<32>) { - storage::mark_op_seen(env, op_id); +pub fn mark_seen(env: &Env, domain: &Address, op_id: &BytesN<32>) { + storage::mark_op_seen(env, domain, op_id); +} + +/// Domain for ops authorized by the configured events contract. +pub fn events_domain(env: &Env) -> Result { + storage::get_events_contract(env).ok_or(Error::EventsContractNotConfigured) } diff --git a/contracts/profile/src/reputation.rs b/contracts/profile/src/reputation.rs index d87ae6f..5f7f597 100644 --- a/contracts/profile/src/reputation.rs +++ b/contracts/profile/src/reputation.rs @@ -15,7 +15,8 @@ pub fn bump( ) -> Result<(), Error> { admin::require_events_contract(env)?; admin::require_not_paused(env)?; - idempotency::require_unseen(env, &op_id)?; + let domain = idempotency::events_domain(env)?; + idempotency::require_unseen(env, &domain, &op_id)?; let mut profile = storage::get_profile(env, &user).ok_or(Error::ProfileNotFound)?; profile.reputation = profile.reputation.saturating_add(delta as u64); @@ -27,7 +28,7 @@ pub fn bump( reason, } .publish(env); - idempotency::mark_seen(env, &op_id); + idempotency::mark_seen(env, &domain, &op_id); Ok(()) } @@ -40,7 +41,8 @@ pub fn slash( ) -> Result<(), Error> { admin::require_events_contract(env)?; admin::require_not_paused(env)?; - idempotency::require_unseen(env, &op_id)?; + let domain = idempotency::events_domain(env)?; + idempotency::require_unseen(env, &domain, &op_id)?; let mut profile = storage::get_profile(env, &user).ok_or(Error::ProfileNotFound)?; profile.reputation = profile.reputation.saturating_sub(delta as u64); @@ -52,7 +54,7 @@ pub fn slash( reason, } .publish(env); - idempotency::mark_seen(env, &op_id); + idempotency::mark_seen(env, &domain, &op_id); Ok(()) } @@ -65,7 +67,8 @@ pub fn admin_slash( ) -> Result<(), Error> { admin::require_admin(env)?; admin::require_not_paused(env)?; - idempotency::require_unseen(env, &op_id)?; + let domain = storage::get_admin(env)?; + idempotency::require_unseen(env, &domain, &op_id)?; if reason.is_empty() { return Err(Error::ReasonRequired); @@ -81,6 +84,6 @@ pub fn admin_slash( reason, } .publish(env); - idempotency::mark_seen(env, &op_id); + idempotency::mark_seen(env, &domain, &op_id); Ok(()) } diff --git a/contracts/profile/src/storage.rs b/contracts/profile/src/storage.rs index 17ceedb..5262252 100644 --- a/contracts/profile/src/storage.rs +++ b/contracts/profile/src/storage.rs @@ -171,15 +171,15 @@ pub fn set_earnings(env: &Env, user: &Address, token: &Address, amount: i128) { // ============================================================ // IDEMPOTENCY (temporary; auto-TTL) // ============================================================ -pub fn is_op_seen(env: &Env, op_id: &BytesN<32>) -> bool { +pub fn is_op_seen(env: &Env, domain: &Address, op_id: &BytesN<32>) -> bool { env.storage() .temporary() - .get(&DataKey::OpSeen(op_id.clone())) + .get(&DataKey::OpSeen(domain.clone(), op_id.clone())) .unwrap_or(false) } -pub fn mark_op_seen(env: &Env, op_id: &BytesN<32>) { +pub fn mark_op_seen(env: &Env, domain: &Address, op_id: &BytesN<32>) { env.storage() .temporary() - .set(&DataKey::OpSeen(op_id.clone()), &true); + .set(&DataKey::OpSeen(domain.clone(), op_id.clone()), &true); } diff --git a/contracts/profile/src/types.rs b/contracts/profile/src/types.rs index a0f4dc5..030457f 100644 --- a/contracts/profile/src/types.rs +++ b/contracts/profile/src/types.rs @@ -61,5 +61,7 @@ pub enum DataKey { PendingUpgrade, MigratedToVersion, - OpSeen(BytesN<32>), + /// Temporary idempotency flag keyed by (caller domain, op_id). + /// Domain separates events-originated ops from unprivileged bootstrap_self. + OpSeen(Address, BytesN<32>), } From 734b6c09d9831c6f4d8cb9d85ef15db59cfe779b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Salazar?= Date: Tue, 21 Jul 2026 11:14:33 -0600 Subject: [PATCH 3/3] test: cover child op_id collision resistance and bootstrap front-run --- contracts/events/src/tests/mod.rs | 1 + contracts/events/src/tests/op_id_security.rs | 210 +++++++++++++++++++ 2 files changed, 211 insertions(+) create mode 100644 contracts/events/src/tests/op_id_security.rs diff --git a/contracts/events/src/tests/mod.rs b/contracts/events/src/tests/mod.rs index b2bd14a..6340211 100644 --- a/contracts/events/src/tests/mod.rs +++ b/contracts/events/src/tests/mod.rs @@ -10,5 +10,6 @@ mod crowdfunding; mod escrow_fee_math; mod grant_pillar; mod hackathon_pillar; +mod op_id_security; mod prize_claim; mod token_whitelist; diff --git a/contracts/events/src/tests/op_id_security.rs b/contracts/events/src/tests/op_id_security.rs new file mode 100644 index 0000000..95db644 --- /dev/null +++ b/contracts/events/src/tests/op_id_security.rs @@ -0,0 +1,210 @@ +//! Security regression for issue #68: +//! - XOR-malleable child op_id derivation +//! - global OpSeen squat via bootstrap_self (payout DoS) + +#![cfg(test)] + +use soroban_sdk::{ + testutils::{Address as _, BytesN as _}, + token, Address, BytesN, Env, Map, String, +}; + +use crate::idempotency::{self, tag}; +use crate::types::{CreateEventParams, Pillar, ReleaseKind, WinnerSpec}; +use crate::{EventsContract, EventsContractClient}; + +use boundless_profile::{ProfileContract, ProfileContractClient}; + +const FEE_BPS: u32 = 250; +const TOTAL_BUDGET: i128 = 10_000_0000000_i128; + +struct Ctx<'a> { + env: Env, + events: EventsContractClient<'a>, + events_id: Address, + profile: ProfileContractClient<'a>, + owner: Address, + applicant: Address, + token_addr: Address, +} + +fn setup<'a>() -> Ctx<'a> { + let env = Env::default(); + env.mock_all_auths_allowing_non_root_auth(); + + let profile_admin = Address::generate(&env); + let profile_id = env.register(ProfileContract, (profile_admin.clone(),)); + let profile = ProfileContractClient::new(&env, &profile_id); + + let events_admin = Address::generate(&env); + let fee_account = Address::generate(&env); + let events_id = env.register( + EventsContract, + ( + events_admin.clone(), + fee_account.clone(), + FEE_BPS, + profile_id.clone(), + ), + ); + let events = EventsContractClient::new(&env, &events_id); + profile.set_events_contract(&events_id); + + let issuer = Address::generate(&env); + let sac = env.register_stellar_asset_contract_v2(issuer); + let token_addr = sac.address(); + let token_admin = token::StellarAssetClient::new(&env, &token_addr); + token_admin.mint(&fee_account, &0); + + let owner = Address::generate(&env); + token_admin.mint(&owner, &1_000_000_0000000_i128); + events.register_supported_token(&token_addr); + + let applicant = Address::generate(&env); + + Ctx { + env, + events, + events_id, + profile, + owner, + applicant, + token_addr, + } +} + +fn dist_100(env: &Env) -> Map { + let mut m = Map::new(env); + m.set(1, 100); + m +} + +fn create_bounty(ctx: &Ctx) -> u64 { + 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(&ctx.env, "https://api.boundless.fi/events/op-id-sec"), + title: String::from_str(&ctx.env, "OpId Security"), + deadline: Some(ctx.env.ledger().timestamp() + 86_400), + winner_distribution: dist_100(&ctx.env), + fee_bps_override: None, + manager: None, + }; + ctx.events.create_event(¶ms, &BytesN::random(&ctx.env)) +} + +/// Two parents that collide under the old XOR scheme must yield distinct children. +#[test] +fn sha256_child_ids_differ_for_xor_colliding_parents() { + let ctx = setup(); + let env = &ctx.env; + + // Distinct parents that only differ in byte0 (the old XOR mutation surface). + let mut a = [0u8; 32]; + let mut b = [0u8; 32]; + a[0] = 0x10; + a[1] = 0x20; + a[2] = 0xAA; + b[0] = 0x10 ^ tag::BOOTSTRAP; + b[1] = 0x20; + b[2] = 0xAA; + let parent_a = BytesN::from_array(env, &a); + let parent_b = BytesN::from_array(env, &b); + assert_ne!(parent_a, parent_b); + + // derive_child reads profile contract storage — must run as the events contract. + let (child_a, child_b, child_rep, child_i0, child_i1) = env.as_contract(&ctx.events_id, || { + let child_a = idempotency::derive_child(env, &parent_a, tag::BOOTSTRAP); + let child_b = idempotency::derive_child(env, &parent_b, tag::BOOTSTRAP); + let child_rep = idempotency::derive_child(env, &parent_a, tag::BUMP_REP); + let child_i0 = idempotency::derive_child_indexed(env, &parent_a, tag::BOOTSTRAP, 0); + let child_i1 = idempotency::derive_child_indexed(env, &parent_a, tag::BOOTSTRAP, 1); + (child_a, child_b, child_rep, child_i0, child_i1) + }); + + assert_ne!( + child_a, child_b, + "distinct parents must produce distinct sha256 children" + ); + assert_ne!(child_a, child_rep); + assert_eq!(child_a, child_i0); + assert_ne!(child_i0, child_i1); + assert_ne!(child_a, parent_a); +} + +/// Attacker squats derived child ids via bootstrap_self; legitimate claim_prize still pays. +#[test] +fn bootstrap_self_cannot_front_run_events_child_op_ids() { + let ctx = setup(); + let bounty_id = create_bounty(&ctx); + + let op_apply = BytesN::random(&ctx.env); + ctx.events + .apply_to_bounty(&bounty_id, &ctx.applicant, &op_apply); + + let winners = soroban_sdk::vec![ + &ctx.env, + WinnerSpec { + recipient: ctx.applicant.clone(), + position: 1, + reputation_bump: 50, + }, + ]; + let op_select = BytesN::random(&ctx.env); + ctx.events.select_winners(&bounty_id, &winners, &op_select); + + // Parent op_id the winner will use for claim_prize — attacker observes it and + // pre-marks the derived profile child ids via unprivileged bootstrap_self. + let claim_op = BytesN::random(&ctx.env); + let (bootstrap_child, rep_child, earnings_child) = ctx.env.as_contract(&ctx.events_id, || { + ( + idempotency::derive_child(&ctx.env, &claim_op, tag::BOOTSTRAP), + idempotency::derive_child(&ctx.env, &claim_op, tag::BUMP_REP), + idempotency::derive_child(&ctx.env, &claim_op, tag::REGISTER_EARNINGS), + ) + }); + + let attacker = Address::generate(&ctx.env); + ctx.profile.bootstrap_self(&attacker, &bootstrap_child); + ctx.profile.bootstrap_self(&attacker, &rep_child); + ctx.profile.bootstrap_self(&attacker, &earnings_child); + + // Legitimate claim must still succeed: OpSeen is namespaced by events domain. + ctx.events.claim_prize(&bounty_id, &1_u32, &claim_op); + + let token = token::Client::new(&ctx.env, &ctx.token_addr); + assert_eq!(token.balance(&ctx.applicant), TOTAL_BUDGET); + + let profile = ctx.profile.get_profile(&ctx.applicant).unwrap(); + assert_eq!(profile.reputation, 50); + + let earnings = ctx.profile.get_earnings(&ctx.applicant, &ctx.token_addr); + assert_eq!(earnings, TOTAL_BUDGET); + + // Sanity: attacker got a profile from bootstrap_self, but that is independent. + assert!(ctx.profile.get_profile(&attacker).is_some()); +} + +/// True replay of the same events-domain child op_id is still rejected. +#[test] +fn events_domain_child_op_id_replay_still_rejected() { + let ctx = setup(); + let env = &ctx.env; + + // Bootstrap via events path twice with the same child id. + let parent = BytesN::random(env); + let child = env.as_contract(&ctx.events_id, || { + idempotency::derive_child(env, &parent, tag::BOOTSTRAP) + }); + let user = Address::generate(env); + + ctx.profile.bootstrap(&user, &child); + let replay = ctx.profile.try_bootstrap(&user, &child); + assert!( + replay.is_err(), + "true replay of the same events-domain op_id must be rejected" + ); +}