From 5ed8d65b7e20be046e0d9354d8af0d2127b6bdf4 Mon Sep 17 00:00:00 2001
From: lycantho <victoradeoba2@gmail.com>
Date: Thu, 30 Jul 2026 02:16:47 +0100
Subject: [PATCH] fix: clawback allowance mechanism, invoice tax_bps overflow
 (#685, #686)

clawback::execute now pulls funds via a pre-authorized SEP-41 allowance
(approve/transfer_from) instead of a plain transfer that required the
target's live signature, so an uncooperative contributor can no longer
block recovery once they've pre-authorized the pull.

invoice::validate_line_items now rejects tax_bps > 10_000 by reusing the
crate's existing math::basis_points_of helper, closing an overflow-panic
DoS on invoice submission.

Verified #687 (fee wiring) and #688 (performance bond proportional claim)
are already fixed on main by prior unrelated commits; no code changes
needed there.

Also fixes a cluster of unrelated pre-existing build/test breakage
(missing enum variant, missing Ledger trait imports, Grant::Default
misuse, stale test fixture field, a use-after-move bug, a malformed
as_contract call, and a redundant double require_auth() in the
clawback_* entry points) that blocked compiling and running the test
suite at all. See contracts/PR_685_686_687_688.md for full details.
---
 contracts/PR_685_686_687_688.md               | 112 +++
 .../stellar-grants/src/access_control.rs      |  19 +-
 .../contracts/stellar-grants/src/audit.rs     |  17 +-
 .../contracts/stellar-grants/src/clawback.rs  | 742 ++++++++++++++----
 .../stellar-grants/src/compliance.rs          |  29 +-
 .../contracts/stellar-grants/src/errors.rs    |   6 +
 .../contracts/stellar-grants/src/events.rs    |  30 +
 .../contracts/stellar-grants/src/invoice.rs   | 123 ++-
 contracts/contracts/stellar-grants/src/lib.rs |  29 +-
 .../contracts/stellar-grants/src/lockup.rs    |   5 +-
 .../contracts/stellar-grants/src/merkle.rs    |   2 +-
 .../stellar-grants/src/milestone_extension.rs |   2 +-
 .../contracts/stellar-grants/src/referral.rs  |   2 +-
 .../stellar-grants/src/split_payment.rs       |  18 +-
 .../stellar-grants/src/storage/helpers.rs     |  15 +-
 15 files changed, 956 insertions(+), 195 deletions(-)
 create mode 100644 contracts/PR_685_686_687_688.md

diff --git a/contracts/PR_685_686_687_688.md b/contracts/PR_685_686_687_688.md
new file mode 100644
index 00000000..f73824c8
--- /dev/null
+++ b/contracts/PR_685_686_687_688.md
@@ -0,0 +1,112 @@
+## Summary
+
+Closes #685. Closes #686. Closes #687. Closes #688.
+
+All four issues target `contracts/contracts/stellar-grants/src/`. Before writing any code I checked each one directly against current `main` rather than trusting the issue text at face value — two were still genuinely broken, two had already been fixed by unrelated prior work and just needed verification.
+
+| Issue | Status on `main` before this PR | Action taken |
+|---|---|---|
+| #685 clawback::execute can't pull funds from an unwilling target | **Broken** — confirmed | Real fix: SEP-41 allowance mechanism |
+| #686 invoice tax_bps overflow | **Broken** — confirmed | Real fix: reuse `math::basis_points_of` |
+| #687 protocol fee collection dead | **Already fixed** (landed via `db08ab2e`) | Verified only, no code change |
+| #688 performance_bond winner-take-all | **Already fixed** (landed via `74921af5`/`7be303bc`) | Verified only, no code change |
+
+## #685 — `clawback::execute` can now actually recover funds
+
+### The bug
+
+`execute()` called `token::Client::transfer(&clawback.target, &treasury, &clawback.amount)`. SEP-41 `transfer` requires `from.require_auth()` — i.e. the contributor being clawed back from has to sign the transaction. Only `caller` (the admin executing the clawback) signed. An uncooperative contributor could simply never sign, permanently blocking recovery — exactly the scenario clawback exists for.
+
+### The fix: pre-authorized SEP-41 allowance (option 1 from the issue)
+
+I chose the pre-authorization/`transfer_from` approach over restructuring payout into a hold-back/vesting model, for two reasons:
+
+1. **Scope.** The issue explicitly points at `execute()` (lines ~161-214) as the fix site. Hold-back would mean redesigning `escrow.rs`'s payout flow and `lib.rs`'s `finalize_grant_release` (which pays out the *entire* grant in one lump sum, not per-milestone) — a materially larger, separate change.
+2. **It's the standard mechanism for exactly this problem.** SEP-41's `approve`/`transfer_from` pair exists so a contract can move funds out of a wallet without needing that wallet's cooperation *at the moment of the transfer* — because the authorizing signature was already given earlier, voluntarily, by the wallet owner.
+
+New function `clawback::authorize_pull(env, contributor, grant_id, token, amount, live_until_ledger)`:
+- Requires `contributor.require_auth()` — the contributor's own signature, given while they're still cooperative (e.g. right after a milestone payout).
+- Calls `token::Client::approve(contributor, contract_address, amount, live_until_ledger)`.
+
+`execute()` now does:
+```rust
+let allowance = token_client.allowance(&clawback.target, &env.current_contract_address());
+if allowance < clawback.amount {
+    return Err(ContractError::InsufficientClawbackAllowance);
+}
+token_client.transfer_from(&env.current_contract_address(), &clawback.target, &treasury, &clawback.amount);
+```
+`transfer_from`'s `spender.require_auth()` requirement is satisfied automatically for calls the contract makes as itself — no signature from `clawback.target` needed at execute time. If no allowance was ever set (or it's insufficient), `execute` now returns a clean `ContractError::InsufficientClawbackAllowance` instead of letting the token contract panic.
+
+New entry point `clawback_authorize_pull` added to `lib.rs` alongside the other five `clawback_*` wrappers. New error variant `ContractError::InsufficientClawbackAllowance = 148`. New event `ClawbackAllowanceAuthorized`.
+
+### Proving it's not `mock_all_auths()` papering over the gap
+
+The issue specifically calls this out: *"test this, don't just assume `mock_all_auths()` masks the real-world gap."* The decisive test, `test_execute_succeeds_via_preauthorized_allowance_without_target_signature`, deliberately avoids blanket mocking:
+
+- Uses `env.mock_all_auths_allowing_non_root_auth()` (records real auth requirements instead of blindly satisfying every `require_auth()` call) and drives the real dispatched entry points (`client.clawback_authorize_pull`, `client.clawback_execute`) rather than calling module functions directly.
+- After `execute`, inspects `env.auths()` — the actual list of addresses whose signature the call required — and asserts `admin` is in it while `owner` (the clawback target) is **not**. Under the old `transfer`-based code this call would have panicked (the token contract's own `from.require_auth()` for the target has nothing to satisfy it); under the fix it succeeds without the target ever being asked.
+
+Other new tests: `test_execute_fails_without_allowance` (clean error, no panic, when nothing was pre-authorized), `test_authorize_pull_rejects_non_owner`, `test_authorize_pull_rejects_non_positive_amount`. All 9 pre-existing clawback tests still pass, updated only to call `authorize_pull` first wherever they reach `execute`.
+
+### A pre-existing gap this PR does *not* fix
+
+While building the end-to-end test I found that no production code path (`governance.rs`, `lib.rs`'s `finalize_grant_release`/`complete_grant`) ever actually sets `MilestoneState::Paid` — milestones only ever reach `Approved`. `clawback::initiate` requires `Paid`. This means the clawback feature is currently unreachable end-to-end via the real payout flow, independent of the auth fix in this PR. This is outside #685's stated scope (which only points at `execute`), and every pre-existing test in `clawback.rs` already isolates the module the same way this PR's new tests do — by constructing a milestone with `.state: MilestoneState::Paid` directly rather than driving it through governance. Flagging this as a candidate follow-up issue.
+
+## #686 — `invoice::validate_line_items` rejects oversized `tax_bps`
+
+### The bug
+```rust
+let tax_amount = (subtotal * (tax_bps as i128)) / 10_000;
+```
+No upper bound on caller-supplied `tax_bps: u32`. Soroban builds with overflow checks enabled, so a large enough value panics the transaction — a clean DoS on invoice submission.
+
+### The fix
+
+Rather than hand-rolling `checked_mul`/`checked_div` as the issue's suggested patch does, I reused `crate::math::basis_points_of(amount, basis_points)` — the crate's existing shared helper for exactly this computation (`fees.rs` already uses it for protocol fee math). It already rejects `basis_points > 10_000` with `ContractError::InvalidInput` and uses checked arithmetic internally:
+
+```rust
+let tax_amount = crate::math::basis_points_of(subtotal, tax_bps)?;
+```
+
+One-line fix, no duplicated overflow-checking logic, and it's the more idiomatic choice given the helper already exists and is already the crate's convention for bps math. Covers both `submit_invoice` and `resubmit_invoice` since both funnel through `validate_line_items`.
+
+New tests: `test_submit_invoice_rejects_tax_bps_over_10000` (submits with `tax_bps = 10_001`, asserts a clean `InvalidInput`, and that no partial invoice is left on record), `test_validate_line_items_rejects_excessive_tax_bps` (`tax_bps = u32::MAX`), `test_validate_line_items_rejects_tax_bps_just_over_limit`, plus a `test_validate_line_items_accepts_valid_tax_bps` regression guard for the normal case.
+
+## #687 / #688 — verified already resolved, no code changes
+
+Checked both against current `main` and ran their full test suites:
+
+- **#687**: `fees::deduct_and_split_fee` is wired into the real payout path in `lib.rs::finalize_grant_release` (line ~536) and calls `Storage::add_fees_collected`, so `total_fees_collected` returns real values. `cargo test -p stellar-grants --lib fees::` → **10/10 pass**, including `test_deduct_and_split_fee_respects_reviewer_reward_split` and `test_deduct_and_split_fee_respects_both_splits`, which directly assert the fee amount matches configured `protocol_fee_bps` and that the reviewer-reward/revenue-share/treasury splits are each correct.
+- **#688**: `performance_bond::claim_bond` already distributes proportionally across `grant.funders` (mirroring `escrow::refund_all`'s pattern) instead of winner-take-all, and locks the bond as `Claimed` after the first successful claim. `cargo test -p stellar-grants --lib performance_bond::` → **12/12 pass**, including `test_claim_bond_proportional_across_funders` (two funders at a 60/40 split, each asserted to receive their exact share) and `test_claim_bond_rejects_double_claim`.
+
+`git log` shows these landed via `db08ab2e` ("fix: wire up reviewer reward pool, fix grant pause tests, integrate bounty grants") and `74921af5`/`7be303bc` ("test: add coverage for performance bond claims" / "fix: add reentrancy guards to reward claim paths") — unrelated PRs that happened to resolve the same underlying bugs these issues describe.
+
+## Unrelated build breakage fixed to enable verification
+
+`origin/main` did not compile at the point this branch was cut — 1 lib-level error and 30 test-compile errors across 9 files with no relation to #685-#688 (`lockup.rs`, `access_control.rs`, `audit.rs`, `compliance.rs`, `merkle.rs`, `milestone_extension.rs`, `open_review.rs`, `referral.rs`, `split_payment.rs`). None of this could be verified or worked around, so it had to be unblocked to run `cargo check`/`cargo test` at all:
+
+- Missing `ContractError::TooManyPublicReviews` variant referenced by `open_review.rs` (added, `= 147`).
+- ~23 missing `use soroban_sdk::testutils::Ledger;` imports (mechanical, per-file).
+- Two test fixtures relying on `Grant: Default`, which doesn't exist (`Address` has no meaningful default) — replaced with explicit struct literals.
+- A stale field name in a `referral.rs` test fixture (`contract_id` → `client.address`).
+- A use-after-move `Bytes` clone in `merkle.rs`.
+- A malformed `env.as_contract(&|| ...)` call missing its first argument in `lockup.rs`.
+
+This got the crate compiling (`cargo check --lib` ✅, `cargo test --no-run` ✅) but running the suite then surfaced a **much larger** problem: 341 of 643 tests failed at runtime, spanning ~60 unrelated modules, from causes unrelated to any of this — mainly this soroban-sdk version rejecting `env.storage()` access outside `env.as_contract(..)`, a pattern dozens of pre-existing tests never used. That is far beyond a mechanical fix and well outside the scope of four specific issues, so **it was left alone**, except in the two files this PR actually modifies (`clawback.rs`, `invoice.rs`), where it had to be fixed for those modules' own tests to run at all. After this PR: `cargo test -p stellar-grants --lib` → 316 passed / 331 failed (up from 302/341 on the unblocked-but-otherwise-untouched baseline) — the remaining 331 failures are pre-existing and out of scope for this PR.
+
+### A second, related pre-existing bug found and fixed in the same files
+
+While making `clawback.rs`'s tests exercise real dispatched entry points, all six `clawback_*` wrappers in `lib.rs` turned out to call `.require_auth()` themselves and then delegate to a `clawback::*` module function that **also** calls `.require_auth()` for the same address at the same invocation depth. This is a genuine redundant/duplicate authorization check (harmless in `mock_all_auths()`-blanket tests, but rejected outright under strict auth verification with `Error(Auth, ExistingValue)` / "frame is already authorized"). Removed the redundant outer `.require_auth()` call from all six wrappers — the module functions already enforce it.
+
+## Test plan
+
+- [x] `cargo fmt --check` (clean for all files touched)
+- [x] `cargo clippy -p stellar-grants --lib --tests -- -D warnings` — no warnings in any file this PR touches (pre-existing unrelated failures remain in `waitlist.rs`, `relay.rs`, and one integration test, all untouched by this PR)
+- [x] `cargo check --lib -p stellar-grants`
+- [x] `cargo check --workspace --target wasm32v1-none`
+- [x] `cargo test -p stellar-grants --lib clawback::` → 13/13 pass
+- [x] `cargo test -p stellar-grants --lib invoice::` → 4/4 pass
+- [x] `cargo test -p stellar-grants --lib fees::` → 10/10 pass (verifies #687)
+- [x] `cargo test -p stellar-grants --lib performance_bond::` → 12/12 pass (verifies #688)
+- [ ] `cargo test -p stellar-grants --lib` (whole crate) — does not pass; 331 pre-existing, unrelated failures documented above, out of scope for this PR
diff --git a/contracts/contracts/stellar-grants/src/access_control.rs b/contracts/contracts/stellar-grants/src/access_control.rs
index c0426bd2..4ec0e390 100644
--- a/contracts/contracts/stellar-grants/src/access_control.rs
+++ b/contracts/contracts/stellar-grants/src/access_control.rs
@@ -201,7 +201,20 @@ pub fn renounce_role(env: &Env, holder: &Address, role: Role) -> Result<(), Cont
 #[cfg(test)]
 mod tests {
     use super::*;
-    use soroban_sdk::{testutils::Address as _, Address, Env};
+    use soroban_sdk::{testutils::Address as _, testutils::Ledger as _, Address, Env};
+
+    fn set_ledger(env: &Env, sequence: u32, timestamp: u64) {
+        env.ledger().set(soroban_sdk::testutils::LedgerInfo {
+            timestamp,
+            protocol_version: 21,
+            sequence_number: sequence,
+            base_reserve: 10,
+            network_id: Default::default(),
+            min_temp_entry_ttl: 100_000,
+            min_persistent_entry_ttl: 100_000,
+            max_entry_ttl: 1_000_000,
+        });
+    }
 
     fn setup() -> (Env, Address) {
         let env = Env::default();
@@ -396,7 +409,7 @@ mod tests {
         let (env, admin) = setup();
         let alice = Address::generate(&env);
         grant_role(&env, &admin, &alice, Role::EmergencyPauser, Some(50)).unwrap();
-        env.ledger().set(1, 51);
+        set_ledger(&env, 1, 51);
         assert!(!has_role(&env, &alice, Role::EmergencyPauser));
     }
 
@@ -405,7 +418,7 @@ mod tests {
         let (env, admin) = setup();
         let alice = Address::generate(&env);
         grant_role(&env, &admin, &alice, Role::EmergencyPauser, Some(100)).unwrap();
-        env.ledger().set(1, 99);
+        set_ledger(&env, 1, 99);
         assert!(has_role(&env, &alice, Role::EmergencyPauser));
     }
 
diff --git a/contracts/contracts/stellar-grants/src/audit.rs b/contracts/contracts/stellar-grants/src/audit.rs
index f05bd023..70361fec 100644
--- a/contracts/contracts/stellar-grants/src/audit.rs
+++ b/contracts/contracts/stellar-grants/src/audit.rs
@@ -50,7 +50,20 @@ pub fn log_length(env: &Env, grant_id: u64) -> u32 {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use soroban_sdk::{testutils::Address as _, Address, Env};
+    use soroban_sdk::{testutils::Address as _, testutils::Ledger as _, Address, Env};
+
+    fn set_ledger(env: &Env, sequence: u32, timestamp: u64) {
+        env.ledger().set(soroban_sdk::testutils::LedgerInfo {
+            timestamp,
+            protocol_version: 21,
+            sequence_number: sequence,
+            base_reserve: 10,
+            network_id: Default::default(),
+            min_temp_entry_ttl: 100_000,
+            min_persistent_entry_ttl: 100_000,
+            max_entry_ttl: 1_000_000,
+        });
+    }
 
     fn setup() -> (Env, Address, u64) {
         let env = Env::default();
@@ -450,7 +463,7 @@ mod tests {
     #[test]
     fn entry_records_timestamp_and_ledger() {
         let (env, actor, grant_id) = setup();
-        env.ledger().set(1000, 1_700_000_000);
+        set_ledger(&env, 1000, 1_700_000_000);
         log(
             &env,
             grant_id,
diff --git a/contracts/contracts/stellar-grants/src/clawback.rs b/contracts/contracts/stellar-grants/src/clawback.rs
index 0d5c8377..a9674f54 100644
--- a/contracts/contracts/stellar-grants/src/clawback.rs
+++ b/contracts/contracts/stellar-grants/src/clawback.rs
@@ -157,6 +157,58 @@ pub fn dispute(
     Ok(())
 }
 
+/// Pre-authorize the contract to later pull up to `amount` of `token` from the
+/// contributor's wallet via the SEP-41 allowance mechanism (`approve` /
+/// `transfer_from`), so a future clawback can actually recover funds even if
+/// the contributor later becomes uncooperative.
+///
+/// Must be called by the contributor themselves (their own signature, via
+/// `contributor.require_auth()`) while they are still willing to cooperate —
+/// e.g. right after a milestone payout, or as a standing authorization tied
+/// to accepting a grant that carries clawback risk. This is the real-world
+/// mechanism `execute` relies on: `approve` requires the contributor's
+/// signature now; the later `transfer_from` in `execute` only requires the
+/// *contract's own* authorization (auto-satisfied for calls the contract
+/// makes as itself), not a fresh signature from the (possibly unwilling)
+/// contributor.
+pub fn authorize_pull(
+    env: &Env,
+    contributor: &Address,
+    grant_id: u64,
+    token: &Address,
+    amount: i128,
+    live_until_ledger: u32,
+) -> Result<(), ContractError> {
+    contributor.require_auth();
+
+    if amount <= 0 {
+        return Err(ContractError::InvalidInput);
+    }
+
+    let grant = Storage::get_grant(env, grant_id).ok_or(ContractError::GrantNotFound)?;
+    if grant.owner != *contributor {
+        return Err(ContractError::Unauthorized);
+    }
+
+    token::Client::new(env, token).approve(
+        contributor,
+        &env.current_contract_address(),
+        &amount,
+        &live_until_ledger,
+    );
+
+    Events::emit_clawback_allowance_authorized(
+        env,
+        grant_id,
+        contributor.clone(),
+        token.clone(),
+        amount,
+        live_until_ledger,
+    );
+
+    Ok(())
+}
+
 /// Execute an approved clawback after the dispute window.
 pub fn execute(
     env: &Env,
@@ -191,11 +243,23 @@ pub fn execute(
     // Get treasury address
     let treasury = Storage::get_treasury(env).ok_or(ContractError::TreasuryNotConfigured)?;
 
-    // Transfer funds from contributor to treasury
-    // Note: This assumes the contract holds the funds or has authorization
-    // In practice, this may require a different mechanism depending on token setup
+    // Pull funds from the contributor via a pre-authorized SEP-41 allowance
+    // (see `authorize_pull`) instead of a plain `transfer`, which requires
+    // the contributor's live signature — something an uncooperative target
+    // will never provide. `transfer_from` only requires *our own* contract
+    // to authorize itself as spender, which is automatic for calls the
+    // contract makes as itself.
     let token_client = token::Client::new(env, &clawback.token);
-    token_client.transfer(&clawback.target, &treasury, &clawback.amount);
+    let allowance = token_client.allowance(&clawback.target, &env.current_contract_address());
+    if allowance < clawback.amount {
+        return Err(ContractError::InsufficientClawbackAllowance);
+    }
+    token_client.transfer_from(
+        &env.current_contract_address(),
+        &clawback.target,
+        &treasury,
+        &clawback.amount,
+    );
 
     // Update status
     clawback.status = ClawbackStatus::Executed;
@@ -257,8 +321,55 @@ mod tests {
     use super::*;
     use crate::access_control::grant_role;
     use crate::types::{Grant, Milestone};
+    use crate::{StellarGrantsContract, StellarGrantsContractClient};
     use soroban_sdk::testutils::{Address as _, Ledger};
-    use soroban_sdk::{Env, Vec};
+    use soroban_sdk::{token, Env, Vec};
+
+    /// Register the contract and return its address so tests can wrap
+    /// module-level calls in `env.as_contract(&contract_id, || { ... })` —
+    /// required because this soroban-sdk version rejects storage access
+    /// outside of a contract execution context.
+    ///
+    /// Each individual authorized call (anything that ends up calling
+    /// `Address::require_auth()`) must live in its *own* `as_contract`
+    /// block: under `mock_all_auths_allowing_non_root_auth`, calling
+    /// `require_auth()` twice for the same address within one block trips
+    /// "frame is already authorized", since direct Rust calls don't create
+    /// the separate invocation frames a real dispatched contract call would.
+    fn register(env: &Env) -> Address {
+        env.register(StellarGrantsContract, ())
+    }
+
+    /// Registers a real SEP-41 token — must be called *outside* any
+    /// `env.as_contract(..)` block, since deploying a new contract while
+    /// nested inside another contract's frame fails auth setup.
+    fn register_token(env: &Env) -> Address {
+        let token_admin = Address::generate(env);
+        env.register_stellar_asset_contract_v2(token_admin)
+            .address()
+    }
+
+    /// `grant_role`'s own authorization check requires the granter to
+    /// already hold a `SuperAdmin` `RoleAssignment` in the RBAC system —
+    /// `Storage::set_global_admin` is a separate, unrelated concept and does
+    /// not grant that role. Seed it directly, mirroring how
+    /// `access_control.rs`'s own tests bootstrap a SuperAdmin.
+    fn bootstrap_super_admin(env: &Env, admin: &Address) {
+        let assignment = crate::types::RoleAssignment {
+            holder: admin.clone(),
+            role: Role::SuperAdmin,
+            granted_by: admin.clone(),
+            granted_at: 0,
+            expires_at: None,
+            is_active: true,
+        };
+        Storage::set_role_assignment(env, admin, &Role::SuperAdmin, &assignment);
+        Storage::set_role_members(
+            env,
+            &Role::SuperAdmin,
+            &soroban_sdk::vec![env, admin.clone()],
+        );
+    }
 
     fn setup_grant(env: &Env, owner: Address, token: Address) -> Grant {
         Grant {
@@ -299,58 +410,104 @@ mod tests {
         }
     }
 
+    /// Bootstraps admin/protocol_admin/arbiter roles and seeds a grant +
+    /// Paid milestone at grant_id=1/milestone_idx=0. Returns the addresses
+    /// used, so callers can drive `initiate`/`approve`/etc. themselves, each
+    /// in its own `as_contract` block.
+    #[allow(clippy::too_many_arguments)]
+    fn seed(
+        env: &Env,
+        contract_id: &Address,
+        owner: &Address,
+        token: &Address,
+        admin: &Address,
+        protocol_admin: &Address,
+        arbiter: &Address,
+        set_treasury: bool,
+    ) {
+        env.as_contract(contract_id, || {
+            Storage::set_global_admin(env, admin);
+            bootstrap_super_admin(env, admin);
+            if set_treasury {
+                Storage::set_treasury(env, admin);
+            }
+            let grant = setup_grant(env, owner.clone(), token.clone());
+            Storage::set_grant(env, 1, &grant);
+            let milestone = setup_milestone(env, 0, 500);
+            Storage::set_milestone(env, 1, 0, &milestone);
+            // `dispute()` calls into `escrow::lock`, which requires an
+            // escrow account to already exist for the grant.
+            crate::escrow::open(env, 1, owner, token).unwrap();
+        });
+        env.as_contract(contract_id, || {
+            grant_role(env, admin, protocol_admin, Role::ProtocolAdmin, None).unwrap();
+        });
+        env.as_contract(contract_id, || {
+            grant_role(env, admin, arbiter, Role::DisputeArbiter, None).unwrap();
+        });
+    }
+
     #[test]
     fn test_initiate_clawback_success() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
         let admin = Address::generate(&env);
         let arbiter = Address::generate(&env);
         let owner = Address::generate(&env);
         let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
-
-        let grant = setup_grant(&env, owner.clone(), token.clone());
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Plagiarism detected");
-        let result = initiate(&env, &arbiter, 1, 0, reason);
-        assert!(result.is_ok());
-
-        let clawback = get_request(&env, 1, 0);
-        assert!(clawback.is_some());
-        assert_eq!(clawback.unwrap().status, ClawbackStatus::Pending);
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &Address::generate(&env),
+            &arbiter,
+            false,
+        );
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Plagiarism detected");
+            let result = initiate(&env, &arbiter, 1, 0, reason);
+            assert!(result.is_ok());
+
+            let clawback = get_request(&env, 1, 0);
+            assert!(clawback.is_some());
+            assert_eq!(clawback.unwrap().status, ClawbackStatus::Pending);
+        });
     }
 
     #[test]
     fn test_initiate_unauthorized() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
-        let stranger = Address::generate(&env);
-        let owner = Address::generate(&env);
-        let token = Address::generate(&env);
+        env.as_contract(&contract_id, || {
+            let stranger = Address::generate(&env);
+            let owner = Address::generate(&env);
+            let token = Address::generate(&env);
 
-        let grant = setup_grant(&env, owner, token);
-        Storage::set_grant(&env, 1, &grant);
+            let grant = setup_grant(&env, owner, token);
+            Storage::set_grant(&env, 1, &grant);
 
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
+            let milestone = setup_milestone(&env, 0, 500);
+            Storage::set_milestone(&env, 1, 0, &milestone);
 
-        let reason = String::from_str(&env, "Test");
-        let result = initiate(&env, &stranger, 1, 0, reason);
-        assert_eq!(result, Err(ContractError::Unauthorized));
+            let reason = String::from_str(&env, "Test");
+            let result = initiate(&env, &stranger, 1, 0, reason);
+            assert_eq!(result, Err(ContractError::Unauthorized));
+        });
     }
 
     #[test]
     fn test_approve_clawback_success() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
         let admin = Address::generate(&env);
         let protocol_admin = Address::generate(&env);
@@ -358,36 +515,48 @@ mod tests {
         let owner = Address::generate(&env);
         let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        grant_role(&env, &admin, &protocol_admin, Role::ProtocolAdmin, None).unwrap();
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
-
-        let grant = setup_grant(&env, owner, token);
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            false,
+        );
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
 
         // First approval
-        approve(&env, &protocol_admin, 1, 0).unwrap();
-        let clawback = get_request(&env, 1, 0).unwrap();
-        assert_eq!(clawback.status, ClawbackStatus::Pending);
-        assert_eq!(clawback.approvals.len(), 1);
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            let clawback = get_request(&env, 1, 0).unwrap();
+            assert_eq!(clawback.status, ClawbackStatus::Pending);
+            assert_eq!(clawback.approvals.len(), 1);
+        });
 
         // Second approval - should change to Approved
-        approve(&env, &arbiter, 1, 0).unwrap();
-        let clawback = get_request(&env, 1, 0).unwrap();
-        assert_eq!(clawback.status, ClawbackStatus::Approved);
-        assert_eq!(clawback.approvals.len(), 2);
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            let clawback = get_request(&env, 1, 0).unwrap();
+            assert_eq!(clawback.status, ClawbackStatus::Approved);
+            assert_eq!(clawback.approvals.len(), 2);
+        });
     }
 
     #[test]
     fn test_dispute_clawback_success() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
         let admin = Address::generate(&env);
         let protocol_admin = Address::generate(&env);
@@ -395,34 +564,43 @@ mod tests {
         let owner = Address::generate(&env);
         let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        Storage::set_treasury(&env, &admin);
-        grant_role(&env, &admin, &protocol_admin, Role::ProtocolAdmin, None).unwrap();
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
-
-        let grant = setup_grant(&env, owner.clone(), token);
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
-        approve(&env, &protocol_admin, 1, 0).unwrap();
-        approve(&env, &arbiter, 1, 0).unwrap();
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            true,
+        );
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
 
         // Contributor disputes
-        let result = dispute(&env, &owner, 1, 0);
-        assert!(result.is_ok());
+        env.as_contract(&contract_id, || {
+            let result = dispute(&env, &owner, 1, 0);
+            assert!(result.is_ok());
 
-        let clawback = get_request(&env, 1, 0).unwrap();
-        assert_eq!(clawback.status, ClawbackStatus::DisputedByContributor);
+            let clawback = get_request(&env, 1, 0).unwrap();
+            assert_eq!(clawback.status, ClawbackStatus::DisputedByContributor);
+        });
     }
 
     #[test]
     fn test_dispute_after_window_fails() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
         let admin = Address::generate(&env);
         let protocol_admin = Address::generate(&env);
@@ -430,35 +608,44 @@ mod tests {
         let owner = Address::generate(&env);
         let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        Storage::set_treasury(&env, &admin);
-        grant_role(&env, &admin, &protocol_admin, Role::ProtocolAdmin, None).unwrap();
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
-
-        let grant = setup_grant(&env, owner.clone(), token);
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
-        approve(&env, &protocol_admin, 1, 0).unwrap();
-        approve(&env, &arbiter, 1, 0).unwrap();
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            true,
+        );
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
 
         // Advance time past dispute window
         env.ledger()
             .set_timestamp(env.ledger().timestamp() + CLAWBACK_DISPUTE_WINDOW_SECONDS + 1);
 
         // Contributor disputes - should fail
-        let result = dispute(&env, &owner, 1, 0);
-        assert_eq!(result, Err(ContractError::DeadlinePassed));
+        env.as_contract(&contract_id, || {
+            let result = dispute(&env, &owner, 1, 0);
+            assert_eq!(result, Err(ContractError::DeadlinePassed));
+        });
     }
 
     #[test]
     fn test_execute_before_window_fails() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
         let admin = Address::generate(&env);
         let protocol_admin = Address::generate(&env);
@@ -466,131 +653,356 @@ mod tests {
         let owner = Address::generate(&env);
         let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        Storage::set_treasury(&env, &admin);
-        grant_role(&env, &admin, &protocol_admin, Role::ProtocolAdmin, None).unwrap();
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            true,
+        );
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
+
+        // Try to execute before window ends - should fail regardless of
+        // allowance state.
+        env.as_contract(&contract_id, || {
+            let result = execute(&env, &admin, 1, 0);
+            assert_eq!(result, Err(ContractError::DeadlinePassed));
+        });
+    }
+
+    #[test]
+    fn test_execute_fails_without_allowance() {
+        let env = Env::default();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
+        let token = register_token(&env);
 
-        let grant = setup_grant(&env, owner, token);
-        Storage::set_grant(&env, 1, &grant);
+        let admin = Address::generate(&env);
+        let protocol_admin = Address::generate(&env);
+        let arbiter = Address::generate(&env);
+        let owner = Address::generate(&env);
 
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            true,
+        );
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
 
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
-        approve(&env, &protocol_admin, 1, 0).unwrap();
-        approve(&env, &arbiter, 1, 0).unwrap();
+        env.ledger()
+            .set_timestamp(env.ledger().timestamp() + CLAWBACK_DISPUTE_WINDOW_SECONDS + 1);
 
-        // Try to execute before window ends - should fail
-        let result = execute(&env, &admin, 1, 0);
-        assert_eq!(result, Err(ContractError::DeadlinePassed));
+        // No authorize_pull was ever called — a clean error, not a
+        // token-contract panic.
+        env.as_contract(&contract_id, || {
+            let result = execute(&env, &admin, 1, 0);
+            assert_eq!(result, Err(ContractError::InsufficientClawbackAllowance));
+        });
     }
 
     #[test]
     fn test_execute_success() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
+        let token = register_token(&env);
 
         let admin = Address::generate(&env);
         let protocol_admin = Address::generate(&env);
         let arbiter = Address::generate(&env);
         let owner = Address::generate(&env);
-        let token = Address::generate(&env);
-
-        Storage::set_global_admin(&env, &admin);
-        Storage::set_treasury(&env, &admin);
-        grant_role(&env, &admin, &protocol_admin, Role::ProtocolAdmin, None).unwrap();
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
 
-        let grant = setup_grant(&env, owner, token);
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
-        approve(&env, &protocol_admin, 1, 0).unwrap();
-        approve(&env, &arbiter, 1, 0).unwrap();
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            true,
+        );
+
+        token::StellarAssetClient::new(&env, &token).mint(&owner, &500);
+        env.as_contract(&contract_id, || {
+            authorize_pull(&env, &owner, 1, &token, 500, env.ledger().sequence() + 1000).unwrap();
+        });
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
 
         // Advance time past dispute window
         env.ledger()
             .set_timestamp(env.ledger().timestamp() + CLAWBACK_DISPUTE_WINDOW_SECONDS + 1);
 
         // Execute should succeed
-        let result = execute(&env, &admin, 1, 0);
-        assert!(result.is_ok());
+        env.as_contract(&contract_id, || {
+            let result = execute(&env, &admin, 1, 0);
+            assert!(result.is_ok());
 
-        let clawback = get_request(&env, 1, 0).unwrap();
-        assert_eq!(clawback.status, ClawbackStatus::Executed);
+            let clawback = get_request(&env, 1, 0).unwrap();
+            assert_eq!(clawback.status, ClawbackStatus::Executed);
+        });
     }
 
     #[test]
     fn test_cancel_success() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
 
         let admin = Address::generate(&env);
         let arbiter = Address::generate(&env);
         let owner = Address::generate(&env);
         let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
-
-        let grant = setup_grant(&env, owner, token);
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        env.as_contract(&contract_id, || {
+            Storage::set_global_admin(&env, &admin);
+            bootstrap_super_admin(&env, &admin);
+            let grant = setup_grant(&env, owner, token);
+            Storage::set_grant(&env, 1, &grant);
+            let milestone = setup_milestone(&env, 0, 500);
+            Storage::set_milestone(&env, 1, 0, &milestone);
+        });
+        env.as_contract(&contract_id, || {
+            grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
+        });
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
 
         // Cancel should succeed
-        let result = cancel(&env, &arbiter, 1, 0);
-        assert!(result.is_ok());
+        env.as_contract(&contract_id, || {
+            let result = cancel(&env, &arbiter, 1, 0);
+            assert!(result.is_ok());
 
-        let clawback = get_request(&env, 1, 0).unwrap();
-        assert_eq!(clawback.status, ClawbackStatus::Cancelled);
+            let clawback = get_request(&env, 1, 0).unwrap();
+            assert_eq!(clawback.status, ClawbackStatus::Cancelled);
+        });
     }
 
     #[test]
     fn test_cancel_after_execute_fails() {
         let env = Env::default();
-        env.mock_all_auths();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
+        let token = register_token(&env);
 
         let admin = Address::generate(&env);
         let protocol_admin = Address::generate(&env);
         let arbiter = Address::generate(&env);
         let owner = Address::generate(&env);
-        let token = Address::generate(&env);
 
-        Storage::set_global_admin(&env, &admin);
-        Storage::set_treasury(&env, &admin);
-        grant_role(&env, &admin, &protocol_admin, Role::ProtocolAdmin, None).unwrap();
-        grant_role(&env, &admin, &arbiter, Role::DisputeArbiter, None).unwrap();
-
-        let grant = setup_grant(&env, owner, token);
-        Storage::set_grant(&env, 1, &grant);
-
-        let milestone = setup_milestone(&env, 0, 500);
-        Storage::set_milestone(&env, 1, 0, &milestone);
-
-        let reason = String::from_str(&env, "Test");
-        initiate(&env, &arbiter, 1, 0, reason).unwrap();
-        approve(&env, &protocol_admin, 1, 0).unwrap();
-        approve(&env, &arbiter, 1, 0).unwrap();
+        seed(
+            &env,
+            &contract_id,
+            &owner,
+            &token,
+            &admin,
+            &protocol_admin,
+            &arbiter,
+            true,
+        );
+
+        token::StellarAssetClient::new(&env, &token).mint(&owner, &500);
+        env.as_contract(&contract_id, || {
+            authorize_pull(&env, &owner, 1, &token, 500, env.ledger().sequence() + 1000).unwrap();
+        });
+
+        env.as_contract(&contract_id, || {
+            let reason = String::from_str(&env, "Test");
+            initiate(&env, &arbiter, 1, 0, reason).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &protocol_admin, 1, 0).unwrap();
+        });
+        env.as_contract(&contract_id, || {
+            approve(&env, &arbiter, 1, 0).unwrap();
+        });
 
         // Advance time past dispute window
         env.ledger()
             .set_timestamp(env.ledger().timestamp() + CLAWBACK_DISPUTE_WINDOW_SECONDS + 1);
 
-        execute(&env, &admin, 1, 0).unwrap();
+        env.as_contract(&contract_id, || {
+            execute(&env, &admin, 1, 0).unwrap();
+        });
 
         // Cancel after execute should fail
-        let result = cancel(&env, &arbiter, 1, 0);
-        assert_eq!(result, Err(ContractError::InvalidState));
+        env.as_contract(&contract_id, || {
+            let result = cancel(&env, &arbiter, 1, 0);
+            assert_eq!(result, Err(ContractError::InvalidState));
+        });
+    }
+
+    #[test]
+    fn test_authorize_pull_rejects_non_owner() {
+        let env = Env::default();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
+        let token = register_token(&env);
+
+        let owner = Address::generate(&env);
+        let stranger = Address::generate(&env);
+
+        env.as_contract(&contract_id, || {
+            let grant = setup_grant(&env, owner, token.clone());
+            Storage::set_grant(&env, 1, &grant);
+        });
+
+        env.as_contract(&contract_id, || {
+            let result = authorize_pull(&env, &stranger, 1, &token, 500, u32::MAX);
+            assert_eq!(result, Err(ContractError::Unauthorized));
+        });
+    }
+
+    #[test]
+    fn test_authorize_pull_rejects_non_positive_amount() {
+        let env = Env::default();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = register(&env);
+        let token = register_token(&env);
+
+        let owner = Address::generate(&env);
+
+        env.as_contract(&contract_id, || {
+            let grant = setup_grant(&env, owner.clone(), token.clone());
+            Storage::set_grant(&env, 1, &grant);
+        });
+
+        env.as_contract(&contract_id, || {
+            let result = authorize_pull(&env, &owner, 1, &token, 0, u32::MAX);
+            assert_eq!(result, Err(ContractError::InvalidInput));
+        });
+    }
+
+    /// The key regression test for #685: proves `execute` moves funds
+    /// without ever requiring the target's own signature — demonstrating a
+    /// truly unwilling contributor can't block recovery once they've
+    /// pre-authorized the pull. Drives real entry points through the
+    /// generated contract client under `mock_all_auths_allowing_non_root_auth`
+    /// (which records, rather than blindly satisfies, every `require_auth`
+    /// call), then inspects `env.auths()` after the `execute` call to assert
+    /// the target's address never appears — the thing `mock_all_auths()`
+    /// alone would silently paper over.
+    #[test]
+    fn test_execute_succeeds_via_preauthorized_allowance_without_target_signature() {
+        let env = Env::default();
+        env.mock_all_auths_allowing_non_root_auth();
+        let contract_id = env.register(StellarGrantsContract, ());
+        let client = StellarGrantsContractClient::new(&env, &contract_id);
+
+        let admin = Address::generate(&env);
+        let owner = Address::generate(&env);
+        let treasury = Address::generate(&env);
+
+        let token_admin = Address::generate(&env);
+        let asset = env.register_stellar_asset_contract_v2(token_admin);
+        let token_id = asset.address();
+        token::StellarAssetClient::new(&env, &token_id).mint(&owner, &1_000);
+
+        // Seed grant/milestone/an already-Approved, past-window clawback
+        // record directly — the initiate/approve authorization flow is
+        // already covered by the tests above; this test isolates the
+        // execute-time authorization gap #685 is about.
+        env.as_contract(&contract_id, || {
+            Storage::set_treasury(&env, &treasury);
+
+            let grant = setup_grant(&env, owner.clone(), token_id.clone());
+            Storage::set_grant(&env, 1, &grant);
+
+            let milestone = setup_milestone(&env, 0, 500);
+            Storage::set_milestone(&env, 1, 0, &milestone);
+
+            let now = env.ledger().timestamp();
+            let clawback = ClawbackRequest {
+                grant_id: 1,
+                milestone_idx: 0,
+                target: owner.clone(),
+                amount: 500,
+                token: token_id.clone(),
+                reason: String::from_str(&env, "test"),
+                initiated_by: admin.clone(),
+                initiated_at: now,
+                dispute_window_ends: now,
+                approvals: Vec::new(&env),
+                required_approvals: 2,
+                status: ClawbackStatus::Approved,
+            };
+            Storage::set_clawback(&env, 1, 0, &clawback);
+        });
+        env.ledger().with_mut(|l| l.timestamp += 1);
+
+        // The contributor pre-authorizes the pull while still cooperative.
+        client.clawback_authorize_pull(
+            &owner,
+            &1,
+            &token_id,
+            &500,
+            &(env.ledger().sequence() + 1000),
+        );
+
+        // Execute the clawback — only `admin` drives this call.
+        client.clawback_execute(&admin, &1, &0);
+
+        // The decisive assertion: whatever addresses had to authorize this
+        // specific `execute` call, the target (`owner`) is not among them.
+        // Under the old `transfer`-based implementation this call would
+        // have panicked (the token contract's own `from.require_auth()` for
+        // the target has no signature to satisfy); under the fix it
+        // succeeds via `transfer_from`, which only needs the contract's own
+        // (automatic) authorization as spender.
+        let authorized = env.auths();
+        assert!(authorized.iter().any(|(addr, _)| addr == &admin));
+        assert!(!authorized.iter().any(|(addr, _)| addr == &owner));
+
+        let token_client = token::Client::new(&env, &token_id);
+        assert_eq!(token_client.balance(&treasury), 500);
+        assert_eq!(token_client.balance(&owner), 500);
+
+        env.as_contract(&contract_id, || {
+            assert_eq!(
+                get_request(&env, 1, 0).unwrap().status,
+                ClawbackStatus::Executed
+            );
+        });
     }
 }
diff --git a/contracts/contracts/stellar-grants/src/compliance.rs b/contracts/contracts/stellar-grants/src/compliance.rs
index 9b07e4c9..6a0b4f4c 100644
--- a/contracts/contracts/stellar-grants/src/compliance.rs
+++ b/contracts/contracts/stellar-grants/src/compliance.rs
@@ -144,7 +144,20 @@ pub fn is_valid(env: &Env, attestation: &ComplianceAttestation) -> bool {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use soroban_sdk::{testutils::Address as _, Address, Env};
+    use soroban_sdk::{testutils::Address as _, testutils::Ledger as _, Address, Env};
+
+    fn set_ledger(env: &Env, sequence: u32, timestamp: u64) {
+        env.ledger().set(soroban_sdk::testutils::LedgerInfo {
+            timestamp,
+            protocol_version: 21,
+            sequence_number: sequence,
+            base_reserve: 10,
+            network_id: Default::default(),
+            min_temp_entry_ttl: 100_000,
+            min_persistent_entry_ttl: 100_000,
+            max_entry_ttl: 1_000_000,
+        });
+    }
 
     fn setup() -> (Env, Address, Address) {
         let env = Env::default();
@@ -382,7 +395,7 @@ mod tests {
     #[test]
     fn attest_records_timestamps() {
         let (env, _, verifier) = setup();
-        env.ledger().set(1, 100);
+        set_ledger(&env, 1, 100);
         let subj = Address::generate(&env);
         let jurisdiction = soroban_sdk::String::from_str(&env, "US");
         attest(
@@ -568,7 +581,7 @@ mod tests {
         )
         .unwrap();
         // Advance past expiry
-        env.ledger().set(1, 101);
+        set_ledger(&env, 1, 101);
         assert_eq!(
             require_compliant(&env, &subj, ComplianceLevel::Basic),
             Err(ContractError::ComplianceCheckFailed)
@@ -590,7 +603,7 @@ mod tests {
             jurisdiction,
         )
         .unwrap();
-        env.ledger().set(1, 199);
+        set_ledger(&env, 1, 199);
         require_compliant(&env, &subj, ComplianceLevel::Basic).unwrap();
     }
 
@@ -631,7 +644,7 @@ mod tests {
         )
         .unwrap();
         // Even far in the future, zero expiry means never expires
-        env.ledger().set(1, 999_999);
+        set_ledger(&env, 1, 999_999);
         require_compliant(&env, &subj, ComplianceLevel::Standard).unwrap();
     }
 
@@ -712,7 +725,7 @@ mod tests {
     #[test]
     fn is_valid_approved_not_expired() {
         let env = Env::default();
-        env.ledger().set(1, 50);
+        set_ledger(&env, 1, 50);
         let att = ComplianceAttestation {
             subject: Address::generate(&env),
             status: ComplianceStatus::Approved,
@@ -758,7 +771,7 @@ mod tests {
     #[test]
     fn is_valid_past_expiry_time() {
         let env = Env::default();
-        env.ledger().set(1, 200);
+        set_ledger(&env, 1, 200);
         let att = ComplianceAttestation {
             subject: Address::generate(&env),
             status: ComplianceStatus::Approved,
@@ -774,7 +787,7 @@ mod tests {
     #[test]
     fn is_valid_zero_expiry_never_expires() {
         let env = Env::default();
-        env.ledger().set(1, 999_999);
+        set_ledger(&env, 1, 999_999);
         let att = ComplianceAttestation {
             subject: Address::generate(&env),
             status: ComplianceStatus::Approved,
diff --git a/contracts/contracts/stellar-grants/src/errors.rs b/contracts/contracts/stellar-grants/src/errors.rs
index cd475c19..74e6ad1f 100644
--- a/contracts/contracts/stellar-grants/src/errors.rs
+++ b/contracts/contracts/stellar-grants/src/errors.rs
@@ -187,4 +187,10 @@ pub enum ContractError {
     DaoVoteRequired = 145,
     // Token swap (#683): no real DEX integration exists yet
     SwapNotImplemented = 146,
+    // Public review (#590): open_review::submit_review enforces
+    // MAX_PUBLIC_REVIEWS_PER_MILESTONE and needs this variant to report it.
+    TooManyPublicReviews = 147,
+    // Clawback (#685): execute() now pulls funds via a pre-authorized SEP-41
+    // allowance instead of assuming the target signs the transfer.
+    InsufficientClawbackAllowance = 148,
 }
diff --git a/contracts/contracts/stellar-grants/src/events.rs b/contracts/contracts/stellar-grants/src/events.rs
index 5ca4c9e7..aaf4388f 100644
--- a/contracts/contracts/stellar-grants/src/events.rs
+++ b/contracts/contracts/stellar-grants/src/events.rs
@@ -253,6 +253,17 @@ pub struct ClawbackCancelled {
     pub timestamp: u64,
 }
 
+#[contractevent]
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct ClawbackAllowanceAuthorized {
+    pub grant_id: u64,
+    pub contributor: Address,
+    pub token: Address,
+    pub amount: i128,
+    pub live_until_ledger: u32,
+    pub timestamp: u64,
+}
+
 // ── Issue #135: Machine-readable receipts ───────────────────────────────────────
 
 #[contractevent]
@@ -793,6 +804,25 @@ impl Events {
         event.publish(env);
     }
 
+    pub fn emit_clawback_allowance_authorized(
+        env: &Env,
+        grant_id: u64,
+        contributor: Address,
+        token: Address,
+        amount: i128,
+        live_until_ledger: u32,
+    ) {
+        let event = ClawbackAllowanceAuthorized {
+            grant_id,
+            contributor,
+            token,
+            amount,
+            live_until_ledger,
+            timestamp: env.ledger().timestamp(),
+        };
+        event.publish(env);
+    }
+
     // ── Issue #135: Machine-readable receipt emit methods ─────────────────────
 
     pub fn emit_payer_receipt(
diff --git a/contracts/contracts/stellar-grants/src/invoice.rs b/contracts/contracts/stellar-grants/src/invoice.rs
index 2596c05d..74320cc0 100644
--- a/contracts/contracts/stellar-grants/src/invoice.rs
+++ b/contracts/contracts/stellar-grants/src/invoice.rs
@@ -220,9 +220,128 @@ pub fn validate_line_items(
         subtotal = subtotal.saturating_add(item.total);
     }
 
-    // Calculate tax
-    let tax_amount = (subtotal * (tax_bps as i128)) / 10_000;
+    // Calculate tax. `basis_points_of` rejects tax_bps > 10_000 and uses
+    // checked arithmetic internally, so a malicious/oversized tax_bps
+    // returns a clean error instead of overflowing.
+    let tax_amount = crate::math::basis_points_of(subtotal, tax_bps)?;
     let total = subtotal.saturating_add(tax_amount);
 
     Ok((subtotal, total))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::storage::Storage;
+    use crate::types::{Grant, GrantStatus, Milestone, MilestoneState};
+    use soroban_sdk::testutils::Address as _;
+    use soroban_sdk::{Address, Env, Map};
+
+    fn setup_grant(env: &Env, owner: Address, token: Address) -> Grant {
+        Grant {
+            id: 1,
+            owner: owner.clone(),
+            title: String::from_str(env, "Test Grant"),
+            description: String::from_str(env, "Test"),
+            token: token.clone(),
+            status: GrantStatus::Active,
+            total_amount: 1000,
+            milestone_amount: 500,
+            reviewers: Vec::new(env),
+            total_milestones: 1,
+            milestones_paid_out: 0,
+            escrow_balance: 500,
+            funders: Vec::new(env),
+            reason: None,
+            timestamp: env.ledger().timestamp(),
+            require_compliance: None,
+        }
+    }
+
+    fn setup_milestone(env: &Env, amount: i128) -> Milestone {
+        Milestone {
+            idx: 0,
+            description: String::from_str(env, "Milestone"),
+            amount,
+            state: MilestoneState::Submitted,
+            votes: Map::new(env),
+            approvals: 0,
+            rejections: 0,
+            reasons: Map::new(env),
+            status_updated_at: env.ledger().timestamp(),
+            proof_url: None,
+            submission_timestamp: env.ledger().timestamp(),
+            deadline: None,
+            reviewer_count_snapshot: 0,
+        }
+    }
+
+    fn line_item(env: &Env, quantity: u32, unit_price: i128) -> LineItem {
+        LineItem {
+            description: String::from_str(env, "Work"),
+            quantity,
+            unit_price,
+            total: (quantity as i128) * unit_price,
+        }
+    }
+
+    #[test]
+    fn test_validate_line_items_accepts_valid_tax_bps() {
+        let env = Env::default();
+        let items = Vec::from_array(&env, [line_item(&env, 10, 100)]);
+
+        // 1000 subtotal, 2.5% tax = 25
+        let (subtotal, total) = validate_line_items(&items, 250).unwrap();
+        assert_eq!(subtotal, 1000);
+        assert_eq!(total, 1025);
+    }
+
+    #[test]
+    fn test_validate_line_items_rejects_excessive_tax_bps() {
+        let env = Env::default();
+        let items = Vec::from_array(&env, [line_item(&env, 10, 100)]);
+
+        // A malicious/oversized tax_bps must return a clean error, not
+        // silently overflow or panic.
+        let result = validate_line_items(&items, u32::MAX);
+        assert_eq!(result, Err(ContractError::InvalidInput));
+    }
+
+    #[test]
+    fn test_validate_line_items_rejects_tax_bps_just_over_limit() {
+        let env = Env::default();
+        let items = Vec::from_array(&env, [line_item(&env, 10, 100)]);
+
+        let result = validate_line_items(&items, 10_001);
+        assert_eq!(result, Err(ContractError::InvalidInput));
+    }
+
+    #[test]
+    fn test_submit_invoice_rejects_tax_bps_over_10000() {
+        let env = Env::default();
+        env.mock_all_auths();
+        let contract_id = env.register(crate::StellarGrantsContract, ());
+
+        env.as_contract(&contract_id, || {
+            let owner = Address::generate(&env);
+            let token = Address::generate(&env);
+
+            let grant = setup_grant(&env, owner.clone(), token);
+            Storage::set_grant(&env, 1, &grant);
+
+            let milestone = setup_milestone(&env, 1000);
+            Storage::set_milestone(&env, 1, 0, &milestone);
+
+            // Subtotal 1000 so the milestone-amount tolerance check never
+            // masks the tax_bps rejection.
+            let items = Vec::from_array(&env, [line_item(&env, 10, 100)]);
+            let invoice_number = String::from_str(&env, "INV-1");
+
+            let result = submit_invoice(&env, &owner, 1, 0, invoice_number, items, 10_001, None);
+            assert_eq!(result, Err(ContractError::InvalidInput));
+
+            // A malicious submission must not leave a partial invoice on record.
+            assert!(get_invoice(&env, 1, 0).is_none());
+        });
+    }
+}
diff --git a/contracts/contracts/stellar-grants/src/lib.rs b/contracts/contracts/stellar-grants/src/lib.rs
index 3e912846..1801c51c 100644
--- a/contracts/contracts/stellar-grants/src/lib.rs
+++ b/contracts/contracts/stellar-grants/src/lib.rs
@@ -1859,6 +1859,12 @@ impl StellarGrantsContract {
     }
 
     // ── Clawback Mechanism Entry Points ───────────────────────────────────────
+    //
+    // Note: none of these wrappers call `.require_auth()` themselves — each
+    // delegates to a `clawback::*` function that already calls it internally.
+    // A duplicate `require_auth()` call for the same address at the same
+    // invocation depth trips Soroban's "frame is already authorized" auth
+    // error, so the check belongs in exactly one place (the module fn).
 
     pub fn clawback_initiate(
         env: Env,
@@ -1867,7 +1873,6 @@ impl StellarGrantsContract {
         milestone_idx: u32,
         reason: String,
     ) -> Result<(), ContractError> {
-        initiator.require_auth();
         clawback::initiate(&env, &initiator, grant_id, milestone_idx, reason)
     }
 
@@ -1877,7 +1882,6 @@ impl StellarGrantsContract {
         grant_id: u64,
         milestone_idx: u32,
     ) -> Result<(), ContractError> {
-        approver.require_auth();
         clawback::approve(&env, &approver, grant_id, milestone_idx)
     }
 
@@ -1887,17 +1891,33 @@ impl StellarGrantsContract {
         grant_id: u64,
         milestone_idx: u32,
     ) -> Result<(), ContractError> {
-        contributor.require_auth();
         clawback::dispute(&env, &contributor, grant_id, milestone_idx)
     }
 
+    pub fn clawback_authorize_pull(
+        env: Env,
+        contributor: Address,
+        grant_id: u64,
+        token: Address,
+        amount: i128,
+        live_until_ledger: u32,
+    ) -> Result<(), ContractError> {
+        clawback::authorize_pull(
+            &env,
+            &contributor,
+            grant_id,
+            &token,
+            amount,
+            live_until_ledger,
+        )
+    }
+
     pub fn clawback_execute(
         env: Env,
         caller: Address,
         grant_id: u64,
         milestone_idx: u32,
     ) -> Result<i128, ContractError> {
-        caller.require_auth();
         clawback::execute(&env, &caller, grant_id, milestone_idx)
     }
 
@@ -1907,7 +1927,6 @@ impl StellarGrantsContract {
         grant_id: u64,
         milestone_idx: u32,
     ) -> Result<(), ContractError> {
-        admin.require_auth();
         clawback::cancel(&env, &admin, grant_id, milestone_idx)
     }
 
diff --git a/contracts/contracts/stellar-grants/src/lockup.rs b/contracts/contracts/stellar-grants/src/lockup.rs
index d490df8f..db400694 100644
--- a/contracts/contracts/stellar-grants/src/lockup.rs
+++ b/contracts/contracts/stellar-grants/src/lockup.rs
@@ -288,10 +288,7 @@ mod tests {
         attach_lockup(&env, &owner, 1, 0, 500).unwrap();
         let record = get_lockup(&env, 1, 0).unwrap();
         assert_eq!(record.unlocks_at, 1500);
-        assert_eq!(
-            record.token,
-            env.as_contract(&|| Storage::get_grant(&env, 1).unwrap().token)
-        );
+        assert_eq!(record.token, Storage::get_grant(&env, 1).unwrap().token);
     }
 
     #[test]
diff --git a/contracts/contracts/stellar-grants/src/merkle.rs b/contracts/contracts/stellar-grants/src/merkle.rs
index 7aec1e53..e1b56a7c 100644
--- a/contracts/contracts/stellar-grants/src/merkle.rs
+++ b/contracts/contracts/stellar-grants/src/merkle.rs
@@ -181,7 +181,7 @@ mod tests {
             let h23 = hash_pair(&env, &h2, &h3);
 
             let mut siblings = Vec::new(&env);
-            siblings.push_back(h1);
+            siblings.push_back(h1.clone());
             siblings.push_back(h23);
 
             let valid = MerkleProof {
diff --git a/contracts/contracts/stellar-grants/src/milestone_extension.rs b/contracts/contracts/stellar-grants/src/milestone_extension.rs
index 802c3dc7..50f14f0d 100644
--- a/contracts/contracts/stellar-grants/src/milestone_extension.rs
+++ b/contracts/contracts/stellar-grants/src/milestone_extension.rs
@@ -230,7 +230,7 @@ mod tests {
     use super::*;
     use crate::storage::Storage;
     use crate::types::{Grant, GrantStatus, Milestone, MilestoneState};
-    use soroban_sdk::{testutils::Address as _, Env, Map, String, Vec};
+    use soroban_sdk::{testutils::Address as _, testutils::Ledger as _, Env, Map, String, Vec};
 
     fn setup() -> (Env, Address, Address, Address) {
         let env = Env::default();
diff --git a/contracts/contracts/stellar-grants/src/referral.rs b/contracts/contracts/stellar-grants/src/referral.rs
index af6bdcf7..e8a6ae3e 100644
--- a/contracts/contracts/stellar-grants/src/referral.rs
+++ b/contracts/contracts/stellar-grants/src/referral.rs
@@ -535,7 +535,7 @@ mod tests {
     fn test_claim_rewards_rejects_reentrant_call_when_guard_is_held() {
         let f = setup();
 
-        f.env.as_contract(&f.contract_id, || {
+        f.env.as_contract(&f.client.address, || {
             f.env
                 .storage()
                 .temporary()
diff --git a/contracts/contracts/stellar-grants/src/split_payment.rs b/contracts/contracts/stellar-grants/src/split_payment.rs
index 784f85cd..8b432c88 100644
--- a/contracts/contracts/stellar-grants/src/split_payment.rs
+++ b/contracts/contracts/stellar-grants/src/split_payment.rs
@@ -104,11 +104,25 @@ mod tests {
     use soroban_sdk::{testutils::Address as _, vec, Env};
 
     fn setup_grant(env: &Env, owner: &Address, grant_id: u64, total_milestones: u32) {
-        use crate::types::Grant;
+        use crate::types::{Grant, GrantStatus};
+        use soroban_sdk::String;
         let grant = Grant {
+            id: grant_id,
             owner: owner.clone(),
+            title: String::from_str(env, "Test Grant"),
+            description: String::from_str(env, "Test"),
+            token: Address::generate(env),
+            status: GrantStatus::Active,
+            total_amount: 0,
+            milestone_amount: 0,
+            reviewers: Vec::new(env),
             total_milestones,
-            ..Default::default()
+            milestones_paid_out: 0,
+            escrow_balance: 0,
+            funders: Vec::new(env),
+            reason: None,
+            timestamp: env.ledger().timestamp(),
+            require_compliance: None,
         };
         Storage::set_grant(env, grant_id, &grant);
     }
diff --git a/contracts/contracts/stellar-grants/src/storage/helpers.rs b/contracts/contracts/stellar-grants/src/storage/helpers.rs
index 72ee8a9f..e8609e33 100644
--- a/contracts/contracts/stellar-grants/src/storage/helpers.rs
+++ b/contracts/contracts/stellar-grants/src/storage/helpers.rs
@@ -2443,9 +2443,22 @@ mod tests {
         let env = Env::default();
         let owner = Address::generate(&env);
         let grant = crate::types::Grant {
+            id: 1,
             owner: owner.clone(),
+            title: soroban_sdk::String::from_str(&env, "Test Grant"),
+            description: soroban_sdk::String::from_str(&env, "Test"),
+            token: Address::generate(&env),
+            status: crate::types::GrantStatus::Active,
+            total_amount: 0,
+            milestone_amount: 0,
+            reviewers: soroban_sdk::Vec::new(&env),
             total_milestones: 3,
-            ..Default::default()
+            milestones_paid_out: 0,
+            escrow_balance: 0,
+            funders: soroban_sdk::Vec::new(&env),
+            reason: None,
+            timestamp: env.ledger().timestamp(),
+            require_compliance: None,
         };
         Storage::set_grant(&env, 1, &grant);