Code review hardening: paymaster client, broadcast panic, eslint, owner rotation - #68
Open
flwrenn wants to merge 6 commits into
Open
Code review hardening: paymaster client, broadcast panic, eslint, owner rotation#68flwrenn wants to merge 6 commits into
flwrenn wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies post-code-review hardening across contracts, indexer, and frontend tooling/UI. It fixes a crash in WebSocket broadcasting, updates the frontend’s paymaster integration to Pimlico’s free-tier-compatible client, restores/strengthens ESLint behavior, and adds owner rotation to the SmartAccount contract alongside updated documentation.
Changes:
- Indexer: fix Hub broadcast loop to avoid sending on a closed channel; add a regression test covering multi-message batches.
- Frontend: switch paymaster integration to Pimlico client + fee estimation and restore/resolve ESLint configuration and lint errors.
- Contracts + docs: add
transferOwnership+ tests and document key account limitations/trade-offs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents additional account limitations and trade-offs (with new ownership/session-key implications). |
| indexer/internal/api/hub.go | Prevents process-killing panic by correctly skipping remaining sends for a dropped slow client. |
| indexer/internal/api/hub_test.go | Adds a regression test ensuring multi-message broadcasts drop a slow client safely and deliver to healthy clients. |
| frontend/src/routes/session/+page.svelte | Adds targeted ESLint suppression for external Etherscan navigation. |
| frontend/src/routes/indexer/+page.svelte | Adds targeted ESLint suppression for control-regex usage and external navigation. |
| frontend/src/routes/+page.svelte | Adds targeted ESLint suppression for external Etherscan navigation. |
| frontend/src/routes/+layout.svelte | Uses SvelteKit resolve() for internal navigation links. |
| frontend/src/lib/userOp.ts | Introduces shared bundler/paymaster client helper using Pimlico client + fee estimation. |
| frontend/src/lib/components/StatsPanel.svelte | Minor rendering tweak (em dash literal). |
| frontend/src/lib/components/SessionKeyManager.svelte | Fixes reactivity/ESLint concerns and adds keyed each-blocks. |
| frontend/src/lib/components/FaucetTokenCard.svelte | Adjusts reactivity dependency tracking; adds ESLint suppression for external navigation. |
| frontend/src/lib/components/CounterCard.svelte | Adjusts reactivity dependency tracking; adds ESLint suppression for external navigation. |
| frontend/src/lib/account.svelte.ts | Deduplicates bundler/paymaster setup by using the shared helper for deploy flow. |
| frontend/eslint.config.js | Enforces consistent unused-var handling (underscore-prefixed ignores). |
| frontend/.gitignore | Restores ignore file needed by ESLint config’s ignore-file inclusion. |
| contracts/test/SmartAccount.t.sol | Adds tests for ownership transfer and signature behavior after rotation. |
| contracts/src/SmartAccount.sol | Adds transferOwnership, event/error wiring, and documentation comments for ownership rotation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+190
to
+204
| /// @notice Transfer ownership of this account to a new EOA. Callable by the | ||
| /// current owner directly or via the EntryPoint (owner-signed UserOp). | ||
| /// Session keys cannot reach this function — same rationale as | ||
| /// registerSessionKey: a self-call via execute has msg.sender = | ||
| /// address(this), which fails the modifier. | ||
| /// @dev Note: SmartAccountFactory.getAddress(owner, salt) derives the | ||
| /// counterfactual address from the INITIAL owner — after a transfer, | ||
| /// the factory mapping no longer corresponds to the current owner. | ||
| /// @param newOwner The EOA that will own this account. Must not be address(0). | ||
| function transferOwnership(address newOwner) external onlyOwnerOrEntryPoint { | ||
| if (newOwner == address(0)) revert InvalidNewOwner(); | ||
| address previousOwner = owner; | ||
| owner = newOwner; | ||
| emit OwnershipTransferred(previousOwner, newOwner); | ||
| } |
Comment on lines
+251
to
+254
| - **No upgrade path.** Accounts sit behind ERC1967 proxies, but no UUPS upgrade authorization is exposed — the implementation address is fixed at deploy time. A bug in the implementation cannot be patched for existing accounts. | ||
| - **No recovery mechanism.** `transferOwnership` requires the current owner's authorization, so a lost owner key still bricks the account — there is no social recovery or guardian scheme. A compromised-but-not-lost key, however, can be rotated out via `transferOwnership`. | ||
| - **Session key fee surface.** Session keys don't constrain UserOp gas fields or paymaster choice. An in-scope compromised session key can grief a self-funded account's EntryPoint deposit by submitting ops with inflated gas parameters — the prefund is drawn from the account's deposit. | ||
| - **Selector-level scoping only.** Session key scope stops at the 4-byte selector — there are no argument-level constraints. A key scoped to `transfer(address,uint256)` on a token can send the full balance to any address. |
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
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.
What
Post-review hardening across all three components, plus owner rotation:
pm_*methods (paid tier) topimlico_*methods viacreatePimlicoClient, with a shared bundler-client helper deduplicatingdeploy()andsendUserOpsetupbreakin the select only exited the select, so the next iteration sent on a closed channel and killed the whole process; now a labeled continue, with a multi-message regression test.gitignorerequired by eslint config —includeIgnoreFilecrashed on ENOENT, so lint had been silently brokentransferOwnership— mirrors the existingonlyOwnerOrEntryPointauth, emitsOwnershipTransferred, 7 new tests including a session-key self-call escalation negativeWhy
A full code review found a process-killing crash in the indexer hub, a paymaster integration on the paid-tier API path, silently broken lint tooling, and no way to rotate a compromised owner key. This PR fixes all four and documents the remaining known gaps.
Scope
How to verify
cd contracts && forge test -vvv— 100 passed (was 93)cd indexer && go test ./...— all pass; the newTestBroadcastMultipleMessagesDropsSlowClientOncepanics against the pre-fix hubcd frontend && pnpm lint && pnpm build— 0 errors, clean buildRelated issues
None closed. #65, #66, #67 were filed from the same review as deferred low-severity follow-ups.