Skip to content

fix(security): Fase 3-A small sweep — release_escrow partial-pay + nft_asset doc (SEV-016 + SEV-017)#334

Merged
alrimarleskovar merged 1 commit into
mainfrom
fix/fase3-a-sev-016-017-escrow-and-nft-doc
May 15, 2026
Merged

fix(security): Fase 3-A small sweep — release_escrow partial-pay + nft_asset doc (SEV-016 + SEV-017)#334
alrimarleskovar merged 1 commit into
mainfrom
fix/fase3-a-sev-016-017-escrow-and-nft-doc

Conversation

@alrimarleskovar
Copy link
Copy Markdown
Owner

Adevar Labs audit — Fase 3 PR-A (first of 3 Fase-3 PRs). Two XS-cost findings bundled.

SEV-016 Low — release_escrow DoS on shared-vault shortfall

escrow_vault is shared across pool members. A settle_default seizure right before a release_escrow can leave the vault holding less than the vesting math owes. Old code: require!(delta <= vault_amount) reverted the entire call → DoS until next contribute refills.

Fix:

// Cap to vault availability, log when partial
let delta = delta_target.min(vault_amount);
if delta < delta_target {
    msg!("release_escrow partial pool=X member=Y owed=N paid=M (vault shortfall)");
    // Do NOT advance last_released_checkpoint on partial — member
    // can re-call once vault refills and collect the remainder.
}

member.escrow_balance decrements by actual delta (not target), so bookkeeping stays consistent.

SEV-017 Informational — nft_asset Signer arbitrary

nft_asset is UncheckedAccount with signer = true. Anchor validates the signature but not freshness. Existing keypair → mpl-core CreateV2 fails at runtime (not fund-loss), but a careless caller could pass a long-lived wallet with display name "RoundFi Position #X" for UX confusion.

Fix: doc-only. sdk/src/actions.ts JoinPoolArgs.nftAsset JSDoc expanded with the audit finding + the explicit rule.

Validation

cargo check -p roundfi-core   # green
pnpm typecheck                 # green
pnpm lint                      # green

Fase 3 status

PR SEVs Status
This PR F3-A: 016 + 017 ✅ Open
⏳ next F3-B: 015 cancel_pending_listing
⏳ next F3-C: 014 indexer decoder + parity
(deferred) F3-D: 012 bankrun-in-CI Blocked on #319

After Fase 3 ships, only SEV-012 (bankrun-in-CI) remains, blocked upstream on the Agave 2.x migration.


Generated by Claude Code

…t_asset doc (SEV-016 + SEV-017)

Two Low/Informational findings bundled — both XS, neither touches
protocol invariants.

SEV-016 Low — release_escrow DoS on shared-vault shortfall
==========================================================

The escrow_vault ATA is shared across all pool members. A
`settle_default` seizure that runs immediately before a legitimate
`release_escrow` can leave the vault holding less than what the
vesting math owes the calling member. Before this fix, the guard
`require!(delta <= vault_amount, EscrowNothingToRelease)` would
revert the release entirely — DoS-ing the legitimate call until
another contribute refills the vault.

The invariant `sum(member.balances) <= vault_amount` holds in steady
state, so this is a transient ordering race rather than a real
accounting hole, but the auditor flagged it as a "rare DoS,
documented edge case."

Fix in programs/roundfi-core/src/instructions/release_escrow.rs:
  - Compute `delta_target` from the vesting math (renamed from `delta`).
  - Cap actual transfer at `delta = delta_target.min(vault_amount)`.
  - When the cap fires, log "release_escrow partial pool=X member=Y
    owed=N paid=M (vault shortfall)" so off-chain monitoring sees
    the rare partial-release path.
  - When `delta < delta_target`, do NOT advance
    `member.last_released_checkpoint` — the member can re-call
    release_escrow with the same args once the vault refills and
    collect the remainder. Otherwise the checkpoint advances and
    the unreleased portion is gone.
  - `member.escrow_balance` decrements by actual `delta` (not target),
    so the bookkeeping stays consistent with what was moved.

No new error variant. No state-size change. Failure path (delta_target
== 0, balance underflow) still hits the existing
EscrowNothingToRelease.

SEV-017 Informational — nft_asset Signer arbitrary
==================================================

The `nft_asset` account in `join_pool` is declared as UncheckedAccount
with `signer = true`. Anchor validates that the address signs, but
NOT that it's a freshly-generated keypair. If a caller passes an
existing keypair, mpl-core's CreateV2 CPI fails at runtime (account
already initialized) — so this isn't a fund-loss risk. But a
careless / malicious caller could pass a long-lived wallet keypair
with a display name like "RoundFi Position #X" and create UX
confusion.

Fix is purely doc — no on-chain change. The SDK's `JoinPoolArgs.nftAsset`
field gets an expanded JSDoc with the audit finding + the explicit
rule: "this keypair MUST be freshly generated and discarded after the
join_pool tx confirms. Never reuse this slot for a long-lived signer."

Validation
==========

  $ cargo check -p roundfi-core   # green (38 warnings, all
                                    pre-existing anchor-debug cfg)
  $ pnpm typecheck                 # green (workspace)
  $ pnpm lint                      # green

Closes Fase 3 PR-A of the Adevar Labs remediation plan.

https://claude.ai/code/session_01YapZy1Z5gzbV5EammBkSQm
@vercel
Copy link
Copy Markdown

vercel Bot commented May 15, 2026

Deployment failed with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/alrimarleskovars-projects?upgradeToPro=build-rate-limit

@alrimarleskovar alrimarleskovar merged commit 664a8e1 into main May 15, 2026
5 of 6 checks passed
alrimarleskovar pushed a commit that referenced this pull request May 15, 2026
The previous "second-pass" audit (ADEVAR_AUDIT_REPORT_PASS_2.md) was
executed against the original snapshot fbc931e — NOT against the
current main with 15 merged fix commits. The team correctly flagged
this. Root cause: failed to `git fetch origin main` before reading
files; the local branch was forked at the pre-fix commit so every
finding was "re-confirmed" against vulnerable code that no longer
existed upstream.

This corrected re-audit verifies against main HEAD e227d95:

✅ 16 findings FIXED (with explicit commit + PR references):
- SEV-001 → #326 (c_token_account ATA constraint)
- SEV-002 → #327 (GRACE_PERIOD_SECS 60s → 7d)
- SEV-003/004/005 → #329 (lp_share_bps to config, vaults_initialized
  flag, PoolStatus::Closed terminal state)
- SEV-006 → #331 (treasury USDC ATA validation)
- SEV-007/008 → #332 (level demotion on default + verified_at_attest)
- SEV-009/010/013 → #330 (webhook auth + B2B salt + salt entropy)
- SEV-011 → #333 (cargo-audit required, no || true)
- SEV-014 → #336 (decoder prefix fix)
- SEV-015 → #335 (cancel_pending_listing)
- SEV-016/017 → #334 (release_escrow partial-pay + nft_asset doc)
- SEV-019/020 → #328 (docs hardening)

❌ 10 findings STILL OPEN against main:
- SEV-012 (bankrun CI — blocked on Agave 2.x upstream)
- SEV-018 (informational, design intent)
- SEV-021/022 (reputation authority/pause asymmetry — High)
- SEV-023/024 (MIN_CYCLE_DURATION 60s, fee_bps_yield 100% cap)
- SEV-025/026/027/028 (pool solvency, cascade refactor, payment
  cooldown, refresh error handling)

Score updated: 6.0 (wrong) → 7.5/10 against actual main.
Recommendation changed: NÃO DEPLOY → DEPLOY EM CANARY COM RESSALVAS
(SEV-001..SEV-005 all closed; SEV-021/022 must close before
canary cap removal).

https://claude.ai/code/session_01CiaV9hd9oFqqr7m9ANgfit
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 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>
@alrimarleskovar alrimarleskovar deleted the fix/fase3-a-sev-016-017-escrow-and-nft-doc branch May 17, 2026 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants