docs(tip-1061): specify native multisig accounts - #7242
Conversation
596d269 to
f847b91
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
TIP-1061 is detailed and generally consistent, but two low-severity specification gaps should be resolved before independent implementations target T11.
Reviewer Callouts
- ⚡ Primitive sender collision assumption: The design intentionally omits a registry check for primitive signatures that recover to a registered multisig address and treats the approximately 2^80 address-collision attack as out of scope. A human should explicitly accept that account-takeover tradeoff.
- ⚡ Pool validation cost: Invalid transactions can pass the claimed sender's fee-affordability check before forcing bounded but expensive multisig verification. Confirm peer scoring or rate limiting handles repeated
NativeMultisigValidationFailedsubmissions.
|
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
Two actionable findings were verified in the T11 configurable-accounts specification.
Reviewer Callouts
- ⚡ State-dependent intrinsic gas: Ensure pool validation, estimation, block building, and execution use the same journaled block-prefix state and recompute after configuration changes.
- ⚡ Direct-frame enforcement:
msg.sender == tx.origindoes not blockDELEGATECALL; require an explicit protocol-frame/depth guard. - ⚡ Configuration storage updates: Clear old owner weight rows before overwriting the owner count or writing new rows.
- ⚡ Stateless signer recovery: Audit
recover_signerandrecover_authoritycallers so bare multisig encodings are not treated as proof of quorum. - ⚡ Pool/executor consistency: Resolve whether unsupported multisig authorization-list signatures are rejected or skipped and enforce one rule everywhere.
- ⚡ Validation work bounds: Consider an explicit pre-verification budget for invalid transactions.
- ⚡ Bootstrap version wording: Clarify that the stored version is
0during bootstrap before a header exists. - ⚡ Configuration events: Include the new version in
MultisigConfigUpdated.
|
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
TIP-1061 is generally detailed and internally consistent, but one verified Medium gas-accounting issue should be fixed before implementation.
Reviewer Callouts
- ⚡ Invalid-transaction validation cost (
tips/tip-1061.md:546): Fee affordability precedes owner-approval verification against a statelessly claimed account, allowing expensive junk trees to consume validation work before rejection. Consider checking derivable owner membership and ordering before P256/WebAuthn verification. - ⚡ Non-minimal quorum sets (
tips/tip-1061.md:211): Earlier removable signatures can produce multiple transaction hashes for the same authorization. Consider requiring every submitted signature to be necessary for quorum.
|
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
TIP-1061 is generally detailed, but one verified Medium-severity ordering rule remains undefined. Prior Cyclops reviews already covered the bootstrap-version wording and the no-code/address-collision tradeoff, so those are not repeated here.
Reviewer Callouts
- ⚡ Authorization failure semantics: Quorum failure after fee-affordability validation must reject the transaction as invalid rather than execute as a charged revert; otherwise unauthenticated transactions could burn a funded multisig account's balance.
- ⚡ Intrinsic-gas read ordering: Warm pricing makes registry-gating gas depend on check order. Pin the evaluation order so independent implementations compute the same intrinsic gas.
- ⚡ Bootstrap gas headroom: The stated worst-case 48-owner bootstrap is roughly 29.0M against a 30M transaction cap before its required non-empty call; confirm the limit remains bootstrappable under future gas-schedule changes.
- ⚡ Encoding discriminator: State explicitly that bootstrap and initialized encodings are distinguished by the first RLP item's list-versus-address shape.
| - The 3,000 gas subtraction MUST apply once at the outer account node, never to nested nodes or key authorization sidecars. | ||
| - Direct multisig authorization MUST NOT add the account keychain's 900 gas processing buffer. | ||
| - Bootstrap MUST charge one warm header SSTORE and `2 * owners.len()` cold SSTOREs, including the active TIP-1060 creditable portion, plus its warm header read, transient guard write, and `MultisigInitialized` event. | ||
| - State-dependent role restrictions MUST add `registry_gating_surcharge` to intrinsic gas. |
There was a problem hiding this comment.
The TIP allows an account to be bootstrapped and registered in the same transaction while requiring registry checks for newly authorized access-key IDs, keychain authorization-list authorities, and keychain callers with code or delegation. It never specifies whether those checks observe state before or after the bootstrap header write. An implementation using the pre-bootstrap snapshot could treat the new account as unregistered and permit a restricted role, including installing EIP-7702 delegation on a live multisig account and violating the no-account-code invariant.
Recommended Fix:
Require every multisig-restricted role check to observe registry state after any same-transaction bootstrap header write. Explicitly sequence bootstrap before key-authorization registration and authorization-list application, and define an account bootstrapped in transaction T as registered for every role check in T.
d02b5cd to
0142fe0
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
TIP-1061 is generally detailed and internally consistent. One unverified, low-severity defense-in-depth specification gap remains; consensus validation is unaffected.
Reviewer Callouts
- ⚡ Implementation pool paths: In the stacked implementation, verify that sender indexing, RPC
from, two-dimensional nonce slots, andkey_authorization.key_idrevocation subjects do not treat statelessrecover_signer()output as authenticated.
| - Keychain-signed authorization-list entries MUST retain the existing T0 behavior: they are skipped, MUST NOT install delegation, and MUST NOT require a multisig-commitment read. | ||
| - Subblock transactions MUST NOT use multisig outer or key authorization signatures. | ||
| - Sponsorship MUST use existing sponsored signing hashes. | ||
| - Pools MUST revalidate transactions carrying an initial or current signature for an account, or naming it as `key_authorization.key_id`, whenever that account's commitment changes, including after reorgs. |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] Require pool authentication before multisig sender attribution
A type-0x05 signature carries account as a caller-controlled field, and stateless recovery explicitly returns it without proving commitment equality, owner membership, or quorum. The specification requires stateful authentication before consensus authorization and requires pool revalidation after commitment changes, but it never explicitly requires pools and RPCs to authenticate the claim before attributing, indexing, or propagating the transaction. An implementation that indexes first could let forged transactions occupy another account's sender/nonce slots or pollute key-authorization revocation indexing, although block validation would still reject them.
Recommended Fix:
Add a normative pool-admission rule requiring commitment comparison and quorum verification against pool tip state before a multisig transaction—or a multisig-signed key_authorization—is attributed, indexed, or propagated under the claimed account. Keep any unauthenticated helper explicitly named or documented as returning only a claimed account.
ec9b966 to
6fb4542
Compare
Reopens #6640 against #7242. Anchors initial multisig identity to a canonical CREATE2 recovery wallet while preserving stateless versioned commitments. The baseline recovery path remains bound to the initial owners and is restricted to asset-transfer call shapes, with a reproducible init-code hash and focused Foundry tests. --------- Co-authored-by: joshieDo <93316087+joshieDo@users.noreply.github.com>
✅ Changelog found on PR. |
|
cyclops audit fast note="TIP-1061 spec/docs layer. Audit this boundary against main; implementation belongs to later PRs. Clearly mark cross-layer findings." |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
TIP-1061 specifies counterfactual native multisig accounts and cross-chain recovery with detailed, reproducible artifacts, but one verified Medium-severity security finding must be addressed before T12.
Reviewer Callouts
- ⚡ T12 transition ordering: Explicitly define how activation handles multisig addresses that already contain code; replacing only the factory does not clean up wallets deployed before activation.
- ⚡ Recovery-validator parity: The recovery contract does not enforce the Tempo rule that an account cannot own itself. Although constructing such a fixed point is impractical, document the intentional validator difference.
- ⚡ Canonical recovery signatures:
recoverSigneraccepts bothv ∈ {0,1}andv ∈ {27,28}, despite the specification requiring canonical secp256k1 signatures.
|
|
||
| - On a supported non-Tempo EVM chain, tooling MUST first verify that `MULTISIG_RECOVERY_SINGLETON_FACTORY` contains code with hash `MULTISIG_RECOVERY_SINGLETON_FACTORY_RUNTIME_HASH`. This is Safe's Singleton Factory; it is the deterministic deployer, not the recovery factory. | ||
| - The dedicated recovery factory MUST be deployed by sending `MULTISIG_RECOVERY_FACTORY_DEPLOYMENT_SALT || TempoMultisigRecoveryFactory.creationCode` as raw calldata to the Safe Singleton Factory. Its EIP-1014 address MUST equal `MULTISIG_RECOVERY_FACTORY`, and its runtime-code hash MUST equal `MULTISIG_RECOVERY_FACTORY_RUNTIME_HASH`. A missing singleton or dedicated factory, or any hash mismatch, means this TIP provides no recovery guarantee on that chain. | ||
| - On every Tempo chain, the T12 state transition MUST reserve `MULTISIG_RECOVERY_FACTORY` with the one-byte `0xEF` marker and a nonce of at least one before native multisig authorization is accepted. The transition MUST replace any code already present at that address while preserving its balance, storage, and any higher nonce. The canonical recovery-factory runtime MUST NOT be installed on Tempo. The Safe Singleton Factory MAY remain available for unrelated deployments. |
There was a problem hiding this comment.
🚨 [SECURITY] Reserve the recovery factory before T12 to prevent multisig-address squatting
The proposal reserves MULTISIG_RECOVERY_FACTORY only during T12, but Tempo genesis already includes the permissionless Safe Singleton Factory that deterministically deploys the canonical recovery factory at that address. Before T12, anyone who knows an account salt can deploy canonical recovery-wallet bytecode at the corresponding future native multisig address. Replacing the factory at T12 does not remove those wallets, so the initial owner quorum could use the wallet's on-Tempo recover() after a Tempo owner rotation; alternatively, enforcing the no-code invariant would permanently reject the squatted account. A Foundry proof of concept reproduced the factory deployment, wallet deployment, and native-asset sweep.
Recommended Fix:
Reserve MULTISIG_RECOVERY_FACTORY with the 0xEF marker in a hardfork before T12 (or in chainspec/genesis where possible). Add a normative rule requiring multisig authorization and updateConfig to reject accounts with bytecode or EIP-7702 delegation, and require derivation tooling to report a pre-existing wallet as an unusable account identity.
📊 Tempo Precompiles CoverageprecompilesCoverage: 6455/10661 lines (60.55%) File details
contractsCoverage: 1/223 lines (0.45%) File details
Total: 6456/10884 lines (59.32%) |
Reopens #6640 against #7242. Anchors initial multisig identity to a canonical CREATE2 recovery wallet while preserving stateless versioned commitments. The baseline recovery path remains bound to the initial owners and is restricted to asset-transfer call shapes, with a reproducible init-code hash and focused Foundry tests. --------- Co-authored-by: joshieDo <93316087+joshieDo@users.noreply.github.com>
cd4dae5 to
f83bf62
Compare
|
cyclops audit fast note="TIP-1061 spec/docs layer. Audit this boundary against main; implementation belongs to later PRs. Clearly mark cross-layer findings." |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
TIP-1061 specifies native multisig accounts and adds a reproducible cross-chain recovery wallet. Two verified low-severity recovery-call behaviors should be clarified or tightened. The verified factory-squatting vulnerability is not repeated because Cyclops already reported the same root cause on this PR.
Reviewer Callouts
- ⚡ Changelog timing:
.changelog/native-multisig.mdannounces T12 activation and minor releases for crates untouched by this specification-only PR; confirm the fragment will not publish before the implementation lands. - ⚡ Artifact-check CI scope: When recovery constants are added under
crates/, extend thespecsworkflow path filter so Rust-only constant changes still run the canonical artifact check.
| /// bridging, arbitrary calls) is rejected. | ||
| function _isAllowedRecoveryCall(Call calldata call_) internal view returns (bool) { | ||
| if (call_.data.length == 0) { | ||
| return call_.value != 0; |
There was a problem hiding this comment.
This accepts every nonzero-value call with empty calldata, including calls to contracts, and recover forwards all remaining gas. The target can therefore execute arbitrary state-changing receive() or fallback behavior as the multisig address—for example wrapping or bridging ETH—despite the comments claiming that bridging and arbitrary calls are rejected.
Recommended Fix:
Prefer correcting the comments and specification to state that selector filtering cannot constrain empty-calldata contract semantics. If the stronger containment guarantee is required, reject contract targets for native sweeps, recognizing that this prevents transfers to smart accounts.
| if (!ok) { | ||
| revert CallFailed(i, returndata); | ||
| } | ||
| if (calls[i].data.length >= 4 && bytes4(calls[i].data[:4]) == ERC20_TRANSFER) { |
There was a problem hiding this comment.
💡 [SUGGESTION] Check the shared ERC-20/ERC-721 transferFrom return value
Selector 0x23b872dd, named ERC721_TRANSFER_FROM, is also ERC-20 transferFrom(address,address,uint256) and is accepted when from is the wallet. This return check only runs for ERC20_TRANSFER, so a token that returns false from transferFrom without reverting makes recovery report success and consume the nonce even though no tokens moved.
Recommended Fix:
Apply the same optional-return validation to successful 0x23b872dd calls when return data is present, or explicitly disallow/document ERC-20 use of this selector.
4 participants
Defines counterfactual native multisig accounts for T12 using carried configuration witnesses and one-slot commitments. Adds the corresponding changelog entry; the stacked implementation begins with #7234.