fix(storage): prevent init DoS and reentrancy by removing cross-contr…

settlement_contract/src/storage.rs

@@ -136,16 +136,22 @@ pub(crate) fn read_pending_recovery(env: &Env) -> PendingRecovery {
         .unwrap_or_else(|| panic_with_error!(env, SettlementError::RecoveryNotPending))
 }
 
+/// Validates that the provided governance address is a non-zero, non-empty address.
+///
+/// Note (Issue #124): This function intentionally avoids making a cross-contract call
+/// to `governance` during `init` or `update_governance`. Making a cross-contract call
+/// during initialization creates a reentrancy / DoS vector where a self-recursive or
+/// broken governance contract can call back into the uninitialized settlement contract
+/// (causing `NotInitialized` panics) or trap. Governance fee config validity is
+/// verified at first use via `try_invoke_contract` in [`read_governance_fee_rule`]
+/// and [`validate_fee_against_governance`].
 pub(crate) fn validate_governance(env: &Env, governance: &Address) {
     validate_nonzero_address(
         env,
         governance,
         SettlementError::InvalidGovernance,
         SettlementError::InvalidGovernance,
     );
-    let args: Vec<Val> = Vec::new(env);
-    let _: Option<GovFeeConfig> =
-        env.invoke_contract(governance, &Symbol::new(env, "get_fee_config"), args);
 }
 
 pub(crate) fn validate_nonzero_address(

settlement_contract/src/tests/governance_error_tests.rs

@@ -188,6 +188,130 @@ fn write_path_set_default_rule_governance_failure_surfaces_typed_error() {
 }
 
 // ---------------------------------------------------------------------------
+// Issue #124: Init and update_governance succeed without cross-contract calls
+// ---------------------------------------------------------------------------
+
+mod reentrant_gov {
+    use soroban_sdk::{contract, contractimpl, IntoVal, Address, Env, Symbol};
+    use crate::{GovFeeConfig, SettlementContractClient};
+
+    /// A governance stub that attempts to call back into SettlementContract
+    /// during `get_fee_config` (simulates reentrancy).
+    #[contract]
+    pub struct ReentrantInitGovernance;
+
+    #[contractimpl]
+    impl ReentrantInitGovernance {
+        pub fn get_fee_config(env: Env) -> Option<GovFeeConfig> {
+            if let Some(settle_addr) = env
+                .storage()
+                .instance()
+                .get::<_, Address>(&Symbol::new(&env, "target_settle"))
+            {
+                let client = SettlementContractClient::new(&env, &settle_addr);
+                let _ = client.is_initialized();
+            }
+            None
+        }
+
+        pub fn set_target(env: Env, target_settle: Address) {
+            env.storage()
+                .instance()
+                .set(&Symbol::new(&env, "target_settle"), &target_settle);
+        }
+    }
+}
+
+use reentrant_gov::ReentrantInitGovernance;
+
+/// `init` must succeed regardless of governance's behavior, because `init`
+/// does not invoke cross-contract calls on `governance` (Issue #124).
+#[test]
+fn init_succeeds_with_panicking_governance() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let panicking_gov = env.register_contract(None, PanickingGovernance);
+    let admin = Address::generate(&env);
+    let recovery = Address::generate(&env);
+
+    let contract_id = env.register_contract(None, SettlementContract);
+    let client = SettlementContractClient::new(&env, &contract_id);
+
+    // init must succeed directly with panicking_gov without cross-calling it
+    client.init(
+        &soroban_sdk::vec![&env, admin.clone()],
+        &1,
+        &panicking_gov,
+        &recovery,
+    );
+
+    assert_eq!(client.get_governance(), panicking_gov);
+    assert_eq!(client.get_admin(), soroban_sdk::vec![&env, admin]);
+}
+
+/// `update_governance` must also succeed directly with a panicking governance
+/// contract without making cross-contract calls during update (Issue #124).
+#[test]
+fn update_governance_succeeds_with_panicking_governance() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let empty_gov = super::register_governance(&env);
+    let panicking_gov = env.register_contract(None, PanickingGovernance);
+    let admin = Address::generate(&env);
+    let recovery = Address::generate(&env);
+
+    let contract_id = env.register_contract(None, SettlementContract);
+    let client = SettlementContractClient::new(&env, &contract_id);
+    let admins = soroban_sdk::vec![&env, admin];
+
+    client.init(&admins, &1, &empty_gov, &recovery);
+    client.update_governance(&admins, &panicking_gov);
+
+    assert_eq!(client.get_governance(), panicking_gov);
+}
+
+/// `init` succeeds with a reentrant governance contract and guards against
+/// double-initialization reentrancy (Issue #124).
+#[test]
+fn init_succeeds_with_reentrant_governance_and_prevents_double_init() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let reentrant_gov_id = env.register_contract(None, ReentrantInitGovernance);
+    let admin = Address::generate(&env);
+    let recovery = Address::generate(&env);
+
+    let contract_id = env.register_contract(None, SettlementContract);
+    let client = SettlementContractClient::new(&env, &contract_id);
+
+    // Configure target for potential reentrancy callback
+    env.invoke_contract::<()>(
+        &reentrant_gov_id,
+        &soroban_sdk::Symbol::new(&env, "set_target"),
+        soroban_sdk::vec![&env, contract_id.to_val()],
+    );
+
+    client.init(
+        &soroban_sdk::vec![&env, admin.clone()],
+        &1,
+        &reentrant_gov_id,
+        &recovery,
+    );
+
+    assert!(client.is_initialized());
+    assert_eq!(client.get_governance(), reentrant_gov_id);
+
+    // Reentry / second initialization must panic with AlreadyInitialized
+    let res = client.try_init(
+        &soroban_sdk::vec![&env, admin],
+        &1,
+        &reentrant_gov_id,
+        &recovery,
+    );
+    assert!(res.is_err());
+}
 // Failure Variant Coverage for Governance Fee Rule Resolution
 // ---------------------------------------------------------------------------