From 2814038a14748538f9f89a5a97b58d4e3a182184 Mon Sep 17 00:00:00 2001 From: D'Angelo Rodriguez <70290504+dangelo352@users.noreply.github.com> Date: Sun, 21 Jun 2026 20:13:01 -0400 Subject: [PATCH] fix: use checked creation fee math --- contracts/commitment_core/src/fuzz_tests.rs | 14 ++- contracts/commitment_core/src/lib.rs | 112 ++++++++++++++------ docs/FEES.md | 2 +- docs/FEE_MODEL_CROSS_CHECK.md | 5 +- 4 files changed, 93 insertions(+), 40 deletions(-) diff --git a/contracts/commitment_core/src/fuzz_tests.rs b/contracts/commitment_core/src/fuzz_tests.rs index e3a00ef9..44084496 100644 --- a/contracts/commitment_core/src/fuzz_tests.rs +++ b/contracts/commitment_core/src/fuzz_tests.rs @@ -8,10 +8,8 @@ use crate::{ CommitmentCoreContract, CommitmentCoreContractClient, CommitmentRules, }; use soroban_sdk::{ - contract, contractimpl, - testutils::Address as _, - token::StellarAssetClient, - Address, Env, String, + contract, contractimpl, testutils::Address as _, token::StellarAssetClient, Address, Env, + String, }; #[contract] @@ -121,6 +119,7 @@ fn test_create_commitment_rejects_fee_math_overflow() { let admin = Address::generate(&e); let owner = Address::generate(&e); + let second_owner = Address::generate(&e); let token_admin = Address::generate(&e); let amount = i128::MAX; @@ -128,6 +127,7 @@ fn test_create_commitment_rejects_fee_math_overflow() { let asset_address = token_contract.address(); let token_admin_client = StellarAssetClient::new(&e, &asset_address); token_admin_client.mint(&owner, &amount); + token_admin_client.mint(&second_owner, &1_000); client.initialize(&admin, &nft_contract); client.set_creation_fee_bps(&admin, &2); @@ -138,4 +138,10 @@ fn test_create_commitment_rejects_fee_math_overflow() { assert_eq!(client.get_total_commitments(), 0); assert_eq!(client.get_total_value_locked(), 0); assert_eq!(client.get_collected_fees(&asset_address), 0); + + client.set_creation_fee_bps(&admin, &100); + assert!(client + .try_create_commitment(&second_owner, &1_000, &asset_address, &default_rules(&e)) + .is_ok()); + assert_eq!(client.get_total_commitments(), 1); } diff --git a/contracts/commitment_core/src/lib.rs b/contracts/commitment_core/src/lib.rs index 0269de6b..72ce1267 100644 --- a/contracts/commitment_core/src/lib.rs +++ b/contracts/commitment_core/src/lib.rs @@ -317,7 +317,11 @@ fn require_authorized_updater(e: &Env, caller: &Address) { .get::<_, bool>(&DataKey::AuthorizedUpdater(caller.clone())) .unwrap_or(false) { - fail(e, CommitmentError::NotAuthorizedUpdater, "require_authorized_updater"); + fail( + e, + CommitmentError::NotAuthorizedUpdater, + "require_authorized_updater", + ); } } @@ -504,11 +508,13 @@ impl CommitmentCoreContract { .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 - }; + // Single source of truth for creation-fee math: use the checked helper so + // overflow routes through `ArithmeticOverflow` and clears the guard. + let creation_fee = + fuzzing::checked_fee_from_bps(amount, creation_fee_bps).unwrap_or_else(|| { + set_reentrancy_guard(&e, false); + fail(&e, CommitmentError::ArithmeticOverflow, "create"); + }); let net_amount = amount.checked_sub(creation_fee).unwrap_or_else(|| { set_reentrancy_guard(&e, false); fail(&e, CommitmentError::ArithmeticOverflow, "create"); @@ -585,7 +591,9 @@ impl CommitmentCoreContract { set_reentrancy_guard(&e, false); fail(&e, CommitmentError::ArithmeticOverflow, "create"); }); - e.storage().instance().set(&DataKey::TotalValueLocked, &updated_tvl); + e.storage() + .instance() + .set(&DataKey::TotalValueLocked, &updated_tvl); let mut all_ids = e .storage() @@ -608,9 +616,7 @@ impl CommitmentCoreContract { set_reentrancy_guard(&e, false); fail(&e, CommitmentError::ArithmeticOverflow, "create"); }); - e.storage() - .instance() - .set(&fee_key, &updated_fees); + e.storage().instance().set(&fee_key, &updated_fees); } let nft_token_id = call_nft_mint( @@ -775,10 +781,9 @@ impl CommitmentCoreContract { /// from commitments to target pools. pub fn add_allocator(e: Env, caller: Address, allocator: Address) { require_admin(&e, &caller); - e.storage().instance().set( - &DataKey::AuthorizedAllocator(allocator.clone()), - &true, - ); + e.storage() + .instance() + .set(&DataKey::AuthorizedAllocator(allocator.clone()), &true); e.events().publish( (Symbol::new(&e, "AuthorizedAllocatorAdded"),), (allocator, e.ledger().timestamp()), @@ -790,7 +795,9 @@ impl CommitmentCoreContract { /// Restricted to the Admin role. pub fn remove_allocator(e: Env, caller: Address, allocator: Address) { require_admin(&e, &caller); - e.storage().instance().remove(&DataKey::AuthorizedAllocator(allocator.clone())); + e.storage() + .instance() + .remove(&DataKey::AuthorizedAllocator(allocator.clone())); e.events().publish( (Symbol::new(&e, "AuthorizedAllocatorRemoved"),), (allocator, e.ledger().timestamp()), @@ -814,10 +821,18 @@ impl CommitmentCoreContract { pub fn is_allocator(e: Env, address: Address) -> bool { let admin = e.storage().instance().get::<_, Address>(&DataKey::Admin); if let Some(a) = admin { - if address == a { return true; } + if address == a { + return true; + } } - if let Some(alloc_contract) = e.storage().instance().get::<_, Address>(&DataKey::AllocationContract) { - if address == alloc_contract { return true; } + if let Some(alloc_contract) = e + .storage() + .instance() + .get::<_, Address>(&DataKey::AllocationContract) + { + if address == alloc_contract { + return true; + } } e.storage() .instance() @@ -852,7 +867,9 @@ impl CommitmentCoreContract { pub fn is_guardian(e: Env, address: Address) -> bool { let admin = e.storage().instance().get::<_, Address>(&DataKey::Admin); if let Some(a) = admin { - if address == a { return true; } + if address == a { + return true; + } } e.storage() .instance() @@ -867,7 +884,9 @@ impl CommitmentCoreContract { pub fn is_treasurer(e: Env, address: Address) -> bool { let admin = e.storage().instance().get::<_, Address>(&DataKey::Admin); if let Some(a) = admin { - if address == a { return true; } + if address == a { + return true; + } } e.storage() .instance() @@ -882,9 +901,14 @@ impl CommitmentCoreContract { pub fn is_operator(e: Env, address: Address) -> bool { let admin = e.storage().instance().get::<_, Address>(&DataKey::Admin); if let Some(a) = admin { - if address == a { return true; } + if address == a { + return true; + } } - e.storage().instance().get::<_, bool>(&DataKey::AuthorizedOperator(address)).unwrap_or(false) + e.storage() + .instance() + .get::<_, bool>(&DataKey::AuthorizedOperator(address)) + .unwrap_or(false) } /// Update the current value of a commitment. @@ -948,12 +972,18 @@ impl CommitmentCoreContract { set_commitment(&e, &commitment); // Update TVL by the delta so the aggregate stays consistent with the persisted value. - let tvl = e.storage().instance().get::<_, i128>(&DataKey::TotalValueLocked).unwrap_or(0); + let tvl = e + .storage() + .instance() + .get::<_, i128>(&DataKey::TotalValueLocked) + .unwrap_or(0); let updated_tvl = tvl .checked_sub(old_value) .and_then(|value| value.checked_add(new_value)) .unwrap_or_else(|| fail(&e, CommitmentError::ArithmeticOverflow, "upd")); - e.storage().instance().set(&DataKey::TotalValueLocked, &updated_tvl); + e.storage() + .instance() + .set(&DataKey::TotalValueLocked, &updated_tvl); } pub fn check_violations(e: Env, commitment_id: String) -> bool { @@ -1070,7 +1100,9 @@ impl CommitmentCoreContract { } else { 0 }; - e.storage().instance().set(&DataKey::TotalValueLocked, &new_tvl); + e.storage() + .instance() + .set(&DataKey::TotalValueLocked, &new_tvl); transfer_assets( &e, @@ -1230,33 +1262,45 @@ impl CommitmentCoreContract { pub fn add_guardian(e: Env, caller: Address, guardian: Address) { require_admin(&e, &caller); - e.storage().instance().set(&DataKey::AuthorizedGuardian(guardian), &true); + e.storage() + .instance() + .set(&DataKey::AuthorizedGuardian(guardian), &true); } pub fn remove_guardian(e: Env, caller: Address, guardian: Address) { require_admin(&e, &caller); - e.storage().instance().remove(&DataKey::AuthorizedGuardian(guardian)); + e.storage() + .instance() + .remove(&DataKey::AuthorizedGuardian(guardian)); } pub fn add_treasurer(e: Env, caller: Address, treasurer: Address) { require_admin(&e, &caller); - e.storage().instance().set(&DataKey::AuthorizedTreasurer(treasurer), &true); + e.storage() + .instance() + .set(&DataKey::AuthorizedTreasurer(treasurer), &true); } pub fn remove_treasurer(e: Env, caller: Address, treasurer: Address) { require_admin(&e, &caller); - e.storage().instance().remove(&DataKey::AuthorizedTreasurer(treasurer)); + e.storage() + .instance() + .remove(&DataKey::AuthorizedTreasurer(treasurer)); } pub fn add_operator(e: Env, caller: Address, operator: Address) { require_admin(&e, &caller); - e.storage().instance().set(&DataKey::AuthorizedOperator(operator), &true); + e.storage() + .instance() + .set(&DataKey::AuthorizedOperator(operator), &true); } pub fn remove_operator(e: Env, caller: Address, operator: Address) { require_admin(&e, &caller); - e.storage().instance().remove(&DataKey::AuthorizedOperator(operator)); + e.storage() + .instance() + .remove(&DataKey::AuthorizedOperator(operator)); } /// Allocates assets from a commitment to a target investment pool. /// /// This operation is restricted to the admin or an authorized allocator contract. - /// It reduces the commitment's internal `current_value` and transfers the + /// It reduces the commitment's internal `current_value` and transfers the /// underlying tokens to the target address. /// /// ### Parameters @@ -1508,7 +1552,9 @@ impl CommitmentCoreContract { } // Update collected fees - e.storage().instance().set(&fee_key, &SafeMath::sub(collected, amount)); + e.storage() + .instance() + .set(&fee_key, &SafeMath::sub(collected, amount)); // Transfer fees to recipient transfer_assets( diff --git a/docs/FEES.md b/docs/FEES.md index 9c3d0fb0..dd29e061 100644 --- a/docs/FEES.md +++ b/docs/FEES.md @@ -68,7 +68,7 @@ Both round toward zero (floor for positive values). The sum of tranche amounts c | Fee | When | Calculation | Token flow | |-----|------|-------------|------------| -| Creation | `create_commitment` | `fees::fee_from_bps(amount, CreationFeeBps)`; default bps `0` | Owner transfers full `amount` to contract; `creation_fee` credited to `CollectedFees(asset_address)`; NFT minted with `net_amount = amount - creation_fee`; TVL incremented by `net_amount` | +| Creation | `create_commitment` | `fuzzing::checked_fee_from_bps(amount, CreationFeeBps)`; default bps `0`; overflow maps to `ArithmeticOverflow` with guard reset | Owner transfers full `amount` to contract; `creation_fee` credited to `CollectedFees(asset_address)`; NFT minted with `net_amount = amount - creation_fee`; TVL incremented by `net_amount` | | Early exit | `early_exit` | `SafeMath::penalty_amount(current_value, rules.early_exit_penalty)` | Penalty added to `CollectedFees(asset)`; `returned = current_value - penalty` transferred to owner when `returned > 0` | #### Storage keys diff --git a/docs/FEE_MODEL_CROSS_CHECK.md b/docs/FEE_MODEL_CROSS_CHECK.md index 5e7553e0..c28f1cd0 100644 --- a/docs/FEE_MODEL_CROSS_CHECK.md +++ b/docs/FEE_MODEL_CROSS_CHECK.md @@ -24,7 +24,7 @@ All four contracts are now documented in a single reconciled view in `docs/FEES. | `CreationFeeBps` storage | Yes | `DataKey::CreationFeeBps` | ✅ | | `CollectedFees(Address)` | Yes | `DataKey::CollectedFees(Address)` | ✅ | | `FeeRecipient` | Yes | `DataKey::FeeRecipient` | ✅ | -| Creation fee on `create_commitment` | `fee_from_bps`, credit `CollectedFees` | Lines ~478–635 | ✅ | +| Creation fee on `create_commitment` | `checked_fee_from_bps`, credit `CollectedFees` | Lines ~478–635 | ✅ | | Early exit penalty to `CollectedFees` | `SafeMath::penalty_amount` (percent / 100) | Lines ~1190–1204 | ✅ | | `set_creation_fee_bps` validates 0–10000 | Yes | `bps > fees::BPS_MAX` | ✅ | | `withdraw_fees` semantics | Treasurer/Admin, recipient required, cap by ledger | Lines ~1495–1537 | ✅ | @@ -133,7 +133,8 @@ Prior `docs/FEES.md` listed marketplace fees as "TBD". They are now documented a | Item | `shared_utils::fees` | Used by | |------|---------------------|---------| | `BPS_SCALE` / `BPS_MAX` = 10_000 | `fees.rs` | `commitment_core`, `commitment_transformation` | -| `fee_from_bps` — floor division | `fees.rs` | Creation, transformation fees | +| `fee_from_bps` — floor division | `fees.rs` | Transformation fees | +| `checked_fee_from_bps` — checked floor division with `Option` overflow signaling | `commitment_core::fuzzing` | Commitment creation fees | | `SafeMath::penalty_amount` — percent ÷ 100 | `math.rs` | `commitment_core::early_exit` | | `SafeMath::div(mul(price, bps), 10_000)` | `math.rs` | `commitment_marketplace` listings/auctions |