fix(security): release_escrow cumulative-paid recurrence (SEV-029 High, regression of SEV-016)#342
Merged
Conversation
…h, regression of SEV-016) Adevar Labs re-audit (2026-05-W3) identified SEV-029 as a regression introduced by the SEV-016 partial-release fix in PR #334. The fix preserved a non-advancing `last_released_checkpoint` on partial pay so the caller could "re-call when the vault refills" — but the vesting math kept recomputing `delta_target = releasable_delta(last_chk, new_chk)`, so the re-call at the same checkpoint returned a fresh full delta instead of the unpaid remainder. Reproducer (stake=1000, cycles=24): - call 1 chk=5 vault=100: delta_target=208 (cumulative_vested(5)-0) delta=min(208,100)=100, chk NOT advanced - call 2 chk=5 vault=200: delta_target=208 (re-computed from same un-advanced chk=0), delta=min(208,200)=200 - member receives 100+200=300; entitlement at chk=5 is 208. - OVERPAY 92 USDC drained from the shared escrow_vault, repeatable per cycle, observable via the SEV-016 "partial" msg! log. Fix shape — math rewrite, no new state field. Use the existing invariant `cumulative_paid = stake_deposited - escrow_balance` (escrow_balance starts at stake and is only decremented by release_escrow on non-defaulted members — settle_default cannot touch a non-defaulted member's escrow_balance because the `!member.defaulted` constraint bars defaulted callers entirely). total_due = cumulative_vested(stake, checkpoint, cycles) paid = stake - escrow_balance owed_now = total_due - paid delta = min(owed_now, vault_amount) // ALWAYS advance last_released_checkpoint (partial-pay encoded in // cumulative counter, not in checkpoint replay) Trace for the regression scenario (call 1 chk=5, call 2 chk=6): call 1: due=208 paid=0 owed=208 delta=100 → escrow 1000→900, chk=5 call 2: due=250 paid=100 owed=150 delta=150 → escrow 900→750, chk=6 total paid: 250 == cumulative_vested(stake, 6, 24) ✓ (no overpay) Coverage — per process feedback ("Critical/High needs negative test before merge"), this PR ships 4 regression unit tests + 2 proptest invariants in crates/math/src/escrow_vesting.rs: - sev_029_partial_then_full_does_not_overpay (auditor's scenario) - sev_029_chained_partials_never_overpay (compound partials) - sev_029_full_horizon_pays_exactly_principal (conservation) - sev_029_zero_vault_at_final_locks_remainder (locks not lost) - p_release_sequence_never_overpays_or_underpays (proptest invariant) - p_replay_same_checkpoint_is_rejected (idempotency) All 16 tests pass locally. cargo check --workspace clean. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm
|
Deployment failed with the following error: Learn More: https://vercel.com/alrimarleskovars-projects?upgradeToPro=build-rate-limit |
🟢 Coverage Report —
|
| Metric | Value |
|---|---|
| Coverage | 90.91% |
| Lines | 110/121 lines covered |
| Threshold | 85% |
| Status | ✅ meets threshold |
Scope:
roundfi-mathonly. Full-workspace coverage gated on Agave 2.x migration (#230).
alrimarleskovar
added a commit
that referenced
this pull request
May 15, 2026
…h, regression of SEV-016) (#342) Adevar Labs re-audit (2026-05-W3) identified SEV-029 as a regression introduced by the SEV-016 partial-release fix in PR #334. The fix preserved a non-advancing `last_released_checkpoint` on partial pay so the caller could "re-call when the vault refills" — but the vesting math kept recomputing `delta_target = releasable_delta(last_chk, new_chk)`, so the re-call at the same checkpoint returned a fresh full delta instead of the unpaid remainder. Reproducer (stake=1000, cycles=24): - call 1 chk=5 vault=100: delta_target=208 (cumulative_vested(5)-0) delta=min(208,100)=100, chk NOT advanced - call 2 chk=5 vault=200: delta_target=208 (re-computed from same un-advanced chk=0), delta=min(208,200)=200 - member receives 100+200=300; entitlement at chk=5 is 208. - OVERPAY 92 USDC drained from the shared escrow_vault, repeatable per cycle, observable via the SEV-016 "partial" msg! log. Fix shape — math rewrite, no new state field. Use the existing invariant `cumulative_paid = stake_deposited - escrow_balance` (escrow_balance starts at stake and is only decremented by release_escrow on non-defaulted members — settle_default cannot touch a non-defaulted member's escrow_balance because the `!member.defaulted` constraint bars defaulted callers entirely). total_due = cumulative_vested(stake, checkpoint, cycles) paid = stake - escrow_balance owed_now = total_due - paid delta = min(owed_now, vault_amount) // ALWAYS advance last_released_checkpoint (partial-pay encoded in // cumulative counter, not in checkpoint replay) Trace for the regression scenario (call 1 chk=5, call 2 chk=6): call 1: due=208 paid=0 owed=208 delta=100 → escrow 1000→900, chk=5 call 2: due=250 paid=100 owed=150 delta=150 → escrow 900→750, chk=6 total paid: 250 == cumulative_vested(stake, 6, 24) ✓ (no overpay) Coverage — per process feedback ("Critical/High needs negative test before merge"), this PR ships 4 regression unit tests + 2 proptest invariants in crates/math/src/escrow_vesting.rs: - sev_029_partial_then_full_does_not_overpay (auditor's scenario) - sev_029_chained_partials_never_overpay (compound partials) - sev_029_full_horizon_pays_exactly_principal (conservation) - sev_029_zero_vault_at_final_locks_remainder (locks not lost) - p_release_sequence_never_overpays_or_underpays (proptest invariant) - p_replay_same_checkpoint_is_rejected (idempotency) All 16 tests pass locally. cargo check --workspace clean. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm Co-authored-by: Claude <noreply@anthropic.com>
alrimarleskovar
pushed a commit
that referenced
this pull request
May 15, 2026
Update the public SEV tracker with the 2026-05-W3 re-audit results: - **SEV-029 (High, regression of SEV-016):** fund-leak overpay in the release_escrow partial-pay branch. Closed by PR #342 with 4 unit tests + 2 proptest invariants per the new "negative test before merge" gate. - **SEV-030 (Low, Open Fase 5):** admin cooldown only covers SCHEMA_PAYMENT; extending to LATE/DEFAULT scheduled. SEV-027 status updated to 🟡 Partial. - **SEV-031 (Low, Open Fase 5):** create_pool lacks runtime viability check — SEV-025 fixed the defaults but not custom pools. - **SEV-032 (Info, Acknowledged):** ReputationConfig padding exhausted by SEV-021. Future field additions require migration. Design constraint, documented; no action. - **SEV-033 (Low, Open Fase 5):** webhook auth fails open when env unset. Indexer fix scheduled. Also: - Updated SEV-016 status to "🟢 Closed (regressed → SEV-029)" with cross-link to #334 → #342. - Updated SEV-027 to "🟡 Partial (extended by SEV-030)". - Expanded status legend with Partial / Open / Acknowledged labels. - Updated summary table totals (28 → 33) and severity counts. - Added W3 re-audit deltas section + process recommendation ("Critical/High needs negative regression test before merge"). - Added W3 entries to the disclosure timeline. Mainnet-blocker status unchanged: all Critical / High (8/8) 🟢 Closed. Remaining Fase 5 items are Low-severity defense-in-depth. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm
alrimarleskovar
added a commit
that referenced
this pull request
May 15, 2026
* docs(security): public SEV tracker for Adevar Labs audit findings Public-facing accountability record for the Adevar Labs security audit of the RoundFi protocol — one row per finding (SEV-001..SEV-028) with severity, status (🟢 Closed / 🟡 Deferred / 🠠 Blocked / 🔵 Won't fix), the PR that closed it (or rationale if intentionally left open), and a one-line technical note. Summary as of this commit: - 25 of 28 findings 🟢 Closed (all Critical + High closed) - 1 🠠 Blocked (SEV-012 bankrun-in-CI, upstream-blocked on mpl-core) - 1 🟡 Deferred (SEV-026 cascade duplication, Fase 5 cleanup) - 1 🔵 Won't fix (SEV-018 settle_default pause bypass, design-correct) Mainnet-blocker status: zero open Critical or High findings. The remaining 3 non-closed entries are coverage-gap / maintenance / design-intentional, none with fund-loss shape. Includes disclosure timeline + methodology notes documenting the "pattern fingerprinting" sweep that followed SEV-002 / SEV-023 (see companion docs/security/constants-audit-2026-05.md from PR #340). Wired into docs/security/README.md as the new §3 of the reviewer reading order; downstream section numbers shifted +1. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm * chore(format): prettier --write on SEV tracker + README CI's prettier-check job rejected the previous commit; running prettier --write makes both files conform to .prettierrc.json. No content change — pure whitespace/wrapping normalization. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm * fix(security): release_escrow cumulative-paid recurrence (SEV-029 High, regression of SEV-016) (#342) Adevar Labs re-audit (2026-05-W3) identified SEV-029 as a regression introduced by the SEV-016 partial-release fix in PR #334. The fix preserved a non-advancing `last_released_checkpoint` on partial pay so the caller could "re-call when the vault refills" — but the vesting math kept recomputing `delta_target = releasable_delta(last_chk, new_chk)`, so the re-call at the same checkpoint returned a fresh full delta instead of the unpaid remainder. Reproducer (stake=1000, cycles=24): - call 1 chk=5 vault=100: delta_target=208 (cumulative_vested(5)-0) delta=min(208,100)=100, chk NOT advanced - call 2 chk=5 vault=200: delta_target=208 (re-computed from same un-advanced chk=0), delta=min(208,200)=200 - member receives 100+200=300; entitlement at chk=5 is 208. - OVERPAY 92 USDC drained from the shared escrow_vault, repeatable per cycle, observable via the SEV-016 "partial" msg! log. Fix shape — math rewrite, no new state field. Use the existing invariant `cumulative_paid = stake_deposited - escrow_balance` (escrow_balance starts at stake and is only decremented by release_escrow on non-defaulted members — settle_default cannot touch a non-defaulted member's escrow_balance because the `!member.defaulted` constraint bars defaulted callers entirely). total_due = cumulative_vested(stake, checkpoint, cycles) paid = stake - escrow_balance owed_now = total_due - paid delta = min(owed_now, vault_amount) // ALWAYS advance last_released_checkpoint (partial-pay encoded in // cumulative counter, not in checkpoint replay) Trace for the regression scenario (call 1 chk=5, call 2 chk=6): call 1: due=208 paid=0 owed=208 delta=100 → escrow 1000→900, chk=5 call 2: due=250 paid=100 owed=150 delta=150 → escrow 900→750, chk=6 total paid: 250 == cumulative_vested(stake, 6, 24) ✓ (no overpay) Coverage — per process feedback ("Critical/High needs negative test before merge"), this PR ships 4 regression unit tests + 2 proptest invariants in crates/math/src/escrow_vesting.rs: - sev_029_partial_then_full_does_not_overpay (auditor's scenario) - sev_029_chained_partials_never_overpay (compound partials) - sev_029_full_horizon_pays_exactly_principal (conservation) - sev_029_zero_vault_at_final_locks_remainder (locks not lost) - p_release_sequence_never_overpays_or_underpays (proptest invariant) - p_replay_same_checkpoint_is_rejected (idempotency) All 16 tests pass locally. cargo check --workspace clean. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm Co-authored-by: Claude <noreply@anthropic.com> * docs(security): tracker — add W3 re-audit findings (SEV-029..033) Update the public SEV tracker with the 2026-05-W3 re-audit results: - **SEV-029 (High, regression of SEV-016):** fund-leak overpay in the release_escrow partial-pay branch. Closed by PR #342 with 4 unit tests + 2 proptest invariants per the new "negative test before merge" gate. - **SEV-030 (Low, Open Fase 5):** admin cooldown only covers SCHEMA_PAYMENT; extending to LATE/DEFAULT scheduled. SEV-027 status updated to 🟡 Partial. - **SEV-031 (Low, Open Fase 5):** create_pool lacks runtime viability check — SEV-025 fixed the defaults but not custom pools. - **SEV-032 (Info, Acknowledged):** ReputationConfig padding exhausted by SEV-021. Future field additions require migration. Design constraint, documented; no action. - **SEV-033 (Low, Open Fase 5):** webhook auth fails open when env unset. Indexer fix scheduled. Also: - Updated SEV-016 status to "🟢 Closed (regressed → SEV-029)" with cross-link to #334 → #342. - Updated SEV-027 to "🟡 Partial (extended by SEV-030)". - Expanded status legend with Partial / Open / Acknowledged labels. - Updated summary table totals (28 → 33) and severity counts. - Added W3 re-audit deltas section + process recommendation ("Critical/High needs negative regression test before merge"). - Added W3 entries to the disclosure timeline. Mainnet-blocker status unchanged: all Critical / High (8/8) 🟢 Closed. Remaining Fase 5 items are Low-severity defense-in-depth. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm --------- Co-authored-by: Claude <noreply@anthropic.com>
5 tasks
alrimarleskovar
added a commit
that referenced
this pull request
May 15, 2026
…4 High — regression of SEV-029) (#349) The W4 internal pre-audit pass caught a regression-of-regression in my SEV-029 fix (PR #342). The fix used: let total_already_paid = stake_deposited - escrow_balance; and the accompanying comment claimed: "escrow_balance starts at stake_deposited and is ONLY decremented by release_escrow on non-defaulted members." **That claim was false.** `contribute()` INCREMENTS escrow_balance by the per-cycle escrow_deposit (25% of installment by default) at every contribution. So in the natural lifecycle (contribute → release → contribute → release), escrow_balance oscillates above stake_deposited and `saturating_sub` returns 0 even when releases already happened. Auditor trace (stake=750, cycles=3, installment=1000, escrow_bps=25%): c0 contribute(+250): esc=1000 release(chk=1): due=250, paid_derived=sat_sub(750,1000)=0, delta=250 (correct first call) c1 contribute(+250): esc=1000 release(chk=2): due=500, paid_derived=0, delta=500 (should be 250; OVERPAY 250) c2 contribute(+250): esc=1000 release(chk=3): due=750, paid_derived=0, delta=750 (should be 250; OVERPAY 500) Total received: 1500 vs stake 750 — 100% leak from shared vault. **Why the SEV-029 unit + proptest suite didn't catch it:** the test simulator (`simulate_release_sequence` in crates/math/src/ escrow_vesting.rs) tracked `cumulative_paid` as an independent u64 counter. That structure proves the *abstract* conservation property (sum of releases ≤ principal) but does NOT mirror the on-chain derivation, which depends on state mutated by contribute(). The simulator never modeled contribute() between releases. The tests proved the SIMULATOR was correct, not the on-chain code. **Fix:** the correct derivation uses two existing monotonic fields on Member (state/member.rs:27-31) — `stake_deposited_initial` and `total_escrow_deposited`: let ever_deposited = stake_deposited_initial + total_escrow_deposited; let total_already_paid = ever_deposited - escrow_balance; let delta_target = cumulative_vested(stake_initial, chk, total) - total_already_paid; For non-defaulted members (the only callers of release_escrow), this holds because: - stake_deposited_initial is set at join_pool, never mutated - total_escrow_deposited is monotonic (incremented by contribute) - escrow_balance is only touched by member's own contribute (+) and member's own release_escrow (-) — settle_default cannot reach a non-defaulted member's escrow_balance No new state field needed. **Test methodology fix:** added a NEW `LifecycleState` simulator in `escrow_vesting.rs` that models the FULL on-chain state shape (stake_deposited_initial, total_escrow_deposited, escrow_balance, last_released_checkpoint) AND `contribute()` between releases. The auditor's exact scenario is now a regression test (`sev_034_auditor_scenario_no_overpay`). 5 new SEV-034 tests: - sev_034_auditor_scenario_no_overpay (the disclosed trace) - sev_034_realistic_pool_no_overpay (24 cycles, real installment) - sev_034_partial_pay_still_works (compose SEV-016 + lifecycle) - sev_034_no_contribute_calls_still_work (degenerate sanity) - sev_034_replay_same_checkpoint_blocked (idempotency) All 21 escrow_vesting tests pass. The original SEV-029 tests still pass against their (insufficient) simulator — kept with a methodological-note header so the history of the gap is preserved. **Stronger process rule documented:** Critical / High fixes need integration-level tests (bankrun when #319 unblocks, anchor ts-mocha against localnet as the bridge). Pure-math simulators prove function properties, NOT on-chain behavior. Tracker (docs/security/internal-audit-findings.md) updated: - SEV-029 status: 🟢 Closed → 🟡 Closed-then-regressed (→ SEV-034) - SEV-034 new row (🟢 Closed, references this PR) - SEV-016 status chain: → SEV-029 → SEV-034 (two regressions of the original partial-pay fix) - W4 methodology added to the intro block - Severity totals: 33 → 34 (1 new High); closed 30 → 31 https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm Co-authored-by: Claude <noreply@anthropic.com>
alrimarleskovar
added a commit
that referenced
this pull request
May 15, 2026
…→31 closed, 8/8→9/9 Critical/High (#353) Auditor (Pass 6 evaluation) flagged the TL;DR row of audit-readiness.md claimed "Critical / High: 8/8 closed" while the severity table 20 lines below showed 2 Critical + 7 High = 9. The inconsistency came from a stale W3-era count that never got refreshed when SEV-034 was added in W4. Reviewing further found stale numbers in 5 docs total — all touched in the same direction. This PR brings every audit-doc claim to the post-W4 state. **Stale → corrected:** - "33 findings" → "34 findings" (W4 added SEV-034) - "30 🟢 closed" / "30 of 33" → "31 🟢 closed" / "31 of 34" - "8/8 closed (Critical/High)" → "9/9 closed (2 Critical + 7 High)" - "8 of 8" → "9 of 9" - "3-pass red-team" → "4-pass red-team" (W1/W2/W3/W4) - "2 Critical, 6 High, 9 Medium..." → "2 Critical, 7 High, 9 Medium..." - "270+ tests" → "280+ tests" (incorporates SEV-029/034 unit/proptest + bankrun integration spec from PRs #342, #345, #349, #350, #351) - "PRs #326..#347" → "PRs #326..#351" (extends through the SEV-034 fix chain and the math-crate refactor) **Files touched:** - README.md — status badge, "Internal pre-audit" intro paragraph, severity table, "Why this matters" footer, status-table row 11 - MAINNET_READINESS.md — "Today" stat line, §1.6 + §1.7 entries - AUDIT_SCOPE.md — companion-doc list, "Pre-audit state" paragraph - docs/security/audit-readiness.md — TL;DR "Critical/High fixes" row - docs/security/internal-audit-findings.md — mainnet-blocker status note **Notes on the count semantics:** The "Critical / High: 9/9 closed" count treats the SEV-029 → SEV-034 chain as **2 separate High findings** (which is how the tracker catalogs them — SEV-029 has its own row marked "🟡 Closed-then-regressed (→ SEV-034)" and SEV-034 has its own row marked "🟢 Closed"). The severity totals row at the bottom of the tracker has always counted them this way (Total High = 7); the inconsistency was only in the prose claims. Auditor's exact recommendation: "trocar para 9/9 🟢 closed para consistência com a tabela de severity. Ou se quiserem manter o framing de 'SEV-029 chain conta como 1', então a tabela High deveria ser 6/7 closed." We took the former: 9/9 matches the severity table. prettier --check clean. https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm Co-authored-by: Claude <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression callout
SEV-029 is a regression introduced by my SEV-016 partial-release fix in PR #334. Surfaced by Adevar Labs in the 2026-05-W3 re-audit. This PR closes it.
The bug
The SEV-016 fix preserved a non-advancing
last_released_checkpointon partial pay so the caller could "re-call when the vault refills." But the vesting math kept recomputingdelta_target = releasable_delta(last_chk, new_chk), so the re-call at the same checkpoint returned a fresh full delta instead of the unpaid remainder.Reproducer (stake=1000, cycles=24):
Member receives 100 + 200 = 300. Entitlement at chk=5: 208. Overpay 92 USDC drained from the shared escrow_vault. Repeatable per cycle. Observable via the SEV-016 "partial"
msg!log.Fix shape — math rewrite, no new state field
Use the existing invariant
cumulative_paid = stake_deposited - escrow_balance(holds becauseescrow_balanceis only decremented byrelease_escrowon non-defaulted members;settle_defaultcannot touch a non-defaulted member'sescrow_balancebecause the!member.defaultedconstraint bars defaulted callers).Trace of the regression scenario, post-fix (call 1 chk=5 partial, call 2 chk=6 full):
Total paid: 250 ==
cumulative_vested(stake, 6, 24)✓ — no overpay. Compounds correctly under chained partials.Coverage — negative tests BEFORE merge (per process feedback)
Per the auditor's process recommendation ("Critical/High needs negative test before merge"), this PR ships 4 regression unit tests + 2 proptest invariants in
crates/math/src/escrow_vesting.rs:sev_029_partial_then_full_does_not_overpay— the auditor's stated scenario, pre-fix would have assertedtotal == 250, actual was 308sev_029_chained_partials_never_overpay— compound partials across 4 checkpoints with varying vault availabilitysev_029_full_horizon_pays_exactly_principal— conservation property across the full vesting horizonsev_029_zero_vault_at_final_locks_remainder— empty vault returnsEscrowNothingToRelease, the remainder stays claimablep_release_sequence_never_overpays_or_underpays(proptest) —cumulative_paid == cumulative_vested(last_chk)for arbitrary (principal, total_checkpoints) pairs and a deterministic partial-pay sequencep_replay_same_checkpoint_is_rejected(proptest) — checkpoint-strictly-increasing guard closes the original regression vectorAll 16 tests pass locally.
cargo check --workspaceclean.Mainnet risk framing
escrow_vault).vault_amountandmember.escrow_balance, observable inmsg!logs, repeatable per cycle but cap isstake_depositedper member.Test plan
cargo test -p roundfi-math --lib escrow_vesting— 16 passedcargo check --workspace— cleanCompanion follow-ups (not in this PR)
The 2026-05-W3 re-audit also surfaced SEV-030..SEV-033 (Low + Info). Sequenced as a Fase 5 batch after this PR merges:
MIN_ADMIN_ATTEST_COOLDOWN_SECSto all schemas (currently onlySCHEMA_PAYMENT)create_pool(custom pools can still be inviable)ReputationConfigpadding exhausted — design constraint, doc-onlyTracker doc (PR #341) is held pending this PR so SEV-029 and the SEV-016 status correction land together.
https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm
Generated by Claude Code