fix(vault): enforce CEI ordering in withdraw and release_payment

[divinemike019]

Summary

Fixes the checks-effects-interactions (CEI) violations in the two fund-moving functions identified in #68.

Both withdraw() and release_payment() previously executed the token::Client::transfer call before writing updated state to storage — a classic CEI violation where an external interaction could observe stale contract state.

Changes

withdraw()

Before (violation):

token_client.transfer(...);   // interaction first — state is stale
asset_account.balance -= amount;
env.storage().persistent().set(&asset_key, &asset_account);

After (correct):

// Effects first
asset_account.balance -= amount;
env.storage().persistent().set(&asset_key, &asset_account);
extend_persistent_ttl(&env, &asset_key);
extend_instance_ttl(&env);
// Interaction last
token_client.transfer(...);

release_payment()

Before (violation):

token_client.transfer(...);   // interaction first — state is stale
task.spent += amount;
env.storage().persistent().set(&task_key, &task);

After (correct):

// Effects first
task.spent += amount;
env.storage().persistent().set(&task_key, &task);
extend_persistent_ttl(&env, &task_key);
// Interaction last
token_client.transfer(...);

CEI doc-comments

Each function now carries a # Checks-Effects-Interactions ordering doc-comment explaining the required ordering, so a future edit cannot silently reintroduce the bug.

Revert-safety reasoning

Soroban transactions are fully atomic. If token_client.transfer panics, the entire transaction reverts — including the storage writes that now precede it. So decrementing balance without moving funds is impossible: either both the write and the transfer succeed, or neither does.

This also means that even if a whitelisted SAC token were replaced in future by a third-party token whose transfer re-entered the contract, the vault's accounting would already reflect the correct post-withdrawal state at the point of re-entry, eliminating the double-spend/double-release class of bug entirely.

Testing

• All 80 existing tests pass unchanged — happy-path behavior is byte-for-byte identical.
• cargo clippy --all-targets -- -D warnings reports zero warnings.
• Re-entrancy safety is verified by reading the reordered code (as noted in the issue, the default Soroban test token does not re-enter).

Closes #68

Summary by CodeRabbit

• Security & Reliability
  • Improved payment and withdrawal processing by updating internal balances and payment records before external token transfers.
  • Added safeguards to ensure state changes are persisted before external interactions, reducing the risk of inconsistent transaction state.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ccd869f-936e-4317-99f8-49ba1fa406dd

📥 Commits

Reviewing files that changed from the base of the PR and between dc27886 and 8b7343e.

📒 Files selected for processing (1)

• contracts/agent-vault/src/lib.rs

---

📝 Walkthrough

Walkthrough

The vault now persists withdrawal and payment accounting changes, including TTL extensions, before calling external token transfers. Documentation on both functions records this checks-effects-interactions ordering. Public signatures and return types remain unchanged.

Changes

Vault transfer ordering

Layer / File(s)	Summary
Reorder vault outflows contracts/agent-vault/src/lib.rs	withdraw persists the reduced balance before transferring tokens, while release_payment persists the updated task.spent before transferring payment; both functions document the ordering guarantee.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• clevercon-protocol/clevercon#59: Also modifies contracts/agent-vault/src/lib.rs’s withdraw() logic, changing withdrawal eligibility based on active-task locking.

Suggested reviewers: tijesunimi004, yerimahoftimes, dopezapha

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Clearly summarizes the CEI reordering fix in withdraw and release_payment.
Linked Issues check	✅ Passed	Implements #68 by moving storage writes and TTL bumps before transfers in both functions, with the required CEI doc-comments.
Out of Scope Changes check	✅ Passed	No unrelated changes are apparent beyond the CEI reordering and related documentation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}