fix: clawback allowance mechanism, invoice tax_bps overflow (#685, #686)

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

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));
     }
 

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,