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

[lycantho]

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:

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

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:

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:

• Wire Up Protocol Fee Collection — Currently Entirely Dead #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.
• Fix performance_bond::claim_bond Paying the Full Bond to a Single Funder #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

• cargo fmt --check (clean for all files touched)
• 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)
• cargo check --lib -p stellar-grants
• cargo check --workspace --target wasm32v1-none
• cargo test -p stellar-grants --lib clawback:: → 13/13 pass
• cargo test -p stellar-grants --lib invoice:: → 4/4 pass
• cargo test -p stellar-grants --lib fees:: → 10/10 pass (verifies Wire Up Protocol Fee Collection — Currently Entirely Dead #687)
• cargo test -p stellar-grants --lib performance_bond:: → 12/12 pass (verifies Fix performance_bond::claim_bond Paying the Full Bond to a Single Funder #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

[drips-wave]

@lycantho Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits