Merge pull request #350 from Bruno755/issue-289-291-292-290-escrow-security-updates

feat: add escrow contract enhancements and security documentation

Author: Dami24-hub

SECURITY.md

@@ -107,3 +107,25 @@ const cfg: AgentConfig = {
 - **Mainnet spending cap:** `AGENT_SPENDING_LIMIT` is rejected at startup if it exceeds `10,000` on `mainnet`, preventing runaway agent spend.
 - **Exponential back-off:** All RPC calls use retry logic with jitter to reduce the attack surface of timing-based denial-of-service against the agent.
 - **Dependency pinning:** Keep `package.json` dependencies pinned to exact versions and audit regularly with `npm audit`.
+
+---
+
+## Known Limitations
+
+Users should be aware of the following limitations when deploying Nodal AI:
+
+### 1. In-Memory Nonce Store
+
+The x402 nonce store is in-memory and not persisted to disk. If the agent process restarts, previously-seen nonces are cleared. This creates a window where replayed x402 challenges could be accepted until the agent is re-initialized with fresh state. See [#207](https://github.com/Nodal-stellar/Nodal-AI/issues/207) for persistent nonce store implementation.
+
+### 2. Soroban Simulation Disabled for Payment Estimates
+
+`StellarPaymentTool` does not use Soroban simulation to estimate transaction fees. Fee estimates are calculated as base-fee only, without accounting for transaction complexity or network congestion. Production deployments should verify fee estimates through a secondary mechanism or use a higher fee buffer.
+
+### 3. AWS Secret Fetch Pattern
+
+`config.ts` uses `execSync` to fetch `AGENT_SECRET_KEY` from AWS Secrets Manager. This pattern has inherent security risks including exposing command output in error logs and blocking the event loop during secret retrieval. See [#210](https://github.com/Nodal-stellar/Nodal-AI/issues/210) for a planned non-blocking alternative.
+
+### 4. Webhook Delivery Retry Limits
+
+Webhook delivery has no retry mechanism for non-2xx responses beyond the initial `withRetry` attempts configured in the deployment. If a webhook consumer is temporarily unavailable, events may be silently dropped without re-queueing or manual intervention capability.

contracts/escrow/src/lib.rs

@@ -33,6 +33,9 @@ pub enum DataKey {
     Amount,
     Expiry,
     Released,
+    InitializedAt,
+    PendingArbiter,
+    PendingArbiterTime,
 }
 
 // ─── Escrow State ─────────────────────────────────────────────────────────────
@@ -46,6 +49,7 @@ pub struct EscrowState {
     pub amount: i128,
     pub expiry: u64,
     pub released: bool,
+    pub initialized_at: u64,
 }
 
 // ─── Contract Errors ──────────────────────────────────────────────────────────
@@ -72,6 +76,10 @@ pub enum EscrowError {
     NotInitialized = 8,
     /// depositor, recipient, and arbiter must all be distinct addresses.
     InvalidParties = 9,
+    /// The arbiter rotation time-lock has not yet expired.
+    RotationLocked = 10,
+    /// No pending arbiter rotation proposal.
+    NoPendingRotation = 11,
 }
 
 // ─── Contract ─────────────────────────────────────────────────────────────────
@@ -88,7 +96,11 @@ impl EscrowContract {
     /// * `depositor` - Party locking the funds.
     /// * `recipient` - Party who receives funds on release.
     /// * `arbiter`   - Trusted party who authorises release.
-    /// * `token`     - SAC token contract address.
+    /// * `token`     - SAC token contract address. **Only Stellar Asset Contract (SAC) tokens
+    ///                 conforming to the Stellar token interface are supported.** Non-SAC tokens
+    ///                 with incompatible interfaces will cause a panic. Verify the token address
+    ///                 is a SAC before calling initialize() by checking the token's contract code
+    ///                 or attempting to read its decimals().
     /// * `amount`    - Token amount (stroop-equivalent units).
     /// * `expiry`    - Unix timestamp after which depositor may refund.
     ///
@@ -148,6 +160,7 @@ impl EscrowContract {
         );
 
         // Persist state
+        let now = env.ledger().timestamp();
         env.storage()
             .instance()
             .set(&DataKey::Depositor, &depositor);
@@ -159,6 +172,9 @@ impl EscrowContract {
         env.storage().instance().set(&DataKey::Amount, &amount);
         env.storage().instance().set(&DataKey::Expiry, &expiry);
         env.storage().instance().set(&DataKey::Released, &false);
+        env.storage()
+            .instance()
+            .set(&DataKey::InitializedAt, &now);
 
         env.events().publish(
             (
@@ -343,6 +359,11 @@ impl EscrowContract {
                 .instance()
                 .get(&DataKey::Released)
                 .unwrap_or(false),
+            initialized_at: env
+                .storage()
+                .instance()
+                .get(&DataKey::InitializedAt)
+                .expect("escrow: state corrupted"),
         }
     }
 
@@ -494,6 +515,99 @@ impl EscrowContract {
         );
     }
 
+    /// Propose a new arbiter. Only callable by the stored depositor.
+    ///
+    /// Initiates a time-locked arbiter rotation to prevent instant hostile takeover.
+    /// After the 24-hour time-lock, `accept_arbiter_rotation` must be called to finalize.
+    ///
+    /// # Arguments
+    /// * `env`        - The execution environment.
+    /// * `depositor`  - Must match the depositor recorded at initialisation.
+    /// * `new_arbiter` - The proposed new arbiter address.
+    ///
+    /// # Panics
+    /// * `NotDepositor` - If the caller is not the stored depositor.
+    /// * `NotInitialized` - If the escrow has not been initialized.
+    ///
+    /// # Return Value
+    /// None.
+    pub fn propose_new_arbiter(env: Env, depositor: Address, new_arbiter: Address) {
+        let stored_depositor: Address = env
+            .storage()
+            .instance()
+            .get(&DataKey::Depositor)
+            .expect("escrow: state corrupted");
+        stored_depositor.require_auth();
+        if depositor != stored_depositor {
+            panic_with_error!(&env, EscrowError::NotDepositor);
+        }
+
+        let now = env.ledger().timestamp();
+        env.storage()
+            .instance()
+            .set(&DataKey::PendingArbiter, &new_arbiter);
+        env.storage()
+            .instance()
+            .set(&DataKey::PendingArbiterTime, &now);
+
+        env.events().publish(
+            (Symbol::new(&env, "arbiter_rotation_proposed"),),
+            (depositor, new_arbiter, now),
+        );
+    }
+
+    /// Accept the pending arbiter rotation after the 24-hour time-lock.
+    ///
+    /// Finalizes the arbiter change if 24 hours have passed since `propose_new_arbiter` was called.
+    /// Can be called by anyone once the time-lock has expired.
+    ///
+    /// # Arguments
+    /// * `env` - The execution environment.
+    ///
+    /// # Panics
+    /// * `NoPendingRotation` - If no arbiter rotation has been proposed.
+    /// * `RotationLocked` - If the 24-hour time-lock has not yet elapsed.
+    ///
+    /// # Return Value
+    /// None.
+    pub fn accept_arbiter_rotation(env: Env) {
+        const ROTATION_DELAY: u64 = 86_400; // 24 hours in seconds
+
+        if !env.storage().instance().has(&DataKey::PendingArbiter) {
+            panic_with_error!(&env, EscrowError::NoPendingRotation);
+        }
+
+        let pending_time: u64 = env
+            .storage()
+            .instance()
+            .get(&DataKey::PendingArbiterTime)
+            .expect("escrow: state corrupted");
+        let now = env.ledger().timestamp();
+
+        if now < pending_time + ROTATION_DELAY {
+            panic_with_error!(&env, EscrowError::RotationLocked);
+        }
+
+        let new_arbiter: Address = env
+            .storage()
+            .instance()
+            .get(&DataKey::PendingArbiter)
+            .expect("escrow: state corrupted");
+
+        env.storage().instance().set(&DataKey::Arbiter, &new_arbiter);
+        env.storage()
+            .instance()
+            .remove(&DataKey::PendingArbiter);
+        env.storage()
+            .instance()
+            .remove(&DataKey::PendingArbiterTime);
+
+        env.events().publish(
+            (Symbol::new(&env, "arbiter_rotation_accepted"),),
+            (new_arbiter, now),
+        );
+    }
+
     // ─── Internal helpers ────────────────────────────────────────────────────
 
     fn assert_not_released(env: &Env) {

contracts/escrow/src/test.rs

@@ -862,4 +862,169 @@ mod tests {
         assert_eq!(token.balance(&impostor), 0, "impostor must receive nothing");
         assert_eq!(token.balance(&contract_id), 0);
     }
+
+    // ── initialized_at field tests (issue #290) ──────────────────────────────
+
+    // 31. get_state returns initialized_at after initialize
+    #[test]
+    fn test_get_state_includes_initialized_at() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let now = env.ledger().timestamp();
+        let expiry = now + EXPIRY_OFFSET;
+        client.initialize(&depositor, &recipient, &arbiter, &token_id, &500, &expiry);
+        let state = client.get_state();
+        assert_eq!(state.initialized_at, now, "initialized_at should match ledger timestamp");
+    }
+
+    // 32. initialized_at reflects ledger timestamp at initialization time
+    #[test]
+    fn test_initialized_at_reflects_ledger_timestamp() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        env.ledger().with_mut(|li| li.timestamp = 500);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let expiry = 500 + EXPIRY_OFFSET;
+        client.initialize(&depositor, &recipient, &arbiter, &token_id, &500, &expiry);
+        let state = client.get_state();
+        assert_eq!(state.initialized_at, 500, "initialized_at should be 500");
+    }
+
+    // ── Arbiter rotation tests (issue #291) ──────────────────────────────────
+
+    // 33. propose_new_arbiter requires depositor auth
+    #[test]
+    fn test_propose_arbiter_requires_depositor_auth() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let new_arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let expiry = env.ledger().timestamp() + EXPIRY_OFFSET;
+        client.initialize(&depositor, &recipient, &arbiter, &token_id, &500, &expiry);
+        // Propose with correct depositor should succeed
+        client.propose_new_arbiter(&depositor, &new_arbiter);
+    }
+
+    // 34. accept_arbiter_rotation fails before time-lock expires
+    #[test]
+    #[should_panic]
+    fn test_accept_arbiter_rotation_locked_panics() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let new_arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let expiry = env.ledger().timestamp() + EXPIRY_OFFSET;
+        client.initialize(&depositor, &recipient, &arbiter, &token_id, &500, &expiry);
+        client.propose_new_arbiter(&depositor, &new_arbiter);
+        // Try to accept before 24 hours have passed — should panic
+        client.accept_arbiter_rotation();
+    }
+
+    // 35. accept_arbiter_rotation succeeds after time-lock expires
+    #[test]
+    fn test_accept_arbiter_rotation_succeeds_after_delay() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let new_arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let now = env.ledger().timestamp();
+        let expiry = now + EXPIRY_OFFSET;
+        client.initialize(&depositor, &recipient, &arbiter, &token_id, &500, &expiry);
+        client.propose_new_arbiter(&depositor, &new_arbiter);
+        // Advance time by 24 hours + 1 second
+        env.ledger()
+            .with_mut(|li| li.timestamp = now + 86_401);
+        client.accept_arbiter_rotation();
+        // Verify the new arbiter is now active
+        let state = client.get_state();
+        assert_eq!(state.arbiter, new_arbiter);
+    }
+
+    // 36. accept_arbiter_rotation panics without pending rotation
+    #[test]
+    #[should_panic]
+    fn test_accept_arbiter_rotation_no_pending_panics() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let expiry = env.ledger().timestamp() + EXPIRY_OFFSET;
+        client.initialize(&depositor, &recipient, &arbiter, &token_id, &500, &expiry);
+        // Try to accept without proposing — should panic
+        client.accept_arbiter_rotation();
+    }
+
+    // 37. arbiter rotation enables recovery from compromised key
+    #[test]
+    fn test_arbiter_rotation_recovery_flow() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let depositor = Address::generate(&env);
+        let recipient = Address::generate(&env);
+        let compromised_arbiter = Address::generate(&env);
+        let trusted_arbiter = Address::generate(&env);
+        let (token_id, _) = create_token(&env, &depositor);
+        StellarAssetClient::new(&env, &token_id).mint(&depositor, &1_000);
+        let contract_id = env.register_contract(None, EscrowContract);
+        let client = EscrowContractClient::new(&env, &contract_id);
+        let now = env.ledger().timestamp();
+        let expiry = now + EXPIRY_OFFSET;
+        client.initialize(
+            &depositor,
+            &recipient,
+            &compromised_arbiter,
+            &token_id,
+            &500,
+            &expiry,
+        );
+        // Propose rotation to trusted arbiter
+        client.propose_new_arbiter(&depositor, &trusted_arbiter);
+        // Wait for time-lock to expire
+        env.ledger()
+            .with_mut(|li| li.timestamp = now + 86_401);
+        // Accept rotation
+        client.accept_arbiter_rotation();
+        // Verify the trusted arbiter is now active
+        let state = client.get_state();
+        assert_eq!(
+            state.arbiter, trusted_arbiter,
+            "arbiter should be rotated to trusted address"
+        );
+    }
 }