test(node): cover TIP-1061 multisig flows - #7241
Conversation
fb8f1f8 to
3165b5a
Compare
3165b5a to
a47745c
Compare
a47745c to
1d7c0e3
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 6457/11165 lines (57.83%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6458/11367 lines (56.81%) |
|
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
Cyclops Review — PR #7241
Summary
PR #7241 adds real-node integration coverage for the canonical TIP-1061 multisig examples and does not modify production code. The review found two low-severity, pre-existing gaps in the TIP-1061 implementation exercised by these tests: an order-dependent access-key invariant and incomplete transaction-pool revalidation after multisig initialization; neither finding received a separate verifier result.
Findings
🛡️ [DEFENSE-IN-DEPTH] Multisig access-key prohibition is order-dependent
Severity: Low
File: crates/precompiles/src/native_multisig/mod.rs:229
Summary: AccountKeychain::authorize_key_internal rejects an address only if it is already a multisig (crates/precompiles/src/account_keychain/mod.rs:271), but NativeMultisig::store_initial_config does not perform the reverse check. An attacker can therefore register a not-yet-bootstrapped derived multisig address as an access key and bootstrap it afterward, leaving state where the address is both a registered multisig and an active key for another account. Current signature validation prevents a demonstrated privilege escalation, but the state violates TIP-1061's hard invariant and is unsafe for future consumers that rely on key-registration state.
Recommended Fix: Track active key registrations by key_id and reject multisig bootstrap when the derived account is registered for another account, including an activation strategy for registrations created before T11. If reverse enforcement is intentionally unsupported, narrow the TIP invariant to explicitly permit this ordering instead.
⚠️ [ISSUE] Multisig initialization misses transactions gated by key_authorization.key_id
Severity: Low
File: crates/transaction-pool/src/transaction.rs:228
Summary: TempoPooledTransaction::multisig_accounts() indexes multisig signature accounts and nested owners, but omits key_authorization.key_id. Consensus validation rejects that key ID once it becomes a registered multisig (crates/revm/src/handler.rs:1133), while pool maintenance revalidates only transactions whose indexed accounts match a MultisigInitialized event (crates/transaction-pool/src/maintain.rs:293). A transaction admitted before its key ID is bootstrapped can consequently remain stale in the pool instead of being revalidated and evicted.
Recommended Fix: Include key_authorization.authorization.key_id in the registry-dependency set used by affects_multisig_transaction, and add a pool-maintenance test covering admission before MultisigInitialized followed by revalidation.
Reviewer Callouts
Reviewer Callouts
- ⚡ RPC simulation hints (
crates/node/src/rpc/mod.rs:388): Hint generation recursively loads owner configs and runs allocation-heavy selection before gas or affordability checks. A 48-owner-of-48-owner account can amplify unauthenticated simulation requests; review whether per-block caching or a bound based onsignature_countis needed. - ⚡ Registry-gating reads (
crates/precompiles/src/native_multisig/mod.rs:192):is_multisig_accountloads the complete stored config rather than only the header, adding up to twice the owner count in storage reads beyond TIP-1061's documented header-read model. Reconcile the implementation and gas specification. - ⚡ Keychain signature cache (
crates/primitives/src/transaction/tt_signature.rs:506):KeychainSignature::key_idis cached without keying the cache bysig_hash. No current multi-hash caller was identified, but an assertion or input-keyed cache would prevent future stale recovery results.
1d7c0e3 to
334ecfa
Compare
334ecfa to
db4d774
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
PR #7241 adds end-to-end TIP-1061 native-multisig coverage. No security or correctness defect was found in the changed test files; review of the underlying stacked implementation identified two unverified, low-severity issues that are not introduced by this PR.
🛡️ [DEFENSE-IN-DEPTH] Stateless multisig shape validation accepts disallowed keychain approvals
File: crates/primitives/src/transaction/multisig.rs:684 (not in this PR's diff)
MultisigSignature::validate_shape() applies the owner-signature size limit only through TempoSignature::as_primitive(), so decoded Keychain approvals bypass both the TIP-1061 Primitive-or-Multisig type restriction and the per-approval byte cap. Stateful authorization rejects keychain approvals before quorum, but stateless callers inherit a weaker contract and always-invalid transactions can carry oversized approvals.
Recommended Fix: Reject TempoSignature::Keychain in validate_shape() and enforce MAX_MULTISIG_OWNER_SIGNATURE_BYTES for primitive approvals.
⚠️ [ISSUE] Native-multisig RPC simulation hint construction performs superlinear work
File: crates/node/src/rpc/mod.rs:519 (not in this PR's diff)
RPC simulation recursively loads a hint for every owner before running a dynamic program whose transitions clone approval vectors. At supported owner and nesting limits, one unauthenticated simulation request can trigger millions of deep clones and thousands of state reads, creating avoidable public-RPC CPU amplification.
Recommended Fix: Memoize hints by account, restrict nested resolution to quorum candidates, and use predecessor indices instead of cloning approval vectors during DP transitions.
Reviewer Callouts
- ⚡ RPC simulation context boundary: Confirm every committing path constructs
ExecutionContext::Transaction; a future path using the simulation-valued default would skip quorum verification. - ⚡ Rejected-transaction CPU policy: Decide whether pool policy should account for funded multisig transactions that perform many P256/WebAuthn checks before a final invalid approval rejects them without gas charge or sender penalty.
- ⚡ End-to-end negative coverage: Add same-transaction
updateConfigand access-key owner-rotation rejection cases to cover the transient guards through the real handler.
5edc0f1 to
986ca13
Compare
986ca13 to
6503c45
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" |
6503c45 to
c1e8883
Compare
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #7241 adds real-node integration coverage for TIP-1061 and changes only test files. The test changes are well scoped, but the production paths they exercise retain two pre-existing defense-in-depth weaknesses on the current head. These production files are outside the PR diff, so the findings are body-only.
🛡️ [DEFENSE-IN-DEPTH] Multisig authorization fails open for a default or empty-encoded execution context
Severity: Medium
File: crates/revm/src/tx.rs:80
ExecutionContext::Simulation is the default for TempoTxEnv, and empty EIP-2718 bytes also select it (tx.rs:467-470). In T11 validation, this is the sole guard around owner-quorum verification (crates/revm/src/handler.rs:1326-1354, 1440-1451). A future consensus, replay, or tracing path that uses a default environment or lacks encoded bytes could execute a multisig transaction without checking owner membership, threshold, or digest binding. Current canonical paths explicitly use ExecutionContext::Transaction, so no live exploit path was established.
Recommended Fix: Make the secure transaction context the default or require explicit construction, stop inferring multisig simulation from empty encoding, and reject simulation contexts at the canonical block-execution boundary.
🛡️ [DEFENSE-IN-DEPTH] The per-transaction multisig bootstrap marker is not explicitly reset
Severity: Low
File: crates/revm/src/handler.rs:429
Transaction setup resets the keychain transaction key and seeds precompile origins, but does not clear NativeMultisig.bootstrapped_account. Revm currently clears transient storage at finalization, so this is fail-closed; journal reuse or a future lifecycle change could retain the marker and incorrectly reject a later legitimate configuration update.
Recommended Fix: Explicitly clear bootstrapped_account in seed_precompile_tx_context.
Reviewer Callouts
- ⚡ Canonical executor boundary: Confirm against pinned upstream Reth that block re-execution never constructs an AA
WithEncodedtransaction with empty bytes. - ⚡ EIP-7702 ordering: Pin authorization-list application versus caller-state validation ordering with a test.
- ⚡ Multisig gas accounting: Confirm that charging config-validation gas for both signatures when outer and key-authorization signatures name the same caller is intentional.
c1e8883 to
d6904cb
Compare
d6904cb to
7cd9e8b
Compare
7cd9e8b to
b1fdef0
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
Cyclops Review — PR #7241
Summary
PR #7241 adds real-node integration tests for TIP-1061 and changes no production code. No defect was found in the changed tests; after removing findings already covered by prior Cyclops reviews, one low-severity, pre-existing robustness issue remains in the underlying RPC simulation-hint implementation.
Findings
⚠️ [ISSUE] Simulation-hint selection panics on a zero threshold
Severity: Low
File: crates/node/src/rpc/mod.rs:558
Summary: select_native_multisig_simulation_approvals allocates each state row with threshold entries and then unconditionally writes states[0][0]. A zero-threshold config therefore causes an index-out-of-bounds panic. Both current callers validate the config first, so this is not presently reachable through RPC input, but the private helper's unchecked precondition makes a future caller or validation-order change a worker-panic footgun.
Recommended Fix: Reject config.threshold == 0 at the start of select_native_multisig_simulation_approvals before allocating or indexing the state table.
Reviewer Callouts
Reviewer Callouts
- ⚡ Nested quorum reachability: Config validation considers the top eight direct owner weights but cannot detect a nested owner cycle whose quorum depends on the parent account. Review whether initialization and updates should prevent or explicitly document configurations that can permanently make their own update quorum unreachable.
2 participants
Adds end-to-end TIP-1061 examples, RPC fill/sign/send coverage, and updated gas-estimation snapshots.
Stacked on #7240; completes the extraction from reference PR #4069.