Merge pull request #40 from Dopezapha/fix/multisig-governance-archival

fix(multisig): fix #10 by adding instance storage TTL extensions

Author: ogazboiz

GOVERNANCE_STORAGE.md

@@ -0,0 +1,64 @@
+# Governance Storage & TTL Strategy
+
+## Objective
+Resolve the instance storage archival issue in the `multisig_governance` contract to prevent in-flight proposal data loss and guarantee finalization.
+
+---
+
+## 1. Storage Class Strategy
+The `multisig_governance` contract stores all configuration state (admin address, target contract, version, proposal count) and in-flight proposal data (`PendingTransfer` structure under `KEY_PENDING`) in **Instance Storage**. 
+
+In Soroban, instance storage has a specific Time-To-Live (TTL) that is checked and decremented on every ledger close. If instance storage is not accessed or explicitly bumped for a duration exceeding its TTL, it gets archived by the host. Once archived, any contract execution that tries to read or write instance storage (including admin verification via `read_admin`, loading target contract, or voting/approving) will fail.
+
+To prevent this:
+- We introduce a centralized helper function `bump_instance_ttl` that calls `env.storage().instance().extend_ttl(...)` using pre-calculated safe bounds.
+- This helper is integrated across all state-modifying entrypoints (write paths) and all read-only views (read paths). By executing a TTL extension on every contract invocation, we ensure that as long as there is active interest (either querying or writing), the contract state is kept alive automatically.
+
+---
+
+## 2. TTL Rationale & Parameters
+
+The Soroban SDK functions require two parameters for TTL management:
+1. `threshold`: The minimum number of ledgers remaining before a bump is triggered.
+2. `bump_to`: The target ledger lifetime to extend to if the threshold is breached.
+
+We define these as:
+```rust
+const INSTANCE_TTL_THRESHOLD: u32 = 17280; // ~1 day (assuming 5-second ledgers)
+const INSTANCE_TTL_BUMP: u32 = 518400;     // ~30 days (assuming 5-second ledgers)
+```
+
+### Rationale
+- **Quorum / Timelock Lifetime**: A proposal must remain pending for a minimum timelock delay of 24 hours (`MIN_TIMELOCK_SECONDS = 86_400`) and expires after a maximum of 7 days (`PROPOSAL_TTL_SECONDS = 604_800`).
+- **Safety Window**: To prevent a proposal from being archived while in-flight (waiting for the timelock to expire or gathering approvals), the instance storage TTL must comfortably exceed the maximum potential proposal lifetime (7 days + 1 day delay = 8 days).
+- **Threshold (1 Day)**: Setting the threshold to 17,280 ledgers (~1 day) ensures that any interaction within a day of potential expiry triggers the extension.
+- **Bump (30 Days)**: Setting the bump to 518,400 ledgers (~30 days) guarantees that the contract instance remains active for a full month on any interaction. This safely accommodates the 7-day proposal TTL + timelock and provides a significant safety margin.
+
+---
+
+## 3. Integration Points
+The `bump_instance_ttl` helper is called in the following functions:
+- `initialize`: Bumps TTL immediately upon contract creation.
+- `version`: Bumps TTL on read.
+- `upgrade`: Bumps TTL before upgrading contract code.
+- `propose_admin_transfer`: Extends TTL on proposal creation.
+- `approve_transfer`: Extends TTL on approval/voting.
+- `finalize_admin_transfer`: Extends TTL before finalization.
+- `cancel_admin_transfer`: Extends TTL on cancellation.
+- `emergency_cancel_proposal`: Extends TTL on emergency actions.
+- `expire_proposal`: Extends TTL when marking a proposal as expired.
+- All view functions (`get_target`, `get_pending_transfer`, `get_pending`, `has_pending_transfer`, `get_approval_count`, `get_timelock_remaining`) and the private `read_admin` helper.
+
+---
+
+## 4. Verification Test
+In `test.rs`, we added the integration test `test_proposal_ttl_extension_keeps_proposal_active_and_finalizable`.
+This test simulates the full lifecycle of a multi-sig admin transfer proposal:
+1. Propose transfer with a 24-hour timelock delay.
+2. Approve from the first signer.
+3. Advance the ledger sequence number by `100,000` blocks (~5.8 days) and timestamp by `200,000` seconds. This simulates a long period of inactivity during the voting window.
+4. Approve from the second signer (this reads and modifies instance storage successfully, showing it was not archived).
+5. Advance beyond the 24-hour timelock.
+6. Finalize the proposal successfully.
+
+This test demonstrates that the governance instance storage remains active, finalizeable, and resilient to ledger sequence advancement.

multisig_governance/src/lib.rs

@@ -129,6 +129,15 @@ pub struct GovernanceContract;
 
 #[contractimpl]
 impl GovernanceContract {
+    const INSTANCE_TTL_THRESHOLD: u32 = 17280; // ~1 day (5s ledgers)
+    const INSTANCE_TTL_BUMP: u32 = 518400; // ~30 days (5s ledgers)
+
+    fn bump_instance_ttl(env: &Env) {
+        env.storage()
+            .instance()
+            .extend_ttl(Self::INSTANCE_TTL_THRESHOLD, Self::INSTANCE_TTL_BUMP);
+    }
+
     // ── Initialization ────────────────────────────────────────────────────────
 
     /// Initialize the governance contract.
@@ -143,15 +152,18 @@ impl GovernanceContract {
         env.storage().instance().set(&KEY_TARGET, &target_contract);
         env.storage().instance().set(&KEY_VERSION, &CURRENT_VERSION);
         env.storage().instance().set(&KEY_PROPOSAL_COUNT, &0u32);
+        Self::bump_instance_ttl(&env);
     }
 
     pub fn version(env: Env) -> u32 {
+        Self::bump_instance_ttl(&env);
         env.storage().instance().get(&KEY_VERSION).unwrap_or(0)
     }
 
     pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) {
         let admin = Self::read_admin(&env);
         admin.require_auth();
+        Self::bump_instance_ttl(&env);
 
         let old_version = Self::version(env.clone());
         let new_version = old_version.saturating_add(1);
@@ -182,6 +194,7 @@ impl GovernanceContract {
     ) {
         let admin = Self::read_admin(&env);
         admin.require_auth();
+        Self::bump_instance_ttl(&env);
 
         if let Some(pending) = env
             .storage()
@@ -284,6 +297,7 @@ impl GovernanceContract {
     /// Soroban's require_auth guarantees the caller genuinely controls `signer`.
     pub fn approve_transfer(env: Env, signer: Address) {
         signer.require_auth();
+        Self::bump_instance_ttl(&env);
 
         let mut pending: PendingTransfer = env
             .storage()
@@ -335,6 +349,7 @@ impl GovernanceContract {
     /// and must verify the caller is this governance contract address.
     pub fn finalize_admin_transfer(env: Env, caller: Address) {
         caller.require_auth();
+        Self::bump_instance_ttl(&env);
 
         let pending: PendingTransfer = env
             .storage()
@@ -408,6 +423,7 @@ impl GovernanceContract {
     pub fn cancel_admin_transfer(env: Env) {
         let admin = Self::read_admin(&env);
         admin.require_auth();
+        Self::bump_instance_ttl(&env);
 
         let mut pending: PendingTransfer = env
             .storage()
@@ -444,6 +460,7 @@ impl GovernanceContract {
     ) {
         let admin = Self::read_admin(&env);
         admin.require_auth();
+        Self::bump_instance_ttl(&env);
 
         let mut pending: PendingTransfer = env
             .storage()
@@ -486,6 +503,7 @@ impl GovernanceContract {
     /// This cleans up stale proposals and allows new ones to be created.
     pub fn expire_proposal(env: Env, caller: Address) {
         caller.require_auth();
+        Self::bump_instance_ttl(&env);
 
         let pending: PendingTransfer = env
             .storage()
@@ -527,24 +545,28 @@ impl GovernanceContract {
     }
 
     pub fn get_target(env: Env) -> Address {
+        Self::bump_instance_ttl(&env);
         env.storage()
             .instance()
             .get(&KEY_TARGET)
             .expect("target contract not set")
     }
 
     pub fn get_pending_transfer(env: Env) -> PendingTransfer {
+        Self::bump_instance_ttl(&env);
         env.storage()
             .instance()
             .get(&KEY_PENDING)
             .expect("no pending transfer (4004)")
     }
 
     pub fn get_pending(env: Env) -> Option<PendingTransfer> {
+        Self::bump_instance_ttl(&env);
         env.storage().instance().get(&KEY_PENDING)
     }
 
     pub fn has_pending_transfer(env: Env) -> bool {
+        Self::bump_instance_ttl(&env);
         if let Some(pending) = env
             .storage()
             .instance()
@@ -557,6 +579,7 @@ impl GovernanceContract {
     }
 
     pub fn get_approval_count(env: Env) -> u32 {
+        Self::bump_instance_ttl(&env);
         let pending: PendingTransfer = env
             .storage()
             .instance()
@@ -568,6 +591,7 @@ impl GovernanceContract {
     /// Returns seconds remaining until the timelock expires.
     /// Returns 0 if already elapsed or no pending transfer exists.
     pub fn get_timelock_remaining(env: Env) -> u64 {
+        Self::bump_instance_ttl(&env);
         match env
             .storage()
             .instance()
@@ -601,6 +625,7 @@ impl GovernanceContract {
     }
 
     fn read_admin(env: &Env) -> Address {
+        Self::bump_instance_ttl(env);
         env.storage()
             .instance()
             .get(&KEY_ADMIN)

multisig_governance/src/test.rs

@@ -32,6 +32,9 @@ impl MockTarget {
             .get(&symbol_short!("admin"))
             .unwrap()
     }
+    pub fn bump_ttl(env: Env) {
+        env.storage().instance().extend_ttl(17280, 518400);
+    }
 }
 
 fn setup() -> (Env, GovernanceContractClient<'static>, Address, Address) {
@@ -658,3 +661,59 @@ fn propose_rejects_too_many_signers() {
     }
     client.propose_admin_transfer(&Address::generate(&env), &addrs, &1, &MIN_TIMELOCK_SECONDS);
 }
+
+#[test]
+fn test_proposal_ttl_extension_keeps_proposal_active_and_finalizable() {
+    let (env, client, admin, target) = setup();
+    let target_client = MockTargetClient::new(&env, &target);
+    let proposed = Address::generate(&env);
+    let s1 = Address::generate(&env);
+    let s2 = Address::generate(&env);
+    let signers = Vec::from_slice(&env, &[s1.clone(), s2.clone()]);
+
+    // Step 1: Set initial ledger info
+    let mut ledger_info = LedgerInfo {
+        timestamp: 1000,
+        protocol_version: 22,
+        sequence_number: 100,
+        network_id: Default::default(),
+        base_reserve: 5_000_000,
+        min_temp_entry_ttl: 1_000_000,
+        min_persistent_entry_ttl: 1_000_000,
+        max_entry_ttl: 10_000_000,
+    };
+    env.ledger().set(ledger_info.clone());
+
+    // Step 2: Propose transfer (this sets KEY_PENDING and bumps TTL)
+    client.propose_admin_transfer(&proposed, &signers, &2, &MIN_TIMELOCK_SECONDS);
+
+    // Step 3: Approve 1 (this bumps TTL)
+    client.approve_transfer(&s1);
+
+    // Ensure the mock target is also bumped so it isn't archived during the sequence jump
+    target_client.bump_ttl();
+
+    // Step 4: Advance the ledger sequence number and timestamp significantly (e.g. 100,000 blocks and 200,000 seconds).
+    // This is within the 7-day proposal TTL, but tests that the instance storage remains active.
+    ledger_info.timestamp += 200_000;
+    ledger_info.sequence_number += 100_000;
+    env.ledger().set(ledger_info.clone());
+
+    // Step 5: Approve 2 (this bumps TTL and reads/updates proposal)
+    client.approve_transfer(&s2);
+
+    // Bump mock target again to keep it alive
+    target_client.bump_ttl();
+
+    // Step 6: Advance sequence number and timestamp beyond the timelock (24 hours).
+    // MIN_TIMELOCK_SECONDS is 86400. Let's advance by 90,000 seconds.
+    ledger_info.timestamp += 90_000;
+    ledger_info.sequence_number += 18_000;
+    env.ledger().set(ledger_info.clone());
+
+    // Step 7: Finalize should succeed because the proposal was kept alive and finalizable by TTL extensions.
+    client.finalize_admin_transfer(&admin);
+
+    assert_eq!(client.get_current_admin(), proposed);
+    assert!(!client.has_pending_transfer());
+}