maintenance-pool::withdraw's recipient is never validated against the contract's own address — self-payment corrupts the balance invariant without theft

[chonilius]

Overview

maintenance-pool::withdraw (contracts/maintenance-pool/src/lib.rs:116-157) takes an arbitrary recipient: Address with no validation against it beyond existing generally (it isn't checked against anything at all — not env.current_contract_address(), not treasury, not admin). This is a structurally different situation from #28 ("Correctness audit of compute_split under zero-recipient-adjacent, duplicate-address, and self-referential edge cases"), which is explicitly scoped to the recipients: Vec<(Address, u32)> parameter of escrow::release/milestones::release_issue — withdraw doesn't use compute_split at all (it does a direct, single transfer, contracts/maintenance-pool/src/lib.rs:146-149), so none of #28's analysis or eventual tests cover this function.

Two concrete scenarios #28 structurally cannot reach:

1. recipient == env.current_contract_address(). If the admin (or a compromised/buggy backend integration) accidentally passes the pool contract's own address as recipient, the payout transfer (contracts/maintenance-pool/src/lib.rs:149) sends tokens from the contract to itself. Regardless of whether the underlying SEP-41 token treats this as a no-op or a real balance-preserving transfer, the bookkeeping proceeds unconditionally: pool.balance -= amount and pool.total_withdrawn += amount (lines 151-152) both still execute, exactly as if the tokens had genuinely left the contract. The result: pool.balance (and its derived invariant, balance == total_deposited - total_withdrawn, the exact invariant Long-run invariant fuzzing of total_deposited/total_withdrawn/balance drift in maintenance-pool #29's long-run fuzzer is meant to protect) silently diverges from the contract's actual on-chain token balance — the contract now holds more tokens than its own accounting believes it does, with no theft involved, just corrupted bookkeeping that Long-run invariant fuzzing of total_deposited/total_withdrawn/balance drift in maintenance-pool #29's fuzzer isn't guaranteed to generate this specific adversarial input for on its own.
2. recipient == treasury or recipient == admin. Not necessarily harmful (an admin might legitimately want to route a withdrawal to the treasury or pay themselves as a maintainer), but currently completely untested — no test in contracts/maintenance-pool/src/test.rs exercises either. Given withdraw already does two separate transfers in sequence when fee > 0 (fee to treasury, then payout to recipient — lines 146-149), a recipient == treasury call means treasury receives two transfers in the same call; this should be confirmed harmless and locked in by a test, exactly as Correctness audit of compute_split under zero-recipient-adjacent, duplicate-address, and self-referential edge cases #28 does for the analogous recipient == treasury case in escrow::release.

Requirements

• Reject recipient == env.current_contract_address() in withdraw with a new Error variant, since (per the analysis above) this scenario has no legitimate use and silently corrupts the balance/total_withdrawn invariant with no recovery path (compounding with the "no sweep function" issue filed in this batch, since even a corrected accounting can't reclaim tokens that a self-payment never actually removed).
• Add explicit tests for recipient == treasury and recipient == admin, confirming (or, if something is found to be actually wrong, fixing) that these are handled correctly — mirroring Correctness audit of compute_split under zero-recipient-adjacent, duplicate-address, and self-referential edge cases #28's methodology exactly, just applied to withdraw's single-recipient parameter instead of a recipients Vec.

Acceptance Criteria

• withdraw rejects recipient == env.current_contract_address() with a new, clearly-named Error variant (e.g. InvalidRecipient)
• test_withdraw_rejects_self_payment_to_contract_address added, reproducing the balance-corruption scenario as a pre-fix demonstration before asserting the new rejection
• test_withdraw_to_treasury_and_admin_addresses added, explicitly locking in the (expected-harmless) two-transfers-same-address behavior
• cargo test --workspace passes

Additional Notes

• Precise references: withdraw's transfer/bookkeeping sequence at contracts/maintenance-pool/src/lib.rs:146-153 — note the balance/total_withdrawn mutation (lines 151-152) is unconditional and happens regardless of what recipient resolves to, which is exactly why a self-payment silently corrupts the invariant rather than erroring out.
• Test sketch: test_withdraw_rejects_self_payment_to_contract_address — deposit into a pool, call withdraw(pool_id, recipient=<the pool contract's own address>, amount); pre-fix, assert this succeeds and demonstrate pool.balance decreased while the contract's actual token balance (queryable via the token client directly in the test) did not decrease by the same amount (or did, depending on the SAC's self-transfer semantics — either way, assert the accounting silently proceeded rather than erroring); post-fix, assert it's rejected outright.
• Cross-references: Correctness audit of compute_split under zero-recipient-adjacent, duplicate-address, and self-referential edge cases #28 (same category of self-referential/known-address edge-case audit, explicitly scoped away from maintenance-pool::withdraw — this issue is the natural completion of that audit for the one function Correctness audit of compute_split under zero-recipient-adjacent, duplicate-address, and self-referential edge cases #28 can't reach); Long-run invariant fuzzing of total_deposited/total_withdrawn/balance drift in maintenance-pool #29 (balance-invariant fuzzer — this issue's self-payment scenario is exactly the kind of adversarial input Long-run invariant fuzzing of total_deposited/total_withdrawn/balance drift in maintenance-pool #29's fuzzer should be seeded with, once both land); the "no sweep function" issue filed in this batch (the recovery path if a self-payment ever does occur despite the new guard, e.g. via a future code path that bypasses this check).