fix(registry): align AgentRegistry with IAgentIdentityRegistry + close #37#38
Conversation
- Add src/registries/AgentRegistry.sol: reverse-lookup contract mapping agent execution wallets to their human AirAccount owners, with registerAgent/deregisterAgent, isRegisteredAgent, balanceOf, and enumeration helpers required by SuperPaymaster. - Fix setAgentWallet() in AAStarAirAccountBase.sol: replace silent best-effort ERC-8004 call with hard-fail AgentRegistry.registerAgent() call; add extcodesize guard to reject EOA registries; add AgentRegistrationFailed custom error. - Add test/AgentRegistry.t.sol: 15 comprehensive tests covering registration, deregistration, access control, enumeration, balanceOf, and full account→registry integration. - Update test/AAStarAirAccountV7_M7.t.sol: update MockRegistry to match new registerAgent(address) interface; fix setAgentWallet tests to expect hard-failure behavior. - Add scripts/deploy-agent-registry.ts: viem-based deploy script for Sepolia. 709 tests passing (was 680).
…#37) - Fix balanceOf() to return actual agent count (was capped at 0 or 1) - Add ownerOf(uint256) ERC-721 stub that returns address(0) for SuperPaymaster IAgentIdentityRegistry interface compatibility - Add revokeAgent(address) as alias for deregisterAgent, matching the IAgentIdentityRegistry.revokeAgent() method name - Add getHumanOwner(address) convenience lookup: agentWallet → owner - Add getAgents(address) convenience alias for ownerAgents[] array Tests: 5 new AgentRegistry tests; 704 total passing (was 709 on m8 branch before cherry-pick, 704 on this slice from main).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e016f56bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (bool ok,) = agentRegistry.call( | ||
| abi.encodeWithSignature("registerAgent(address)", agentWallet) | ||
| ); | ||
| (ok); // silence unused variable warning | ||
| if (!ok) revert AgentRegistrationFailed(); |
There was a problem hiding this comment.
setAgentWallet treats any non-reverting low-level call as success, but that does not guarantee registerAgent(address) actually executed (a contract with a permissive fallback can return success without updating state). In that case the function still emits AgentWalletSet, leaving the account in a false-success state where downstream sponsorship checks fail because the wallet was never truly registered.
Useful? React with 👍 / 👎.
| const pub0 = createPublicClient({ chain: sepolia, transport: http(RPC_URLS[0]) }); | ||
| const bal = await pub0.getBalance({ address: owner.address }); |
There was a problem hiding this comment.
The script claims multi-RPC resiliency, but it performs an unconditional getBalance against RPC_URLS[0] before entering the retry loop. If the first endpoint is down or rate-limited, deployment aborts before trying SEPOLIA_RPC_URL2/3, so the fallback mechanism never runs in exactly the failure mode it is meant to handle.
Useful? React with 👍 / 👎.
- registerAgent() now requires an ECDSA signature from agentWallet proving the caller controls it, preventing griefing/front-run attacks (HIGH-1) - Add InvalidAgentSignature error; hash includes chainId, registry address, humanOwner, and agentWallet to prevent cross-chain/cross-registry replay - Replace O(n) linear scan in deregisterAgent/revokeAgent with O(1) swap-and-pop backed by _agentIndexPlusOne index mapping (MEDIUM) - setAgentWallet() in AAStarAirAccountBase accepts agentWalletSig and forwards it to AgentRegistry.registerAgent(address,bytes) - Update MockRegistry in M7 test to match new registerAgent signature - Add 4 new AgentRegistry tests: validSig_succeeds, invalidSig_reverts, frontRunPrevented, DeregisterAgent_O1_multipleAgents
Security fixes applied (commit a63417d)HIGH-1 fixed:
|
…try field, pagination - registerAgent: revert SelfRegistrationForbidden when agentWallet == msg.sender - ownerOf(): revert NotSupported instead of returning address(0) - AgentWalletSet event: add agentRegistry field for auditability - getAgentsPage(): paginated enumeration for large agent lists - Tests: update ownerOf test, add pagination tests, fix emit assertions
… wallets, LOW-1 overflow - HIGH-1 (AgentRegistry): require msg.sender to implement accountId() with "airaccount." prefix; plain EOAs and unrelated contracts now revert with CallerNotAirAccount - HIGH-2 (AgentRegistry): support ERC-1271 smart-contract agent wallets in addition to EOA ECDSA; extcodesize dispatch selects ECDSA.recover for EOAs and isValidSignature() for contracts - LOW-1 (AgentRegistry): overflow-safe getAgentsPage; replace start+count with remaining = total-start, size = min(count,remaining) to prevent uint256 wraparound - test/AgentRegistry.t.sol: full rewrite with MockAirAccount wrapper (implements accountId()), MockERC1271Wallet, HIGH-1 EOA/no-accountId revert tests, HIGH-2 ERC-1271 success/wrong-magic tests, LOW-1 countZero and overflowSafe pagination tests; 39 tests total - Total suite: 719 tests, all pass
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac4d621d7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!ok || data.length < 32) revert CallerNotAirAccount(); | ||
| if (!_startsWith(abi.decode(data, (string)), "airaccount.")) revert CallerNotAirAccount(); |
There was a problem hiding this comment.
registerAgent currently accepts any contract whose accountId() string starts with "airaccount.", which is trivially spoofable by unrelated contracts. In environments where this registry is used to gate SuperPaymaster sponsorship, an attacker can deploy a fake contract that returns that prefix and register agent wallets as if they were valid AirAccount owners, defeating the intended caller restriction.
Useful? React with 👍 / 👎.
| function ownerOf(uint256) external pure returns (address) { | ||
| revert NotSupported(); |
There was a problem hiding this comment.
This method is documented as an interface-compatibility shim, but it unconditionally reverts. Any integrator that performs ERC-721-style probing via ownerOf (including paymaster-side compatibility logic) will hard-fail the whole flow instead of receiving a non-owner sentinel result, which breaks interoperability in the exact path this shim is meant to support.
Useful? React with 👍 / 👎.
fanhousanbu
left a comment
fanhousanbu
left a comment
There was a problem hiding this comment.
Review: APPROVE ✅
Six-commit progressive hardening — well structured.
What's good:
- HIGH-1:
registerAgentrequires ECDSA sig fromagentWallet— prevents front-run griefing - HIGH-2: ERC-1271 support for smart-contract agent wallets
- HIGH-1 caller check:
msg.sendermust implementaccountId()with "airaccount." prefix - O(1) swap-and-pop replaces O(n) linear scan in deregister
- Overflow-safe pagination
- 719 tests passing
Minor note:
The accountId() prefix check is a soft guard — any contract can implement the interface without being a real AirAccount. This is acceptable as a defense-in-depth layer (primary security is the signature), but worth documenting so future maintainers don't treat it as a hard identity guarantee.
Re PR#34: This PR fully supersedes #34. Recommend closing #34 directly to avoid confusion.
2 participants
Summary
Closes #37
Already done (from PR #34 / commit a9c11ff)
src/registries/AgentRegistry.sol— initial implementation withregisterAgent,deregisterAgent,isRegisteredAgent,balanceOf, enumeration helperssetAgentWallet()fix inAAStarAirAccountBase.sol— replaced silent best-effort ERC-8004 call with hard-failAgentRegistry.registerAgent()call; addedextcodesizeguard; addedAgentRegistrationFailedcustom errortest/AgentRegistry.t.sol— 15 tests covering registration, deregistration, access control, enumeration, and account→registry integrationThis PR adds (issue #37 alignment)
balanceOf()count fix: returnsownerAgents[owner].length(actual count) instead of the old> 0 ? 1 : 0capownerOf(uint256): ERC-721 stub always returningaddress(0), required byIAgentIdentityRegistryinterface expected by SuperPaymasterrevokeAgent(address): alias forderegisterAgent, matching the method name inIAgentIdentityRegistrygetHumanOwner(address): convenience reverse-lookupagentWallet → humanOwnergetAgents(address): convenience full-array accessor forownerAgents[humanOwner]test_BalanceOf_returnsActualCount,test_OwnerOf_returnsZero,test_RevokeAgent_success/notOwner/unregistered/twoAgents,test_GetHumanOwner,test_GetAgents,test_GetAgents_afterRevokeTest result: 704 passed, 0 failed
After Merge — Deployment Steps