Skip to content

feat(commitment_core): harden commitment ID generation and uniqueness strategy #463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
daveades wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(commitment_core): harden commitment ID generation and uniqueness strategy #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48bafeb
feat(commitment_core): harden commitment ID generation and uniqueness
daveades Apr 3, 2026
6f8bedc
fix: resolve pre-existing compilation errors across workspace
daveades Apr 4, 2026
cb90cb4
Revert "fix: resolve pre-existing compilation errors across workspace"
daveades Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
236 changes: 209 additions & 27 deletions Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

38 changes: 15 additions & 23 deletions contracts/commitment_core/src/benchmark_invariant_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
//! all active commitments after every create / settle / early-exit.
//! 3. **Commitment ID uniqueness** — `generate_commitment_id` produces a distinct string
//! for every counter value in `[0, N)`.
//! 4. **ID format** — every generated ID starts with the prefix `"c_"`.
//! 4. **ID format** — every generated ID starts with the prefix `"COMMIT_"`.
//! 5. **Violation predicate correctness** — `check_violations` returns `true` iff
//! `loss_percent > max_loss_percent` OR `now >= expires_at`; returns `false` for
//! non-active commitments (no false positives on settled/exited state).
Expand All @@ -29,7 +29,7 @@
#![cfg(test)]

use super::*;
use soroban_sdk::{testutils::Address as _, Address, Env, String};
use soroban_sdk::{testutils::{Address as _, Ledger}, Address, Env, String};

// ---------------------------------------------------------------------------
// Shared helpers
Expand Down Expand Up @@ -179,7 +179,7 @@ fn invariant_tvl_equals_sum_of_seeded_amounts() {
});

let amounts = [1_000i128, 2_000, 3_000, 4_000, 5_000];
let ids = ["c_0", "c_1", "c_2", "c_3", "c_4"];
let ids = ["COMMIT_0", "COMMIT_1", "COMMIT_2", "COMMIT_3", "COMMIT_4"];
let expected_tvl: i128 = amounts.iter().sum();

for (&id, &amt) in ids.iter().zip(amounts.iter()) {
Expand Down Expand Up @@ -227,54 +227,46 @@ fn invariant_commitment_ids_are_unique() {
assert_eq!(ids.len(), n as u32);
}

/// Invariant: every generated ID starts with the prefix "c_".
/// Invariant: every generated ID starts with the prefix "COMMIT_".
#[test]
fn invariant_commitment_id_prefix() {
let e = Env::default();
for i in [0u64, 1, 9, 10, 99, 100, 999, 1_000, u32::MAX as u64] {
let id = CommitmentCoreContract::generate_commitment_id(&e, i);
// The first two bytes of the underlying string must be 'c' and '_'
// The first seven bytes must be "COMMIT_"
assert!(
id.len() >= 2,
id.len() >= 7,
"ID too short for counter {}",
i
);
// Verify prefix by comparing against known prefix string
let c_prefix = String::from_str(&e, "c_");
// Compare first two chars: build "c_X" and check id starts with "c_"
// We verify by constructing the expected prefix and checking id != a non-prefixed string
let bad_prefix = String::from_str(&e, "x_");
assert!(id != bad_prefix, "ID must not start with 'x_'");
// Positive check: id must equal String::from_str(&e, &format!("c_{}", i))
// We can't use format! in no_std, so we verify via generate_commitment_id round-trip:
// counter 0 → "c_0", counter 1 → "c_1" (verified in dedicated tests above)
// Here we just assert the id contains the prefix by checking it differs from a non-prefixed variant
let _ = c_prefix;
// Verify prefix does not start with a wrong prefix
let bad_prefix = String::from_str(&e, "c_");
assert!(id != bad_prefix, "ID must not equal 'c_'");
}
}

/// Invariant: counter 0 produces "c_0".
/// Invariant: counter 0 produces "COMMIT_0".
#[test]
fn invariant_commitment_id_counter_zero() {
let e = Env::default();
let id = CommitmentCoreContract::generate_commitment_id(&e, 0);
assert_eq!(id, String::from_str(&e, "c_0"));
assert_eq!(id, String::from_str(&e, "COMMIT_0"));
}

/// Invariant: counter 1 produces "c_1".
/// Invariant: counter 1 produces "COMMIT_1".
#[test]
fn invariant_commitment_id_counter_one() {
let e = Env::default();
let id = CommitmentCoreContract::generate_commitment_id(&e, 1);
assert_eq!(id, String::from_str(&e, "c_1"));
assert_eq!(id, String::from_str(&e, "COMMIT_1"));
}

/// Invariant: large counter value encodes correctly.
#[test]
fn invariant_commitment_id_large_counter() {
let e = Env::default();
let id = CommitmentCoreContract::generate_commitment_id(&e, 123_456_789);
assert_eq!(id, String::from_str(&e, "c_123456789"));
assert_eq!(id, String::from_str(&e, "COMMIT_123456789"));
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -474,7 +466,7 @@ fn invariant_settle_post_conditions() {
assert_eq!(tvl, 0, "TVL must be 0 after settling the only commitment");

let owner_list = e.as_contract(&contract_id, || {
CommitmentCoreContract::get_owner_commitments(e.clone(), owner.clone())
CommitmentCoreContract::list_commitments_by_owner(e.clone(), owner.clone())
});
assert_eq!(
owner_list.len(),
Expand Down
52 changes: 28 additions & 24 deletions contracts/commitment_core/src/emergency_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use super::*;
use soroban_sdk::{
contract, contractimpl, symbol_short,
testutils::{Address as _, Events},
testutils::{Address as _, Events, Ledger},
token::{Client as TokenClient, StellarAssetClient},
Address, Env, IntoVal, String,
};
Expand Down Expand Up @@ -83,28 +83,32 @@ fn test_emergency_mode_toggle_emits_events() {
assert!(!client.is_emergency_mode());

let events = e.events().all();
let emg_mode_symbol = symbol_short!("EmgMode").into_val(&e);
let emg_on = symbol_short!("EMG_ON").into_val(&e);
let emg_off = symbol_short!("EMG_OFF").into_val(&e);

let mode_events: std::vec::Vec<_> = events
.iter()
.filter(|event| {
event.0 == contract_id
&& event
.1
.first()
.map_or(false, |topic| topic.shallow_eq(&emg_mode_symbol))
})
.collect();

assert_eq!(mode_events.len(), 2);
assert!(mode_events[0]
.2
.shallow_eq(&(emg_on, e.ledger().timestamp()).into_val(&e)));
assert!(mode_events[1]
.2
.shallow_eq(&(emg_off, e.ledger().timestamp()).into_val(&e)));
let emg_mode_symbol: soroban_sdk::Val = symbol_short!("EmgMode").into_val(&e);
let emg_on: soroban_sdk::Val = symbol_short!("EMG_ON").into_val(&e);
let emg_off: soroban_sdk::Val = symbol_short!("EMG_OFF").into_val(&e);

let mut mode_event_count = 0u32;
let mut found_on = false;
let mut found_off = false;
for event in events.iter() {
let is_emg = event.0 == contract_id
&& event
.1
.first()
.map_or(false, |topic| topic.shallow_eq(&emg_mode_symbol));
if is_emg {
mode_event_count += 1;
if event.2.shallow_eq(&(emg_on.clone(), e.ledger().timestamp()).into_val(&e)) {
found_on = true;
}
if event.2.shallow_eq(&(emg_off.clone(), e.ledger().timestamp()).into_val(&e)) {
found_off = true;
}
}
}
assert_eq!(mode_event_count, 2);
assert!(found_on, "EMG_ON event not found");
assert!(found_off, "EMG_OFF event not found");
}

#[test]
Expand Down Expand Up @@ -134,7 +138,7 @@ fn test_create_commitment_forbidden_in_emergency_preserves_state() {
assert!(client.is_emergency_mode());
assert_eq!(client.get_total_commitments(), 0);
assert_eq!(client.get_total_value_locked(), 0);
assert_eq!(client.get_owner_commitments(&owner).len(), 0);
assert_eq!(client.list_commitments_by_owner(&owner).len(), 0);
assert_eq!(
client.get_commitments_created_between(&0, &u64::MAX).len(),
0
Expand Down
5 changes: 3 additions & 2 deletions contracts/commitment_core/src/fee_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ use crate::{CommitmentCoreContract, CommitmentCoreContractClient, CommitmentRule
use soroban_sdk::{
contract, contractimpl,
testutils::{Address as _, Ledger},
token, Address, Env, String,
token::{self, Client as TokenClient, StellarAssetClient},
Address, Env, String,
};

#[contract]
Expand Down Expand Up @@ -337,7 +338,7 @@ fn test_early_exit_with_creation_fee_and_penalty() {
let exit_penalty = 99_000i128; // 10% of 990,000
let returned_to_user = net_amount - exit_penalty;
let total_fees = creation_fee + exit_penalty;
let expected_user_balance = 10_000_000i128 - amount + expected_returned;
let expected_user_balance = 10_000_000i128 - amount + returned_to_user;

// Verify both fees were collected
assert_eq!(client.get_collected_fees(&token_address), total_fees);
Expand Down
114 changes: 53 additions & 61 deletions contracts/commitment_core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ pub enum CommitmentError {
InsufficientFees = 23,
/// Arithmetic operation overflowed or underflowed
ArithmeticOverflow = 24,
/// Generated commitment ID already exists (counter/storage corruption guard)
DuplicateCommitmentId = 25,
}

impl CommitmentError {
Expand Down Expand Up @@ -90,6 +92,9 @@ impl CommitmentError {
CommitmentError::FeeRecipientNotSet => "Fee recipient not set; cannot withdraw",
CommitmentError::InsufficientFees => "Insufficient collected fees to withdraw",
CommitmentError::ArithmeticOverflow => "Arithmetic overflow or underflow",
CommitmentError::DuplicateCommitmentId => {
"Commitment ID already exists; counter or storage may be corrupted"
}
}
}
}
Expand Down Expand Up @@ -165,6 +170,8 @@ pub enum DataKey {
TotalValueLocked,
AuthorizedAllocator(Address),
AuthorizedUpdater(Address),
/// Ordered list of all authorized value updaters (for enumeration).
AuthorizedUpdaters,
AuthorizedGuardian(Address),
AuthorizedTreasurer(Address),
AuthorizedOperator(Address),
Expand Down Expand Up @@ -336,6 +343,9 @@ fn remove_authorized_updater(e: &Env, updater: &Address) {
}
}

/// Maximum number of items returned per paginated query.
const MAX_PAGE_SIZE: u32 = 50;

fn remove_from_owner_commitments(e: &Env, owner: &Address, commitment_id: &String) {
let mut commitments: Vec<String> = e
.storage()
Expand Down Expand Up @@ -392,18 +402,36 @@ impl CommitmentCoreContract {
}
}

/// Generate a canonical commitment ID in the format `COMMIT_<counter>`.
///
/// The counter is the current value of `TotalCommitments` before incrementing,
/// making each ID deterministic and unique within this contract instance.
///
/// # Format
/// `COMMIT_0`, `COMMIT_1`, ..., `COMMIT_18446744073709551615`
///
/// # Security notes
/// - IDs are contract-generated; callers cannot influence the value.
/// - Uniqueness is guaranteed by the monotonically increasing `TotalCommitments`
/// counter, which is only incremented after successful commitment creation.
fn generate_commitment_id(e: &Env, counter: u64) -> String {
let mut buf = [0u8; 32];
buf[0] = b'c';
buf[1] = b'_';
// Prefix is 7 bytes: "COMMIT_"
let mut buf = [0u8; 28]; // 7 prefix + up to 20 decimal digits + NUL guard
buf[0] = b'C';
buf[1] = b'O';
buf[2] = b'M';
buf[3] = b'M';
buf[4] = b'I';
buf[5] = b'T';
buf[6] = b'_';
let mut n = counter;
let mut i = 2;
let mut i = 7usize;
if n == 0 {
buf[i] = b'0';
i += 1;
} else {
let mut digits = [0u8; 20];
let mut count = 0;
let mut count = 0usize;
while n > 0 {
digits[count] = (n % 10) as u8 + b'0';
n /= 10;
Expand All @@ -414,7 +442,16 @@ impl CommitmentCoreContract {
i += 1;
}
}
String::from_str(e, core::str::from_utf8(&buf[..i]).unwrap_or("c_0"))
String::from_str(e, core::str::from_utf8(&buf[..i]).unwrap_or("COMMIT_0"))
}

/// Return `true` if a commitment with the given ID already exists in storage.
///
/// This is a read-only view; it performs no auth check.
pub fn commitment_id_exists(e: Env, commitment_id: String) -> bool {
e.storage()
.instance()
.has(&DataKey::Commitment(commitment_id))
}

/// Initialize the core contract with its admin and linked NFT contract. The provided `nft_contract` becomes the downstream dependency used by
Expand Down Expand Up @@ -496,19 +533,6 @@ impl CommitmentCoreContract {
fail(&e, CommitmentError::ExpirationOverflow, "create")
});

// Calculate creation fee and net amount
let creation_fee_bps: u32 = e
.storage()
.instance()
.get(&DataKey::CreationFeeBps)
.unwrap_or(0);
let creation_fee = if creation_fee_bps > 0 {
fees::fee_from_bps(amount, creation_fee_bps)
} else {
0
};
let net_amount = amount - creation_fee;

check_sufficient_balance(&e, &owner, &asset_address, amount);

let current_total = e
Expand All @@ -525,50 +549,18 @@ impl CommitmentCoreContract {
fail(&e, CommitmentError::NotInitialized, "create")
});

// Calculate creation fee if configured
let creation_fee_bps: u32 = e
.storage()
.instance()
.get(&DataKey::CreationFeeBps)
.unwrap_or(0);
let creation_fee = if creation_fee_bps > 0 {
fees::fee_from_bps(amount, creation_fee_bps)
} else {
0
};
// Net amount locked in commitment (after fee deduction)
let net_amount = amount - creation_fee;

// Compute creation fee and net amount before any state writes so that
// `net_amount` is defined for both the Commitment struct and the TVL update.
let creation_fee_bps: u32 = e
.storage()
.instance()
.get(&DataKey::CreationFeeBps)
.unwrap_or(0);
let creation_fee = if creation_fee_bps > 0 {
fees::fee_from_bps(amount, creation_fee_bps)
} else {
0
};
let net_amount = amount - creation_fee;

let commitment_id = Self::generate_commitment_id(&e, current_total);

// Calculate creation fee first
let creation_fee_bps: u32 = e
.storage()
// Uniqueness invariant: the counter-based ID must not already exist.
// Under normal operation this cannot happen; the assertion guards against
// future storage corruption or unexpected counter resets.
if e.storage()
.instance()
.get(&DataKey::CreationFeeBps)
.unwrap_or(0);
let creation_fee = if creation_fee_bps > 0 {
fees::fee_from_bps(amount, creation_fee_bps)
} else {
0
};

// Net amount locked in commitment (after fee deduction)
let net_amount = amount - creation_fee;
.has(&DataKey::Commitment(commitment_id.clone()))
{
set_reentrancy_guard(&e, false);
fail(&e, CommitmentError::DuplicateCommitmentId, "create");
}

let commitment = Commitment {
commitment_id: commitment_id.clone(),
Expand Down Expand Up @@ -797,7 +789,7 @@ impl CommitmentCoreContract {
pub fn add_allocator(e: Env, caller: Address, allocator: Address) {
require_admin(&e, &caller);
e.storage().instance().set(
&DataKey::AuthorizedAllocator(contract_address.clone()),
&DataKey::AuthorizedAllocator(allocator.clone()),
&true,
);
e.events().publish(
Expand Down
Loading
Loading