feat: [contract] Fix corrupted total_assets oracle block (.expect + d…

contracts/bridge-compat/src/lib.rs

@@ -30,6 +30,7 @@ use soroban_sdk::{
     contract, contractclient, contracterror, contractimpl, contracttype, symbol_short, Address,
     Bytes, Env, String, Vec,
 };
+use soroban_sdk::xdr::ToXdr;
 
 // ── Error types ────────────────────────────────────────────────────────────
 
@@ -159,6 +160,7 @@ enum DataKey {
     EpochStart,
     Limits,
     DefaultProvider,
+    TransferNonceFor(Address, u32),
 }
 
 // ── Constants ──────────────────────────────────────────────────────────────
@@ -441,21 +443,23 @@ impl BridgeCompat {
             return Err(BridgeError::InsufficientBalance);
         }
 
-        // Transfer tokens from sender to this contract
-        token_client.transfer(&sender, &env.current_contract_address(), &amount);
-
-        // Create transfer record
-        let transfer_id = env
+        // Nonces are monotonic within a sender/provider domain. Transfer IDs
+        // additionally bind that nonce to both identities so they cannot be
+        // confused with replay nonces or reused across providers.
+        let nonce_key = DataKey::TransferNonceFor(sender.clone(), pid);
+        let previous_nonce = env
             .storage()
             .instance()
-            .get::<_, u64>(&DataKey::TransferNonce)
-            .unwrap_or(0)
-            + 1;
-        let nonce = env
-            .storage()
-            .instance()
-            .get::<_, u64>(&DataKey::TransferNonce)
+            .get::<_, u64>(&nonce_key)
             .unwrap_or(0);
+        let nonce = previous_nonce
+            .checked_add(1)
+            .ok_or(BridgeError::NonceAlreadyUsed)?;
+        let transfer_id = Self::derive_transfer_id(&env, &sender, nonce, pid);
+        let transfer_key = DataKey::Transfer(transfer_id);
+        if env.storage().instance().has(&transfer_key) {
+            return Err(BridgeError::NonceAlreadyUsed);
+        }
 
         let transfer = BridgeTransfer {
             transfer_id,
@@ -473,10 +477,10 @@ impl BridgeCompat {
 
         env.storage()
             .instance()
-            .set(&DataKey::Transfer(transfer_id), &transfer);
+            .set(&transfer_key, &transfer);
         env.storage()
             .instance()
-            .set(&DataKey::TransferNonce, &(transfer_id));
+            .set(&nonce_key, &nonce);
 
         // Update user transfer count
         let user_count: i128 = env
@@ -490,10 +494,14 @@ impl BridgeCompat {
         );
 
         env.events().publish(
-            (symbol_short!("brgout"), sender),
+            (symbol_short!("brgout"), sender.clone()),
             (transfer_id, amount, dest_chain, pid),
         );
 
+        // Persist the nonce and transfer record before the token interaction.
+        // A callback therefore cannot observe or reuse an uncommitted nonce.
+        token_client.transfer(&sender, &env.current_contract_address(), &amount);
+
         Ok(transfer_id)
     }
 
@@ -602,6 +610,17 @@ impl BridgeCompat {
             .ok_or(BridgeError::ProviderNotFound)
     }
 
+    fn derive_transfer_id(env: &Env, sender: &Address, nonce: u64, provider_id: u32) -> u64 {
+        let mut payload = sender.to_xdr(env);
+        payload.append(&nonce.to_xdr(env));
+        payload.append(&provider_id.to_xdr(env));
+        let digest = env.crypto().sha256(&payload).to_array();
+        u64::from_be_bytes([
+            digest[0], digest[1], digest[2], digest[3],
+            digest[4], digest[5], digest[6], digest[7],
+        ])
+    }
+
     fn check_epoch_volume(
         env: &Env,
         amount: i128,
@@ -733,6 +752,80 @@ mod tests {
         assert_eq!(limits.epoch_duration, 86_400);
     }
 
+    #[test]
+    fn transfer_ids_are_context_bound_and_duplicate_ids_are_rejected() {
+        let env = Env::default();
+        env.mock_all_auths();
+
+        let bridge_id = env.register(BridgeCompat, ());
+        let client = BridgeCompatClient::new(&env, &bridge_id);
+        let admin = Address::generate(&env);
+        let token_admin = Address::generate(&env);
+        let token_id = env
+            .register_stellar_asset_contract_v2(token_admin.clone())
+            .address();
+        let token_admin_client = soroban_sdk::token::StellarAssetClient::new(&env, &token_id);
+        client.initialize(&admin, &token_id);
+
+        let sender_a = Address::generate(&env);
+        let sender_b = Address::generate(&env);
+        token_admin_client.mint(&bridge_id, &1_000);
+        token_admin_client.mint(&sender_a, &300);
+        token_admin_client.mint(&sender_b, &300);
+
+        let endpoint_a = Address::generate(&env);
+        let endpoint_b = Address::generate(&env);
+        let chains = Vec::from_array(&env, &[1u32]);
+        let provider_a = client.register_provider(
+            &String::from_str(&env, "provider-a"),
+            &BridgeProviderKind::Wormhole,
+            &endpoint_a,
+            &0,
+            &1_000,
+            &chains,
+        );
+        let provider_b = client.register_provider(
+            &String::from_str(&env, "provider-b"),
+            &BridgeProviderKind::LayerZero,
+            &endpoint_b,
+            &0,
+            &1_000,
+            &chains,
+        );
+        let recipient = Bytes::new(&env);
+
+        let id_a = client.transfer_out(&sender_a, &recipient, &100, &1, &Some(provider_a));
+        let id_b = client.transfer_out(&sender_b, &recipient, &100, &1, &Some(provider_b));
+        let id_a_next = client.transfer_out(&sender_a, &recipient, &100, &1, &Some(provider_a));
+
+        let transfer_a = client.transfer(&id_a).unwrap();
+        let transfer_b = client.transfer(&id_b).unwrap();
+        let transfer_a_next = client.transfer(&id_a_next).unwrap();
+        assert_eq!(transfer_a.nonce, 1);
+        assert_eq!(transfer_b.nonce, 1);
+        assert_eq!(transfer_a_next.nonce, 2);
+        assert_ne!(id_a, transfer_a.nonce);
+        assert_ne!(id_a, id_b);
+        assert_ne!(id_a, id_a_next);
+        assert_eq!(transfer_a.transfer_id, id_a);
+
+        // Simulate replayed nonce state: the derived ID is already occupied,
+        // so a transfer must fail instead of replacing the original record.
+        env.as_contract(&bridge_id, || {
+            env.storage().instance().set(
+                &DataKey::TransferNonceFor(sender_a.clone(), provider_a),
+                &0u64,
+            );
+        });
+        let duplicate = client.try_transfer_out(
+            &sender_a,
+            &recipient,
+            &100,
+            &1,
+            &Some(provider_a),
+        );
+        assert_eq!(duplicate, Err(Ok(BridgeError::NonceAlreadyUsed)));
+        assert_eq!(client.transfer(&id_a), Some(transfer_a));
     // ── Acceptance Criteria Tests ────────────────────────────────────────────
 
     #[test]

contracts/share-price-math/src/fuzz_invariants.rs

@@ -1,12 +1,12 @@
 //! Share-price math invariants shared by proptest and libFuzzer.
 
-use crate::{try_assets_to_shares, try_shares_to_assets};
+use crate::{try_assets_to_shares, try_shares_to_assets, MathError};
 
-fn shares_for(assets: i128, total_shares: i128, total_assets: i128) -> Option<i128> {
+fn shares_for(assets: i128, total_shares: i128, total_assets: i128) -> Result<i128, MathError> {
     try_assets_to_shares(assets, total_shares, total_assets)
 }
 
-fn assets_for(shares: i128, total_shares: i128, total_assets: i128) -> Option<i128> {
+fn assets_for(shares: i128, total_shares: i128, total_assets: i128) -> Result<i128, MathError> {
     try_shares_to_assets(shares, total_shares, total_assets)
 }
 
@@ -80,19 +80,20 @@ pub fn assert_share_price_invariants(
     let _ = assets_for(assets.min(total_shares.max(1)), total_shares, total_assets);
 
     if total_shares == 0 && total_assets == 0 {
-        let shares = shares_for(assets, 0, 0).expect("bootstrap shares");
-        assert_eq!(
-            shares, assets,
-            "first deposit must be 1:1: assets={assets} shares={shares}"
-        );
+        if let Ok(shares) = shares_for(assets, 0, 0) {
+            assert_eq!(
+                shares, assets,
+                "first deposit must be 1:1: assets={assets} shares={shares}"
+            );
+        }
     }
 
-    if let Some(shares) = shares_for(assets, total_shares, total_assets) {
+    if let Ok(shares) = shares_for(assets, total_shares, total_assets) {
         if let (Some(new_shares), Some(new_assets)) = (
             total_shares.checked_add(shares),
             total_assets.checked_add(assets),
         ) {
-            if let Some(returned) = assets_for(shares, new_shares, new_assets) {
+            if let Ok(returned) = assets_for(shares, new_shares, new_assets) {
                 assert!(
                     returned <= assets,
                     "round-trip returned more than deposited: {returned} > {assets}"
@@ -102,9 +103,9 @@ pub fn assert_share_price_invariants(
     }
 
     let probe_shares = assets.min(total_shares.max(1));
-    let before = assets_for(probe_shares, total_shares, total_assets);
+    let before = assets_for(probe_shares, total_shares, total_assets).ok();
     if let Some(after_total_assets) = total_assets.checked_add(yield_amount) {
-        let after = assets_for(probe_shares, total_shares, after_total_assets);
+        let after = assets_for(probe_shares, total_shares, after_total_assets).ok();
         if let (Some(before), Some(after)) = (before, after) {
             assert!(
                 after >= before,

contracts/share-price-math/src/lib.rs

@@ -16,50 +16,67 @@
 pub mod fuzz_invariants;
 pub mod rounding;
 
-/// Checked variant of [`assets_to_shares`] for fuzzing and property tests.
+/// Error returned when checked math cannot produce an i128 result.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub enum MathError {
+    Overflow,
+    DivisionByZero,
+}
+
+/// Checked assets-to-shares conversion using round-down arithmetic.
 ///
-/// Returns `None` when intermediate multiplication would overflow i128.
-pub fn try_assets_to_shares(assets: i128, total_shares: i128, total_assets: i128) -> Option<i128> {
+/// An empty vault (zero shares or assets) uses a 1:1 conversion.
+pub fn try_assets_to_shares(
+    assets: i128,
+    total_shares: i128,
+    total_assets: i128,
+) -> Result<i128, MathError> {
     if total_assets == 0 || total_shares == 0 {
-        return Some(assets);
+        return Ok(assets);
     }
-    assets.checked_mul(total_shares)?.checked_div(total_assets)
+    let product = assets
+        .checked_mul(total_shares)
+        .ok_or(MathError::Overflow)?;
+    product
+        .checked_div(total_assets)
+        .ok_or(MathError::Overflow)
 }
 
-/// Checked variant of [`shares_to_assets`] for fuzzing and property tests.
+/// Checked shares-to-assets conversion using round-down arithmetic.
 ///
-/// Returns `None` when intermediate multiplication would overflow i128.
-pub fn try_shares_to_assets(shares: i128, total_shares: i128, total_assets: i128) -> Option<i128> {
+/// A vault with no outstanding shares redeems to zero.
+pub fn try_shares_to_assets(
+    shares: i128,
+    total_shares: i128,
+    total_assets: i128,
+) -> Result<i128, MathError> {
     if total_shares == 0 {
-        return Some(0);
+        return Ok(0);
     }
-    shares.checked_mul(total_assets)?.checked_div(total_shares)
+    let product = shares
+        .checked_mul(total_assets)
+        .ok_or(MathError::Overflow)?;
+    product
+        .checked_div(total_shares)
+        .ok_or(MathError::Overflow)
 }
 
 /// Converts assets to shares using the current vault state (round-down).
-pub fn assets_to_shares(assets: i128, total_shares: i128, total_assets: i128) -> i128 {
-    if total_assets == 0 || total_shares == 0 {
-        return assets;
-    }
-
-    assets
-        .checked_mul(total_shares)
-        .expect("overflow in assets_to_shares multiplication")
-        .checked_div(total_assets)
-        .expect("division by zero in assets_to_shares")
+pub fn assets_to_shares(
+    assets: i128,
+    total_shares: i128,
+    total_assets: i128,
+) -> Result<i128, MathError> {
+    try_assets_to_shares(assets, total_shares, total_assets)
 }
 
 /// Converts shares to assets using the current vault state (round-down).
-pub fn shares_to_assets(shares: i128, total_shares: i128, total_assets: i128) -> i128 {
-    if total_shares == 0 {
-        return 0;
-    }
-
-    shares
-        .checked_mul(total_assets)
-        .expect("overflow in shares_to_assets multiplication")
-        .checked_div(total_shares)
-        .expect("division by zero in shares_to_assets")
+pub fn shares_to_assets(
+    shares: i128,
+    total_shares: i128,
+    total_assets: i128,
+) -> Result<i128, MathError> {
+    try_shares_to_assets(shares, total_shares, total_assets)
 }
 
 #[cfg(test)]
@@ -68,21 +85,32 @@ mod tests {
 
     #[test]
     fn test_first_deposit_one_to_one() {
-        let shares = assets_to_shares(1000, 0, 0);
+        let shares = assets_to_shares(1000, 0, 0).unwrap();
         assert_eq!(shares, 1000);
     }
 
     #[test]
     fn test_round_trip_never_increases_value() {
         let original_assets = 300;
-        let shares = assets_to_shares(original_assets, 1000, 1500);
-        let recovered_assets = shares_to_assets(shares, 1000 + shares, 1500 + original_assets);
+        let shares = assets_to_shares(original_assets, 1000, 1500).unwrap();
+        let recovered_assets =
+            shares_to_assets(shares, 1000 + shares, 1500 + original_assets).unwrap();
         assert!(recovered_assets <= original_assets);
     }
 
     #[test]
-    #[should_panic(expected = "overflow")]
     fn test_overflow_protection_assets_to_shares() {
-        let _ = assets_to_shares(i128::MAX, i128::MAX, 1);
+        assert_eq!(
+            try_assets_to_shares(i128::MAX, i128::MAX, 1),
+            Err(MathError::Overflow)
+        );
+    }
+
+    #[test]
+    fn test_overflow_protection_shares_to_assets() {
+        assert_eq!(
+            try_shares_to_assets(i128::MAX, 1, i128::MAX),
+            Err(MathError::Overflow)
+        );
     }
 }