Merge pull request Emeka000#1071 from shakurJJ/fix/fee-overflow-address-length-custody-index

fix: address gas DoS, fee overflow, and address length issues

Author: Emeka000

contracts/src/constants.rs

@@ -96,6 +96,15 @@ pub const MAX_UNIT_ID_LENGTH: u32 = 64;
 /// Used for custody event IDs and other cryptographic identifiers.
 pub const HEX_HASH_LENGTH: usize = 64;
 
+// ── DELIVERY ADDRESS VALIDATION ────────────────────────────────────────────────
+
+/// Maximum length for a delivery_address string stored in BloodRequest.
+///
+/// Prevents callers from bloating on-chain storage with multi-KB strings while
+/// still accommodating real-world addresses. Every read of BloodRequest pays
+/// for the full record size, so this cap keeps storage costs bounded.
+pub const MAX_DELIVERY_ADDRESS_LENGTH: u32 = 200;
+
 // ── SUPER ADMIN NOMINATION ────────────────────────────────────────────────────
 
 /// Nomination expiry window in seconds (24 hours).

contracts/src/lib.rs

@@ -73,6 +73,8 @@ pub enum Error {
     DuplicateApproval = 31,
     EscrowNotReleasable = 32,
     InvalidFeePayload = 33,
+    /// delivery_address string exceeds MAX_DELIVERY_ADDRESS_LENGTH.
+    DeliveryAddressTooLong = 34,
 }
 
 // Alias for issue/docs terminology.
@@ -556,6 +558,8 @@ pub enum DataKey {
     HospitalUnits(Address),
     /// Per-unit pending custody event index: unit_id -> String (event_id of the active Pending custody event)
     UnitCustodyIndex(u64),
+    /// Per-unit custody events list: unit_id -> Vec<String> (all event_ids ever created for this unit)
+    UnitCustodyEvents(u64),
     /// Custody trail page: (unit_id, page_number) -> Vec<String> (max 20 event IDs)
     UnitTrailPage(u64, u32),
     /// Custody trail metadata: unit_id -> TrailMetadata
@@ -581,9 +585,10 @@ pub use storage_lifecycle::{
 
 // Re-export constants for internal use
 pub(crate) use constants::{
-    HEX_HASH_LENGTH, MAX_BATCH_EXPIRY_SIZE, MAX_BATCH_SIZE, MAX_EVENTS_PER_PAGE, MAX_QUANTITY_ML,
-    MAX_REQUEST_ML, MAX_SHELF_LIFE_DAYS, MAX_UNIT_ID_LENGTH, MIN_QUANTITY_ML, MIN_REQUEST_ML,
-    MIN_SHELF_LIFE_DAYS, NOMINATION_EXPIRY_SECONDS, SECONDS_PER_DAY, TRANSFER_EXPIRY_SECONDS,
+    HEX_HASH_LENGTH, MAX_BATCH_EXPIRY_SIZE, MAX_BATCH_SIZE, MAX_DELIVERY_ADDRESS_LENGTH,
+    MAX_EVENTS_PER_PAGE, MAX_QUANTITY_ML, MAX_REQUEST_ML, MAX_SHELF_LIFE_DAYS, MAX_UNIT_ID_LENGTH,
+    MIN_QUANTITY_ML, MIN_REQUEST_ML, MIN_SHELF_LIFE_DAYS, NOMINATION_EXPIRY_SECONDS,
+    SECONDS_PER_DAY, TRANSFER_EXPIRY_SECONDS,
 };
 
 /// Pending SuperAdmin nomination entry.
@@ -1393,6 +1398,19 @@ impl HealthChainContract {
         let index_key = DataKey::UnitCustodyIndex(unit_id);
         env.storage().persistent().set(&index_key, &event_id);
 
+        // Maintain per-unit custody events list so archive_custody_events can find all events
+        // for this unit in O(k) (k = events per unit) instead of scanning the full CUSTODY_EVENTS map
+        let unit_events_key = DataKey::UnitCustodyEvents(unit_id);
+        let mut unit_event_ids: Vec<String> = env
+            .storage()
+            .persistent()
+            .get(&unit_events_key)
+            .unwrap_or(Vec::new(&env));
+        unit_event_ids.push_back(event_id.clone());
+        env.storage()
+            .persistent()
+            .set(&unit_events_key, &unit_event_ids);
+
         let old_status = unit.status;
         unit.status = BloodStatus::InTransit;
         unit.transfer_timestamp = Some(current_time);
@@ -2484,6 +2502,10 @@ impl HealthChainContract {
             return Err(Error::InvalidDeliveryAddress);
         }
 
+        if delivery_address.len() > MAX_DELIVERY_ADDRESS_LENGTH {
+            return Err(Error::DeliveryAddressTooLong);
+        }
+
         let current_time = env.ledger().timestamp();
         if required_by <= current_time {
             return Err(Error::InvalidRequiredBy);
@@ -5204,6 +5226,31 @@ mod test {
         );
     }
 
+    #[test]
+    #[should_panic(expected = "Error(Contract, #34)")]
+    fn test_create_request_delivery_address_too_long() {
+        let env = Env::default();
+        let (_, _, hospital, client) = setup_contract_with_hospital(&env);
+
+        env.mock_all_auths();
+        let current_time = env.ledger().timestamp();
+        let required_by = current_time + 3600;
+
+        // 201 bytes — one byte over MAX_DELIVERY_ADDRESS_LENGTH (200)
+        let addr_bytes = [b'a'; 201];
+        let addr_str = core::str::from_utf8(&addr_bytes).unwrap();
+        let long_addr = String::from_str(&env, addr_str);
+
+        client.create_request(
+            &hospital,
+            &BloodType::OPositive,
+            &200,
+            &UrgencyLevel::High,
+            &required_by,
+            &long_addr,
+        );
+    }
+
     #[test]
     #[should_panic(expected = "Error(Contract, #14)")]
     fn test_create_request_empty_delivery_address() {

contracts/src/payments.rs

@@ -346,9 +346,12 @@ impl PendingApproval {
 }
 
 impl FeeStructure {
-    /// Calculates total fees
-    pub fn total(&self) -> i128 {
-        self.service_fee + self.network_fee + self.performance_bonus + self.fixed_fee
+    /// Calculates total fees, returning None if any intermediate sum overflows i128.
+    pub fn total(&self) -> Option<i128> {
+        self.service_fee
+            .checked_add(self.network_fee)?
+            .checked_add(self.performance_bonus)?
+            .checked_add(self.fixed_fee)
     }
 
     /// Validates fee structure
@@ -365,7 +368,7 @@ impl FeeStructure {
 
     /// Calculates net amount after deducting fees
     pub fn calculate_net_amount(&self, gross_amount: i128) -> Result<i128, PaymentError> {
-        let total_fees = self.total();
+        let total_fees = self.total().ok_or(PaymentError::Overflow)?;
         if total_fees > gross_amount {
             return Err(PaymentError::FeesExceedAmount);
         }
@@ -386,4 +389,5 @@ pub enum PaymentError {
     EscrowNotReleasable,
     InvalidMultiSigConfig,
     DuplicateApproval,
+    Overflow,
 }

contracts/src/storage_lifecycle.rs

@@ -325,6 +325,19 @@ pub fn archive_custody_events(env: &Env, unit_id: u64) -> Result<bool, Error> {
         return Ok(false);
     }
 
+    // Use the per-unit index to find only this unit's event_ids — O(k) where k = events
+    // for this unit, avoiding an O(n) scan over all custody events across all units.
+    let unit_events_key = DataKey::UnitCustodyEvents(unit_id);
+    let event_ids: Vec<SorobanString> = env
+        .storage()
+        .persistent()
+        .get(&unit_events_key)
+        .unwrap_or(Vec::new(env));
+
+    if event_ids.is_empty() {
+        return Ok(false);
+    }
+
     let mut custody_events: Map<SorobanString, CustodyEvent> = env
         .storage()
         .persistent()
@@ -334,36 +347,30 @@ pub fn archive_custody_events(env: &Env, unit_id: u64) -> Result<bool, Error> {
     let mut confirmed: u32 = 0;
     let mut cancelled: u32 = 0;
     let mut last_event_at: u64 = 0;
-    let mut keys_to_remove: Vec<SorobanString> = Vec::new(env);
 
-    for (event_id, event) in custody_events.iter() {
-        if event.unit_id != unit_id {
-            continue;
+    for i in 0..event_ids.len() {
+        let event_id = event_ids.get(i).unwrap();
+        if let Some(event) = custody_events.get(event_id.clone()) {
+            match event.status {
+                CustodyStatus::Confirmed => confirmed += 1,
+                CustodyStatus::Cancelled => cancelled += 1,
+                CustodyStatus::Pending | CustodyStatus::Recovered => {}
+            }
+            if event.initiated_at > last_event_at {
+                last_event_at = event.initiated_at;
+            }
+            custody_events.remove(event_id);
         }
-        match event.status {
-            CustodyStatus::Confirmed => confirmed += 1,
-            CustodyStatus::Cancelled => cancelled += 1,
-            CustodyStatus::Pending | CustodyStatus::Recovered => {}
-        }
-        if event.initiated_at > last_event_at {
-            last_event_at = event.initiated_at;
-        }
-        keys_to_remove.push_back(event_id);
     }
 
-    if keys_to_remove.is_empty() {
-        return Ok(false);
-    }
-
-    for i in 0..keys_to_remove.len() {
-        let key = keys_to_remove.get(i).unwrap();
-        custody_events.remove(key);
-    }
     env.storage()
         .persistent()
         .set(&CUSTODY_EVENTS, &custody_events);
     bump_persistent(env, &CUSTODY_EVENTS);
 
+    // Clear the per-unit index now that its events have been archived
+    env.storage().persistent().remove(&unit_events_key);
+
     let summary = ArchivedCustodySummary {
         total_confirmed: confirmed,
         total_cancelled: cancelled,

contracts/src/test_payments.rs

@@ -437,7 +437,7 @@ fn fee_calculation_is_correct() {
         fixed_fee: 0,
     };
 
-    assert_eq!(fees.total(), 20);
+    assert_eq!(fees.total(), Some(20));
     assert_eq!(fees.calculate_net_amount(1_000).unwrap(), 980);
 }
 
@@ -1064,3 +1064,49 @@ fn test_create_payment_fails_with_unauthorized_backend_auth() {
         assert_eq!(e, crate::Error::Unauthorized);
     }
 }
+
+// ======================================================
+// FeeStructure::total() overflow tests (#941)
+// ======================================================
+
+#[test]
+fn fee_structure_total_returns_correct_sum() {
+    let env = Env::default();
+    let fee = FeeStructure {
+        policy_id: Symbol::new(&env, "p"),
+        service_fee: 100,
+        network_fee: 50,
+        performance_bonus: 25,
+        fixed_fee: 10,
+    };
+    assert_eq!(fee.total(), Some(185));
+}
+
+#[test]
+fn fee_structure_total_returns_none_on_i128_overflow() {
+    let env = Env::default();
+    let fee = FeeStructure {
+        policy_id: Symbol::new(&env, "p"),
+        service_fee: i128::MAX / 2,
+        network_fee: i128::MAX / 2,
+        performance_bonus: 3,
+        fixed_fee: 0,
+    };
+    assert_eq!(fee.total(), None);
+}
+
+#[test]
+fn fee_structure_calculate_net_errors_on_overflow() {
+    let env = Env::default();
+    let fee = FeeStructure {
+        policy_id: Symbol::new(&env, "p"),
+        service_fee: i128::MAX / 2,
+        network_fee: i128::MAX / 2,
+        performance_bonus: 3,
+        fixed_fee: 0,
+    };
+    assert_eq!(
+        fee.calculate_net_amount(1_000_000),
+        Err(PaymentError::Overflow)
+    );
+}

contracts/src/test_storage_layout.rs

@@ -842,3 +842,50 @@ fn test_storage_layout_fingerprint_regression_guard() {
         "Storage layout compatibility changed: duplicate key symbols detected. Add migration guardrails before changing key names."
     );
 }
+
+// ── #944: archive_custody_events per-unit index ─────────────────────────────
+
+#[test]
+fn test_initiate_transfer_populates_unit_custody_events_index() {
+    use crate::{BloodComponent, BloodType, DataKey, HealthChainContract, HealthChainContractClient};
+    use soroban_sdk::{testutils::Address as _, Address, Env, String, Vec};
+
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let contract_id = env.register(HealthChainContract, ());
+    let client = HealthChainContractClient::new(&env, &contract_id);
+
+    let admin = Address::generate(&env);
+    let bank = Address::generate(&env);
+    let hospital = Address::generate(&env);
+
+    client.initialize(&admin);
+    client.register_blood_bank(&bank);
+    client.register_hospital(&hospital);
+
+    let expiration = env.ledger().timestamp() + 86400 * 10;
+    let unit_id = client.register_blood(
+        &bank,
+        &BloodType::OPositive,
+        &BloodComponent::WholeBlood,
+        &450,
+        &expiration,
+        &None,
+    );
+    client.allocate_blood(&bank, &unit_id, &hospital);
+    let event_id = client.initiate_transfer(&bank, &unit_id);
+
+    // Verify UnitCustodyEvents index was populated with the event_id
+    env.as_contract(&contract_id, || {
+        let key = DataKey::UnitCustodyEvents(unit_id);
+        let stored: Vec<String> = env
+            .storage()
+            .persistent()
+            .get(&key)
+            .expect("UnitCustodyEvents index must be populated after initiate_transfer");
+
+        assert_eq!(stored.len(), 1);
+        assert_eq!(stored.get(0).unwrap(), event_id);
+    });
+}