Skip to content

fix(cketh): recover a signature against the key that made it - #11254

Draft
gregorydemay wants to merge 1 commit into
greg/DEFI-2926-sweeper-send-taskfrom
greg/DEFI-2926-recover-with-signing-key
Draft

fix(cketh): recover a signature against the key that made it#11254
gregorydemay wants to merge 1 commit into
greg/DEFI-2926-sweeper-send-taskfrom
greg/DEFI-2926-recover-with-signing-key

Conversation

@gregorydemay

@gregorydemay gregorydemay commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

DEFI-2926, first layer of the stack that drives the sweeper pipeline from the balance-sweep queue. Stacked on #11237.

Why

compute_recovery_id recovers a signature's parity bit against lazy_call_ecdsa_public_key() — the minter's master key — while sign and Authorization::sign sign under a caller-supplied derivation path. Recovery only ever succeeds against the key that produced the signature, so signing under anything but the empty main path finds no parity at all: try_recovery_from_digest returns an error and the unwrap_or_else traps. The debug_assert! above it fails first in debug builds.

Nothing calls it with a non-empty path yet, so this is latent today. It stops being latent with the first EIP-7702 authorization, which is signed by a deposit address' own key by definition — and a tuple that does not ecrecover to that address is skipped silently by the protocol, leaving the batch's inner call to hit a code-less address and revert the whole sweep.

What

Recovery now derives the master key along the same path before recovering. The derivation is shared with address derivation rather than duplicated: deposit_address::derive_public_key becomes the single place a subkey is computed, and address derivation is that key's address. The key a signature is recovered against is therefore, by construction, the key whose address it belongs to.

The signing functions now take the path as Vec<ByteBuf> — the form MAIN_DERIVATION_PATH, sweeper_derivation_path() and deposit_derivation_path() already produce — and build the management-canister DerivationPath themselves. That is the structural half of the fix: one owner for both uses of the path, so the signing key and the recovery key cannot drift apart again. Call sites lose a wrapper.

An empty path derives to the master key, so the main address and every withdrawal are unaffected.

Tests

Three pure unit tests, no canister involved. The one that pins the bug signs a digest with a key derived along a deposit path and asserts recovery succeeds against the derived key and fails against the master key, so it demonstrates the trap rather than asserting the new code agrees with itself.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes recovery-ID computation to use the same derived ECDSA key that produced each signature.

Changes:

  • Shares public-key derivation with deposit-address derivation.
  • Passes raw derivation paths through transaction and authorization signing.
  • Adds unit coverage for derived-key recovery.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
withdraw.rs Passes the main derivation path directly.
tx/signed.rs Uses one path for signing and recovery.
tx/mod.rs Recovers signatures against the derived key.
tx/eip_7702.rs Applies derived-key recovery to authorizations.
sweep.rs Passes the sweeper path directly.
deposit_address/tests.rs Tests key derivation and signature recovery.
deposit_address/mod.rs Extracts shared public-key derivation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gregorydemay
gregorydemay force-pushed the greg/DEFI-2926-recover-with-signing-key branch from 2082fa4 to 253fa99 Compare August 21, 2026 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants