feat(account-keychain): implement admin access keys - #4265
Conversation
✅ Changelog found on PR. |
f6c0b9a to
ee23eda
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/tempoxyz/tempo/blob/f6c0b9aa65966dc20ab2c05c1c1fdeec6df1c1ff/crates/revm/src/handler.rs#L1715-L1717
Limit key-type equality check to same-tx auth+use
With T6, access_key_addr != key_auth.key_id is now allowed for admin delegation, but this unconditional check still requires key_auth.key_type to equal the outer keychain signature type. That constraint was only valid for same-tx auth+use and now incorrectly rejects valid admin flows where an admin key authorizes a different key type (for example, a WebAuthn admin authorizing a secp256k1 key), which undermines TIP-1049 delegation behavior.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mattsse
left a comment
There was a problem hiding this comment.
Review findings
[P1] Admin-signed non-admin KeyAuthorizations are not account-bound
admin_account is only present when the authorization creates an admin key, while the T6 signer validation accepts any signer that is currently admin for tx.caller (validation path) and only enforces account binding when admin_account is present (admin-only binding check). This leaves admin-signed non-admin authorizations replayable across accounts where the same admin key is authorized. If an admin key is admin for accounts A and B, a signature intended to add a non-admin child key to A can also add that same child key to B. The TIP calls out account-bound digests for verifyAdmin, but the transaction authorization path needs the same account binding.
[P1] Admin delegation still runs same-tx auth+use handling
The fee precheck is documented as same-tx auth+use, but it is not gated on access_key_addr == key_auth.key_id (pre-fee limit check). Later, every keychain-signed transaction with a KeyAuthorization overwrites the transient transaction key with the newly authorized key_auth.key_id (transient key overwrite). For admin delegation, the signer is the existing admin key and key_auth.key_id is the child key, so fee/spending checks and getTransactionKey() become attributed to a key that did not sign the transaction. These paths should only run for true same-tx auth+use.
[P2] TIP-required SignatureVerifier.verifyAdmin is not implemented
The TIP specifies verifyAdmin(account, digest, signature) as part of the feature (TIP text), but ISignatureVerifier still exposes only recover and verify (interface), and dispatch still only handles those two calls (dispatch). If verifyAdmin is intended to ship in this TIP, the ABI, selector gating, stateful implementation, and coverage tests are missing.
[P2] Admin signer state reads are not reflected in gas or pool invalidation
T6 admin-signed authorizations add an is_admin_key_for(tx.caller, auth_signer) state read (state read), but key-authorization intrinsic gas still prices only the existing key check/write and optional limits/witness/scope costs (gas model). The transaction pool also only caches the returned key_expiry for expiry eviction (expiry cache), while revoked-key invalidation is keyed off the outer keychain signature subject (subject extraction). For root-signed transactions carrying an admin-signed KeyAuthorization, the admin signer can expire or be revoked after pool admission without matching the existing keychain-subject eviction path, leaving stale transactions in the pool until inclusion-time validation.
0xrusowsky
left a comment
There was a problem hiding this comment.
the ai-audit findings seem legit, we should address them.
additionally, leaving some style/clarity cmnts
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
VULNERABILITY_FOUND. PR #4265 implements TIP-1049 admin access keys. Head drift was detected (ee23eda0cbe7 audited vs current 6cf2e78ea09ee56c3c9b20f4904506f4edd54e61), and the consolidated findings were rechecked against the current head.
Inline comments cover findings on changed lines. Additional actionable findings that map to files outside the PR diff:
-
🚨 [SECURITY] Admin-key revocation does not evict pending transactions that depend on the revoked sidecar signer
Severity: Medium
File:crates/transaction-pool/src/transaction.rs:152
keychain_subject()tracks only the outer keychain signer and ignores the signer recovered from an inlineKeyAuthorization. A transaction signed by another key (or root) can remain in the pool after the admin sidecar signer is revoked, only to fail later during EVM validation.
Recommended Fix: Recover and index the inlineKeyAuthorizationsigner independently of the outer signature type, and evict when any dependent key is revoked. -
⚠️ [ISSUE] TIP-1049SignatureVerifier.verifyAdminis specified but not implemented
Severity: Medium
File:crates/precompiles/src/signature_verifier/dispatch.rs:28
TIP-1049 advertisesSignatureVerifier.verifyAdmin(account, digest, signature), but the interface and dispatch still expose onlyrecoverandverify, so integrators calling the specified method will hit an unknown selector/revert.
Recommended Fix: Add the Solidity interface, selector coverage, dispatch, calldata limit update, and Rust implementation, validating the recovered signer as root or an active admin key for the account.
Reviewer Callouts
- ⚡ Witness-burn policy:
burn_key_authorization_witnessnow uses the widened T6ensure_admin_caller, allowing admin access keys to burn TIP-1053 witnesses for the root account. Confirm whether admin keys are intentionally exempt from TIP-1053's previous access-key restriction or keep witness burning root-only.
|
Addressed |
|
Addressed |
Merging this PR will not alter performance
Comparing Footnotes
|
|
Addressed |
|
Addressed |
|
Addressed |
|
Addressed |
|
Addressed |
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Found 5 actionable findings. Four are inline on changed lines; one non-diff integration issue is included below.
SignatureVerifier.verifyAdmin API is missing
Severity: Low
File: crates/contracts/src/precompiles/signature_verifier.rs:6
TIP-1049 specifies a stateful SignatureVerifier.verifyAdmin(account, digest, signature) method, but the SignatureVerifier ABI, implementation, and dispatcher still expose only recover and verify. Contracts expecting TIP-1049’s admin-signature helper will hit an unknown selector/revert or need to manually compose recovery with AccountKeychain.isAdminKey.
Recommended Fix: Implement verifyAdmin in the ABI, precompile implementation, dispatcher, gas accounting, and selector tests, or explicitly update the TIP/release notes to defer that API.
Reviewer Callouts
- ⚡ T6 activation: The
handler.rsfindings are T6-gated and should be fixed before T6 is activated or advertised as TIP-1049-complete. - ⚡ Validation phasing: T6 no longer recovers the inline authorization signer in
validate_env; signer/admin checks are deferred to state validation. Confirm no current or future pool/RPC fast path usesvalidate_envalone as an admission decision. - ⚡ Legacy self-revocation edge case: Legacy pre-T6
keys[account][account]rows can produceKeyRevoked(account, account)events that self-evict that user’s root-signed inline-authorization transactions. This is not an external attack, but it is worth confirming as acceptable compatibility behavior.
tempoxyz-cyclops-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
VULNERABILITY_FOUND
Summary
PR #4265 implements TIP-1049 admin access keys across AccountKeychain storage/ABI, transaction encoding and validation, gas accounting, and txpool invalidation. The integration is generally consistent, but one verified spec/API gap remains.
Findings
🚨 [SECURITY] Missing verifyAdmin Implementation in SignatureVerifier Precompile
Severity: Medium
File: crates/precompiles/src/signature_verifier/dispatch.rs:28
Summary: tips/tip-1049.md:87-104 specifies a stateful verifyAdmin(address account, bytes32 digest, bytes signature) returns (bool) method that recovers the signer and returns whether it is the root key or an active admin access key for account. The generated ABI still defines only recover and verify, and the SignatureVerifier dispatcher handles only ISVCalls::recover and ISVCalls::verify. Contracts using the TIP-1049 selector will hit the unknown-selector path instead of receiving the required admin-key verification primitive.
Recommended Fix: Add verifyAdmin(address,bytes32,bytes) to the ISignatureVerifier ABI, implement it by charging verification gas, recovering the signer, and calling AccountKeychain::is_admin_key(account, recovered), then wire it in dispatch.rs and add selector/behavior tests. If this method is intentionally out of scope, amend TIP-1049 before merging the admin-key feature.
Reviewer Callouts
- ⚡ Root/self-key semantics:
AccountKeychain::is_admin_key(account, account)short-circuits totrueeven if a storedkeys[account][account]row exists withis_admin = falseor was later revoked. Pool invalidation can then treatKeyRevoked(account, account)as revoking the signer subject for root-signed inline authorizations and self-evict otherwise valid pending transactions. Impact appears low and self-inflicted, but a human should decide whether to reject non-adminauthorizeKey(keyId == account)or special-case root/self-key pool matching.
0xrusowsky
left a comment
There was a problem hiding this comment.
handler keyauth-related logic got so complex that its hard to have confidence we don't miss any edgecases
with that being said, everything makes sense and anything wrong stands out
Co-authored-by: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com>
Stacked on top of tempoxyz#4265 Adds the T6 `verifyAdmin(account, hash, signature)` selector to `SignatureVerifier`. The method recovers Tempo signatures with the existing rules and returns whether the recovered signer is the account root or an active admin access key.
Implements TIP-1049 admin access keys on top of the rebased TIP branch. Adds admin key authorization, root/admin keychain permissions, account-bound admin key authorizations, and updates the TIP text for the ABI witness parameter.
Verification: