fix: harden governance role topology

contract/vault/soroban/governance/src/lib.rs

@@ -1,14 +1,16 @@
 #![no_std]
 
 extern crate alloc;
+#[cfg(test)]
+extern crate std;
 
 mod types;
 pub use types::*;
 
 use alloc::{string::String as AllocString, vec::Vec as AllocVec};
 use soroban_sdk::{
     auth::{ContractContext, InvokerContractAuthEntry, SubContractInvocation},
-    contract, contractimpl, Address, Bytes, BytesN, Env, IntoVal, String, Symbol, Vec,
+    contract, contractimpl, Address, Bytes, BytesN, Env, Executable, IntoVal, String, Symbol, Vec,
 };
 use templar_curator_primitives::governance::{
     timelock_config_decision, CapChangeError, FeeChangeError, FeeConfig, MembershipChangeError,
@@ -120,6 +122,7 @@ impl SorobanVaultGovernanceContract {
     ) -> Result<(), GovernanceError> {
         extend_instance_ttl(&env);
         require_contract_address(&vault)?;
+        require_constructor_topology(&env, &admin, &vault)?;
         validate_timelock_ns(timelock_ns)?;
 
         env.storage().instance().set(&DataKey::Admin, &admin);
@@ -180,7 +183,7 @@ impl SorobanVaultGovernanceContract {
         caller: Address,
         governance: Address,
     ) -> Result<u64, GovernanceError> {
-        require_contract_address(&governance)?;
+        require_governance_target(&env, &governance)?;
         Self::submit(env, caller, GovernanceAction::SetGovernance(governance))
     }
 
@@ -604,7 +607,7 @@ impl SorobanVaultGovernanceContract {
         extend_instance_ttl(&env);
         require_admin(&env, &caller)?;
         require_not_abdicated(&env, &action)?;
-        validate_action(&action)?;
+        validate_action(&env, &action)?;
 
         let id = next_proposal_id(&env)?;
         let decision = decide_submission(&env, &action)?;
@@ -717,9 +720,9 @@ fn require_unique_target_ids(target_ids: &Vec<u32>) -> Result<(), GovernanceErro
     Ok(())
 }
 
-fn validate_action(action: &GovernanceAction) -> Result<(), GovernanceError> {
+fn validate_action(env: &Env, action: &GovernanceAction) -> Result<(), GovernanceError> {
     match action {
-        GovernanceAction::SetGovernance(governance) => require_contract_address(governance),
+        GovernanceAction::SetGovernance(governance) => require_governance_target(env, governance),
         GovernanceAction::SetFees(params) => {
             let _ = to_wad(params.performance_fee_wad)?;
             let _ = to_wad(params.management_fee_wad)?;
@@ -1654,8 +1657,10 @@ fn ledger_timestamp_ns(env: &Env) -> Result<TimestampNs, GovernanceError> {
 }
 
 fn is_contract_address(addr: &Address) -> bool {
-    let bytes = addr.to_string().to_bytes();
-    matches!(bytes.get(0), Some(b'C'))
+    matches!(
+        addr.executable(),
+        Some(Executable::Wasm(_)) | Some(Executable::StellarAsset)
+    )
 }
 
 fn require_contract_address(addr: &Address) -> Result<(), GovernanceError> {
@@ -1666,6 +1671,34 @@ fn require_contract_address(addr: &Address) -> Result<(), GovernanceError> {
     }
 }
 
+fn require_wasm_contract_address(addr: &Address) -> Result<(), GovernanceError> {
+    match addr.executable() {
+        Some(Executable::Wasm(_)) => Ok(()),
+        _ => Err(GovernanceError::InvalidInput),
+    }
+}
+
+fn require_governance_target(env: &Env, governance: &Address) -> Result<(), GovernanceError> {
+    require_wasm_contract_address(governance)?;
+    let vault = get_address(env, DataKey::Vault)?;
+    if governance == &vault || governance == &env.current_contract_address() {
+        return Err(GovernanceError::InvalidInput);
+    }
+    Ok(())
+}
+
+fn require_constructor_topology(
+    env: &Env,
+    admin: &Address,
+    vault: &Address,
+) -> Result<(), GovernanceError> {
+    let current = env.current_contract_address();
+    if admin == vault || admin == &current || vault == &current {
+        return Err(GovernanceError::InvalidInput);
+    }
+    Ok(())
+}
+
 fn extend_instance_ttl(env: &Env) {
     env.storage()
         .instance()

contract/vault/soroban/governance/src/tests.rs

@@ -1269,6 +1269,68 @@ fn cap_group_membership_clear_uses_mirrored_current_membership() {
     assert_eq!(duplicate_clear, Err(GovernanceError::NoChange));
 }
 
+#[test]
+fn governance_constructor_rejects_self_referential_or_colliding_roles() {
+    let env = Env::default();
+    env.mock_all_auths();
+    let admin = Address::generate(&env);
+    let vault = env.register(MockVault, ());
+
+    let admin_is_vault = Address::generate(&env);
+    let self_as_admin = Address::generate(&env);
+    let self_as_vault = Address::generate(&env);
+
+    let admin_is_vault_result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        env.register_at(
+            &admin_is_vault,
+            SorobanVaultGovernanceContract,
+            (&vault, &vault, &(0u64)),
+        );
+    }));
+    assert!(admin_is_vault_result.is_err());
+
+    let self_admin_result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        env.register_at(
+            &self_as_admin,
+            SorobanVaultGovernanceContract,
+            (&self_as_admin, &vault, &(0u64)),
+        );
+    }));
+    assert!(self_admin_result.is_err());
+
+    let self_vault_result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        env.register_at(
+            &self_as_vault,
+            SorobanVaultGovernanceContract,
+            (&admin, &self_as_vault, &(0u64)),
+        );
+    }));
+    assert!(self_vault_result.is_err());
+}
+
+#[test]
+fn set_governance_rejects_obvious_invalid_contract_targets() {
+    let env = Env::default();
+    env.mock_all_auths();
+    let admin = Address::generate(&env);
+    let vault = env.register(MockVault, ());
+    let governance = env.register(SorobanVaultGovernanceContract, (&admin, &vault, &(0u64)));
+    let asset_contract = env
+        .register_stellar_asset_contract_v2(Address::generate(&env))
+        .address();
+
+    for target in [vault.clone(), governance.clone(), asset_contract] {
+        let result = env.as_contract(&governance, || {
+            SorobanVaultGovernanceContract::submit_set_governance(
+                env.clone(),
+                admin.clone(),
+                target.clone(),
+            )
+        });
+        assert_eq!(result, Err(GovernanceError::InvalidInput));
+    }
+}
+
 #[test]
 fn governance_change_is_timelocked_and_routes_to_vault() {
     let env = Env::default();
@@ -1286,7 +1348,7 @@ fn governance_change_is_timelocked_and_routes_to_vault() {
         (&admin, &vault, &(5_000_000_000u64)),
     );
 
-    let new_governance = Address::generate(&env);
+    let new_governance = env.register(SorobanVaultGovernanceContract, (&admin, &vault, &(0u64)));
 
     let proposal_id = env.as_contract(&governance, || {
         SorobanVaultGovernanceContract::submit_set_governance(

contract/vault/soroban/share-token/src/lib.rs

@@ -1,9 +1,14 @@
 #![no_std]
 
+#[cfg(test)]
+extern crate std;
+
 mod types;
 pub use types::*;
 
-use soroban_sdk::{contract, contractimpl, panic_with_error, Address, Env, MuxedAddress, String};
+use soroban_sdk::{
+    contract, contractimpl, panic_with_error, Address, Env, Executable, MuxedAddress, String,
+};
 use stellar_tokens::fungible::{
     burnable::{emit_burn, FungibleBurnable},
     Base, FungibleToken,
@@ -95,6 +100,7 @@ impl SorobanShareTokenContract {
     ) {
         extend_instance_ttl(&env);
         require_contract_address(&env, &vault);
+        require_vault_admin(&env, &admin, &vault);
         env.storage().instance().set(&DataKey::Admin, &admin);
         env.storage().instance().set(&DataKey::Vault, &vault);
         Base::set_metadata(&env, decimals, name, symbol);
@@ -109,6 +115,8 @@ impl SorobanShareTokenContract {
     pub fn set_admin(env: Env, caller: Address, admin: Address) {
         extend_instance_ttl(&env);
         require_admin(&env, &caller);
+        let vault = Self::vault(env.clone());
+        require_vault_admin(&env, &admin, &vault);
         env.storage().instance().set(&DataKey::Admin, &admin);
     }
 
@@ -171,8 +179,10 @@ fn require_vault_invoker(env: &Env) {
 }
 
 fn is_contract_address(addr: &Address) -> bool {
-    let bytes = addr.to_string().to_bytes();
-    matches!(bytes.get(0), Some(b'C'))
+    matches!(
+        addr.executable(),
+        Some(Executable::Wasm(_)) | Some(Executable::StellarAsset)
+    )
 }
 
 fn require_contract_address(env: &Env, addr: &Address) {
@@ -181,6 +191,12 @@ fn require_contract_address(env: &Env, addr: &Address) {
     }
 }
 
+fn require_vault_admin(env: &Env, admin: &Address, vault: &Address) {
+    if admin != vault {
+        panic_with_error!(env, ShareTokenError::InvalidInput);
+    }
+}
+
 fn extend_instance_ttl(env: &Env) {
     env.storage()
         .instance()

contract/vault/soroban/share-token/src/tests.rs

@@ -35,8 +35,8 @@ fn setup() -> (Env, Address, Address, Address) {
         ..Default::default()
     });
 
-    let admin = Address::generate(&env);
     let vault = env.register(VaultCaller, ());
+    let admin = vault.clone();
     let token = env.register(
         SorobanShareTokenContract,
         (
@@ -50,6 +50,45 @@ fn setup() -> (Env, Address, Address, Address) {
     (env, admin, vault, token)
 }
 
+#[test]
+fn constructor_rejects_external_share_token_admin() {
+    let env = Env::default();
+    env.mock_all_auths();
+    let external_admin = Address::generate(&env);
+    let vault = env.register(VaultCaller, ());
+    let token = Address::generate(&env);
+
+    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        env.register_at(
+            &token,
+            SorobanShareTokenContract,
+            (
+                &external_admin,
+                &vault,
+                &String::from_str(&env, "Templar Share"),
+                &String::from_str(&env, "tvSHARE"),
+                &7u32,
+            ),
+        );
+    }));
+
+    assert!(result.is_err());
+}
+
+#[test]
+fn set_admin_rejects_non_vault_admin() {
+    let (env, _admin, vault, _token) = setup();
+    let new_admin = Address::generate(&env);
+
+    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        env.as_contract(&vault, || {
+            SorobanShareTokenContract::set_admin(env.clone(), vault.clone(), new_admin.clone());
+        });
+    }));
+
+    assert!(result.is_err());
+}
+
 #[test]
 fn vault_can_mint() {
     let (env, _admin, vault, token) = setup();

contract/vault/soroban/src/contract/entrypoints.rs

@@ -9,8 +9,9 @@ use super::helpers::{
     ensure_governance_identity, ensure_sentinel_identity, extend_storage_ttl, get_config_address,
     governance_caller, kernel_address_from_sdk, load_virtual_offsets, migrate_legacy_paused,
     migration_in_progress, require_contract_address, require_governance,
-    require_governance_control_plane, require_sentinel, require_signed, sdk_string_to_alloc,
-    set_config_address, set_migration_in_progress, store_fees_spec, store_virtual_offsets,
+    require_governance_control_plane, require_sentinel, require_signed,
+    require_wasm_or_account_address, sdk_string_to_alloc, set_config_address,
+    set_migration_in_progress, store_fees_spec, store_virtual_offsets,
     with_contract_vault_contract_error,
 };
 use super::*;
@@ -57,9 +58,11 @@ fn require_unique_addresses(
     Ok(())
 }
 
-fn apply_curator_config(env: &Env, new_curator: soroban_sdk::Address) {
+fn apply_curator_config(env: &Env, new_curator: soroban_sdk::Address) -> Result<(), ContractError> {
+    require_wasm_or_account_address(&new_curator)?;
     set_config_address(env, &VaultDataKey::Curator, &new_curator);
     emit_admin_event(env, symbol_short!("s_curatr"));
+    Ok(())
 }
 
 fn apply_governance_config(
@@ -72,17 +75,22 @@ fn apply_governance_config(
     Ok(())
 }
 
-fn apply_sentinel_config(env: &Env, sentinel: soroban_sdk::Address) {
+fn apply_sentinel_config(env: &Env, sentinel: soroban_sdk::Address) -> Result<(), ContractError> {
+    require_wasm_or_account_address(&sentinel)?;
     env.storage()
         .instance()
         .set(&VaultDataKey::Sentinel, &sentinel);
     emit_admin_event(env, symbol_short!("s_sntnl"));
+    Ok(())
 }
 
 fn apply_guardians_config(
     env: &Env,
     guardians: soroban_sdk::Vec<soroban_sdk::Address>,
 ) -> Result<(), ContractError> {
+    for guardian in guardians.iter() {
+        require_wasm_or_account_address(&guardian)?;
+    }
     require_unique_addresses(&guardians)?;
     env.storage()
         .instance()
@@ -95,6 +103,9 @@ fn apply_allocators_config(
     env: &Env,
     allocators: soroban_sdk::Vec<soroban_sdk::Address>,
 ) -> Result<(), ContractError> {
+    for allocator in allocators.iter() {
+        require_wasm_or_account_address(&allocator)?;
+    }
     require_unique_addresses(&allocators)?;
     env.storage()
         .instance()
@@ -580,11 +591,11 @@ fn set_governance_config_impl(
     value_b: Option<i128>,
 ) -> Result<(), ContractError> {
     match kind {
-        GOVERNANCE_CONFIG_KIND_CURATOR => apply_curator_config(env, required_address(primary)?),
+        GOVERNANCE_CONFIG_KIND_CURATOR => apply_curator_config(env, required_address(primary)?)?,
         GOVERNANCE_CONFIG_KIND_GOVERNANCE => {
             apply_governance_config(env, required_address(primary)?)?
         }
-        GOVERNANCE_CONFIG_KIND_SENTINEL => apply_sentinel_config(env, required_address(primary)?),
+        GOVERNANCE_CONFIG_KIND_SENTINEL => apply_sentinel_config(env, required_address(primary)?)?,
         GOVERNANCE_CONFIG_KIND_GUARDIANS => apply_guardians_config(env, required_addresses(many)?)?,
         GOVERNANCE_CONFIG_KIND_ALLOCATORS => {
             apply_allocators_config(env, required_addresses(many)?)?
@@ -919,6 +930,9 @@ impl SorobanVaultContract {
         let virtual_shares = to_u128(virtual_shares)?;
         let virtual_assets = to_u128(virtual_assets)?;
 
+        require_wasm_or_account_address(&curator)?;
+        require_contract_address(&governance)?;
+
         set_config_address(&env, &VaultDataKey::Curator, &curator);
         set_config_address(&env, &VaultDataKey::Governance, &governance);
         set_config_address(&env, &VaultDataKey::AssetToken, &asset_token);