diff --git a/settlement_contract/src/storage.rs b/settlement_contract/src/storage.rs index 1fe92d68..31974eb9 100644 --- a/settlement_contract/src/storage.rs +++ b/settlement_contract/src/storage.rs @@ -136,6 +136,15 @@ 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, @@ -143,9 +152,6 @@ pub(crate) fn validate_governance(env: &Env, governance: &Address) { SettlementError::InvalidGovernance, SettlementError::InvalidGovernance, ); - let args: Vec = Vec::new(env); - let _: Option = - env.invoke_contract(governance, &Symbol::new(env, "get_fee_config"), args); } pub(crate) fn validate_nonzero_address( diff --git a/settlement_contract/src/tests/governance_error_tests.rs b/settlement_contract/src/tests/governance_error_tests.rs index fa319b21..f0eafeb9 100644 --- a/settlement_contract/src/tests/governance_error_tests.rs +++ b/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 { + 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 // ---------------------------------------------------------------------------