feat(accounts): support multisig key authorizations - #7236
Conversation
✅ Changelog found on PR. |
274e6cb to
a724637
Compare
a724637 to
e9cbf83
Compare
e9cbf83 to
d6b533d
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 6437/10593 lines (60.77%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6438/10795 lines (59.64%) |
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change safely widens signed key authorizations to TempoSignature, but the stacked native-multisig support contains one verified medium-severity denial-of-service risk.
🚨 [SECURITY] Multisig authorization-list signatures bypass signature-byte gas metering
Severity: Medium
Location: crates/revm/src/signature_gas.rs:44 and crates/revm/src/handler.rs:1833-1837 (the root-cause line and missing authorization-list guard are not changed lines in this PR)
tempo_signature_verification_gas() charges zero for TempoSignature::Multisig on the assumption that native multisig is rejected before intrinsic-gas calculation. validate_env does not reject multisig in tempo_authorization_list; recovery instead marks the entry invalid and authorization application skips it without rejecting the transaction. An attacker can therefore attach roughly 128 KiB of nested multisig/WebAuthn data to a mineable transaction without paying signature-byte gas, amplifying block bandwidth, decoding, propagation, and storage costs.
Recommended Fix: Reject multisig in the tempo_authorization_list validation loop before activation. When it becomes valid there, replace the zero-gas arm with TIP-1061's size/owner-based formula in the same fork.
Reviewer Callouts
- ⚡ Future consensus panic (
crates/revm/src/handler.rs:1373-1378): The new.expect()depends on a validation guard that T11 is expected to relax. Replace it with verified multisig handling or a typed error before activation. - ⚡ Unauthenticated multisig signer (
crates/primitives/src/transaction/key_authorization.rs:445-456):recover_signer()caches a claimed multisig account without checking approvals or threshold, and the transaction pool consumes it. ReturnRecoveryErroruntil stateful verification is wired in. - ⚡ Permanent-invalid encoding classified as transient (
crates/revm/src/error.rs:329-334): Keychain-encoded key authorizations are deterministic invalid forms butKeychainValidationFailedis classified as non-bad/state-dependent.
d6b533d to
6ecd1a2
Compare
6ecd1a2 to
5a29cf1
Compare
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change preserves primitive wire compatibility and rejects keychain and native-multisig key-authorization signatures during current validation. One defense-in-depth issue remains: the widened primitive API itself no longer guarantees that signer recovery authenticates the returned root account.
Reviewer Callouts
- ⚡ T11 activation path: Before removing the current multisig rejection, require stateful owner/threshold verification with the key-authorization digest and remove the block-execution
expect(). - ⚡ Mutable cached inputs:
authorizationandsignatureremain public while the recovered signer is cached in aOnceLock; in-place mutation can leave stale signer state. - ⚡ Pre-T11 decoding: Confirm that newly decodable but permanently rejected variants intentionally avoid bad-transaction peer penalties and caching.
298e910 to
7602c6d
Compare
7602c6d to
fd7322d
Compare
|
cyclops audit fast note="this belongs to the GH stack #7243 and the TIP 1061 is specified in the PR 7242. any bug not introduced by this PR should be clearly marked" |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change safely preserves primitive key-authorization behavior and keeps native multisig execution disabled, but one low-severity defense-in-depth issue should be fixed before activation.
Reviewer Callouts
- ⚡ Native multisig activation path: Re-check
validate_against_state_and_deduct_callerwhen replacingrecover_signer()withrecover_authorizing_account(); the latter returns a claimed multisig account without verifying its owner quorum.
fd7322d to
45938c2
Compare
45938c2 to
6baa8af
Compare
2 participants
Expands signed key authorizations to
TempoSignaturewhile preserving primitive wire compatibility and wallet-store decoding. Multisig execution remains inactive until the later activation layer.Stacked on #7234; extracted from #4069.