feat(covenantsigner): wallet-signed EIP-712 approvals + cooperative REDEEM/RENEW - #4169
feat(covenantsigner): wallet-signed EIP-712 approvals + cooperative REDEEM/RENEW#4169piotr-roslaniec wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Security-review follow-ups (deferred from this PR)The hardening findings addressed in 1. REDEEM/RENEW carry no freshness/authority bound — deferred (needs decision)MIGRATION is gated by an ed25519 Why deferred: the "cryptographically bind and enforce signer-approval expiry" feature was reverted twice recently ( Options: add an expiry field to the approval payload; require a parallel REDEEM/RENEW plan-quote; or explicitly document that epoch-scoped UTXO binding satisfies the threat model and why. 2.
|
|
2 issues found, both fixed and pushed. Fixed: REDEEM/RENEW requests unconditionally rejected when Fixed: wallet-signed (ETH-identity) approval accepted at Submit is rejected at every subsequent Poll (Major) Both fixes are covered by new regression tests (added to No unresolved findings. |
Silent regression: #4169 refactored the migration-destination output logic into a new covenantDestinationOutputScript helper whose MIGRATION case returns the raw deposit script directly—in lines 776-780, there's no P2WSH wrap. #4126's audited fix (TOB-TBTCACEXT-2) wraps that same destination in payToWitnessScriptHash at line 854. #4169 has zero calls to payToWitnessScriptHash anywhere in pkg/tbtc — so a naive merge that keeps #4169's helper as-is would silently undo the audited fix.
db4f6bf to
7a556f3
Compare
b5e1166 to
d1a909a
Compare
…-signed ETH leg (v2)
Enable wallet-signed (eth_signTypedData_v4) covenant artifact approvals by
domain-wrapping the approval digest and binding the depositor approval to a
pinned Ethereum identity.
- Wrap the digest: keccak256(0x1901 ‖ domainSeparator ‖ structHash) with an
off-chain, salt-based EIP712Domain(name,version,chainId,salt). chainId+salt are
configured on the signer (Config.EIP712ChainId/EIP712Salt) and threaded through
validation and the tBTC engine so recomputed digests stay consistent.
- Verify the depositor approval against an optional pinned DepositorTrustRoot
ethAddress via ecrecover-and-compare (65-byte sig, v in {27,28}/{0,1}, low-S
enforced); fall back to the secp256k1 script key when unset. Custodian
verification is unchanged.
- Hard cut approvalVersion to 2 (v1 rejected).
- Regenerate approval vectors to _v2; add a real-wallet signature vector whose
digest is cross-verified byte-identical against an independent EIP-712 encoder,
an ecrecover integration test (accept + reject), and a startup guard that fails
loudly when an ethAddress is pinned without a configured chainId.
Refs: keep-core prerequisite for vba-dashboard#172.
keep-core previously implemented only the MIGRATION covenant action. Add the cooperative REDEEM (pay out to a depositor-chosen address) and RENEW (re-lock into the next-epoch covenant) actions end to end. - Add a CovenantAction discriminator (MIGRATION/REDEEM/RENEW) to RouteSubmitRequest (empty defaults to MIGRATION) plus RedeemDestinationReservation and RenewDestinationReservation, alongside the existing migration destination. - Define the redeem/renew destination commitment schemas (SHA-256 over canonical JSON, mirroring the migration commitment) binding the covenant identity to the payout/next-covenant script hash and value. These are the normative schemas the client (covenant-manager / dashboard / verification kit) must mirror. - Dispatch destination validation on the action and reject destinations that do not belong to it; the migration plan-quote path stays MIGRATION-only. Bind the transaction plan's destinationValueSats to the committed output value so the signed output cannot diverge from what the depositor approved. - Branch the transaction builder on the action to pay the migration deposit script, the redeem payout script, or the renew next-covenant script; the input, anchor output, and threshold signing are shared unchanged. Tests cover the commitment schemas, action-dispatched validation (accept/reject), and assert the signed transaction pays exactly the committed redeem/renew output. Refs: keep-core prerequisite for vba-dashboard#172.
…new actions trustRootLookupScope read the scope network only from MigrationDestination, so a wallet-signed REDEEM/RENEW request (which carries a redeem/renew destination, not a migration one) resolved an empty network and failed depositor trust-root matching. Read the network from the destination for the request's action. Add the headline end-to-end test for vba-dashboard#172: a cooperative REDEEM whose depositor artifact approval is signed by a pinned ETH identity is accepted.
Address findings from the PR security/simplicity review: - Reject the zero address as a pinned depositor ethAddress so a misconfiguration cannot make ecrecover compare against 0x0. - Reject mixing ethAddress presence across network entries of the same (route, reserve) depositor trust root, closing a silent ETH->secp verification downgrade steered by the request-supplied network value. - Fail closed on unrecognized artifact-approval roles in the authenticity loop instead of silently skipping them. - Correct the defaultArtifactApprovalDomainSalt comment: cross-deployment separation comes from the unique reserve/vault in destinationCommitmentHash plus trust-root scoping, not from scriptTemplateId; recommend per-deployment eip712Salt overrides. - Drop the misleading "predate the field" backward-compatibility note on CovenantAction; v1 approvals are hard-rejected.
…TION action normalizeRouteSubmitRequest called normalizeMigrationPlanQuote unconditionally, unlike validateCommonRequest which already gates the same call on request.ResolvedAction() == CovenantActionMigration. Since REDEEM and RENEW requests carry no migrationPlanQuote field, any deployment configuring WithMigrationPlanQuoteTrustRoots (a normal production setup for a deployment that also verifies MIGRATION plan quotes) rejected every REDEEM and RENEW submission at Submit, regardless of how well-formed the request otherwise was. Apply the same action gate in normalizeRouteSubmitRequest so the plan quote is only normalized for MIGRATION, defaulting to nil for REDEEM/RENEW. Add coverage submitting REDEEM and RENEW requests against a service configured with migrationPlanQuoteTrustRoots, asserting acceptance.
…ation Poll re-validates the resubmitted request via validateCommonRequest with policyIndependentDigest set, which skips depositor trust-root resolution entirely (and Poll never even passed depositorTrustRoots in). For the secp256k1 depositor key this was harmless: Submit already enforces scriptTemplate.depositorPublicKey == trust root, so the value embedded in the resubmitted request is equivalent. The wallet-signed ETH identity has no such request-embedded equivalent, so Poll always fell back to verifying the ETH-style ecrecover signature against the secp256k1 script key instead, which can never succeed. A depositor approval accepted at Submit was therefore rejected at every subsequent Poll, making the wallet-signed approval flow unusable end to end. Resolve the depositor's ETH address once at submit time and persist it on the job record. Poll now looks up that pinned value and threads it through its re-validation instead of trying to re-derive it from live depositorTrustRoots config, keeping the policy-independence Poll relies on intact. Added a submit-then-poll regression test for the ETH-signed self_v1 path.
…o the remediation base Replaying the EIP-712 ETH-leg work onto the Trail of Bits remediation base widens two signatures that the base's own tests still called with the old arity, and moves the request digest: the certificate contract the base introduced (version 2 plus a required endBlock) is part of the normalized request the digest covers. Adapt the base's newCovenantSignerEngine and normalizeSignerApprovalCertificate call sites, and regenerate the three canonical vectors from the production digest code. The canonical requests themselves are unchanged; only the derived request digests move. Add a guarded in-package generator so the next protocol-version change can refresh the fixture reproducibly instead of by hand-edited hashes. Without UPDATE_GOLDEN it verifies the checked-in digests, so a stale fixture fails loudly.
The live-only wallet-state gate predates the cooperative actions, so its rationale was written as a migration-only argument even though REDEEM and RENEW now flow through the same check. Nothing asserted that the two new actions are in fact held to that rule. Cover every non-live state for both actions, and restate the helper's rationale in action-neutral terms: a signer approval certificate binds wallet identity, members hash, and threshold, but nothing about wallet state, so a certificate issued while a wallet was live stays replayable after the protocol has deauthorized it. Widening any cell needs an explicit decision about why that reuse is safe, which this matrix now forces to be deliberate.
7a556f3 to
41f4d9e
Compare
…4178) ## What Adds five regression tests closing genuinely-missing defensive-property coverage for the covenant signer. Stacked on `feat/psbt-covenant-final-project-pr` (#3882); tests only, no production changes. ## Why A coverage review of the signer's defensive properties (auth, body cap, timeouts, replay, reorg guard, approval-certificate validation) started from a ~27-item wishlist but found that **~22 already exist** in the suite. Rather than duplicate them, this PR adds only the handful that were actually absent, plus one boundary complement. ## Tests added **`pkg/tbtc/signer_approval_certificate_test.go`** - `TestVerifySignerApprovalCertificateRejectsUnsupportedVersion` — the `certificateVersion == 1` hard gate (`signer_approval_certificate.go:230`) had no test; rejects both `0` and `2`. - `TestVerifySignerApprovalCertificateRejectsUnsupportedSignatureAlgorithm` — the `tecdsa-secp256k1` algorithm gate (`:233`) had no test. **`pkg/tbtc/covenant_signer_test.go`** - `TestEnsureActiveOutpointFinalityPropagatesProviderError` — the finality guard's error path (`covenant_signer.go:614`) was untested; existing tests only cover known confirmation counts above/below the threshold. Asserts fail-closed when confirmations cannot be determined. **`pkg/covenantsigner/server_test.go`** - `TestServerAcceptsBodyExactlyAtLimit` — complements the existing `maxRequestBodyBytes+1` rejection in `TestServerBoundaryErrorMatrix` by pinning the inclusive boundary (a body of exactly `maxRequestBodyBytes` must pass the size guard). **`pkg/covenantsigner/store_lock_test.go`** - `TestNewServiceReleasesLockOnInitFailure` — asserts `NewService` releases the store file lock when a later init step fails (`service.go:135-142` defer), so a signer that failed to start once can restart against the same data dir. Only the store-level `Close()` release was tested. Dropped from the original list as **already covered**: the non-loopback-without-token startup guard (already asserted inside `TestInitializeRejectsInvalidOrUnavailablePort`), Poll expiry checks, confirmation thresholds/boundaries, dedup, lock contention, digest/signer-set/high-S/DER cert rejections, trust-root pinning, quote expiry, oversized-body, and healthz-exempt. ## Verification - `go test ./pkg/tbtc/ ./pkg/covenantsigner/` — pass. - `go test -race ./pkg/covenantsigner/` — pass. - `gofmt -l` clean, `go vet` clean. - Note: `go test -race ./pkg/tbtc/` trips a **pre-existing** data race in `TestSigningExecutor_SignBatch` (reproduces on this branch's base tip `af236a25a` with none of these changes; unrelated to the covenant signer). Flagging, not fixing here. ## Open questions for @lrsaturnino Two items surfaced during the review that need an owner decision; they gate two further tests not included here: 1. **Signer-approval expiry binding.** The three tip commits (`e572ea3a8` / `a882cf142` / `ccbc4c3ac`, "cryptographically bind + enforce signer-approval expiry") were reverted. `SignerApprovalCertificate.EndBlock` is still checked at Poll (`service.go:275-290`), but the ECDSA signature covers `approvalDigest` only — `EndBlock` is not inside the signed digest, so its value is caller-settable. Is the binding intended to re-land, or is Poll-time check-only the accepted design? (Determines whether we add a `TestEndBlockIsBoundToSignature`.) 2. **`chainId == 0` guard on the EIP-712 leg.** On the follow-up `feat/covenant-eip712-eth-leg` (#4169), `EIP712ChainID` is folded into the EIP-712 domain separator, but nothing rejects `EIP712ChainID == 0` at startup — a misconfig fails soft (no real `eth_signTypedData_v4` signature is ever produced under chainId 0, so verification just never matches). Intentional, or should we add a startup guard + test?
|
@lrsaturnino Not merging this yet because it's based on audit remediation branch and I don't want to pollute it |
2 participants
Summary
Enables wallet-signed (
eth_signTypedData_v4) covenant artifact approvals and addsthe cooperative REDEEM / RENEW action paths to the keep-core covenant signer.
This is the keep-core prerequisite (section A of the VBA EIP-712 change-list) that must land
before this covenant work goes live: today the signer verifies a bare EIP-712 struct
hash (no
0x1901, no domain separator) against the depositor's secp256k1 key, whichno connected ETH wallet can produce. This PR domain-wraps the digest, binds the depositor
approval to a pinned ETH identity via
ecrecover, and builds out the redeem/renewtransaction path.
Stacked on top of #3882 (
feat/psbt-covenant-final-project-pr) — the only branch carryingpkg/covenantsigner/. Tracked by tlabs-xyz/vba-dashboard#172:https://github.com/tlabs-xyz/vba-dashboard/issues/172
What changed
A1 - Domain-wrap the digest (EIP-712 v2, off-chain / salt-based).
The approval digest is now
keccak256(0x1901 ‖ domainSeparator ‖ structHash)with domainEIP712Domain(string name,string version,uint256 chainId,bytes32 salt)(noverifyingContract, since verification is off-chain).name = "tBTC Covenant Artifact Approval",version = "2",chainId+saltfrom signer config (default salt =keccak256("tBTC Covenant Artifact Approval Domain v2")). Domain params are threaded fromconfig → validation options → digest, including the exported
ComputeArtifactApprovalDigestused by
pkg/tbtc.A2 - ETH identity binding. Optional
EthAddressonDepositorTrustRoot(keyed byroute/reserve/network). When pinned, the depositor approval is verified via
ecrecover-and-compare (65-byter‖s‖v,vin {27,28} or {0,1}, low-S enforced);otherwise it falls back to the secp256k1 script key. Custodian role stays secp256k1.
A3 - Hard cut to
approvalVersion = 2(v1 rejected). Nothing is deployed yet, so notransition window is needed.
A4 - Vectors. Regenerated the v2 approval vectors and added a real-wallet vector
whose digest is cross-verified byte-identical against the independent
eth_account(0.13.7) EIP-712 encoder — genuine
eth_signTypedData_v4compatibility, not aself-referential check. A startup guard rejects a config that pins depositor ETH addresses
while
eip712ChainId == 0.A5 - Cooperative REDEEM / RENEW path. New
CovenantActiondiscriminator(MIGRATION / REDEEM / RENEW; empty defaults to MIGRATION),
RedeemDestinationReservation/RenewDestinationReservation, redeem/renew commitment schemas (SHA-256 over canonical JSON,mirroring migration), action-dispatched validation, and an action branch in
buildCovenantTransactionBuilder. The built transaction is proven to pay exactly thecommitted output — the transaction plan's
destinationValueSatsis bound to the committedoutput value.
Commitment schemas (normative contract for the client repos to mirror):
{reserve, epoch, route, revealer, vault, network, outputScriptHash, outputValueSats}{reserve, epoch, route, revealer, vault, network, nextCovenantScriptHash, nextMaturityHeight, outputValueSats}Client-side contract (must be mirrored byte-for-byte)
keccak256(0x1901 ‖ domainSeparator ‖ structHash).ArtifactApproval(uint8 approvalVersion,bytes32 route,bytes32 scriptTemplateId,bytes32 destinationCommitmentHash,bytes32 planCommitmentHash),approvalVersion = 2.route/scriptTemplateIdarebytes32 = keccak256(identifier string)— the dApp mustpre-hash them; a wallet encoder given the raw bytes32 reproduces the digest.
Verification
go test ./pkg/covenantsigner/...— 156 passedgo test ./pkg/tbtc/...— 275 passed (full suite)go build ./...— green;gofmtcleanaccepted through
Service.Submit, and the signed transaction pays exactly the committedredeem/renew output (scriptPubKey + value).
Out of scope
The section-B/C client sync (covenant-manager, dashboard
engine.ts, verification kit,co-sign service, dual-connection wallet UX) lives in other repos. The pre-existing migration
depositScript-as-output gap is flagged, not fixed here — the redeem/renew paths sidestep itby carrying an explicit output scriptPubKey.