Account security hardening#39
Merged
Merged
Conversation
Two under-documented sharp edges in the reference session policy: - Value-moving selectors this policy cannot decode (increase/decreaseAllowance, ERC-721 safeTransferFrom, ERC-1155 transfers) are gated by the allowlist but NOT debited from a token's spend cap, so they bypass it if listed. - `approve` is debited at grant time, but a standing ERC-20 allowance outlives the period and is pulled by a third party, so cross-period reuse can exceed a single period's cap. Also documents the calldata-length handling (empty receive allowed, 1-3 bytes rejected, 4+ treated as a selector). Doc-only.
…act example wallets 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.
Reverts the cross-instance recursive-delegation guard from this PR; the change is not needed here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security hardening of the account and session policy, plus a repo reorg that moves the example policies to a top-level
src/policies/and extracts the unaudited example wallets into a separate repository (base/eip-8130-examples).This branch also carries the doc-only commit documenting SessionPolicy spend-cap / approval limitations (
b640d5c).Security fixes
targetis the account itself at install (SelfTargetNotAllowed). The account is always an authorized caller of its ownexecuteBatch, so letting a session key target it allowed re-entering with an arbitrary, unchecked batch that bypassed every policy dimension.transferFromsource: enforcefrom == account, so a session key may only spend the account's own resources, not third-party allowances the account holds._isAuthorizedCallernow (a) enforces the actor'sexpiry(previously silently ignored on the execution path) and (b) requires operational/sender authority — admin (scope 0) orSCOPE_SENDERwithoutSCOPE_POLICY— mirroringAccountConfiguration.verifySignature, so a policy-gated actor can't get directexecuteBatch.Note:
WebAuthnAuthenticator'srequireUV = falsewas intentionally left unchanged. The DelegateAuthenticator recursive-delegation guard was explored but dropped from this PR (see the revert commit).Repo reorg
src/examples/policies/*→src/policies/*and re-pointed all test/script imports.UpgradeableAccount,UpgradeableProxy,BackwardsCompatible4337Account) and their tests to base/eip-8130-examples; trimmedscript/Deploy.s.soland updatedREADME.md/ NatSpec references.Test plan
forge build(core) passesforge test(core) — 309 passing