#50 All three contracts: no integration tests against a real Soroban network — mock_all_auths() fully bypasses genuine signature verification Repo Avatar MergeFi/contracts Overview All 35 tests across the three contracts (contracts/escrow/src/test.rs, contracts/milestones/src/test.rs, contracts/maintenance-pool/src/test.rs) run exclusively against soroban_sdk::testutils::Env::default() — an in-process, simulated host, never a real Soroban RPC endpoint or local sandbox network. This is confirmed by every setup() helper across all three test files using Env::default() plus env.register(...), and by the README's own "Build, test, deploy" section stating plainly: "Add integration tests against stellar-cli's local sandbox network once available, to validate actual RPC-level invocation from a mergefi-backend-shaped client rather than only testutils" is listed under Roadmap, i.e. explicitly acknowledged as not yet done.
This gap is more significant than "we haven't gotten to it yet" for one specific, load-bearing reason: env.mock_all_auths(), used in the overwhelming majority of tests in all three suites (every setup() call site), completely bypasses genuine Ed25519 signature verification. It doesn't approximate real authorization behavior with a shortcut that's close enough — it removes the check entirely, unconditionally treating every require_auth() call as satisfied regardless of whether any real key ever signed anything. The narrower env.set_auths(&[]) used in the access-control boundary tests (added for #30) is closer to reality (it tests that no auth was provided), but even that doesn't exercise real signature verification — it only tests the presence/absence of a mocked authorization entry in the test harness's bookkeeping, not the actual cryptographic check a real Soroban host performs against a real transaction's signatures.
Concretely, this means: every access-control test added for #30 (test_refund_before_deadline_requires_admin_auth, test_extend_deadline_requires_sponsor_auth, and their siblings across all three contracts) proves that the contract's logic correctly calls require_auth() on the right address at the right time — it does not, and structurally cannot, prove that a real attacker cannot forge or replay an authorization against a real deployed instance, because the entire signature-verification layer is mocked out in every test that exists today. The same applies to the Soroban resource/instruction-budget concerns #23 aims to benchmark — testutils::Env does not enforce real network resource limits, so a test suite that passes entirely under testutils provides no evidence about real transaction size/CPU-instruction/memory-metering limits, XDR encoding-size boundaries, or real TTL/archival behavior (relevant to the TTL issues filed elsewhere in this batch, which also can't be fully validated under testutils alone).
Requirements Stand up integration tests against a real Soroban environment — either stellar-cli's local sandbox network (the README already names this as the target) or a lightweight equivalent (e.g. a dockerized stellar/quickstart local network), driven by an actual RPC client (matching how mergefi-backend itself would integrate, per README's "Backend integration" section) rather than the in-process testutils::Env. At minimum, port the #30 access-control boundary matrix to run against the real network with genuinely unsigned/wrongly-signed transactions, proving those tests' guarantees hold outside the mocked-auth simulation. Document in README.md's "Build, test, deploy" section which specific guarantees the existing testutils-based suite does not provide (signature verification, real resource/TTL limits, real XDR size limits) so future contributors don't mistake "34/34 tests pass" for full confidence on those axes. Acceptance Criteria A working integration test setup against a real Soroban RPC/sandbox network, checked into the repo (e.g. scripts/integration-test.mjs or a dedicated CI job, following the style of the existing scripts/deploy.mjs/scripts/invoke.mjs) At least the core access-control boundary assertions (admin-gated functions reject unsigned/wrongly-signed calls; sponsor-gated functions likewise; refund's post-deadline path genuinely succeeds with zero signatures) ported and passing against the real network README's testing section updated to explicitly scope what testutils-based tests do and don't prove make test (or a new make integration-test target) documented and working Additional Notes Confirmed via grep -n "mock_all_auths|set_auths" contracts/*/src/test.rs: mock_all_auths() appears in every setup() helper (used by the overwhelming majority of tests across all three files); set_auths(&[]) appears only in the smaller set of access-control-specific tests added for #30 — neither exercises real cryptographic signature verification. This is distinct from #23 ("Build a Soroban instruction/resource-budget regression benchmark suite") — #23 is about measuring and regression-testing resource costs, which presupposes a way to run against something resource-metering-accurate in the first place; this issue is the prerequisite infrastructure (a real-network test harness) that #23's benchmarks would need to be meaningful, rather than a benchmark suite itself. Cross-references: #23 (resource-budget benchmarking — depends on this issue's infrastructure for accuracy); the two TTL issues filed in this batch (instance-storage TTL never extended, and extend_deadline's false-security TTL-math issue) — both describe behavior (archival, RestoreFootprint) that fundamentally cannot be exercised under testutils::Env's simulated storage model and would benefit most directly from this issue's real-network harness; the README's own Roadmap bullet naming this exact gap, which this issue formally tracks.
#45 maintenance-pool::deposit's deposit_count: u32 unchecked increment panics uncatchably at overflow instead of returning a typed Error Repo Avatar MergeFi/contracts Overview maintenance-pool::deposit (contracts/maintenance-pool/src/lib.rs:57-110) tracks how many deposits a pool has received via a plain u32 counter, incremented with an unchecked +=:
let index = pool.deposit_count; pool.deposit_count += 1; (contracts/maintenance-pool/src/lib.rs:92-93). MaintenancePool.deposit_count: u32 (contracts/maintenance-pool/src/types.rs:16). The workspace's release profile sets overflow-checks = true (Cargo.toml:21), so at deposit_count == u32::MAX (4,294,967,295), the next pool.deposit_count += 1 panics rather than wrapping or returning a typed error. Under panic = "abort" (Cargo.toml:25), this aborts the host invocation entirely, uncatchably — there is no Result/Error variant involved at all, unlike every other validation failure in this codebase (InvalidAmount, TokenMismatch, PoolNotFound, etc., all returned as typed Err(Error::X) values that a caller/backend can inspect and handle).
This is inconsistent with the rest of the codebase's error-handling philosophy: every other rejectable condition across all three contracts — including ones with far lower real-world likelihood, like fee_bps exceeding 10000 — is surfaced as a typed contracterror variant, not a raw panic. deposit_count overflowing is the one arithmetic path in the whole codebase (besides the general i128 overflow surface already tracked by #7, which is explicitly scoped to i128 arithmetic and doesn't mention deposit_count's u32 counter) where hitting the limit produces an opaque host trap instead of something mergefi-backend could catch and report meaningfully.
The realistic attack cost is high (4.29 billion separate deposit transactions against the same pool_id, each paying real Stellar transaction fees), so this is not a practically executable griefing vector today — but it's a real, concrete latent bug: a pool that happens to be extremely popular over a very long operational lifetime (this platform's maintenance-pool contract is explicitly designed to be "recurring... never finishes," per its own module doc) is the one place in this codebase where organic, honest, high-volume usage — not an attacker — could eventually hit an unrecoverable panic instead of a clean, typed rejection.
Requirements Replace pool.deposit_count += 1 with pool.deposit_count.checked_add(1).ok_or(Error::DepositCountOverflow)? (or equivalent), adding a new typed Error variant so this failure mode is caught and reported the same way every other invalid-input condition in this codebase is. Since a pool that has exhausted u32::MAX deposit slots can never accept a new deposit again under the current Deposit(pool_id, index: u32) keying scheme (contracts/maintenance-pool/src/types.rs:34), document this as a known, permanent-per-pool-id limit in the README's "Data models" section, distinct from being silently unbounded. Acceptance Criteria deposit_count increment uses checked_add and returns a new typed Error variant on overflow instead of panicking test_deposit_rejects_when_deposit_count_would_overflow added — construct a pool with deposit_count pre-set near u32::MAX (directly via test-only storage manipulation, since reaching it organically via 4 billion real deposit calls isn't a practical test) and confirm the typed error is returned rather than a panic README's data-model section notes the per-pool deposit-count ceiling cargo test --workspace passes Additional Notes Precise reference: contracts/maintenance-pool/src/lib.rs:92-93 (the unchecked increment), contracts/maintenance-pool/src/types.rs:16 (deposit_count: u32 field), contracts/maintenance-pool/src/error.rs:1-15 (current variants, none of which cover this case). Test sketch: since driving deposit_count to u32::MAX via 4 billion real deposit() calls is infeasible in a unit test, use env.as_contract(&contract_id, || { ... }) (the same pattern already used in contracts/escrow/src/test.rs:269-271 for directly exercising internal state) to write a MaintenancePool record with deposit_count: u32::MAX directly into persistent storage via env.storage().persistent().set(...), then call deposit() through the public client and assert the new typed error rather than a panic/trap. Cross-references: #7 (i128 overflow/panic-DoS surface analysis — explicitly scoped to i128 arithmetic; this issue covers the one u32 arithmetic path in the codebase that #7's stated scope doesn't reach, and should probably be folded into whatever fix #7 produces if that issue's scope is broadened during implementation, but is filed separately here since #7 as currently titled doesn't cover it); #10 (bounded/paginated access pattern for maintenance-pool deposit history — a pool that's actually approaching this ceiling would also be deep in #10's "how do you even enumerate this many records" territory, though reaching either limit organically is extremely unlikely with the same platform).
#47 escrow::release + milestones::release_issue: a single frozen/unauthorized-trustline recipient reverts the ENTIRE team payout, blocking everyone Repo Avatar MergeFi/contracts Overview Both escrow::release and milestones::release_issue pay out a team split as a sequence of individual token::Client::transfer calls inside one function, all within one Soroban host transaction:
for (recipient, share) in payouts.shares.iter() { if share > 0 { token_client.transfer(&contract_address, &recipient, &share); } } (contracts/escrow/src/lib.rs:132-136, byte-identical in contracts/milestones/src/lib.rs:171-175). Soroban token transfers — including the Stellar Asset Contract, and by extension any SEP-41 asset with AUTHORIZATION REQUIRED or clawback-enabled flags set, both standard, supported Stellar asset features — panic if the destination account cannot legally receive the asset (no trustline, unauthorized trustline, frozen/clawback-restricted account, etc.), rather than returning a false/error value the caller could catch and route around.
Because Soroban transactions are atomic, a panic anywhere inside the transfer loop reverts the entire release/release_issue call — including every transfer that already succeeded earlier in the same loop iteration. Concretely: a 3-person team split where recipient #1 and #2 have perfectly normal, receivable wallets, and recipient #3's wallet happens to have a frozen or unauthorized trustline for the escrow's token, results in nobody getting paid — not even #1 and #2, whose transfers would have succeeded on their own. The escrow's/allocation's status also never advances past Funded/Allocated (the status write happens after the transfer loop, contracts/escrow/src/lib.rs:138-139), so the whole payout is stuck exactly where it was, indefinitely, until someone fixes recipient #3's trustline situation or the admin resubmits release/release_issue with a recomputed recipients vector that excludes them entirely.
That "resubmit excluding them" workaround is itself non-trivial: recipients' basis points must sum to exactly 10_000 (InvalidSplit otherwise, contracts/escrow/src/lib.rs:269), so removing one recipient means recomputing everyone else's bps from scratch (their agreed percentages no longer sum correctly once a party is dropped), and there is no way to "hold back" just the blocked recipient's share for later — the contract has no concept of a partial/staged release for a single issue_id/allocation (a release/release_issue call is a strictly one-shot, all-or-nothing operation per the terminal-status design). This turns one team member's account-level restriction — something entirely outside the sponsor's, the admin's, or the other recipients' control — into a full denial-of-service on the entire team's payout, for a piece of already-verified, already-merged work.
This is distinct from #3 (non-standard SEP-41 token accounting — about arithmetic/balance correctness under fee-on-transfer/rebasing tokens) and from #4 (formal reentrancy audit — about malicious/reentrant token behavior). This issue is about the atomicity/all-or-nothing consequence of a single legitimately restricted recipient within an otherwise completely standard, honest SEP-41 token, which neither #3 nor #4's stated scope addresses.
Requirements Add a test in both escrow and milestones demonstrating the failure using a mock token contract whose transfer panics for one specific recipient address (simulating a frozen/unauthorized account) while behaving normally for others, confirming the whole release/release_issue call reverts and no recipient — including the unaffected ones — is paid. Design and implement a mitigation. Two shapes worth evaluating: (a) a "best-effort" release mode that catches per-recipient transfer failures and reroutes the blocked share to a claimable/pending state rather than reverting the whole call (harder — Soroban's panic-based token interface doesn't offer a way to "try" a transfer and continue on failure within the same call, so this would likely require a two-phase design: compute-and-reserve shares first, then transfer individually via separate transactions the admin submits per recipient, rather than the current single-call-does-everything design); or (b) a documented, tooling-level mitigation (backend pre-validates every recipient's ability to receive the token, e.g. via a Soroban RPC simulation, before ever submitting release/release_issue, catching the problem before it becomes an on-chain revert) — cheaper to ship, but doesn't eliminate the underlying contract-level fragility, only reduces how often it's hit. Whichever direction is chosen, document the failure mode and the chosen mitigation's coverage explicitly in the README's "Security model" section, since the current text doesn't mention this atomicity/all-or-nothing risk at all. Acceptance Criteria Mock-token-based reproduction test added to both escrow and milestones, demonstrating the all-or-nothing revert with a mixed valid/blocked recipient set Mitigation implemented (per-recipient isolation) or, at minimum, explicitly documented as an accepted risk with the backend-side pre-validation approach specified concretely enough to be implementable in mergefi-backend README's Security model section updated to describe this risk and mitigation cargo test --workspace passes Additional Notes Precise references: contracts/escrow/src/lib.rs:107-143 (release, transfer loop at 132-136), contracts/milestones/src/lib.rs:133-183 (release_issue, transfer loop at 171-175). Test sketch: write a minimal mock Soroban token contract (or use a custom #[contract] test double implementing just enough of the token::Interface/transfer signature) whose transfer panics when to == blocked_address and succeeds otherwise; fund/create_milestone+allocate normally against this mock token; call release/release_issue with a 3-recipient split including blocked_address; assert the call panics/reverts (via try_release/try_release_issue returning an error, or the test harness's panic-catching pattern) and that a subsequent get_escrow/get_issue_status shows the record is still in its pre-release state, and that none of the other recipients' balances changed either — the "innocent parties also unpaid" consequence is the crux of this issue and should be asserted explicitly, not just "the call failed." Cross-references: #3 (non-standard token accounting — different failure category, correctness not atomicity); #4 (reentrancy audit — different failure category, malicious not restricted tokens); #8 (unbounded recipients — a longer recipients vector increases the odds that at least one entry hits this failure mode, compounding the two issues' severity together); the "partial release" architecture idea is a natural fix direction worth exploring alongside this issue but is intentionally left as an open design question here rather than prescribed, since it's a substantial redesign of release's one-shot contract.
#54 milestones: no long-run invariant fuzzing of budget conservation (total_budget == remaining_budget + Σallocations), unlike maintenance-pool's #29 Repo Avatar MergeFi/contracts Overview #29 ("Long-run invariant fuzzing of total_deposited/total_withdrawn/balance drift in maintenance-pool") establishes a clear, valuable pattern: pick a contract's core numeric conservation invariant, then long-run/property fuzz it under adversarial sequences of operations to catch drift that unit tests, which only check a handful of hand-picked scenarios, would miss. milestones has an analogous, equally real conservation invariant that has no equivalent fuzzing issue tracking it anywhere in the existing 31-issue backlog.
The invariant, stated precisely from the code: for any Milestone record, at all times,
milestone.total_budget == milestone.remaining_budget + sum(milestone.allocations.values()) This holds by construction at every individual write site I traced: create_milestone sets remaining_budget = total_budget with an empty allocations map (contracts/milestones/src/lib.rs:75-84, trivially true); allocate moves amount from remaining_budget into allocations atomically in the same function call (contracts/milestones/src/lib.rs:116-117, preserves the sum); release_issue pays out an already-allocated amount but — notably — never removes or zeroes the corresponding entry in allocations (confirmed by re-reading contracts/milestones/src/lib.rs:133-183 in full: it writes IssueStatus::Released but never touches milestone.allocations or milestone.remaining_budget), so the invariant continues to hold, just with allocations now containing a mix of released and unreleased amounts, requiring get_issue_status to distinguish them, not allocations alone; cancel_milestone only moves remaining_budget to zero (contracts/milestones/src/lib.rs:201-209), leaving allocations untouched, again preserving the top-level sum.
A hand-verified invariant across individual write sites is exactly the kind of claim #29's methodology exists to stress-test rather than trust on inspection alone — the milestones data model has more moving parts than maintenance-pool's simple balance/total_deposited/total_withdrawn triple (a Map of per-issue allocations, plus the derived-but-not-stored "released vs. still-allocated" split tracked only via separate IssueStatus keys, not reflected in allocations itself), giving a long-run fuzzer meaningfully more surface to find drift in than a straightforward manual trace can rule out — particularly once #5, #8 (deallocate/reallocate), and the milestone-timeout issue filed elsewhere in this batch land and start mutating remaining_budget/allocations via new code paths this manual trace didn't have to consider.
Requirements Build a property-based/long-run fuzz harness for milestones, structurally mirroring whatever approach #29 lands on for maintenance-pool (same tooling choice, same "run many random sequences of operations and check the invariant after each" methodology), generating random sequences of create_milestone/allocate/release_issue/cancel_milestone (and, once available, deallocate) calls against one or more milestones. Assert, after every operation in every generated sequence: total_budget == remaining_budget + sum(allocations.values()), plus the secondary invariant that every issue_id in allocations has a corresponding IssueStatus entry and vice versa (an "orphaned" entry on either side would indicate the two data structures have drifted out of sync with each other, a failure mode #29's simpler maintenance-pool model doesn't have an analog for at all). Seed the fuzzer with the specific adversarial sequences #17/#28 already identify as edge cases for compute_split (duplicate recipients, self-referential recipients) composed with milestone-level operations, so the fuzzer isn't purely random but starts from known-interesting regions. Acceptance Criteria Fuzz harness added for milestones, generating randomized operation sequences Both invariants (total_budget conservation, allocations/IssueStatus consistency) asserted after every step Harness run for a substantial number of iterations/sequence lengths (matching whatever bar #29 sets for maintenance-pool, for consistency across the two fuzzing efforts) Any drift found is fixed, with the specific failing sequence captured as a permanent regression test cargo test --workspace passes (or the fuzzer's dedicated invocation, if it runs outside the standard cargo test harness, e.g. via cargo fuzz or proptest) Additional Notes Precise references for the manual invariant trace: contracts/milestones/src/lib.rs:75-84 (create_milestone), :92-128 (allocate), :133-183 (release_issue — critically, allocations is never mutated here, only read), :187-214 (cancel_milestone). This issue is deliberately scoped as "the milestones-side equivalent of #29," not a generic "add more tests" ask — the specific value is applying the exact same rigor #29 is already committed to for one contract, to the contract whose data model (a growing Map plus a parallel status-key scheme) is arguably more failure-prone than a flat i128 counter triple, yet currently has zero fuzzing coverage of its own. Cross-references: #29 (the direct methodological precedent this issue asks to replicate); #17 (compute_split fuzzing — a different invariant, at the split-math level rather than the milestone-budget-conservation level, worth composing together per the seeding suggestion above); #5, #8 (deallocate/reallocate), and the milestones timeout-escape-hatch issue filed in this batch — all three introduce new code paths that mutate remaining_budget/allocations, and should be fuzzed against once they land, making this issue's harness valuable both now and as ongoing regression coverage for that future work.