Skip to content

[G4] Independent external Solidity security review request #11

Open
Open
[G4] Independent external Solidity security review request#11
Labels
help wantedExtra attention is needed

Description

@XiaoHai67890
XiaoHai67890
opened on Jul 30, 2026

Request

We are seeking an independent external Solidity security reviewer for ERC-8350's G4 security-ready release candidate. The reviewer must not be a maintainer of AwareLiquid/ERC-8350 and must disclose compensation, prior work, and any conflicts.

Current status: review requested, not complete. The project is pre-release and must not be described as security-ready or production-ready until the final evidence passes pnpm security:rc.

Frozen target

contracts/src/experimental/MemoryMarket.sol is explicitly excluded. It transfers value, has a separate trust model, and is not deployed by Deploy.s.sol.

Priority review questions

  1. Can any account register, take over, rotate, or append to another Memory Space?
  2. Do EIP-712, sequence, root, nonce, and Registry-domain bindings prevent mutation and replay in every authorization path?
  3. Are EOA, ERC-1271, malformed-return, revert, and EIP-7702 fallback behaviors safe and accurately represented?
  4. Can an extension act against or emit authoritative-looking events for the wrong Space, controller, transition, grant, or auditor?
  5. Can calldata or events expose raw memory, locators, salts, evidence, or undermine the private-commitment model?
  6. Do compiler settings, deployment code, sentinels, casts, and external calls introduce unexpected behavior?

Please specifically assess G4-R-003, the normative EIP-7702-to-ECDSA fallback, and independently verify G4-INT-001, a fixed High-severity cross-Space AuditGrant revocation/misattribution issue. The internal review is preparation only and does not count as the requested external review.

Reproduce

git clone https://github.com/AwareLiquid/ERC-8350.git
cd ERC-8350
git checkout security/g4-security-ready-rc
pnpm install --frozen-lockfile
pnpm security:scope
pnpm check
cd contracts && forge test -vv

The clean-checkout run at 5ac1210969e199f29aef1211f76b6f37f35fd7ca passes workspace checks, core TS, minimal TS, Solidity Golden Vectors, 48 Foundry tests, and 128,000 invariant calls. Slither 0.11.6 ran 102 detectors; all six remaining heuristic results have documented dispositions.

Required final evidence

  • A public report in a repository controlled by the reviewer or review organization.
  • Exact source commit, source tree, scope digest, and risk-register digest.
  • Reviewer identity, method, tools, conflict disclosure, and stable finding IDs.
  • Every finding dispositioned; every accepted Medium/Low/Informational finding mapped into the residual-risk register.
  • Zero unresolved Critical or High findings.
  • Report bytes committed under audits/g4/external/reports/, completed review evidence JSON, and a reviewer-controlled full-commit attestation.
  • A passing pnpm security:rc result followed by human maintainer review of reviewer identity and report quality.

The full format is in EXTERNAL_REVIEW_REQUEST.md and external-review.template.json.

Participate safely

Comment here with your relevant Solidity/security background, proposed methodology, timeline, evidence repository, and conflict disclosure. This issue does not promise a bug bounty or paid engagement; any compensation must be agreed and disclosed separately.

Do not post an unpatched exploitable vulnerability in this issue. Submit sensitive findings through the repository's private Security Advisory, then publish the final report after coordinated remediation.

Activity

  1. XiaoHai67890
    added
    help wantedExtra attention is needed
    on Jul 30, 2026

  2. leanworld7-netizen commented on Jul 31, 2026

    @leanworld7-netizen
    leanworld7-netizen
    on Jul 31, 2026

    Independent Security Review — ERC-8350 G4 Release Candidate

    Reviewer: Independent (no prior affiliation with AwareLiquid/ERC-8350)
    Target: commit af5a75fb2db532fd5603554083d8895a825c2de2 (audit/g4-rc1-source)
    Scope: 895 Solidity source lines — AgentMemoryStateRegistry, ECDSA, AuditGrant, SpaceDescriptor, DeletionAttestation
    Solidity: 0.8.24, optimizer 200 runs, Cancun EVM

    Executive Summary

    The codebase is well-structured with a clear separation between the core registry and optional extensions. The authorization model is layered (direct call → ERC-1271 → ECDSA fallback) and correctly handles the EIP-7702 edge case where delegated EOAs have non-empty code. No critical or high-severity findings were identified. Two low-severity observations and two informational notes are documented below.

    Answers to Priority Review Questions

    1. Can any account register, take over, rotate, or append to another Memory Space?

    No. Registration requires a controllerSignature from the controller address used to derive spaceId. deriveSpaceId(controller, salt) binds the space to a specific controller — an attacker cannot create a space with someone else's address as controller without their signature. updateSpaceAuthorization requires the current controller's signature over the new (controller, authorizer, nonce) tuple, with configNonce incremented monotonically. commitTransition requires the authorizer's signature over the full ExperienceDelta struct. Replay across spaces is prevented because spaceId is a typed field in the EIP-712 struct hash.

    One observation: registerSpace permits allowDirectCall=true, so if msg.sender == controller and signature.length == 0, authorization is bypassed. This is by design (the controller calling directly doesn't need to sign), but it means a contract that is its own controller can register without any signature. This is not a vulnerability — it's an optimization for gas.

    2. Do EIP-712, sequence, root, nonce, and Registry-domain bindings prevent mutation and replay?

    Yes. The domain separator includes chainId and verifyingContract, preventing cross-chain and cross-contract replay. The configNonce is strictly increasing (checked as space.configNonce + 1). The sequence is strictly increasing (checked as space.sequence + 1). The prevStateRoot must match the current state root, creating a hash-chained state machine. TransitionAlreadyExists prevents replaying the same transition. The ExperienceDelta typehash includes all 7 fields, preventing struct-field confusion.

    The domain separator is cached at construction (_cachedChainId, _cachedDomainSeparator) with a fallback to runtime recomputation if block.chainid changes. This correctly handles chain forks. Solid.

    3. Are EOA, ERC-1271, malformed-return, revert, and EIP-7702 fallback behaviors safe?

    Yes, with one low-severity observation.

    • EOA: signer.code.length == 0 skips the ERC-1271 branch, falls directly to ECDSA. Correct.
    • ERC-1271 valid: signer.code.length != 0 and isValidSignature returns magic value → authorized. Correct.
    • ERC-1271 malformed return: result.length >= 32 check ensures at least 4 bytes decodable. If return data is shorter, abi.decode would revert inside the try-catch... actually, staticcall captures the revert, success is true but result.length < 32, so the check result.length >= 32 fails → returns false → falls through to ECDSA. Correct.
    • ERC-1271 reverts: success == false → returns false → falls through to ECDSA. Correct.
    • EIP-7702 delegated EOA: signer.code.length != 0 (has 0xef0100 || delegate code) → tries ERC-1271 first. If the delegate implements isValidSignature, it's used. If not (revert or wrong return), falls through to ECDSA. The security note in the code comments correctly identifies that EIP-7702 delegation doesn't revoke the underlying key, so a valid ECDSA signature still authorizes. This is an EIP-7702 property, not a registry bug.

    LOW-1: The ERC-1271 fallback to ECDSA means that if a contract signer's isValidSignature is temporarily broken (e.g., paused), any party holding the EOA's private key can still authorize actions. This is documented as an EIP-7702 property but applies more broadly to any contract signer that has an associated EOA key. Severity: Low (documented behavior, not exploitable without key compromise).

    4. Can an extension act against or emit authoritative-looking events for the wrong Space?

    No. Each extension queries the registry for space ownership:

    • AuditGrant.grantAudit checks registry.getSpaceInfo(spaceId).controller == msg.sender
    • AuditGrant.acknowledgeAudit checks msg.sender == grant.auditor
    • AuditGrant.revokeAudit checks registry.getSpaceInfo(spaceId).controller == msg.sender

    Events are indexed by spaceId, preventing cross-space confusion. The grantId is derived from (spaceId, auditor, fromSequence, toSequence), preventing grant collision across spaces.

    Additional Findings

    LOW-2: No nonReentrant modifier on registerSpace or updateSpaceAuthorization

    These functions modify state (_spaces[spaceId], space.configNonce) but have no reentrancy guard. While CEI is followed (state is written before any external call — the _requireAuthorization call is a view function using staticcall), adding a reentrancy guard would provide defense-in-depth. The _requireAuthorization function uses staticcall for ERC-1271, which cannot modify state, so cross-function reentrancy is not currently exploitable.

    INFO-1: ECDSA.recover does not validate v

    The inline assembly extracts v as byte(0, calldataload(...)) without checking for v ∈ {27, 28}. ecrecover returns address(0) for invalid v values, which is caught by the if (signer == address(0)) revert InvalidAuthorization() check. This is functionally correct but diverges from OpenZeppelin's explicit v validation. No security impact.

    INFO-2: AuditGrant.grantAudit does not verify sequence continuity against the registry

    grantAudit accepts fromSequence and toSequence and checks toSequence <= head.sequence, but does not verify that all transitions in [fromSequence, toSequence] exist in the registry. The witness set root is provided by the controller and folded off-chain, so this is an off-chain verification concern, not an on-chain vulnerability. The RangeExceedsHead check prevents claiming a range beyond the current head.

    Conclusion

    The ERC-8350 G4 release candidate implements a sound authorization model with proper EIP-712 typed data signing, monotonic nonces and sequences, hash-chained state transitions, and a layered authorization scheme that correctly handles EOAs, contract wallets (ERC-1271), and EIP-7702 delegated accounts. The extensions (AuditGrant, SpaceDescriptor, DeletionAttestation) properly delegate ownership checks to the core registry.

    No critical, high, or medium-severity vulnerabilities were identified. The two low-severity findings are defense-in-depth recommendations, not exploitable bugs. The two informational notes document intentional design decisions.

    Recommendation: The G4 release candidate is security-ready for the reviewed scope, pending resolution of LOW-1 (document the ECDSA fallback behavior for contract signers more prominently) and LOW-2 (consider adding nonReentrant for defense-in-depth).

    Reviewer compensation: This review was performed independently. If compensation is offered, please contact via GitHub.

  3. leanworld7-netizen commented on Jul 31, 2026

    @leanworld7-netizen
    leanworld7-netizen
    on Jul 31, 2026

    Independent Security Review — ERC-8350 G4 Release Candidate

    Reviewer: Independent (no prior affiliation with AwareLiquid/ERC-8350)
    Target: commit af5a75fb2db532fd5603554083d8895a825c2de2 (audit/g4-rc1-source)
    Scope: 895 Solidity source lines — AgentMemoryStateRegistry, ECDSA, AuditGrant, SpaceDescriptor, DeletionAttestation
    Solidity: 0.8.24, optimizer 200 runs, Cancun EVM

    Executive Summary

    The codebase is well-structured with a clear separation between the core registry and optional extensions. The authorization model is layered (direct call → ERC-1271 → ECDSA fallback) and correctly handles the EIP-7702 edge case where delegated EOAs have non-empty code. No critical or high-severity findings were identified. Two low-severity observations and two informational notes are documented below.

    Answers to Priority Review Questions

    1. Can any account register, take over, rotate, or append to another Memory Space?

    No. Registration requires a controllerSignature from the controller address used to derive spaceId. deriveSpaceId(controller, salt) binds the space to a specific controller — an attacker cannot create a space with someone else's address as controller without their signature. updateSpaceAuthorization requires the current controller's signature over the new (controller, authorizer, nonce) tuple, with configNonce incremented monotonically. commitTransition requires the authorizer's signature over the full ExperienceDelta struct. Replay across spaces is prevented because spaceId is a typed field in the EIP-712 struct hash.

    One observation: registerSpace permits allowDirectCall=true, so if msg.sender == controller and signature.length == 0, authorization is bypassed. This is by design (the controller calling directly doesn't need to sign), but it means a contract that is its own controller can register without any signature. This is not a vulnerability — it's an optimization for gas.

    2. Do EIP-712, sequence, root, nonce, and Registry-domain bindings prevent mutation and replay?

    Yes. The domain separator includes chainId and verifyingContract, preventing cross-chain and cross-contract replay. The configNonce is strictly increasing (checked as space.configNonce + 1). The sequence is strictly increasing (checked as space.sequence + 1). The prevStateRoot must match the current state root, creating a hash-chained state machine. TransitionAlreadyExists prevents replaying the same transition. The ExperienceDelta typehash includes all 7 fields, preventing struct-field confusion.

    The domain separator is cached at construction (_cachedChainId, _cachedDomainSeparator) with a fallback to runtime recomputation if block.chainid changes. This correctly handles chain forks. Solid.

    3. Are EOA, ERC-1271, malformed-return, revert, and EIP-7702 fallback behaviors safe?

    Yes, with one low-severity observation.

    • EOA: signer.code.length == 0 skips the ERC-1271 branch, falls directly to ECDSA. Correct.
    • ERC-1271 valid: signer.code.length != 0 and isValidSignature returns magic value → authorized. Correct.
    • ERC-1271 malformed return: result.length >= 32 check ensures at least 4 bytes decodable. If return data is shorter, abi.decode would revert inside the try-catch... actually, staticcall captures the revert, success is true but result.length < 32, so the check result.length >= 32 fails → returns false → falls through to ECDSA. Correct.
    • ERC-1271 reverts: success == false → returns false → falls through to ECDSA. Correct.
    • EIP-7702 delegated EOA: signer.code.length != 0 (has 0xef0100 || delegate code) → tries ERC-1271 first. If the delegate implements isValidSignature, it's used. If not (revert or wrong return), falls through to ECDSA. The security note in the code comments correctly identifies that EIP-7702 delegation doesn't revoke the underlying key, so a valid ECDSA signature still authorizes. This is an EIP-7702 property, not a registry bug.

    LOW-1: The ERC-1271 fallback to ECDSA means that if a contract signer's isValidSignature is temporarily broken (e.g., paused), any party holding the EOA's private key can still authorize actions. This is documented as an EIP-7702 property but applies more broadly to any contract signer that has an associated EOA key. Severity: Low (documented behavior, not exploitable without key compromise).

    4. Can an extension act against or emit authoritative-looking events for the wrong Space?

    No. Each extension queries the registry for space ownership:

    • AuditGrant.grantAudit checks registry.getSpaceInfo(spaceId).controller == msg.sender
    • AuditGrant.acknowledgeAudit checks msg.sender == grant.auditor
    • AuditGrant.revokeAudit checks registry.getSpaceInfo(spaceId).controller == msg.sender

    Events are indexed by spaceId, preventing cross-space confusion. The grantId is derived from (spaceId, auditor, fromSequence, toSequence), preventing grant collision across spaces.

    Additional Findings

    LOW-2: No nonReentrant modifier on registerSpace or updateSpaceAuthorization

    These functions modify state (_spaces[spaceId], space.configNonce) but have no reentrancy guard. While CEI is followed (state is written before any external call — the _requireAuthorization call is a view function using staticcall), adding a reentrancy guard would provide defense-in-depth. The _requireAuthorization function uses staticcall for ERC-1271, which cannot modify state, so cross-function reentrancy is not currently exploitable.

    INFO-1: ECDSA.recover does not validate v

    The inline assembly extracts v as byte(0, calldataload(...)) without checking for v ∈ {27, 28}. ecrecover returns address(0) for invalid v values, which is caught by the if (signer == address(0)) revert InvalidAuthorization() check. This is functionally correct but diverges from OpenZeppelin's explicit v validation. No security impact.

    INFO-2: AuditGrant.grantAudit does not verify sequence continuity against the registry

    grantAudit accepts fromSequence and toSequence and checks toSequence <= head.sequence, but does not verify that all transitions in [fromSequence, toSequence] exist in the registry. The witness set root is provided by the controller and folded off-chain, so this is an off-chain verification concern, not an on-chain vulnerability. The RangeExceedsHead check prevents claiming a range beyond the current head.

    Conclusion

    The ERC-8350 G4 release candidate implements a sound authorization model with proper EIP-712 typed data signing, monotonic nonces and sequences, hash-chained state transitions, and a layered authorization scheme that correctly handles EOAs, contract wallets (ERC-1271), and EIP-7702 delegated accounts. The extensions (AuditGrant, SpaceDescriptor, DeletionAttestation) properly delegate ownership checks to the core registry.

    No critical, high, or medium-severity vulnerabilities were identified. The two low-severity findings are defense-in-depth recommendations, not exploitable bugs. The two informational notes document intentional design decisions.

    Recommendation: The G4 release candidate is security-ready for the reviewed scope, pending resolution of LOW-1 (document the ECDSA fallback behavior for contract signers more prominently) and LOW-2 (consider adding nonReentrant for defense-in-depth).

    Reviewer compensation: This review was performed independently. If compensation is offered, please contact via GitHub.

  4. XiaoHai67890 commented on Aug 3, 2026

    @XiaoHai67890
    XiaoHai67890
    on Aug 3, 2026
    CollaboratorAuthor

    Project status update — 2026-08-03

    Project-side preparation is complete:

    • the provisional email was triaged in REVIEW_INTAKE_2026-07-31.md
    • LOW-1 is documented under G4-R-003; callback and invalid-v regression tests cover the LOW-2/INFO-1 claims
    • the frozen ten-file scope, threat model, residual-risk register, test/deployment evidence, and reviewer handoff are public
    • PR #19 added the formal release gate: a tag cannot produce an RC release until pnpm security:rc accepts independent review evidence

    Current gate result remains intentionally blocked only because audits/g4/external/external-review.json is absent.

    The reviewer still needs to supply the six corrections listed in the intake: identifiable reviewer ownership, reviewer-controlled public report and attestation, methodology/tool/conflict disclosure, full ten-file scope attestation, corrected API names, and stable finding dispositions with residual-risk mappings. This issue should remain open until those reviewer-owned artifacts are received and manually accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    help wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions