From 8b7343e215a4fde079329cc5895ddfaa454264da Mon Sep 17 00:00:00 2001
From: divinemike019 <divinemike019@gmail.com>
Date: Fri, 24 Jul 2026 03:30:19 +0000
Subject: [PATCH] fix(vault): enforce CEI ordering in withdraw and
 release_payment
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Reorder both fund-moving functions so every storage write and TTL
extension completes before the token::Client::transfer call:

withdraw()
- asset_account.balance -= amount
- env.storage().persistent().set(&asset_key, &asset_account)
- extend_persistent_ttl (asset key)
- extend_instance_ttl
- token_client.transfer(...)  ← final interaction

release_payment()
- task.spent += amount
- env.storage().persistent().set(&task_key, &task)
- extend_persistent_ttl (task key)
- token_client.transfer(...)  ← final interaction

The transfer now happens last. Because Soroban transactions are
atomic, a panicking transfer reverts the whole transaction without
leaving any state change behind. Whitelisted assets are still
admin-controlled today, so this is not currently exploitable, but
ordering writes first removes the bug class entirely and is required
practice ahead of the multi-asset roadmap.

Each function now carries a doc-comment that calls out the required
CEI ordering so a future edit cannot silently reintroduce the bug.

All 80 tests pass unchanged (happy-path behavior is identical).
cargo clippy --all-targets -- -D warnings reports zero warnings.

Closes #68
---
 contracts/agent-vault/src/lib.rs | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/contracts/agent-vault/src/lib.rs b/contracts/agent-vault/src/lib.rs
index ba4e9f3..4dc5da6 100644
--- a/contracts/agent-vault/src/lib.rs
+++ b/contracts/agent-vault/src/lib.rs
@@ -409,6 +409,13 @@ impl AgentVault {
     /// Only the unlocked portion (`balance - locked`) of the given asset may be
     /// withdrawn; funds locked by active tasks for that asset stay reserved, so a
     /// withdrawal succeeds even while other funds — or other assets — are locked.
+    ///
+    /// # Checks-Effects-Interactions ordering
+    /// All validation (checks) and storage mutations (effects) are completed
+    /// **before** the `token::Client::transfer` call (interaction). This ensures
+    /// the vault's accounting is already consistent if a transfer ever re-enters
+    /// the contract, and that a panicking transfer reverts the whole transaction
+    /// without leaving any state change behind.
     pub fn withdraw(
         env: Env,
         user: Address,
@@ -451,13 +458,15 @@ impl AgentVault {
             return Err(VaultError::InsufficientAvailable);
         }
 
-        Self::extend_instance_ttl(&env);
-        let token_client = token::Client::new(&env, &asset);
-        token_client.transfer(&env.current_contract_address(), &user, &amount);
-
+        // --- Effects: update state before the external call ---
         asset_account.balance -= amount;
         env.storage().persistent().set(&asset_key, &asset_account);
         Self::extend_persistent_ttl(&env, &asset_key);
+        Self::extend_instance_ttl(&env);
+
+        // --- Interaction: token transfer is the final step ---
+        let token_client = token::Client::new(&env, &asset);
+        token_client.transfer(&env.current_contract_address(), &user, &amount);
 
         WithdrawEvent {
             user: user.clone(),
@@ -697,6 +706,14 @@ impl AgentVault {
 
     /// Release funds for one step: contract transfers `amount` tokens to the ORCHESTRATOR.
     /// Returns true on success.
+    ///
+    /// # Checks-Effects-Interactions ordering
+    /// All validation (checks) and storage mutations (effects) — including
+    /// `task.spent` increment and the persistent-storage write — are completed
+    /// **before** the `token::Client::transfer` call (interaction). This ensures
+    /// the vault's accounting is consistent if a transfer ever re-enters the
+    /// contract, and that a panicking transfer reverts the whole transaction
+    /// without leaving any state change behind.
     pub fn release_payment(
         env: Env,
         orchestrator: Address,
@@ -732,13 +749,16 @@ impl AgentVault {
         }
 
         Self::extend_instance_ttl(&env);
-        let token_client = token::Client::new(&env, &asset);
-        token_client.transfer(&env.current_contract_address(), &orchestrator, &amount);
 
+        // --- Effects: update state before the external call ---
         task.spent += amount;
         env.storage().persistent().set(&task_key, &task);
         Self::extend_persistent_ttl(&env, &task_key);
 
+        // --- Interaction: token transfer is the final step ---
+        let token_client = token::Client::new(&env, &asset);
+        token_client.transfer(&env.current_contract_address(), &orchestrator, &amount);
+
         ReleaseEvent {
             user: task.user.clone(),
             orchestrator: orchestrator.clone(),