fix: agent-vault self-transfer bug, admin idempotency/audit gaps, CI supply-chain hardening - #475
Merged
ExcelDsigN-tech merged 2 commits intoAug 25, 2026
Conversation
…supply-chain hardening - contracts/agent_vault: reject self-transfers in transfer_float. The function read both sides of a transfer into separate structs and wrote them back sequentially; when from == to the two writes aliased the same storage slot and the second silently clobbered the first, minting `amount` of float for free on every self-transfer with no matching mint/collateral movement. Adds a regression test. - backend/src/routes/adminRoutes.ts: apply the existing idempotencyMiddleware and auditLog middleware to the admin endpoints that mutate loan/dispute state (build-reject, dispute resolve/reject, check-defaults) but previously lacked them, so retried admin actions replay a cached response instead of re-executing, and every privileged state change is captured in the audit trail. - .github/workflows/ci.yml, security-gates.yml: pin third-party Actions (gitleaks-action, trivy-action, paths-filter, rust-cache, install-action) to commit SHAs instead of mutable tags/branches (trivy-action was on `@master`), pin the semgrep container off `:latest`, and add a signed SLSA build-provenance attestation (actions/attest-build-provenance, Sigstore keyless via OIDC) for the compiled contract wasm artifacts. Closes ExcelDsigN-tech#413 Closes ExcelDsigN-tech#414 Closes ExcelDsigN-tech#466 Closes ExcelDsigN-tech#467
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@JamesVictor-O 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! 🚀 |
actions/attest-build-provenance needs an OIDC token, which GitHub does not issue to pull_request runs triggered from a fork — the step failed outright on this PR. Gate it to push events (post-merge on main), which is also the more correct place for provenance: it should attest what actually lands on main, not an unmerged fork build.
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.
Summary
Addresses all four issues currently assigned to me, scoped to concrete, verifiable changes against the actual code in this repo (rather than the infra described in the issue templates, which references tooling — Teleport/Boundary, external SLSA reusable workflows — that isn't part of this codebase).
#413 — Race condition in agent-to-agent float transfers
The issue describes DB-level locking, but agent float balances live in the
agent_vaultSoroban contract (contracts/agent_vault/src/lib.rs), not Postgres — Soroban executes each contract invocation atomically and sequentially, so there's no multi-threaded race there. Auditingtransfer_floatfor the equivalent on-chain hazard turned up a real bug: it readsfrom's andto's vaults into two separate structs, then writes each back. Whenfrom == to, both reads observe the same starting float and the secondwrite_vaultcall silently clobbers the first — netting the caller+amountfloat per call with no corresponding mint or collateral movement. That's an unbounded free-float mint, and it breaks the global solvency invariant the whole contract exists to enforce. Fixed by rejectingfrom == tobefore doing any state reads, with a regression test (test_transfer_float_rejects_self_transfer) verifying the float balance is unchanged after a rejected self-transfer.#414 — Idempotency keys for cash-in/out endpoints
transactionController.tsreferenced in the issue only exposes a read-onlyGET /metoday — there's no cash-in/out endpoint yet, and the repo already has a workingidempotencyMiddleware(Redis-backed, 24h TTL, replays cached response on duplicateIdempotency-Key) applied to loan/pool/remittance routes. It was missing from the admin endpoints that actually mutate loan/dispute financial state:build-reject, bothdisputes/:id/resolveroutes (API-key and JWT variants),disputes/:id/reject, andcheck-defaults. A retried request to any of these previously either re-executed the mutation or hit a misleading 404 ("already resolved") on the natural DB guard. Wired upidempotencyMiddlewareon all of them.#466 — Insider threat prevention (audit trail slice)
Full JIT access / session recording infra (Teleport, Boundary) is out of scope for a code PR here. The concrete, already-existing primitive in this repo is the
auditLogmiddleware (records actor, action, target, sanitized payload, IP, status toaudit_logs) — it was applied to some admin routes but not the dispute-resolution endpoints, which are exactly the privileged actions an insider-threat control needs visibility into (an admin can confirm or reverse a borrower's loan default). AppliedauditLogto all three dispute resolve/reject routes so every privileged financial admin action is now captured in the audit trail. Also scoped thecontractsCI job'sGITHUB_TOKENdown to least-privilege (contents: read,id-token: write,attestations: write) instead of the workflow default.#467 — Supply chain security for CI/CD (hermetic builds + provenance)
Two concrete, verifiable steps toward the DoD items that don't require standing up new infrastructure:
ci.yml/security-gates.ymlwere pinned to mutable tags/branches (aquasecurity/trivy-action@masterwas on a branch, not even a tag). Pinnedtrivy-action,gitleaks-action,dorny/paths-filter,Swatinem/rust-cache, andtaiki-e/install-actionto commit SHAs, and thesemgrep/semgrepcontainer off:latestto a specific release.actions/attest-build-provenanceto thecontractsjob, generating a signed (Sigstore keyless, via GitHub OIDC) in-toto SLSA provenance attestation for the compiled contract.wasmartifacts. Verifiable withgh attestation verify <file> --repo ExcelDsigN-tech/dukapay.Full SLSA Level 3 (two-party build approval, reusable SLSA workflows) and the insider-threat program (JIT access, anomaly detection, automated offboarding) remain open — this PR is a real, working slice of both, not a claim of full completion.
Test plan
cargo test -p agent_vault— 23/23 pass, including new self-transfer regression testcargo fmt --all -- --check/cargo clippy -p agent_vault --all-targets --all-features -- -D warnings— cleannpm run lint/npm run typecheck(backend) — clean (pre-existing warnings only, none in touched files)npx jest src/tests/idempotency.test.ts— 5/5 passsrc/__tests__/loanDispute.test.tsfails identically onmainbefore this change (pre-existing DB-mocking gap in that test file, unrelated to this PR)Closes #413
Closes #414
Closes #466
Closes #467