From af47279b1939fe7b1cb1e9c24b63ee5c2b7d9703 Mon Sep 17 00:00:00 2001 From: Chris Hunter Date: Tue, 14 Jul 2026 12:22:48 -0400 Subject: [PATCH 1/2] fix: harden session policy, trusted-executor, and delegate auth; extract example wallets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security hardening: - SessionPolicy: reject a call scope targeting the account itself at install (SelfTargetNotAllowed) — the account is always authorized to call its own executeBatch, so allowing a session key to target it would let the key re-enter with an arbitrary, unchecked batch that bypasses every policy check. - SessionPolicy: enforce transferFrom `from == account`, so a session key can only spend the account's own resources, not third-party allowances it holds. - DefaultAccount._isAuthorizedCaller: enforce TRUSTED_EXECUTOR actor expiry (previously silently ignored) and require operational (sender) authority — admin (scope 0) or SCOPE_SENDER without SCOPE_POLICY — mirroring the operational-actor definition in AccountConfiguration.verifySignature. - DelegateAuthenticator: prevent cross-instance recursive delegation. The previous guard only rejected this exact instance, so two instances A/B could vouch for each other (A->B->A...). Now reject any nested authenticator that self-identifies via isDelegateAuthenticator(), enforcing a true single hop. Repo reorg: - Move example policies from src/examples/policies to src/policies (top level) and re-point test/script imports. - Extract the unaudited example wallets (UpgradeableAccount, UpgradeableProxy, BackwardsCompatible4337Account) and their tests to a separate repository; trim Deploy.s.sol and update README/NatSpec accordingly. --- README.md | 17 +- script/Deploy.s.sol | 35 +- script/DeployExamplePolicies.s.sol | 4 +- src/accounts/DefaultAccount.sol | 22 +- src/accounts/UpgradeableAccount.sol | 100 ---- src/accounts/UpgradeableProxy.sol | 38 -- .../BackwardsCompatible4337Account.sol | 169 ------ src/authenticators/DelegateAuthenticator.sol | 27 +- src/{examples => }/policies/Policy.sol | 0 src/{examples => }/policies/PolicyManager.sol | 4 +- src/{examples => }/policies/README.md | 0 .../policies/RecurringAllowance.sol | 0 src/{examples => }/policies/SessionPolicy.sol | 34 +- .../BackwardsCompatible4337Account.t.sol | 499 ------------------ test/unit/accounts/UpgradeableAccount.t.sol | 379 ------------- test/unit/examples/ExternalPolicyCaller.t.sol | 6 +- test/unit/examples/PolicyManager.t.sol | 6 +- test/unit/examples/SessionPolicy.t.sol | 6 +- test/unit/examples/SessionPolicyGas.t.sol | 4 +- 19 files changed, 100 insertions(+), 1250 deletions(-) delete mode 100644 src/accounts/UpgradeableAccount.sol delete mode 100644 src/accounts/UpgradeableProxy.sol delete mode 100644 src/accounts/example/BackwardsCompatible4337Account.sol rename src/{examples => }/policies/Policy.sol (100%) rename src/{examples => }/policies/PolicyManager.sol (99%) rename src/{examples => }/policies/README.md (100%) rename src/{examples => }/policies/RecurringAllowance.sol (100%) rename src/{examples => }/policies/SessionPolicy.sol (90%) delete mode 100644 test/unit/accounts/BackwardsCompatible4337Account.t.sol delete mode 100644 test/unit/accounts/UpgradeableAccount.t.sol diff --git a/README.md b/README.md index 5ef2526..34f9588 100644 --- a/README.md +++ b/README.md @@ -10,28 +10,21 @@ EIP-8130 defines a new transaction type and onchain system contract that togethe ## Contracts -Four account implementations are deployed: `DefaultAccount` (the bare building block, deployed standalone as the direct EIP-7702 delegation target for EOAs), `DefaultHighRateAccount` (immutable smart account), `UpgradeableAccount` (general, upgradeable smart account), and `BackwardsCompatible4337Account` (opt-in ERC-4337 example). `DefaultHighRateAccount`, `UpgradeableAccount`, and `BackwardsCompatible4337Account` all inherit from `DefaultAccount`, but each is deployed as its own singleton, since a smart-account proxy is a permanent address that cannot re-delegate the way a 7702 EOA can. `BackwardsCompatible4337Account` is deployed for callers that need it, but nothing deployed by default depends on it. +Two account implementations are deployed: `DefaultAccount` (the bare building block, deployed standalone as the direct EIP-7702 delegation target for EOAs) and `DefaultHighRateAccount` (immutable smart account). `DefaultHighRateAccount` inherits from `DefaultAccount` but is deployed as its own singleton, since a smart-account proxy is a permanent address that cannot re-delegate the way a 7702 EOA can. + +Additional, **unaudited** example account variants — `UpgradeableAccount`/`UpgradeableProxy` (general upgradeable UUPS account) and `BackwardsCompatible4337Account` (opt-in ERC-4337) — live in a separate examples repository and are not deployed by this repo. | Contract | Role | Description | |----------|------|-------------| | `AccountConfiguration` | System | Actor authorization, account creation, and change sequencing | | `DefaultAccount` | Deployed account (EOAs) | Bare minimum account: batched execution (`executeBatch`) + ERC-1271 (`isValidSignature`), all authorization deferred to `AccountConfiguration`. No ERC-4337. Works natively on 8130 chains via direct dispatch. Deployed standalone as the EIP-7702 delegation target for EOAs — they can re-delegate to a new implementation anytime, so no upgrade wrapper is needed | -| `BackwardsCompatible4337Account` | Deployed account (opt-in) | `DefaultAccount` + ERC-4337 (`validateUserOp`), so an account works on non-8130 chains via a bundler + EntryPoint. The EntryPoint is not hardcoded — it is a revocable `TRUSTED_EXECUTOR` actor in `AccountConfiguration`, so a compromised EntryPoint is disabled with one signed change and any version (v0.7/v0.8/…) is supported. Deployed by default but not used by either other deployed account; extend it (with or without UUPS) when 4337 is actually needed | | `DefaultHighRateAccount` | Deployed account | The immutable account (behind a 45-byte ERC-1167 proxy). A `DefaultAccount` variant that blocks ETH transfers when locked for higher mempool rate limits | -| `UpgradeableAccount` | Deployed account | The general upgradeable account: UUPS-upgradeable `DefaultAccount` (behind `UpgradeableProxy`), with upgrades authorized by a CONFIG-scoped key. Carries no ERC-4337 surface by default — upgrade to a `BackwardsCompatible4337Account`-derived implementation if a given deployment needs it | ### Accounts and proxies -Every account is a small per-account proxy (deployed at a deterministic CREATE2 address) that delegatecalls to one shared implementation singleton. Two proxy strategies: - -| Proxy bytecode (what the account *is*) | Implementation (what it *runs*) | Upgradeable? | -|----------|-------------|:---:| -| ERC-1167 minimal proxy (45 bytes) | `DefaultHighRateAccount` | No — immutable | -| `UpgradeableProxy` (93 bytes, ERC-1967 slot) | `UpgradeableAccount` | Yes — UUPS | - -`UpgradeableAccount` and `UpgradeableProxy` are the two halves of the upgradeable path: the former is the logic (a singleton), the latter generates the per-account bytecode that holds the ERC-1967 implementation slot (empty slot → the hardcoded default implementation; set slot → the upgraded one). Only implementations that carry UUPS logic (`UpgradeableAccount`, or any implementation upgraded to) can be deployed behind `UpgradeableProxy`. +Every account is a small per-account proxy (deployed at a deterministic CREATE2 address) that delegatecalls to one shared implementation singleton. The immutable strategy deployed here is an ERC-1167 minimal proxy (45 bytes) in front of `DefaultHighRateAccount`. An upgradeable (UUPS) strategy — `UpgradeableAccount` behind `UpgradeableProxy` — is provided as an unaudited example in a separate repository. -`AccountConfiguration.createAccount(userSalt, bytecode, initialActors)` is itself proxy-agnostic — it just `CREATE2`s whatever `bytecode` it's given. The caller decides which proxy strategy to use: an ERC-1167 clone of `DefaultHighRateAccount`, or the 93-byte result of `UpgradeableProxy.bytecode(upgradeableImpl)` for an upgradeable account. +`AccountConfiguration.createAccount(userSalt, bytecode, initialActors)` is itself proxy-agnostic — it just `CREATE2`s whatever `bytecode` it's given. The caller decides which proxy strategy to use: an ERC-1167 clone of `DefaultHighRateAccount`, or an upgradeable proxy for an upgradeable account. ### Authenticators diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index be82772..ebf9018 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -6,8 +6,6 @@ import {Script, console} from "forge-std/Script.sol"; import {AccountConfiguration} from "../src/AccountConfiguration.sol"; import {DefaultAccount} from "../src/accounts/DefaultAccount.sol"; import {DefaultHighRateAccount} from "../src/accounts/DefaultHighRateAccount.sol"; -import {UpgradeableAccount} from "../src/accounts/UpgradeableAccount.sol"; -import {BackwardsCompatible4337Account} from "../src/accounts/example/BackwardsCompatible4337Account.sol"; import {P256Authenticator} from "../src/authenticators/P256Authenticator.sol"; import {WebAuthnAuthenticator} from "../src/authenticators/WebAuthnAuthenticator.sol"; import {DelegateAuthenticator} from "../src/authenticators/DelegateAuthenticator.sol"; @@ -23,19 +21,16 @@ bytes32 constant SALT = bytes32(0); /// @notice Deploys the full EIP-8130 system: the AccountConfiguration system contract, the account /// implementations, and every canonical authenticator. /// -/// Four account implementations are deployed: +/// Two account implementations are deployed: /// - DefaultAccount — the bare building block, deployed standalone as the direct /// EIP-7702 delegation target for EOAs (no proxy needed; a 7702 EOA /// can just re-delegate to a new address later, so it needs no UUPS /// wrapper); /// - DefaultHighRateAccount — the immutable smart-account variant (deployed behind a 45-byte -/// ERC-1167 proxy); -/// - UpgradeableAccount — the general upgradeable smart-account variant (behind -/// UpgradeableProxy); -/// - BackwardsCompatible4337Account — an opt-in ERC-4337 example (DefaultAccount + validateUserOp) for -/// accounts that need bundler/EntryPoint support on non-8130 chains. -/// Deployed as a singleton so it's available for callers to use, but -/// neither deployed account depends on it by default. +/// ERC-1167 proxy). +/// +/// Example/unaudited account variants (UpgradeableAccount, BackwardsCompatible4337Account) live in a +/// separate repository and are not deployed here. /// /// All addresses are canonical: determined solely by salt + bytecode, independent of the /// deployer's address or nonce, and identical on every chain. @@ -82,14 +77,6 @@ contract Deploy is Script { return abi.encodePacked(type(DefaultHighRateAccount).creationCode, abi.encode(accountConfig)); } - function _upgradeableAccountInit(address accountConfig) internal pure returns (bytes memory) { - return abi.encodePacked(type(UpgradeableAccount).creationCode, abi.encode(accountConfig)); - } - - function _backwardsCompatible4337Init(address accountConfig) internal pure returns (bytes memory) { - return abi.encodePacked(type(BackwardsCompatible4337Account).creationCode, abi.encode(accountConfig)); - } - function _delegateAuthInit(address accountConfig) internal pure returns (bytes memory) { return abi.encodePacked(type(DelegateAuthenticator).creationCode, abi.encode(accountConfig)); } @@ -109,8 +96,6 @@ contract Deploy is Script { console.log("=== Account implementations ==="); console.log("DefaultAccount: ", _addr(_defaultAccountInit(accountConfig))); console.log("DefaultHighRateAccount: ", _addr(_defaultHighRateInit(accountConfig))); - console.log("UpgradeableAccount: ", _addr(_upgradeableAccountInit(accountConfig))); - console.log("BackwardsCompatible4337: ", _addr(_backwardsCompatible4337Init(accountConfig))); console.log(""); console.log("=== Authenticators ==="); console.log("(secp256k1 is built in: AccountConfiguration.K1_AUTHENTICATOR() == address(1))"); @@ -133,15 +118,11 @@ contract Deploy is Script { // ── Account implementations (singletons; every account proxy — and every 7702 EOA — delegates to one) ── // DefaultAccount is deployed standalone as the direct EIP-7702 delegation target for EOAs. - // DefaultHighRateAccount is the immutable (ERC-1167) smart-account variant; UpgradeableAccount is the - // general upgradeable smart-account variant. BackwardsCompatible4337Account is a separate, opt-in - // example (deployed as a singleton for callers that need it) that neither deployed account depends on - // by default. + // DefaultHighRateAccount is the immutable (ERC-1167) smart-account variant. Upgradeable and 4337 example + // variants live in a separate, unaudited repository and are not deployed here. address defaultAccount = _create2(_defaultAccountInit(accountConfig)); address defaultHighRate = _create2(_defaultHighRateInit(accountConfig)); - address upgradeableAccount = _create2(_upgradeableAccountInit(accountConfig)); - address backwardsCompatible4337 = _create2(_backwardsCompatible4337Init(accountConfig)); // ── Authenticators (secp256k1 is built into AccountConfiguration; no contract to deploy) ── @@ -155,8 +136,6 @@ contract Deploy is Script { console.log("=== Account implementations ==="); console.log("DefaultAccount: ", defaultAccount); console.log("DefaultHighRateAccount: ", defaultHighRate); - console.log("UpgradeableAccount: ", upgradeableAccount); - console.log("BackwardsCompatible4337: ", backwardsCompatible4337); console.log(""); console.log("=== Authenticators ==="); console.log("(secp256k1 is built in: AccountConfiguration.K1_AUTHENTICATOR() == address(1))"); diff --git a/script/DeployExamplePolicies.s.sol b/script/DeployExamplePolicies.s.sol index 93265d6..6ca6e4e 100644 --- a/script/DeployExamplePolicies.s.sol +++ b/script/DeployExamplePolicies.s.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.30; import {Script, console} from "forge-std/Script.sol"; import {AccountConfiguration} from "../src/AccountConfiguration.sol"; -import {PolicyManager} from "../src/examples/policies/PolicyManager.sol"; -import {SessionPolicy} from "../src/examples/policies/SessionPolicy.sol"; +import {PolicyManager} from "../src/policies/PolicyManager.sol"; +import {SessionPolicy} from "../src/policies/SessionPolicy.sol"; /// @dev Nick's deterministic deployment proxy — same address on every EVM chain. address constant CREATE2_FACTORY = 0x4e59b44847b379578588920cA78FbF26c0B4956C; diff --git a/src/accounts/DefaultAccount.sol b/src/accounts/DefaultAccount.sol index a05763a..b61de10 100644 --- a/src/accounts/DefaultAccount.sol +++ b/src/accounts/DefaultAccount.sol @@ -35,13 +35,20 @@ address constant TRUSTED_EXECUTOR = address(uint160(uint256(keccak256("trustedEx /// example ERC-4337 on a chain without native EIP-8130 support) should delegate or deploy to a purpose-built /// account, not to this one. If this bytecode is deployed anyway, it MUST sit behind an upgradeable (UUPS) /// proxy: adopting those features later means swapping in different bytecode, which is only possible if the -/// deployment is upgradeable. See {UpgradeableAccount} for that upgradeable variant. +/// deployment is upgradeable. An upgradeable (UUPS) variant is provided as an unaudited example in a separate +/// repository. /// /// @author Coinbase contract DefaultAccount is Receiver { /// @notice The AccountConfiguration system contract that owns this account's authorization state. AccountConfiguration public immutable ACCOUNT_CONFIGURATION; + /// @dev Local mirrors of {AccountConfiguration.SCOPE_SENDER} / {AccountConfiguration.SCOPE_POLICY}: a contract's + /// public constants are not accessible via its type, and reading the getters would add external calls to the + /// execution hot path. Kept in sync with AccountConfiguration. + uint8 private constant SCOPE_SENDER = 0x01; + uint8 private constant SCOPE_POLICY = 0x02; + /// @notice The caller is neither the account itself nor a registered TRUSTED_EXECUTOR actor. error UnauthorizedCaller(); @@ -107,11 +114,20 @@ contract DefaultAccount is Receiver { // INTERNALS // ══════════════════════════════════════════════ - /// @dev Authorized if `caller` is the account itself or holds the TRUSTED_EXECUTOR authenticator. + /// @dev Authorized if `caller` is the account itself, or holds the TRUSTED_EXECUTOR authenticator with an + /// unexpired config AND operational (sender) authority. Expiry: 0 means none; a non-zero expiry is enforced + /// so a user-set expiry is honored on the execution path. Scope: driving execution requires sender + /// authority — the unrestricted admin (scope == 0x00) or an actor with SCOPE_SENDER that is not gated by a + /// policy (SCOPE_POLICY unset). A POLICY-gated actor must route every call through its manager, so granting + /// it direct executeBatch would bypass that gate; fail closed. This mirrors the operational-actor definition + /// in AccountConfiguration.verifySignature, keeping the execution and signing authorization surfaces aligned. function _isAuthorizedCaller(address caller) internal view virtual returns (bool) { if (caller == address(this)) return true; AccountConfiguration.ActorConfig memory config = ACCOUNT_CONFIGURATION.getActorConfig(address(this), bytes32(bytes20(caller))); - return config.authenticator == TRUSTED_EXECUTOR; + if (config.authenticator != TRUSTED_EXECUTOR) return false; + if (config.expiry != 0 && block.timestamp > config.expiry) return false; + uint8 scope = config.scope; + return scope == 0 || ((scope & SCOPE_SENDER != 0) && (scope & SCOPE_POLICY == 0)); } } diff --git a/src/accounts/UpgradeableAccount.sol b/src/accounts/UpgradeableAccount.sol deleted file mode 100644 index 17d313d..0000000 --- a/src/accounts/UpgradeableAccount.sol +++ /dev/null @@ -1,100 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.36; - -import {UUPSUpgradeable} from "solady/utils/UUPSUpgradeable.sol"; - -import {DefaultAccount} from "./DefaultAccount.sol"; - -/// @notice UUPS-upgradeable version of {DefaultAccount}: the general-purpose deployed account, holding no -/// ERC-4337 surface by default. executeBatch, isValidSignature, and caller authorization are inherited -/// unchanged; upgrade to a 4337-capable implementation (see {BackwardsCompatible4337Account}) or any -/// future capability as needed. -/// -/// Deploy behind an UpgradeableProxy instead of ERC-1167. -/// 7702 accounts don't need this — they can re-delegate anytime. -/// -/// Upgrades are authorized via upgradeBySignature — an unrestricted-owner-signed, relayable upgrade with -/// compare-and-swap replay protection that is safe to broadcast across every chain the account lives on. -/// See {_authorizeUpgrade} for how the signature requirement is enforced on the actual implementation -/// change. -contract UpgradeableAccount is DefaultAccount, UUPSUpgradeable { - /// @dev One-shot flag set by {upgradeBySignature} immediately before its internal self-call to - /// `upgradeToAndCall`, and consumed by {_authorizeUpgrade} to confirm an unrestricted-owner (scope 0) - /// signature has already authorized this specific upgrade. - bool private _upgradeAuthorized; - - /// @dev Typehash binding a signed upgrade to (account, from, to, dataHash). chainId is intentionally omitted: - /// the same owner signature applies on every chain whose current implementation equals - /// `fromImplementation` (compare-and-swap), and is naturally skipped on chains that have diverged. - bytes32 public constant SIGNED_UPGRADE_TYPEHASH = keccak256( - "SignedUpgrade(address account,address fromImplementation,address toImplementation,bytes32 dataHash)" - ); - - /// @dev The current implementation does not match the signed `fromImplementation` (compare-and-swap failed). - error UpgradeFromMismatch(); - /// @dev The authenticated actor may not authorize upgrades (not an unrestricted owner). - error UpgradeUnauthorized(); - - /// @dev upgradeToAndCall was called directly instead of through {upgradeBySignature}, so no unrestricted-owner - /// signed authorization set the one-shot flag. - error UpgradeNotInitiated(); - - constructor(address accountConfiguration) DefaultAccount(accountConfiguration) {} - - /// @dev {upgradeBySignature} is the only place that sets {_upgradeAuthorized}, so satisfying this confirms an - /// unrestricted-owner (scope 0) signature has already authorized the implementation change being applied. - /// @dev Reverts with UpgradeNotInitiated when the flag is unset (a direct upgradeToAndCall call). - function _authorizeUpgrade(address) internal override { - if (!_upgradeAuthorized) revert UpgradeNotInitiated(); - _upgradeAuthorized = false; - } - - /// @notice Upgrade the implementation using an owner signature, with compare-and-swap replay protection. - /// @dev Replay protection is state-based, not nonce-based: the signature is bound to `fromImplementation` and - /// is only valid while the ERC-1967 slot still holds it. Once applied the slot becomes `toImplementation`, - /// so the same signature can no longer be replayed on this chain. Because chainId is not part of the - /// digest, one signature upgrades every chain currently at `fromImplementation` and is skipped on any - /// chain that has diverged. A fresh account (slot unset, running the hardcoded default) requires - /// `fromImplementation == address(0)`. Anyone may submit this call (e.g. a relayer); the signature is what - /// proves owner intent. - /// - /// NOTE (deferred for audit/discussion): compare-and-swap is not ABA-proof. If an implementation is - /// upgraded away from `fromImplementation` and later restored to it, a previously-used signature for that - /// transition becomes replayable. This is only harmful if an owner deliberately downgrades away from a - /// malicious/broken implementation and an attacker then forces it back. We accept this for now because - /// downgrades are expected to be rare; a forward-only version ratchet or a used-digest guard can close it - /// without a deadline if we decide we need to. To be revisited during the security review. - /// @param fromImplementation Expected current implementation (raw ERC-1967 slot value; address(0) when unset). - /// @param toImplementation The implementation to upgrade to. - /// @param data Optional initialization calldata delegatecalled on `toImplementation` (empty to skip). - /// @param auth Authenticator(20) || authenticator-specific data, authenticated by AccountConfiguration. - function upgradeBySignature( - address fromImplementation, - address toImplementation, - bytes calldata data, - bytes calldata auth - ) external { - if (_currentImplementation() != fromImplementation) revert UpgradeFromMismatch(); - - bytes32 digest = keccak256( - abi.encode(SIGNED_UPGRADE_TYPEHASH, address(this), fromImplementation, toImplementation, keccak256(data)) - ); - - // Only an unrestricted owner (scope 0) may authorize an upgrade; there is no elevated "admin" scope bit. - (uint8 scope,) = ACCOUNT_CONFIGURATION.authenticateActor(address(this), digest, auth); - if (scope != 0) revert UpgradeUnauthorized(); - - // Reuse Solady's tested upgrade path (proxiableUUID check, Upgraded event, optional init delegatecall). - // The flag set above is what satisfies _authorizeUpgrade for this call. - _upgradeAuthorized = true; - this.upgradeToAndCall(toImplementation, data); - } - - /// @dev Reads the raw ERC-1967 implementation slot (address(0) when unset on a fresh account). - function _currentImplementation() internal view returns (address impl) { - bytes32 slot = _ERC1967_IMPLEMENTATION_SLOT; - assembly ("memory-safe") { - impl := sload(slot) - } - } -} diff --git a/src/accounts/UpgradeableProxy.sol b/src/accounts/UpgradeableProxy.sol deleted file mode 100644 index 943e970..0000000 --- a/src/accounts/UpgradeableProxy.sol +++ /dev/null @@ -1,38 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.36; - -/// @notice Generates runtime bytecode for an ERC-1967 proxy with a hardcoded default -/// implementation. Works immediately on deployment — no initialization required. -/// -/// This is the per-account bytecode half of the upgradeable path; {UpgradeableAccount} is the -/// implementation half (the singleton this proxy delegates to). Pass an {UpgradeableAccount} as the -/// default implementation — only implementations carrying UUPS logic can ever write the slot this proxy -/// reads. Immutable accounts ({DefaultAccount}, {DefaultHighRateAccount}, and example variants like -/// {BackwardsCompatible4337Account}) use a 45-byte ERC-1167 proxy instead. -/// -/// Proxy logic: -/// 1. SLOAD the ERC-1967 implementation slot -/// 2. If non-zero, delegatecall to that address (upgraded path) -/// 3. If zero, delegatecall to the hardcoded default (fresh deployment path) -/// -/// 93 bytes total. Overhead vs ERC-1167: one SLOAD per call (2100 gas cold, 100 warm). -library UpgradeableProxy { - function bytecode(address defaultImplementation) internal pure returns (bytes memory) { - return abi.encodePacked( - // Phase 1: resolve implementation - // PUSH32 ERC1967_SLOT, SLOAD, DUP1, ISZERO - // PUSH2 default_label(0x002c), JUMPI - // PUSH2 delegate_label(0x0043), JUMP - hex"7f360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc", - hex"5480156100", - hex"2c", - hex"576100", - hex"4356", - // default_label: POP, PUSH20 - hex"5b5073", - defaultImplementation, - // delegate_label: delegatecall, return/revert - hex"5b363d3d373d3d3d363d855af43d82803e903d91605b57fd5bf3" - ); - } -} diff --git a/src/accounts/example/BackwardsCompatible4337Account.sol b/src/accounts/example/BackwardsCompatible4337Account.sol deleted file mode 100644 index 42b45ab..0000000 --- a/src/accounts/example/BackwardsCompatible4337Account.sol +++ /dev/null @@ -1,169 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.30; - -import {AccountConfiguration} from "../../AccountConfiguration.sol"; -import {DefaultAccount} from "../DefaultAccount.sol"; - -struct PackedUserOperation { - address sender; - uint256 nonce; - bytes initCode; - bytes callData; - bytes32 accountGasLimits; - uint256 preVerificationGas; - bytes32 gasFees; - bytes paymasterAndData; - bytes signature; -} - -/// @notice One independently-signed batch of actor changes, mirroring a single -/// `AccountConfiguration.applySignedActorChanges` call. Multiple sets may be -/// supplied in a UserOperation signature and are applied in order (e.g. a set -/// signed by the current owner, followed by a set signed by a key it just -/// authorized). -struct SignedActorChanges { - AccountConfiguration.ActorChange[] changes; - bytes auth; -} - -/// @notice Example ERC-4337-compatible account for EIP-8130: {DefaultAccount} plus `validateUserOp`, reproducing -/// the 8130 authorization semantics (scope + policy) so an account works on non-8130 chains via a bundler + -/// EntryPoint, identically to native dispatch. -/// -/// The EntryPoint is authorized like any other caller: a revocable TRUSTED_EXECUTOR actor in -/// AccountConfiguration (see {DefaultAccount}). A signed CONFIG change swaps it for a different address at -/// any time, even on non-upgradeable accounts, and supports any EntryPoint version since the account's -/// CREATE2 address never depends on it. -/// -/// ERC-7562: authorizing the caller and authenticating the op both read the account's own associated -/// storage in AccountConfiguration, keeping both within the same validation-phase storage category. -/// -/// Bootstrapping: the EntryPoint must already be a TRUSTED_EXECUTOR actor before its first call (gated by -/// `_isAuthorizedCaller`). Seed it into the initial actor set at `createAccount` for a counterfactual -/// account's first op to work out of the box; otherwise register it later via a signed actor change. -contract BackwardsCompatible4337Account is DefaultAccount { - /// @dev Signature discriminator for validation-phase actor changes: when `userOp.signature` starts with this - /// 32-byte magic, it decodes as `abi.encode(magic, SignedActorChanges[] changeSets, bytes opAuth)` and - /// each change set is applied in order (e.g. rotating the controlling key to a P-256 actor) before the op - /// is authenticated. Each change is bound to this account + a monotonic sequence and authorized by the - /// account's own key, so applying it only ever mutates this account's own config — it never authorizes - /// the op itself. The trailing `opAuth` (a plain `authenticator || data` blob) must still sign for this - /// exact `userOpHash`, and may come from a key the changes just added/rotated to. A signature without the - /// magic prefix is itself treated as the plain `opAuth` blob, preserving the base behaviour. - bytes32 internal constant SIGNED_ACTOR_CHANGES_MAGIC = keccak256("ERC4337Account.signedActorChanges.v1"); - - /// @dev Elevated-scope bitflags, mirroring AccountConfiguration. A scope of 0x00 is an unrestricted owner. - uint8 internal constant SCOPE_SENDER = 0x01; // may initiate transactions (authorize the op's calls) - uint8 internal constant SCOPE_SELF_PAYER = 0x08; // may self-pay gas for the account's own op (payer == sender) - - constructor(address accountConfiguration) DefaultAccount(accountConfiguration) {} - - // ══════════════════════════════════════════════ - // ERC-4337 - // ══════════════════════════════════════════════ - - /// @notice Validates a UserOperation signature via the AccountConfiguration system. - /// Signature format follows 8130 authenticator conventions (authenticator_type || data), - /// and optionally carries signed actor/owner changes applied during validation - /// (see {SIGNED_ACTOR_CHANGES_MAGIC}). - /// - /// @dev Reverts with UnauthorizedCaller when the caller is neither the account nor a TRUSTED_EXECUTOR actor - /// (typically the EntryPoint). - function validateUserOp(PackedUserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds) - external - returns (uint256 validationData) - { - if (!_isAuthorizedCaller(msg.sender)) revert UnauthorizedCaller(); - - validationData = _validateSignature(userOp, userOpHash, missingAccountFunds) ? 0 : 1; - - if (missingAccountFunds != 0) { - assembly { - pop(call(gas(), caller(), missingAccountFunds, 0, 0, 0, 0)) - } - } - } - - /// @notice Validates `userOp` by authenticating it as a plain authenticator blob over `userOpHash` and - /// enforcing the verified actor's elevated scope. A signature carrying signed actor/owner changes - /// additionally applies them during validation before the op itself is authenticated. - /// @dev Signed-actor-changes path: each set is applied via `applySignedActorChanges` (empty batch rejected). - /// Every slot it writes is keyed by `account`, so under ERC-7562 it's the account's own associated - /// storage — allowed by STO-021 for an existing account; only a combined create+change op falls under - /// STO-022, requiring the AccountConfiguration factory to be staked. - /// - /// Op authentication (both paths) enforces the verified actor's elevated scope: - /// - the actor must be unrestricted (scope 0x00) or hold {SCOPE_SENDER} to authorize the calls; - /// - a self-funded op (`missingAccountFunds != 0`) additionally requires {SCOPE_SELF_PAYER}. - /// This reduced 4337 bridge does not replicate the native-dispatch policy-target gate: a SCOPE_POLICY - /// actor without SCOPE_SENDER is rejected here by construction (see {_authorize}), and this repo does not - /// implement protocol-side lane/exclusivity checks for actors that combine SCOPE_POLICY with SCOPE_SENDER. - function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds) - internal - returns (bool) - { - bytes calldata signature = userOp.signature; - bytes memory opAuth = signature; - - if (signature.length >= 32 && bytes32(signature[:32]) == SIGNED_ACTOR_CHANGES_MAGIC) { - SignedActorChanges[] memory changeSets; - (, changeSets, opAuth) = abi.decode(signature, (bytes32, SignedActorChanges[], bytes)); - - if (changeSets.length == 0) return false; - - // Each set is bound to address(this) + a monotonic sequence (see SIGNED_ACTOR_CHANGES_MAGIC above), - // so it only ever mutates this account's own config. - for (uint256 i; i < changeSets.length; i++) { - try ACCOUNT_CONFIGURATION.applySignedActorChanges( - address(this), uint64(block.chainid), changeSets[i].changes, changeSets[i].auth - ) {} - catch { - return false; - } - } - } - - // Applying changes never authorizes the op; `opAuth` must still sign for this `userOpHash`. - (bool valid, uint8 scope) = _authenticate(userOpHash, opAuth); - if (!valid) return false; - - // Authentication only proves WHO signed; authorization decides whether that actor may drive THIS op. - return _authorize(scope, missingAccountFunds); - } - - /// @notice Authenticates `auth` over `hash` via AccountConfiguration, resolving the signing actor's scope. - /// This answers only "who signed", never "may they do this" — see {_authorize}. - /// @return valid True if `auth` is a valid signature from a live actor of this account. - /// @return scope The verified actor's scope (0x00 = unrestricted owner). - function _authenticate(bytes32 hash, bytes memory auth) internal view returns (bool valid, uint8 scope) { - // policyTarget is a protocol-side / policy-manager concern this reduced 4337 bridge does not replicate: - // a SCOPE_POLICY actor is rejected below by the SCOPE_SENDER check (see {_authorize}), since this repo - // does not implement protocol-side lane/exclusivity checks. - try ACCOUNT_CONFIGURATION.authenticateActor(address(this), hash, auth) returns (uint8 s, address) { - return (true, s); - } catch { - return (false, 0); - } - } - - /// @notice Decides whether an already-authenticated actor may drive this UserOperation, from its scope. - /// Split out from {_authenticate} so the two concerns — who signed vs. what they may do — are - /// independently reviewable and overridable. - /// @dev Enforces: - /// - scope 0x00 is an unrestricted owner; any other actor must hold {SCOPE_SENDER} to authorize the calls; - /// - a self-funded op (`missingAccountFunds != 0`) additionally requires {SCOPE_SELF_PAYER}. - /// A SCOPE_POLICY actor without SCOPE_SENDER fails the check below by construction — this reduced 4337 - /// bridge does not give policy-gated actors special call-target enforcement (that is native-dispatch, - /// protocol-side behavior out of scope for this repo). An actor combining SCOPE_POLICY | SCOPE_SENDER is - /// authorized here exactly like any other SENDER-scoped actor. - /// @param scope The verified actor's scope (0x00 = unrestricted owner). - /// @param missingAccountFunds The prefund the account owes the EntryPoint; non-zero means a self-funded op. - function _authorize(uint8 scope, uint256 missingAccountFunds) internal view virtual returns (bool) { - // scope 0x00 = unrestricted owner; otherwise the actor must explicitly hold the required scopes. - if (scope != 0) { - if (scope & SCOPE_SENDER == 0) return false; - if (missingAccountFunds != 0 && scope & SCOPE_SELF_PAYER == 0) return false; - } - return true; - } -} diff --git a/src/authenticators/DelegateAuthenticator.sol b/src/authenticators/DelegateAuthenticator.sol index dd492a6..a9f070a 100644 --- a/src/authenticators/DelegateAuthenticator.sol +++ b/src/authenticators/DelegateAuthenticator.sol @@ -20,7 +20,8 @@ contract DelegateAuthenticator is IAuthenticator { /// @notice The auth data is shorter than the 40-byte delegate + nested-authenticator prefix. error InvalidDataLength(); - /// @notice The nested authenticator points back to this contract; only one delegation hop is permitted. + /// @notice The nested authenticator is itself a delegate authenticator (this instance or any other); only one + /// delegation hop is permitted, so a delegate may never vouch through another delegate. error RecursiveDelegation(); /// @notice The nested signer is not authorized to sign for the delegate account. @@ -32,11 +33,18 @@ contract DelegateAuthenticator is IAuthenticator { ACCOUNT_CONFIGURATION = AccountConfiguration(accountConfiguration); } + /// @notice Self-identifying marker so one delegate authenticator can detect another (of any instance) and refuse + /// to delegate through it, enforcing the single-hop invariant across separately deployed instances. + /// @return Always true for a delegate authenticator. + function isDelegateAuthenticator() external pure returns (bool) { + return true; + } + /// @notice Authenticates by delegating to another account's actor configuration; only one hop is permitted. /// /// @dev Reverts with InvalidDataLength when `data` is shorter than 40 bytes. - /// @dev Reverts with RecursiveDelegation when the nested authenticator is this contract (recursive delegation - /// is not permitted). + /// @dev Reverts with RecursiveDelegation when the nested authenticator is this contract or any other delegate + /// authenticator (recursive/chained delegation is not permitted; only a single hop). /// @dev Reverts with InvalidNestedSignature when the delegate account does not validate the nested signature. /// /// @param hash The digest being authenticated. @@ -50,9 +58,20 @@ contract DelegateAuthenticator is IAuthenticator { actorId = bytes32(bytes20(delegate)); - // Prevent recursive delegation (only 1 hop permitted) + // Prevent recursive delegation (only 1 hop permitted). Rejecting this exact instance is not enough: two + // distinct delegate authenticators A and B could otherwise vouch for each other (A->B->A...), so we also + // reject any nested authenticator that self-identifies as a delegate authenticator. Honest delegate + // authenticators expose isDelegateAuthenticator(); a contract that hides it is not a delegate and cannot form + // a delegate cycle through this guard. Only probe addresses with code — the K1 sentinel (address(1), + // ecrecover) and other precompiles have none and are never delegates, so we skip them to avoid a spurious + // precompile call. address nestedAuthenticator = address(bytes20(nestedAuth[:20])); if (nestedAuthenticator == address(this)) revert RecursiveDelegation(); + if (nestedAuthenticator.code.length > 0) { + try DelegateAuthenticator(nestedAuthenticator).isDelegateAuthenticator() returns (bool isDelegate) { + if (isDelegate) revert RecursiveDelegation(); + } catch {} + } // The nested actor MUST be the admin (scope == 0x00) of the delegate account. This is enforced // independently of verifySignature, which is now operational (signing is not admin-only, but a delegate diff --git a/src/examples/policies/Policy.sol b/src/policies/Policy.sol similarity index 100% rename from src/examples/policies/Policy.sol rename to src/policies/Policy.sol diff --git a/src/examples/policies/PolicyManager.sol b/src/policies/PolicyManager.sol similarity index 99% rename from src/examples/policies/PolicyManager.sol rename to src/policies/PolicyManager.sol index 4e1013e..d3c9a9e 100644 --- a/src/examples/policies/PolicyManager.sol +++ b/src/policies/PolicyManager.sol @@ -4,8 +4,8 @@ pragma solidity ^0.8.30; import {Address} from "openzeppelin/utils/Address.sol"; import {ReentrancyGuard} from "openzeppelin/utils/ReentrancyGuard.sol"; -import {AccountConfiguration} from "../../AccountConfiguration.sol"; -import {ITransactionContext, TX_CONTEXT_ADDRESS} from "../../interfaces/ITransactionContext.sol"; +import {AccountConfiguration} from "../AccountConfiguration.sol"; +import {ITransactionContext, TX_CONTEXT_ADDRESS} from "../interfaces/ITransactionContext.sol"; import {Policy} from "./Policy.sol"; /// @dev Recommended `authenticator` for an actor that represents an *external caller* governed by a policy (e.g. a diff --git a/src/examples/policies/README.md b/src/policies/README.md similarity index 100% rename from src/examples/policies/README.md rename to src/policies/README.md diff --git a/src/examples/policies/RecurringAllowance.sol b/src/policies/RecurringAllowance.sol similarity index 100% rename from src/examples/policies/RecurringAllowance.sol rename to src/policies/RecurringAllowance.sol diff --git a/src/examples/policies/SessionPolicy.sol b/src/policies/SessionPolicy.sol similarity index 90% rename from src/examples/policies/SessionPolicy.sol rename to src/policies/SessionPolicy.sol index 6bcc604..26f1139 100644 --- a/src/examples/policies/SessionPolicy.sol +++ b/src/policies/SessionPolicy.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.30; import {IERC20} from "openzeppelin/token/ERC20/IERC20.sol"; -import {Call, DefaultAccount} from "../../accounts/DefaultAccount.sol"; +import {Call, DefaultAccount} from "../accounts/DefaultAccount.sol"; import {Policy} from "./Policy.sol"; import {RecurringAllowance} from "./RecurringAllowance.sol"; @@ -156,6 +156,13 @@ contract SessionPolicy is Policy { /// without debiting the spend cap. Native-ETH limits (`token == address(0)`) are unaffected — they gate /// call `value`, not a call target. error AnySelectorOnLimitedToken(address token); + /// @notice A call scope targeted the account itself. The account is always an authorized caller of its own + /// `executeBatch`, so allowing a session key to call it would let the key re-enter with an arbitrary + /// batch that bypasses every policy check. Reject at install to fail closed. + error SelfTargetNotAllowed(); + /// @notice A `transferFrom` moved funds from an address other than the account. A session key may only spend the + /// account's own resources, not third-party allowances the account happens to hold. + error TransferFromNotSelf(address from); constructor(address policyManager) Policy(policyManager) {} @@ -255,7 +262,7 @@ contract SessionPolicy is Policy { /// {AnySelectorOnLimitedToken} when a limited token would be left with untracked selectors) and, per /// selector rule, whether a recipient allowlist applies (rejecting {RecipientRuleUnsupportedSelector} for a /// selector whose recipient cannot be decoded) plus the recipient set itself. - function _onInstall(bytes32 commitment, address, bytes calldata policyConfig) internal override { + function _onInstall(bytes32 commitment, address account, bytes calldata policyConfig) internal override { Config memory config = abi.decode(policyConfig, (Config)); for (uint256 i; i < config.tokenLimits.length; i++) { @@ -269,6 +276,9 @@ contract SessionPolicy is Policy { for (uint256 i; i < config.callScopes.length; i++) { CallScope memory scope = config.callScopes[i]; + // Fail closed: the account is always authorized to call its own executeBatch, so a session key allowed to + // target the account could re-enter with an arbitrary, unchecked batch and escape every policy dimension. + if (scope.target == account) revert SelfTargetNotAllowed(); bool anySelector = scope.selectorRules.length == 0; // Fail closed: a TokenLimit on this target only tracks transfer/transferFrom/approve, so anySelector // would let other methods move value untracked. Require an explicit selector allowlist instead. @@ -299,7 +309,7 @@ contract SessionPolicy is Policy { /// ({RecipientNotAllowed}); the ERC-20 spend cap for decodable spend selectors on a limited token; and the /// native-ETH spend cap consumed from the call `value`. Calldata of 1–3 bytes is rejected ({MissingSelector}). /// On success returns the `executeBatch` calldata for a single {Call} mirroring the action. - function _onExecute(bytes32 commitment, address, bytes calldata executionData, address) + function _onExecute(bytes32 commitment, address account, bytes calldata executionData, address) internal override returns (bytes memory accountCallData) @@ -325,6 +335,13 @@ contract SessionPolicy is Policy { } } + // 3. transferFrom source: a session key spends only the account's own resources, never a third-party + // allowance the account holds. Enforce `from == account` regardless of token limits or recipient rules. + if (selector == TRANSFER_FROM) { + address from = _decodeTransferFromSender(action.data); + if (from != account) revert TransferFromNotSelf(from); + } + // 3a. ERC-20 spend limit: consume the target token's cap for decodable spend selectors. Note `approve` // debits at grant time; a standing allowance can still be reused across periods (see contract NatSpec). if (_isErc20Selector(selector)) { @@ -407,4 +424,15 @@ contract SessionPolicy is Policy { // Truncate to the low 160 bits: ABI pads the high bytes, but a caller could dirty them. recipient = address(uint160(recipientWord)); } + + /// @dev Decode the `from` (source) address of a `transferFrom(address from, address to, uint256 amount)` call. + function _decodeTransferFromSender(bytes memory data) internal pure returns (address from) { + if (data.length < 4 + 96) revert MalformedTokenCall(TRANSFER_FROM); + uint256 fromWord; + assembly ("memory-safe") { + fromWord := mload(add(data, 0x24)) + } + // Truncate to the low 160 bits: ABI pads the high bytes, but a caller could dirty them. + from = address(uint160(fromWord)); + } } diff --git a/test/unit/accounts/BackwardsCompatible4337Account.t.sol b/test/unit/accounts/BackwardsCompatible4337Account.t.sol deleted file mode 100644 index 9cb4684..0000000 --- a/test/unit/accounts/BackwardsCompatible4337Account.t.sol +++ /dev/null @@ -1,499 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.30; - -import {DefaultAccount, Call, TRUSTED_EXECUTOR} from "../../../src/accounts/DefaultAccount.sol"; -import { - BackwardsCompatible4337Account, - PackedUserOperation, - SignedActorChanges -} from "../../../src/accounts/example/BackwardsCompatible4337Account.sol"; -import {AccountConfiguration} from "../../../src/AccountConfiguration.sol"; -import {AccountConfigurationTest} from "../../lib/AccountConfigurationTest.sol"; - -contract UserOpMockTarget { - uint256 public value; - - function setValue(uint256 v) external payable { - value = v; - } - - function reverting() external pure { - revert("boom"); - } -} - -/// @notice ERC-4337 conformance suite for {BackwardsCompatible4337Account}. The EntryPoint is authorized as a -/// config-driven TRUSTED_EXECUTOR actor (seeded into the initial actor set at creation), so it is revocable -/// and version-agnostic — the single opinionated 4337 model for the repo. -contract BackwardsCompatible4337AccountTest is AccountConfigurationTest { - uint256 constant ACTOR_PK = 100; - - uint8 constant SCOPE_SENDER = 0x01; - uint8 constant SCOPE_SELF_PAYER = 0x08; - uint8 constant SCOPE_SPONSOR_PAYER = 0x10; - - bytes32 constant SIGNED_ACTOR_CHANGES_MAGIC = keccak256("ERC4337Account.signedActorChanges.v1"); - - UserOpMockTarget public target; - address public impl; - - function setUp() public virtual override { - super.setUp(); - target = new UserOpMockTarget(); - impl = address(new BackwardsCompatible4337Account(address(accountConfiguration))); - } - - /// @dev Create an account from `impl` with `pk` as the unrestricted owner, seeding the EntryPoint as a - /// TRUSTED_EXECUTOR actor so it is an authorized caller from the account's first op (the config-driven - /// model has no hardcoded EntryPoint). Returns (account, ownerActorId). - function _create4337Account(uint256 pk) internal returns (address account, bytes32 actorId) { - actorId = bytes32(bytes20(vm.addr(pk))); - AccountConfiguration.InitialActor memory owner = AccountConfiguration.InitialActor({ - actorId: actorId, authenticator: address(k1Authenticator), scope: 0, policyData: "" - }); - AccountConfiguration.InitialActor memory ep = AccountConfiguration.InitialActor({ - actorId: bytes32(bytes20(ENTRY_POINT)), authenticator: TRUSTED_EXECUTOR, scope: 0, policyData: "" - }); - - AccountConfiguration.InitialActor[] memory actors = new AccountConfiguration.InitialActor[](2); - (actors[0], actors[1]) = owner.actorId < ep.actorId ? (owner, ep) : (ep, owner); - account = accountConfiguration.createAccount(bytes32(0), _computeERC1167Bytecode(impl), actors); - } - - // ── Shared helpers ── - - function _singleCall(address t, uint256 v, bytes memory d) internal pure returns (Call[] memory calls) { - calls = new Call[](1); - calls[0] = Call(t, v, d); - } - - function _buildUserOp(address account, bytes memory signature) internal pure returns (PackedUserOperation memory) { - return _buildUserOp(account, "", signature); - } - - function _buildUserOp(address account, bytes memory callData, bytes memory signature) - internal - pure - returns (PackedUserOperation memory) - { - return PackedUserOperation({ - sender: account, - nonce: 0, - initCode: "", - callData: callData, - accountGasLimits: bytes32(0), - preVerificationGas: 0, - gasFees: bytes32(0), - paymasterAndData: "", - signature: signature - }); - } - - function _executeBatchCallData(address t, uint256 v, bytes memory d) internal pure returns (bytes memory) { - return abi.encodeCall(DefaultAccount.executeBatch, (_singleCall(t, v, d))); - } - - /// @dev Authorizes a new K1 actor on `account` with the given scope/policy, signed by the unrestricted owner - /// (`ownerPk`) via `applySignedActorChanges`. Returns the new actor's id. Policy data is attached whenever - /// `scope` carries SCOPE_POLICY. - function _authorizeScopedActor( - address account, - uint256 ownerPk, - uint256 newPk, - uint8 scope, - address policyManager, - bytes32 commitment - ) internal returns (bytes32 newActorId) { - newActorId = bytes32(bytes20(vm.addr(newPk))); - bytes memory policyData = - scope & accountConfiguration.SCOPE_POLICY() == 0 ? bytes("") : abi.encodePacked(policyManager, commitment); - - AccountConfiguration.ActorChange[] memory changes = new AccountConfiguration.ActorChange[](1); - changes[0] = AccountConfiguration.ActorChange({ - actorId: newActorId, - changeType: 0x01, - data: abi.encode( - AccountConfiguration.ActorConfig({authenticator: address(k1Authenticator), scope: scope, expiry: 0}), - policyData - ) - }); - - uint64 seq = accountConfiguration.getChangeSequences(account).local; - bytes32 digest = _computeActorChangeBatchDigest(account, uint64(block.chainid), seq, changes); - accountConfiguration.applySignedActorChanges( - account, uint64(block.chainid), changes, _buildK1Auth(ownerPk, digest) - ); - } - - function _authorizeK1ActorChange(uint256 newPk) - internal - view - returns (AccountConfiguration.ActorChange[] memory changes, bytes32 newActorId) - { - newActorId = bytes32(bytes20(vm.addr(newPk))); - changes = new AccountConfiguration.ActorChange[](1); - changes[0] = AccountConfiguration.ActorChange({ - actorId: newActorId, - changeType: 0x01, - data: abi.encode( - AccountConfiguration.ActorConfig({authenticator: address(k1Authenticator), scope: 0x00, expiry: 0}), - bytes("") - ) - }); - } - - function _signedSet( - address account, - uint64 seq, - uint256 signerPk, - AccountConfiguration.ActorChange[] memory changes - ) internal view returns (SignedActorChanges memory) { - bytes32 changeDigest = _computeActorChangeBatchDigest(account, uint64(block.chainid), seq, changes); - return SignedActorChanges({changes: changes, auth: _buildK1Auth(signerPk, changeDigest)}); - } - - // ── Always-authorized callers ── - - function test_entryPointIsAuthorized() public { - (address account,) = _create4337Account(ACTOR_PK); - assertTrue(DefaultAccount(payable(account)).isAuthorizedCaller(ENTRY_POINT)); - } - - function test_selfIsAlwaysAuthorized() public { - (address account,) = _create4337Account(ACTOR_PK); - assertTrue(DefaultAccount(payable(account)).isAuthorizedCaller(account)); - } - - function test_unknownCallerNotAuthorized() public { - (address account,) = _create4337Account(ACTOR_PK); - assertFalse(DefaultAccount(payable(account)).isAuthorizedCaller(address(0xdead))); - } - - // ── Trusted-executor actor (relayer / PolicyManager registered via AccountConfiguration) ── - - function test_trustedExecutorActorIsAuthorized() public { - (address account,) = _create4337Account(ACTOR_PK); - address relayer = address(0xBEEF); - - AccountConfiguration.ActorChange[] memory changes = new AccountConfiguration.ActorChange[](1); - changes[0] = AccountConfiguration.ActorChange({ - actorId: bytes32(bytes20(relayer)), - changeType: 0x01, - data: abi.encode( - AccountConfiguration.ActorConfig({authenticator: TRUSTED_EXECUTOR, scope: SCOPE_SENDER, expiry: 0}), - bytes("") - ) - }); - uint64 seq = accountConfiguration.getChangeSequences(account).local; - bytes32 digest = _computeActorChangeBatchDigest(account, uint64(block.chainid), seq, changes); - accountConfiguration.applySignedActorChanges( - account, uint64(block.chainid), changes, _buildK1Auth(ACTOR_PK, digest) - ); - - assertTrue(DefaultAccount(payable(account)).isAuthorizedCaller(relayer)); - - vm.prank(relayer); - DefaultAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0, abi.encodeCall(UserOpMockTarget.setValue, (7)))); - assertEq(target.value(), 7); - } - - // ── executeBatch from EntryPoint ── - - function test_executeBatch_fromEntryPoint() public { - (address account,) = _create4337Account(ACTOR_PK); - - vm.prank(ENTRY_POINT); - DefaultAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0, abi.encodeCall(UserOpMockTarget.setValue, (77)))); - - assertEq(target.value(), 77); - } - - // ── validateUserOp ── - - function test_validateUserOp_validSignature() public { - (address account,) = _create4337Account(ACTOR_PK); - - bytes32 userOpHash = keccak256("user-op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(ACTOR_PK, userOpHash)); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 0); - } - - function test_validateUserOp_invalidSignature() public { - (address account,) = _create4337Account(ACTOR_PK); - - bytes32 userOpHash = keccak256("user-op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(999, userOpHash)); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 1); - } - - function test_validateUserOp_revertsFromUnauthorizedCaller() public { - (address account,) = _create4337Account(ACTOR_PK); - - bytes32 userOpHash = keccak256("user-op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(ACTOR_PK, userOpHash)); - - vm.prank(address(0xdead)); - vm.expectRevert(DefaultAccount.UnauthorizedCaller.selector); - BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0); - } - - function test_validateUserOp_paysPrefund() public { - (address account,) = _create4337Account(ACTOR_PK); - vm.deal(account, 1 ether); - - bytes32 userOpHash = keccak256("user-op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(ACTOR_PK, userOpHash)); - - uint256 prefund = 0.1 ether; - uint256 epBalanceBefore = ENTRY_POINT.balance; - - vm.prank(ENTRY_POINT); - BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, prefund); - - assertEq(ENTRY_POINT.balance - epBalanceBefore, prefund); - } - - // ── validateUserOp: validation-phase actor changes ── - - /// @notice A single UserOperation can rotate/add a key during validation; the op is then authenticated as - /// usual by `opAuth` — produced here by the brand-new key. - function test_validateUserOp_appliesSignedActorChanges() public { - (address account,) = _create4337Account(ACTOR_PK); - - uint256 newPk = 101; - (AccountConfiguration.ActorChange[] memory changes, bytes32 newActorId) = _authorizeK1ActorChange(newPk); - - uint64 seq = accountConfiguration.getChangeSequences(account).local; - SignedActorChanges[] memory changeSets = new SignedActorChanges[](1); - changeSets[0] = _signedSet(account, seq, ACTOR_PK, changes); - - bytes32 userOpHash = keccak256("rotate-and-go"); - bytes memory opAuth = _buildK1Auth(newPk, userOpHash); - bytes memory signature = abi.encode(SIGNED_ACTOR_CHANGES_MAGIC, changeSets, opAuth); - PackedUserOperation memory userOp = _buildUserOp(account, signature); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 0); - assertTrue(accountConfiguration.isActor(account, newActorId)); - } - - /// @notice Multiple independently-signed change sets are applied in order: the owner authorizes key B, then - /// key B (now active) authorizes key C, all in one op. The op is then authenticated by C's `opAuth`. - function test_validateUserOp_appliesMultipleSignedActorChangeSets() public { - (address account,) = _create4337Account(ACTOR_PK); - - uint256 pkB = 101; - uint256 pkC = 102; - (AccountConfiguration.ActorChange[] memory changesB, bytes32 actorB) = _authorizeK1ActorChange(pkB); - (AccountConfiguration.ActorChange[] memory changesC, bytes32 actorC) = _authorizeK1ActorChange(pkC); - - uint64 seq = accountConfiguration.getChangeSequences(account).local; - SignedActorChanges[] memory changeSets = new SignedActorChanges[](2); - changeSets[0] = _signedSet(account, seq, ACTOR_PK, changesB); // signed by owner - changeSets[1] = _signedSet(account, seq + 1, pkB, changesC); // signed by B (active after set 0) - - bytes32 userOpHash = keccak256("chain-of-rotations"); - bytes memory opAuth = _buildK1Auth(pkC, userOpHash); // op signed by C (active after set 1) - bytes memory signature = abi.encode(SIGNED_ACTOR_CHANGES_MAGIC, changeSets, opAuth); - PackedUserOperation memory userOp = _buildUserOp(account, signature); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 0); - assertTrue(accountConfiguration.isActor(account, actorB)); - assertTrue(accountConfiguration.isActor(account, actorC)); - } - - /// @notice Applying changes does NOT authorize the op: a valid change set with an `opAuth` that does not sign - /// this userOpHash fails validation. - function test_validateUserOp_signedActorChanges_requiresOpAuth() public { - (address account,) = _create4337Account(ACTOR_PK); - - uint256 newPk = 101; - (AccountConfiguration.ActorChange[] memory changes,) = _authorizeK1ActorChange(newPk); - - uint64 seq = accountConfiguration.getChangeSequences(account).local; - SignedActorChanges[] memory changeSets = new SignedActorChanges[](1); - changeSets[0] = _signedSet(account, seq, ACTOR_PK, changes); - - bytes32 userOpHash = keccak256("rotate-but-no-op-auth"); - bytes memory opAuth = _buildK1Auth(999, userOpHash); // unauthorized key - bytes memory signature = abi.encode(SIGNED_ACTOR_CHANGES_MAGIC, changeSets, opAuth); - PackedUserOperation memory userOp = _buildUserOp(account, signature); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 1); - } - - /// @notice An invalid change authorization fails validation and applies nothing. - function test_validateUserOp_signedActorChanges_invalidChangeAuthFails() public { - (address account,) = _create4337Account(ACTOR_PK); - - uint256 newPk = 101; - (AccountConfiguration.ActorChange[] memory changes, bytes32 newActorId) = _authorizeK1ActorChange(newPk); - - uint64 seq = accountConfiguration.getChangeSequences(account).local; - SignedActorChanges[] memory changeSets = new SignedActorChanges[](1); - changeSets[0] = _signedSet(account, seq, 999, changes); // signed by a non-owner key - - bytes32 userOpHash = keccak256("op"); - bytes memory opAuth = _buildK1Auth(ACTOR_PK, userOpHash); - bytes memory signature = abi.encode(SIGNED_ACTOR_CHANGES_MAGIC, changeSets, opAuth); - PackedUserOperation memory userOp = _buildUserOp(account, signature); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 1); - assertFalse(accountConfiguration.isActor(account, newActorId)); - } - - /// @notice An empty change-set batch is rejected (it must not authorize any op). - function test_validateUserOp_signedActorChanges_emptyBatchFails() public { - (address account,) = _create4337Account(ACTOR_PK); - - SignedActorChanges[] memory changeSets = new SignedActorChanges[](0); - bytes32 userOpHash = keccak256("op"); - bytes memory opAuth = _buildK1Auth(ACTOR_PK, userOpHash); - bytes memory signature = abi.encode(SIGNED_ACTOR_CHANGES_MAGIC, changeSets, opAuth); - PackedUserOperation memory userOp = _buildUserOp(account, signature); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 1); - } - - // ── ERC-1271 signing (operational authority) ── - - function test_isValidSignature_nonOperationalActorCannotSign() public { - (address account,) = _create4337Account(ACTOR_PK); - uint256 scopedPk = 201; - // A payer-only (non-SENDER) actor is not operational. - _authorizeScopedActor(account, ACTOR_PK, scopedPk, SCOPE_SPONSOR_PAYER, address(0), bytes32(0)); - - bytes32 hash = keccak256("sign me"); - bytes memory authData = _buildK1Auth(scopedPk, hash); - - // A non-operational scoped actor cannot ERC-1271 sign, so validation must fail. - assertEq(DefaultAccount(payable(account)).isValidSignature(hash, authData), bytes4(0xFFFFFFFF)); - } - - function test_isValidSignature_operationalSenderSigns() public { - (address account,) = _create4337Account(ACTOR_PK); - uint256 senderPk = 202; - // A SENDER-without-POLICY actor is operational and can ERC-1271 sign. - _authorizeScopedActor(account, ACTOR_PK, senderPk, SCOPE_SENDER, address(0), bytes32(0)); - - bytes32 hash = keccak256("sign me"); - bytes memory authData = _buildK1Auth(senderPk, hash); - - assertEq(DefaultAccount(payable(account)).isValidSignature(hash, authData), bytes4(0x1626ba7e)); - } - - function test_isValidSignature_adminSucceeds() public { - (address account,) = _create4337Account(ACTOR_PK); - - // The unrestricted admin actor (scope == 0x00) is operational and can ERC-1271 sign. - bytes32 hash = keccak256("sign me"); - bytes memory authData = _buildK1Auth(ACTOR_PK, hash); - - assertEq(DefaultAccount(payable(account)).isValidSignature(hash, authData), bytes4(0x1626ba7e)); - } - - // ── SENDER scope (validateUserOp) ── - - function test_validateUserOp_senderScopeAuthorizes() public { - (address account,) = _create4337Account(ACTOR_PK); - uint256 senderPk = 203; - _authorizeScopedActor(account, ACTOR_PK, senderPk, SCOPE_SENDER, address(0), bytes32(0)); - - bytes32 userOpHash = keccak256("op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(senderPk, userOpHash)); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 0); - } - - function test_validateUserOp_requiresSenderScope() public { - (address account,) = _create4337Account(ACTOR_PK); - uint256 nonSenderPk = 204; - _authorizeScopedActor(account, ACTOR_PK, nonSenderPk, SCOPE_SPONSOR_PAYER, address(0), bytes32(0)); - - bytes32 userOpHash = keccak256("op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(nonSenderPk, userOpHash)); - - // An actor without SENDER scope cannot initiate transactions. - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 1); - } - - // ── SELF_PAYER scope (self-funded ops) ── - - function test_validateUserOp_selfFundedRequiresSelfPayerScope() public { - (address account,) = _create4337Account(ACTOR_PK); - vm.deal(account, 1 ether); - uint256 senderOnlyPk = 205; - _authorizeScopedActor(account, ACTOR_PK, senderOnlyPk, SCOPE_SENDER, address(0), bytes32(0)); - - bytes32 userOpHash = keccak256("op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(senderOnlyPk, userOpHash)); - - // SENDER but not SELF_PAYER: cannot authorize spending the account's funds on gas. - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0.1 ether), 1); - } - - function test_validateUserOp_senderSelfPayerScope_selfFundedSucceeds() public { - (address account,) = _create4337Account(ACTOR_PK); - vm.deal(account, 1 ether); - uint256 pk = 206; - _authorizeScopedActor(account, ACTOR_PK, pk, SCOPE_SENDER | SCOPE_SELF_PAYER, address(0), bytes32(0)); - - bytes32 userOpHash = keccak256("op"); - PackedUserOperation memory userOp = _buildUserOp(account, _buildK1Auth(pk, userOpHash)); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0.1 ether), 0); - } - - // ── SCOPE_POLICY (no native-dispatch call-target gating in this reduced 4337 bridge) ── - - function test_validateUserOp_policyScopeOnly_rejectedForLackingSenderScope() public { - (address account,) = _create4337Account(ACTOR_PK); - address policyManager = address(0xB0B); - uint256 pk = 207; - _authorizeScopedActor( - account, ACTOR_PK, pk, accountConfiguration.SCOPE_POLICY(), policyManager, keccak256("commit") - ); - - // A pure-SCOPE_POLICY actor lacks SCOPE_SENDER, so this reduced 4337 bridge rejects it outright — it does - // not replicate native-dispatch's policy-target call gating. - bytes memory callData = _executeBatchCallData(policyManager, 0, abi.encodeCall(UserOpMockTarget.setValue, (1))); - bytes32 userOpHash = keccak256(abi.encode("op", callData)); - PackedUserOperation memory userOp = _buildUserOp(account, callData, _buildK1Auth(pk, userOpHash)); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 1); - } - - function test_validateUserOp_policyAndSenderScope_authorizedWithoutTargetGating() public { - (address account,) = _create4337Account(ACTOR_PK); - address policyManager = address(0xB0B); - uint256 pk = 208; - uint8 scope = SCOPE_SENDER | accountConfiguration.SCOPE_POLICY(); - _authorizeScopedActor(account, ACTOR_PK, pk, scope, policyManager, keccak256("commit")); - - // An actor combining SCOPE_POLICY | SCOPE_SENDER is authorized here exactly like any other SENDER-scoped - // actor: this reduced 4337 bridge does not confine its calls to the policy target (that enforcement is - // native-dispatch, protocol-side behavior out of scope for this repo). - bytes memory callData = - _executeBatchCallData(address(target), 0, abi.encodeCall(UserOpMockTarget.setValue, (1))); - bytes32 userOpHash = keccak256(abi.encode("op", callData)); - PackedUserOperation memory userOp = _buildUserOp(account, callData, _buildK1Auth(pk, userOpHash)); - - vm.prank(ENTRY_POINT); - assertEq(BackwardsCompatible4337Account(payable(account)).validateUserOp(userOp, userOpHash, 0), 0); - } -} diff --git a/test/unit/accounts/UpgradeableAccount.t.sol b/test/unit/accounts/UpgradeableAccount.t.sol deleted file mode 100644 index 52f9388..0000000 --- a/test/unit/accounts/UpgradeableAccount.t.sol +++ /dev/null @@ -1,379 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.30; - -import {UpgradeableAccount} from "../../../src/accounts/UpgradeableAccount.sol"; -import {UpgradeableProxy} from "../../../src/accounts/UpgradeableProxy.sol"; -import {UUPSUpgradeable} from "solady/utils/UUPSUpgradeable.sol"; -import {Call, DefaultAccount} from "../../../src/accounts/DefaultAccount.sol"; -import {AccountConfiguration} from "../../../src/AccountConfiguration.sol"; -import {AccountConfigurationTest} from "../../lib/AccountConfigurationTest.sol"; - -contract MockTarget { - uint256 public value; - - function setValue(uint256 v) external payable { - value = v; - } - - function reverting() external pure { - revert("boom"); - } -} - -/// @dev A second implementation for testing upgrades. -contract UpgradeableAccountV2 is UpgradeableAccount { - constructor(address accountConfiguration) UpgradeableAccount(accountConfiguration) {} - - function isValidSignature(bytes32, bytes calldata) external pure override returns (bytes4) { - return bytes4(0xdeadbeef); - } - - function version() external pure returns (uint256) { - return 2; - } -} - -contract UpgradeableAccountTest is AccountConfigurationTest { - uint256 constant ACTOR_PK = 100; - uint256 constant SCOPED_PK = 101; - MockTarget public target; - address public upgradeableImpl; - - bytes32 constant SIGNED_UPGRADE_TYPEHASH = keccak256( - "SignedUpgrade(address account,address fromImplementation,address toImplementation,bytes32 dataHash)" - ); - - function setUp() public override { - super.setUp(); - target = new MockTarget(); - upgradeableImpl = address(new UpgradeableAccount(address(accountConfiguration))); - } - - function _createUpgradeableAccount(uint256 pk) internal returns (address account, bytes32 actorId) { - address signer = vm.addr(pk); - actorId = bytes32(bytes20(signer)); - - AccountConfiguration.InitialActor[] memory actors = new AccountConfiguration.InitialActor[](1); - actors[0] = AccountConfiguration.InitialActor({ - actorId: actorId, authenticator: address(k1Authenticator), scope: 0, policyData: "" - }); - - bytes memory proxyBytecode = UpgradeableProxy.bytecode(upgradeableImpl); - account = accountConfiguration.createAccount(bytes32(0), proxyBytecode, actors); - } - - function _singleCall(address t, uint256 v, bytes memory d) internal pure returns (Call[] memory calls) { - calls = new Call[](1); - calls[0] = Call(t, v, d); - } - - // ── Proxy basics ── - - function test_proxyDelegatesToDefault() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - - bytes32 hash = keccak256("test"); - bytes memory authData = _buildK1Auth(ACTOR_PK, hash); - - bytes4 result = UpgradeableAccount(payable(account)).isValidSignature(hash, authData); - assertEq(result, bytes4(0x1626ba7e)); - } - - function test_proxyBytecodeLength() public view { - bytes memory proxyBytecode = UpgradeableProxy.bytecode(upgradeableImpl); - assertEq(proxyBytecode.length, 93); - } - - function test_deterministicAddress() public { - address signer = vm.addr(ACTOR_PK); - bytes32 actorId = bytes32(bytes20(signer)); - - AccountConfiguration.InitialActor[] memory actors = new AccountConfiguration.InitialActor[](1); - actors[0] = AccountConfiguration.InitialActor({ - actorId: actorId, authenticator: address(k1Authenticator), scope: 0, policyData: "" - }); - - bytes memory proxyBytecode = UpgradeableProxy.bytecode(upgradeableImpl); - address predicted = accountConfiguration.computeAddress(bytes32(0), proxyBytecode, actors); - - (address actual,) = _createUpgradeableAccount(ACTOR_PK); - assertEq(actual, predicted); - } - - // ── Caller authorization ── - - function test_selfIsAlwaysAuthorized() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - assertTrue(UpgradeableAccount(payable(account)).isAuthorizedCaller(account)); - } - - // ── executeBatch ── - - function test_executeBatch_success() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - - vm.prank(account); - UpgradeableAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0, abi.encodeCall(MockTarget.setValue, (42)))); - - assertEq(target.value(), 42); - } - - function test_executeBatch_withETHValue() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - vm.deal(account, 1 ether); - - vm.prank(account); - UpgradeableAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0.5 ether, abi.encodeCall(MockTarget.setValue, (1)))); - - assertEq(address(target).balance, 0.5 ether); - } - - function test_executeBatch_revertsFromUnauthorizedCaller() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - - vm.prank(address(0xdead)); - vm.expectRevert(DefaultAccount.UnauthorizedCaller.selector); - UpgradeableAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0, abi.encodeCall(MockTarget.setValue, (1)))); - } - - function test_executeBatch_revertsOnFailedCall() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - - vm.prank(account); - vm.expectRevert(DefaultAccount.CallFailed.selector); - UpgradeableAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0, abi.encodeCall(MockTarget.reverting, ()))); - } - - // ── UUPS upgrade ── - - /// @dev A plain self-call to upgradeToAndCall must revert: {_authorizeUpgrade} is gated on the one-shot - /// `_upgradeAuthorized` flag, not on `msg.sender == address(this)`, so upgrading always requires going - /// through {upgradeBySignature}'s unrestricted-owner-scoped signature check. - function test_upgrade_revertsFromDirectSelfCall() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - vm.prank(account); - vm.expectRevert(UpgradeableAccount.UpgradeNotInitiated.selector); - UpgradeableAccount(payable(account)).upgradeToAndCall(address(v2Impl), ""); - } - - function test_upgrade_revertsFromNonSelf() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - vm.prank(address(0xdead)); - vm.expectRevert(UpgradeableAccount.UpgradeNotInitiated.selector); - UpgradeableAccount(payable(account)).upgradeToAndCall(address(v2Impl), ""); - } - - function test_upgrade_executeBatchStillWorks() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - _signedUpgrade(account, ACTOR_PK, address(0), address(v2Impl), ""); - - vm.prank(account); - UpgradeableAccount(payable(account)) - .executeBatch(_singleCall(address(target), 0, abi.encodeCall(MockTarget.setValue, (999)))); - - assertEq(target.value(), 999); - } - - /// @dev Closes the gap the plain self-call check would otherwise leave open: batching a call to - /// `upgradeToAndCall` targeting `address(this)` makes that inner call's `msg.sender == address(this)` - /// too, but `executeBatch` never checks for an unrestricted-owner scope specifically (only that the - /// caller is authorized to drive calls at all, e.g. any SENDER-scoped actor) — so this must revert - /// regardless of who can call `executeBatch`. - function test_upgrade_viaExecuteBatch_reverts() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - Call[] memory calls = new Call[](1); - calls[0] = Call(account, 0, abi.encodeCall(UUPSUpgradeable.upgradeToAndCall, (address(v2Impl), ""))); - - vm.prank(account); - vm.expectRevert(DefaultAccount.CallFailed.selector); - UpgradeableAccount(payable(account)).executeBatch(calls); - } - - // ── isValidSignature ── - - function test_isValidSignature_validK1() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - - bytes32 hash = keccak256("validate me"); - bytes memory authData = _buildK1Auth(ACTOR_PK, hash); - - bytes4 result = UpgradeableAccount(payable(account)).isValidSignature(hash, authData); - assertEq(result, bytes4(0x1626ba7e)); - } - - function test_isValidSignature_invalidSignature() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - - bytes32 hash = keccak256("validate me"); - bytes memory authData = _buildK1Auth(999, hash); - - bytes4 result = UpgradeableAccount(payable(account)).isValidSignature(hash, authData); - assertEq(result, bytes4(0xFFFFFFFF)); - } - - // ── upgradeBySignature (owner-signed, compare-and-swap) ── - - function _upgradeDigest(address account, address from, address to, bytes memory data) - internal - pure - returns (bytes32) - { - return keccak256(abi.encode(SIGNED_UPGRADE_TYPEHASH, account, from, to, keccak256(data))); - } - - /// @dev Submits upgradeBySignature as an arbitrary relayer (no prank): the signature is what authorizes it. - function _signedUpgrade(address account, uint256 pk, address from, address to, bytes memory data) internal { - bytes32 digest = _upgradeDigest(account, from, to, data); - bytes memory auth = _buildK1Auth(pk, digest); - UpgradeableAccount(payable(account)).upgradeBySignature(from, to, data, auth); - } - - function _addScopedActor(address account, uint256 ownerPk, uint256 newPk, uint8 scope) internal { - bytes32 newActorId = bytes32(bytes20(vm.addr(newPk))); - AccountConfiguration.ActorChange[] memory changes = new AccountConfiguration.ActorChange[](1); - changes[0] = AccountConfiguration.ActorChange({ - actorId: newActorId, - changeType: 0x01, - data: abi.encode( - AccountConfiguration.ActorConfig({authenticator: address(k1Authenticator), scope: scope, expiry: 0}), - bytes("") - ) - }); - - uint64 seq = accountConfiguration.getChangeSequences(account).local; - bytes32 digest = _computeActorChangeBatchDigest(account, uint64(block.chainid), seq, changes); - accountConfiguration.applySignedActorChanges( - account, uint64(block.chainid), changes, _buildK1Auth(ownerPk, digest) - ); - } - - function test_upgradeBySignature_fromZero_success() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - // Fresh account: ERC-1967 slot is unset, so the compare-and-swap `from` is address(0). - _signedUpgrade(account, ACTOR_PK, address(0), address(v2Impl), ""); - - assertEq(UpgradeableAccountV2(payable(account)).version(), 2); - } - - function test_upgradeBySignature_withInitData() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - // `data` is delegatecalled on the new implementation post-upgrade, so the selector must exist there. - bytes memory initData = abi.encodeCall(UpgradeableAccountV2.version, ()); - _signedUpgrade(account, ACTOR_PK, address(0), address(v2Impl), initData); - - assertEq(UpgradeableAccountV2(payable(account)).version(), 2); - } - - function test_upgradeBySignature_chained() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2a = new UpgradeableAccountV2(address(accountConfiguration)); - UpgradeableAccountV2 v2b = new UpgradeableAccountV2(address(accountConfiguration)); - - _signedUpgrade(account, ACTOR_PK, address(0), address(v2a), ""); - // Second hop: `from` is now the previously installed implementation. - _signedUpgrade(account, ACTOR_PK, address(v2a), address(v2b), ""); - - assertEq(UpgradeableAccountV2(payable(account)).version(), 2); - } - - function test_upgradeBySignature_anyRelayerCanSubmit() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - bytes32 digest = _upgradeDigest(account, address(0), address(v2Impl), ""); - bytes memory auth = _buildK1Auth(ACTOR_PK, digest); - - vm.prank(address(0xBEEF)); // unrelated relayer - UpgradeableAccount(payable(account)).upgradeBySignature(address(0), address(v2Impl), "", auth); - - assertEq(UpgradeableAccountV2(payable(account)).version(), 2); - } - - function test_upgradeBySignature_revertsStaleFrom() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - // Current slot is address(0), but the signature claims `from = upgradeableImpl`. - bytes32 digest = _upgradeDigest(account, upgradeableImpl, address(v2Impl), ""); - bytes memory auth = _buildK1Auth(ACTOR_PK, digest); - - vm.expectRevert(UpgradeableAccount.UpgradeFromMismatch.selector); - UpgradeableAccount(payable(account)).upgradeBySignature(upgradeableImpl, address(v2Impl), "", auth); - } - - function test_upgradeBySignature_revertsReplay() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - bytes32 digest = _upgradeDigest(account, address(0), address(v2Impl), ""); - bytes memory auth = _buildK1Auth(ACTOR_PK, digest); - - UpgradeableAccount(payable(account)).upgradeBySignature(address(0), address(v2Impl), "", auth); - - // Slot now holds v2Impl, so the same signature (from == address(0)) no longer matches: replay fails. - vm.expectRevert(UpgradeableAccount.UpgradeFromMismatch.selector); - UpgradeableAccount(payable(account)).upgradeBySignature(address(0), address(v2Impl), "", auth); - } - - function test_upgradeBySignature_revertsNonAdminScope() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - // Admin is exactly scope == 0: a scoped key (any non-zero scope) cannot authorize an upgrade. - _addScopedActor(account, ACTOR_PK, SCOPED_PK, accountConfiguration.SCOPE_SPONSOR_PAYER()); - - bytes32 digest = _upgradeDigest(account, address(0), address(v2Impl), ""); - bytes memory auth = _buildK1Auth(SCOPED_PK, digest); - - vm.expectRevert(UpgradeableAccount.UpgradeUnauthorized.selector); - UpgradeableAccount(payable(account)).upgradeBySignature(address(0), address(v2Impl), "", auth); - } - - function test_upgradeBySignature_revertsInvalidSignature() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2Impl = new UpgradeableAccountV2(address(accountConfiguration)); - - bytes32 digest = _upgradeDigest(account, address(0), address(v2Impl), ""); - bytes memory badAuth = _buildK1Auth(999, digest); // not an authorized actor - - vm.expectRevert(); - UpgradeableAccount(payable(account)).upgradeBySignature(address(0), address(v2Impl), "", badAuth); - } - - /// @dev Documents the accepted compare-and-swap ABA caveat (deferred for audit/discussion): if the - /// implementation is upgraded away from and later restored to a prior value, a previously-used signature - /// for that transition becomes replayable again. There is no deadline; this is the accepted trade-off for - /// coordination-free, nonce-less multichain upgrades, on the assumption that downgrades are rare. - function test_upgradeBySignature_abaReplayIsPossible() public { - (address account,) = _createUpgradeableAccount(ACTOR_PK); - UpgradeableAccountV2 v2a = new UpgradeableAccountV2(address(accountConfiguration)); - UpgradeableAccountV2 v2b = new UpgradeableAccountV2(address(accountConfiguration)); - - // 0 -> v2a, then cycle v2a -> v2b -> v2a (a downgrade restores the v2a state). - _signedUpgrade(account, ACTOR_PK, address(0), address(v2a), ""); - _signedUpgrade(account, ACTOR_PK, address(v2a), address(v2b), ""); - _signedUpgrade(account, ACTOR_PK, address(v2b), address(v2a), ""); - - // Slot is back at v2a, so a v2a -> v2b signature is honored again (state, not nonce, gates it). - bytes32 digestAtoB = _upgradeDigest(account, address(v2a), address(v2b), ""); - bytes memory authAtoB = _buildK1Auth(ACTOR_PK, digestAtoB); - UpgradeableAccount(payable(account)).upgradeBySignature(address(v2a), address(v2b), "", authAtoB); - assertEq(UpgradeableAccountV2(payable(account)).version(), 2); - } -} diff --git a/test/unit/examples/ExternalPolicyCaller.t.sol b/test/unit/examples/ExternalPolicyCaller.t.sol index ec47b86..d8443b9 100644 --- a/test/unit/examples/ExternalPolicyCaller.t.sol +++ b/test/unit/examples/ExternalPolicyCaller.t.sol @@ -4,9 +4,9 @@ pragma solidity ^0.8.30; import {AccountConfiguration} from "../../../src/AccountConfiguration.sol"; import {TRUSTED_EXECUTOR} from "../../../src/accounts/DefaultAccount.sol"; -import {PolicyManager, EXTERNAL_POLICY_AUTHENTICATOR} from "../../../src/examples/policies/PolicyManager.sol"; -import {SessionPolicy} from "../../../src/examples/policies/SessionPolicy.sol"; -import {RecurringAllowance} from "../../../src/examples/policies/RecurringAllowance.sol"; +import {PolicyManager, EXTERNAL_POLICY_AUTHENTICATOR} from "../../../src/policies/PolicyManager.sol"; +import {SessionPolicy} from "../../../src/policies/SessionPolicy.sol"; +import {RecurringAllowance} from "../../../src/policies/RecurringAllowance.sol"; import {AccountConfigurationTest} from "../../lib/AccountConfigurationTest.sol"; diff --git a/test/unit/examples/PolicyManager.t.sol b/test/unit/examples/PolicyManager.t.sol index 1532c96..2481c37 100644 --- a/test/unit/examples/PolicyManager.t.sol +++ b/test/unit/examples/PolicyManager.t.sol @@ -5,9 +5,9 @@ import {AccountConfiguration} from "../../../src/AccountConfiguration.sol"; import {ITransactionContext, TX_CONTEXT_ADDRESS} from "../../../src/interfaces/ITransactionContext.sol"; import {TRUSTED_EXECUTOR} from "../../../src/accounts/DefaultAccount.sol"; -import {PolicyManager} from "../../../src/examples/policies/PolicyManager.sol"; -import {Policy} from "../../../src/examples/policies/Policy.sol"; -import {SessionPolicy} from "../../../src/examples/policies/SessionPolicy.sol"; +import {PolicyManager} from "../../../src/policies/PolicyManager.sol"; +import {Policy} from "../../../src/policies/Policy.sol"; +import {SessionPolicy} from "../../../src/policies/SessionPolicy.sol"; import {AccountConfigurationTest} from "../../lib/AccountConfigurationTest.sol"; diff --git a/test/unit/examples/SessionPolicy.t.sol b/test/unit/examples/SessionPolicy.t.sol index a06de82..862e63f 100644 --- a/test/unit/examples/SessionPolicy.t.sol +++ b/test/unit/examples/SessionPolicy.t.sol @@ -5,9 +5,9 @@ import {AccountConfiguration} from "../../../src/AccountConfiguration.sol"; import {ITransactionContext, TX_CONTEXT_ADDRESS} from "../../../src/interfaces/ITransactionContext.sol"; import {TRUSTED_EXECUTOR} from "../../../src/accounts/DefaultAccount.sol"; -import {PolicyManager} from "../../../src/examples/policies/PolicyManager.sol"; -import {SessionPolicy} from "../../../src/examples/policies/SessionPolicy.sol"; -import {RecurringAllowance} from "../../../src/examples/policies/RecurringAllowance.sol"; +import {PolicyManager} from "../../../src/policies/PolicyManager.sol"; +import {SessionPolicy} from "../../../src/policies/SessionPolicy.sol"; +import {RecurringAllowance} from "../../../src/policies/RecurringAllowance.sol"; import {AccountConfigurationTest} from "../../lib/AccountConfigurationTest.sol"; diff --git a/test/unit/examples/SessionPolicyGas.t.sol b/test/unit/examples/SessionPolicyGas.t.sol index e901c3d..40f0689 100644 --- a/test/unit/examples/SessionPolicyGas.t.sol +++ b/test/unit/examples/SessionPolicyGas.t.sol @@ -7,8 +7,8 @@ import {AccountConfiguration} from "../../../src/AccountConfiguration.sol"; import {ITransactionContext, TX_CONTEXT_ADDRESS} from "../../../src/interfaces/ITransactionContext.sol"; import {TRUSTED_EXECUTOR} from "../../../src/accounts/DefaultAccount.sol"; -import {PolicyManager} from "../../../src/examples/policies/PolicyManager.sol"; -import {SessionPolicy} from "../../../src/examples/policies/SessionPolicy.sol"; +import {PolicyManager} from "../../../src/policies/PolicyManager.sol"; +import {SessionPolicy} from "../../../src/policies/SessionPolicy.sol"; import {AccountConfigurationTest} from "../../lib/AccountConfigurationTest.sol"; import {SessionMockERC20, SessionMockTarget} from "./SessionPolicy.t.sol"; From 9b3deff2a44bd9495f3a81e9f73800d016dc69bf Mon Sep 17 00:00:00 2001 From: Chris Hunter Date: Tue, 14 Jul 2026 12:26:02 -0400 Subject: [PATCH 2/2] revert: drop DelegateAuthenticator recursive-delegation changes Reverts the cross-instance recursive-delegation guard from this PR; the change is not needed here. --- src/authenticators/DelegateAuthenticator.sol | 27 +++----------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/src/authenticators/DelegateAuthenticator.sol b/src/authenticators/DelegateAuthenticator.sol index a9f070a..dd492a6 100644 --- a/src/authenticators/DelegateAuthenticator.sol +++ b/src/authenticators/DelegateAuthenticator.sol @@ -20,8 +20,7 @@ contract DelegateAuthenticator is IAuthenticator { /// @notice The auth data is shorter than the 40-byte delegate + nested-authenticator prefix. error InvalidDataLength(); - /// @notice The nested authenticator is itself a delegate authenticator (this instance or any other); only one - /// delegation hop is permitted, so a delegate may never vouch through another delegate. + /// @notice The nested authenticator points back to this contract; only one delegation hop is permitted. error RecursiveDelegation(); /// @notice The nested signer is not authorized to sign for the delegate account. @@ -33,18 +32,11 @@ contract DelegateAuthenticator is IAuthenticator { ACCOUNT_CONFIGURATION = AccountConfiguration(accountConfiguration); } - /// @notice Self-identifying marker so one delegate authenticator can detect another (of any instance) and refuse - /// to delegate through it, enforcing the single-hop invariant across separately deployed instances. - /// @return Always true for a delegate authenticator. - function isDelegateAuthenticator() external pure returns (bool) { - return true; - } - /// @notice Authenticates by delegating to another account's actor configuration; only one hop is permitted. /// /// @dev Reverts with InvalidDataLength when `data` is shorter than 40 bytes. - /// @dev Reverts with RecursiveDelegation when the nested authenticator is this contract or any other delegate - /// authenticator (recursive/chained delegation is not permitted; only a single hop). + /// @dev Reverts with RecursiveDelegation when the nested authenticator is this contract (recursive delegation + /// is not permitted). /// @dev Reverts with InvalidNestedSignature when the delegate account does not validate the nested signature. /// /// @param hash The digest being authenticated. @@ -58,20 +50,9 @@ contract DelegateAuthenticator is IAuthenticator { actorId = bytes32(bytes20(delegate)); - // Prevent recursive delegation (only 1 hop permitted). Rejecting this exact instance is not enough: two - // distinct delegate authenticators A and B could otherwise vouch for each other (A->B->A...), so we also - // reject any nested authenticator that self-identifies as a delegate authenticator. Honest delegate - // authenticators expose isDelegateAuthenticator(); a contract that hides it is not a delegate and cannot form - // a delegate cycle through this guard. Only probe addresses with code — the K1 sentinel (address(1), - // ecrecover) and other precompiles have none and are never delegates, so we skip them to avoid a spurious - // precompile call. + // Prevent recursive delegation (only 1 hop permitted) address nestedAuthenticator = address(bytes20(nestedAuth[:20])); if (nestedAuthenticator == address(this)) revert RecursiveDelegation(); - if (nestedAuthenticator.code.length > 0) { - try DelegateAuthenticator(nestedAuthenticator).isDelegateAuthenticator() returns (bool isDelegate) { - if (isDelegate) revert RecursiveDelegation(); - } catch {} - } // The nested actor MUST be the admin (scope == 0x00) of the delegate account. This is enforced // independently of verifySignature, which is now operational (signing is not admin-only, but a delegate