[Task]: Harden withdraw and release_payment to follow checks-effects-interactions

[grantfox-oss]

Two fund-moving functions move tokens out of the vault before they update the vault's own accounting:

• withdraw() — the token transfer (contract → user) happens at lib.rs:447, and asset_account.balance -= amount only afterwards at lib.rs:449.
• release_payment() — the transfer (contract → orchestrator) happens at lib.rs:723, and task.spent += amount only afterwards at lib.rs:725.

deposit() also transfers before writing state, but that direction (user → contract) is safe. The two above perform an external call while internal state is stale — a textbook checks-effects-interactions (CEI) violation.

Whitelisted assets are admin-controlled today, so this is not currently exploitable. But trust-minimization is the entire purpose of the vault, and the multi-asset roadmap explicitly invites third-party SAC tokens whose transfer could re-enter the contract. Ordering the writes before the transfer removes this whole class of bug and matches standard treasury-contract practice.

Goal

Reorder withdraw and release_payment so that all storage writes and TTL bumps complete before the token::Client::transfer(...) call. The transfer becomes the final interaction; if it panics the whole transaction reverts, so there is no risk of decrementing balance without moving funds.

Requirements & constraints

• Happy-path behavior must be byte-for-byte identical — this is a reordering, not a behavior change.
• Emitted events must still reflect the final post-transfer state (same field values as today).
• Add a short doc-comment on each function stating the CEI ordering so a future edit doesn't silently reintroduce the bug.

Edge cases to consider

• release_payment with a partial spend (spent + amount < plan_cost) and with an exact-fill (== plan_cost).
• withdraw of exactly the available (balance - locked) amount.
• A transfer that reverts must leave no state change (verify the revert-safety reasoning holds after reordering).

Acceptance criteria

• In both functions, every storage write and TTL extension precedes the token transfer
• All existing tests pass unchanged (behavior is identical on success paths)
• Each function carries a doc-comment noting the required CEI ordering
• (Stretch) A test using a mock SAC that re-enters on transfer demonstrates the reordered version is safe
• cargo test and cargo clippy --all-targets -- -D warnings pass

Pointers

• withdraw ~ lib.rs:403–468; release_payment ~ lib.rs:687–747.
• The default test token does not re-enter, so this is primarily verified by reading the reordered code; call that out in the PR description.

Notes for contributors

If you'd like to work on this, comment below so we can assign it to you.
Questions welcome — see CONTRIBUTING.md for setup
and workflow.

[divinemike019]

Hi, I'd like to work on this issue. This is a security-relevant checks-effects-interactions violation — since withdraw and release_payment currently move tokens out of the vault before updating the vault's own accounting, this creates a window where the accounting state is stale while the external transfer is in flight, which is exactly the kind of pattern that enables reentrancy exploitation.

My approach:

1. Reorder both withdraw and release_payment so all internal state updates (vault accounting, balances) happen before the external token transfer call, not after.
2. Add a reentrancy guard as defense in depth on both functions, since relying solely on ordering can regress silently if either function is refactored later without awareness of this constraint.
3. Write tests that specifically simulate a malicious token contract attempting to reenter either function during the transfer callback, confirming the exploit path is closed.
4. Review the rest of the contract for the same pattern elsewhere, since if it exists in these two functions it may exist in others handling fund movement.

Given this is a security fix, I have relevant experience with checks-effects-interactions and reentrancy protection in Rust/Soroban contracts, and I'd want to share my fix for review before merging rather than rushing a patch on something security-critical.

[grantfox-oss]

🦊 GrantFox — @divinemike019 has been assigned to this issue as part of the Official Campaign | FWC26 campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #68)
2. Your PR will be reviewed by the clevercon-protocol maintainers

Good luck! Track your progress on GrantFox.