Open
Conversation
Add TEE-attested OFAC root updates via updateOfacRootsWithProof - Implementation: 0x530eEA7E5b286108926B05510491560c4bAE018e - Adds updateOfacRootsWithProof() for ZK-verified OFAC root updates - New errors: InvalidRootsHash, InvalidRootsCount - New event: OfacRootsUpdatedWithProof
Add previousRoot storage variables to all identity registries so that checkOfacRoots accepts both the current and previous root for each OFAC tree. This prevents verification failures for users mid-proof when roots are updated on-chain between proof generation and on-chain verification. - Passport: 3 prev roots (passportNo, nameAndDob, nameAndYob) - KYC, ID Card, Aadhaar: 2 prev roots each (nameAndDob, nameAndYob) - KYC updateOfacRootsWithProof also rotates previous roots - Added getPrev* getter functions on all registries - Storage appended at end of each storage contract (UUPS-safe) - 17 new tests covering all registries (window=1 acceptance/rejection)
Add rolling OFAC root window: store previous roots alongside current, accept either in checkOfacRoots for graceful mid-verification transitions. New impl: 0x6E2889Bc9baa6F53bDdf4843675155811F0AAAEd Proxy: 0x90e907E4AaB6e9bcFB94997Af4A097e8CAadBdf3 Pending Safe multisig execution for proxy upgrade.
…port registries Extend updateOfacRootsWithProof() to the remaining 3 identity registries, matching the pattern already deployed on the KYC registry (v1.1.0). - Add GCP JWT verifier, PCR0Manager, TEE address, and root CA pubkey hash storage to each registry - Add initializeOfacProof() reinitializer for upgrade path - Add onlyTEE modifier and updateOfacRootsWithProof() with Groth16 proof verification, TEE attestation validation, timestamp checks, and global roots hash commitment verification - Rolling window behavior preserved: previous roots saved before overwrite - Admin functions for updating TEE infrastructure (SECURITY_ROLE gated) - Bumps all 3 registries to v1.3.0
Contributor
📝 WalkthroughWalkthroughAdds OFAC proof-driven update support across multiple IdentityRegistry variants: introduces GCP JWT verifier and PCR0 manager interfaces, TEE gating, rolling previous OFAC roots, proof-validated update functions, governance setters, tests, deployment/upgrade modules, and manifest updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TEE as TEE (authorized signer)
participant Registry as IdentityRegistry (proxy)
participant GCP as GCPJWTVerifier
participant PCR as PCR0Manager
participant Block as Blockchain
TEE->>Registry: call updateOfacRootsWithProof(pA,pB,pC,pubSignals,roots)
Registry->>GCP: GCP.verifyProof(pA,pB,pC,pubSignals)
GCP-->>Registry: returns true/false
Registry->>PCR: isPCR0Set(imageHashFromPubSignals)
PCR-->>Registry: returns true/false
Registry->>Block: read block.timestamp, validate timestamp window
alt proof valid & checks pass
Registry->>Registry: compute rootsHash, rotate prev roots, set new roots
Registry-->>TEE: emit OfacRootsUpdatedWithProof(rootsHash, timestamp)
else any check fails
Registry-->>TEE: revert with appropriate INVALID_* error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
contracts/contracts/registry/IdentityRegistryAadhaarImplV1.sol (1)
372-380:⚠️ Potential issue | 🟠 MajorThis rolling-window check accepts mixed root pairs.
Matching DOB and YOB roots independently means a proof can clear OFAC with a pair that was never published together, such as the current DOB root plus the previous YOB root. Since this function is the final sanctions gate, compare the full current pair vs the full previous pair instead of building a cross-product.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/registry/IdentityRegistryAadhaarImplV1.sol` around lines 372 - 380, The checkOfacRoots function accepts mixed DOB/YOB root pairs by matching each root independently, allowing combinations that were never published together; change the logic in checkOfacRoots to return true only if the input pair exactly equals the current pair (_nameAndDobOfacRoot == nameAndDobRoot && _nameAndYobOfacRoot == nameAndYobRoot) or exactly equals the previous pair (_prevNameAndDobOfacRoot != 0 && _prevNameAndDobOfacRoot == nameAndDobRoot && _prevNameAndYobOfacRoot != 0 && _prevNameAndYobOfacRoot == nameAndYobRoot), so you compare full pairs instead of building a cross-product of roots.contracts/contracts/registry/IdentityRegistryImplV1.sol (1)
435-446:⚠️ Potential issue | 🟠 MajorRequire one coherent OFAC snapshot, not per-root mixing.
This accepts each root independently, so a proof can pass with a tuple that never existed together—for example, the current passport root plus previous DOB/YOB roots.
OfacCheckLib.performOfacCheck()treats this boolean as the sanctions gate, so the rolling window should compare full current vs full previous snapshots, or rotate the whole snapshot atomically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/registry/IdentityRegistryImplV1.sol` around lines 435 - 446, The checkOfacRoots function currently allows mixing roots from current and previous snapshots; change it so it only returns true when the supplied tuple matches an entire coherent snapshot: either all three match the current roots (_passportNoOfacRoot, _nameAndDobOfacRoot, _nameAndYobOfacRoot) OR all three match the previous snapshot (use _prevPassportNoOfacRoot, _prevNameAndDobOfacRoot, _prevNameAndYobOfacRoot) and ensure the previous-snapshot branch only applies when the previous roots are non-zero; update checkOfacRoots to perform a full-current OR full-previous comparison rather than per-root ORs.contracts/deployments/registry.json (1)
95-99:⚠️ Potential issue | 🟠 MajorAdd the matching
2.13.0version-history entry for celo-sepolia.
networks.celo-sepolia.deployments.IdentityVerificationHubnow points atcurrentVersion: 2.13.0, butversions.IdentityVerificationHub["2.13.0"].deploymentsin this file only containscelo. That leaves the registry internally inconsistent and breaks history/metadata lookups for the sepolia hub deployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/deployments/registry.json` around lines 95 - 99, The registry is inconsistent: networks.celo-sepolia.deployments.IdentityVerificationHub has currentVersion "2.13.0" but versions.IdentityVerificationHub["2.13.0"].deployments lacks a celo-sepolia entry; add a "celo-sepolia" deployments entry under versions.IdentityVerificationHub["2.13.0"] with the same fields (proxy and currentImpl) used by networks.celo-sepolia.deployments.IdentityVerificationHub so that versions.IdentityVerificationHub["2.13.0"].deployments.celo-sepolia matches the network record and restores consistent version-history lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/contracts/registry/IdentityRegistryAadhaarImplV1.sol`:
- Around line 271-281: The initializeOfacProof function is externally callable
and can be front-run; restrict who can call it by adding an access-control guard
(e.g., onlyOwner, onlyRole(DEFAULT_ADMIN_ROLE), or onlyProxyAdmin) or make it
internal so it cannot be invoked by arbitrary callers; update
initializeOfacProof (the reinitializer(3) that sets _gcpJwtVerifier,
_pcr0Manager, _gcpRootCAPubkeyHash, and _tee) to require the chosen admin
role/owner before writing state, and ensure the contract imports/uses the same
access-control mechanism already present in the contract (Owner/AccessControl)
to avoid introducing new patterns.
In `@contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol`:
- Around line 408-413: checkOfacRoots currently allows mixing current and
previous roots independently, letting callers accept combinations that were
never attested together; change the logic in checkOfacRoots(uint256
nameAndDobRoot, uint256 nameAndYobRoot) to only return true if the provided pair
exactly matches the current pair (_nameAndDobOfacRoot && _nameAndYobOfacRoot) or
exactly matches the previous pair (_prevNameAndDobOfacRoot &&
_prevNameAndYobOfacRoot) (and still guard against zero previous roots), instead
of OR-ing dobMatch and yobMatch independently.
- Around line 284-294: The initializeOfacProof function is an externally
callable reinitializer that sets sensitive config (_gcpJwtVerifier,
_pcr0Manager, _gcpRootCAPubkeyHash, _tee) and must be protected by the same
governance guard used elsewhere; update the function signature to require the
governance role (e.g., add onlyRole(GOVERNANCE_ROLE) or onlyOwner) or add an
in-function require(hasRole(GOVERNANCE_ROLE, msg.sender)) check, matching the
existing access control pattern in this contract so only governance can call
initializeOfacProof.
In `@contracts/contracts/registry/IdentityRegistryImplV1.sol`:
- Around line 294-304: The initializeOfacProof function is currently externally
callable and can be front-run to set critical validators; protect it by
requiring the same governance access control used elsewhere in this contract
(e.g., the owner/governance role modifier used on other initializer functions)
so only the governance/admin can call it; locate initializeOfacProof and add the
existing governance modifier (the same one applied to other init functions or
the contract's OWNER/GOVERNANCE role) to the function signature and keep the
rest of the logic unchanged.
In `@contracts/contracts/registry/IdentityRegistryKycImplV1.sol`:
- Around line 372-377: The checkOfacRoots function currently allows DOB and YOB
roots to match independently (accepting mixed current/previous combos); change
it to accept only atomic pairs by returning true if both provided roots equal
the current pair ( _nameAndDobOfacRoot == nameAndDobRoot && _nameAndYobOfacRoot
== nameAndYobRoot ) or if both equal the previous pair ( _prevNameAndDobOfacRoot
!= 0 && _prevNameAndDobOfacRoot == nameAndDobRoot && _prevNameAndYobOfacRoot !=
0 && _prevNameAndYobOfacRoot == nameAndYobRoot ); update checkOfacRoots to
perform those paired comparisons instead of independent dobMatch/yobMatch
booleans.
In `@contracts/tasks/upgrade/types.ts`:
- Line 14: CONTRACT_IDS now includes IdentityRegistryKyc but prepare.ts still
omits wiring IdentityRegistryKycImplV1 in the PoseidonT3 linking branch; update
contracts/tasks/upgrade/prepare.ts to treat IdentityRegistryKyc the same as
other contracts by adding IdentityRegistryKycImplV1 to the branch that performs
PoseidonT3 library linking (the same branch that currently handles other *ImplV1
contracts), ensuring the identity constant/enum CONTRACT_IDS and the
prepare/upgrade path both reference IdentityRegistryKyc and
IdentityRegistryKycImplV1 so upgrade:prepare --contract IdentityRegistryKyc goes
through the linked-library path.
In `@contracts/test/v2/ofacProofUpdate.test.ts`:
- Around line 174-242: The test only verifies current roots after one update and
misses asserting that previous roots are preserved; modify the test around
updateOfacRootsWithProof (for deployedActors.registry, registryAadhaar,
registryId, registryKyc) to either seed initial roots first and call
updateOfacRootsWithProof twice or perform an additional proof-based update so
you can assert the older values moved into prev slots (use getPrev* accessors or
the existing getNameAndDobOfacRoot/getNameAndYobOfacRoot/getPassportNoOfacRoot
for current values and call checkOfacRoots to validate prior roots are still
available); ensure you add assertions for prev* getters (or checkOfacRoots) for
each registry after the second update to confirm the rolling window behavior.
- Around line 111-114: Tests currently only deploy MockGCPJWTVerifier
(MockGCPJWTVerifier) which is a boolean stub; add at least one test case that
exercises the real verifier path by deploying the actual verifier contract
(e.g., GCPJWTVerifier or the production verifier implementation), wiring it into
the same proof verification flow used by ofacProofUpdate.test.ts, and generating
a real proof/JWT with the test harness so the verifier performs full
verification instead of returning a stubbed boolean; update the test setup where
MockGCPJWTVerifier is created (and any helper that mints/verifies proofs) to
deploy and use the real verifier for that new test, ensuring the produced proof
matches the contract’s expected public-signal and witness layout so verification
passes.
In `@contracts/test/v2/ofacRollingWindow.test.ts`:
- Around line 93-107: The test "should accept previous roots after one update"
only asserts after both DOB and YOB roots are rotated; update the test to
exercise the transient window by adding assertions immediately after the first
root update (i.e., call registryKyc.updateNameAndDobOfacRoot(ROOT_B_DOB) then
assert mixed and previous combinations via registryKyc.checkOfacRoots before
calling updateNameAndYobOfacRoot(ROOT_B_YOB)), and mirror this pattern for the
other affected tests (lines referenced around 169-182, 237-246, 295-304) so that
each single-root rotation state (only DOB or only YOB changed) is validated
using registryKyc.checkOfacRoots for current, previous, and mixed permutations.
In `@contracts/test/v2/ofacUpgradePath.test.ts`:
- Around line 101-160: After the proof-based update (after calling
updateOfacRootsWithProof and the checks using
getNameAndDobOfacRoot/getNameAndYobOfacRoot), add assertions that the
rolled/previous slots contain the prior roots (i.e., assert the prev* slots
equal initialDobRoot and initialYobRoot) by calling the contract getters for the
previous slots; then after calling the legacy setter
updateNameAndDobOfacRoot(11111n) assert the prev* slots were rotated again (they
should equal the pre-set values 800n/900n) to ensure the rolling-window storage
alignment is correct. Use the existing symbols getNameAndDobOfacRoot,
getNameAndYobOfacRoot, updateOfacRootsWithProof, and updateNameAndDobOfacRoot to
locate where to insert these extra assertions.
---
Outside diff comments:
In `@contracts/contracts/registry/IdentityRegistryAadhaarImplV1.sol`:
- Around line 372-380: The checkOfacRoots function accepts mixed DOB/YOB root
pairs by matching each root independently, allowing combinations that were never
published together; change the logic in checkOfacRoots to return true only if
the input pair exactly equals the current pair (_nameAndDobOfacRoot ==
nameAndDobRoot && _nameAndYobOfacRoot == nameAndYobRoot) or exactly equals the
previous pair (_prevNameAndDobOfacRoot != 0 && _prevNameAndDobOfacRoot ==
nameAndDobRoot && _prevNameAndYobOfacRoot != 0 && _prevNameAndYobOfacRoot ==
nameAndYobRoot), so you compare full pairs instead of building a cross-product
of roots.
In `@contracts/contracts/registry/IdentityRegistryImplV1.sol`:
- Around line 435-446: The checkOfacRoots function currently allows mixing roots
from current and previous snapshots; change it so it only returns true when the
supplied tuple matches an entire coherent snapshot: either all three match the
current roots (_passportNoOfacRoot, _nameAndDobOfacRoot, _nameAndYobOfacRoot) OR
all three match the previous snapshot (use _prevPassportNoOfacRoot,
_prevNameAndDobOfacRoot, _prevNameAndYobOfacRoot) and ensure the
previous-snapshot branch only applies when the previous roots are non-zero;
update checkOfacRoots to perform a full-current OR full-previous comparison
rather than per-root ORs.
In `@contracts/deployments/registry.json`:
- Around line 95-99: The registry is inconsistent:
networks.celo-sepolia.deployments.IdentityVerificationHub has currentVersion
"2.13.0" but versions.IdentityVerificationHub["2.13.0"].deployments lacks a
celo-sepolia entry; add a "celo-sepolia" deployments entry under
versions.IdentityVerificationHub["2.13.0"] with the same fields (proxy and
currentImpl) used by networks.celo-sepolia.deployments.IdentityVerificationHub
so that versions.IdentityVerificationHub["2.13.0"].deployments.celo-sepolia
matches the network record and restores consistent version-history lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c266f21-5953-4acb-a5c6-d810b41ab4db
📒 Files selected for processing (13)
contracts/.gitignorecontracts/contracts/registry/IdentityRegistry.solcontracts/contracts/registry/IdentityRegistryAadhaarImplV1.solcontracts/contracts/registry/IdentityRegistryIdCardImplV1.solcontracts/contracts/registry/IdentityRegistryImplV1.solcontracts/contracts/registry/IdentityRegistryKycImplV1.solcontracts/deployments/registry.jsoncontracts/tasks/upgrade/README.mdcontracts/tasks/upgrade/types.tscontracts/tasks/upgrade/upgrade.tscontracts/test/v2/ofacProofUpdate.test.tscontracts/test/v2/ofacRollingWindow.test.tscontracts/test/v2/ofacUpgradePath.test.ts
Comment on lines
+111
to
+114
| // Deploy fresh MockGCPJWTVerifier | ||
| const MockVerifierFactory = await ethers.getContractFactory("MockGCPJWTVerifier"); | ||
| mockVerifier = await MockVerifierFactory.deploy(); | ||
| await mockVerifier.waitForDeployment(); |
Contributor
There was a problem hiding this comment.
Don't leave the core proof path mock-only.
MockGCPJWTVerifier is a pure boolean stub, so these tests can stay green even if the real verifier would reject the proof because the public-signal layout or witness generation drifted from the contract. Please keep the unit coverage, but add at least one case that uses the real verifier path.
Based on learnings: "Ensure proof generation works and verification passes for all test cases after PR creation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/test/v2/ofacProofUpdate.test.ts` around lines 111 - 114, Tests
currently only deploy MockGCPJWTVerifier (MockGCPJWTVerifier) which is a boolean
stub; add at least one test case that exercises the real verifier path by
deploying the actual verifier contract (e.g., GCPJWTVerifier or the production
verifier implementation), wiring it into the same proof verification flow used
by ofacProofUpdate.test.ts, and generating a real proof/JWT with the test
harness so the verifier performs full verification instead of returning a
stubbed boolean; update the test setup where MockGCPJWTVerifier is created (and
any helper that mints/verifies proofs) to deploy and use the real verifier for
that new test, ensuring the produced proof matches the contract’s expected
public-signal and witness layout so verification passes.
Comment on lines
+174
to
+242
| it("should update all 4 registries with valid proof", async () => { | ||
| const registryHashes = computeRegistryHashes(passportRoots, aadhaarRoots, idCardRoots, kycRoots); | ||
| const pubSignals = buildPubSignals(registryHashes); | ||
|
|
||
| // Update Passport (index 0, 3 roots) | ||
| await expect( | ||
| deployedActors.registry.updateOfacRootsWithProof( | ||
| mockProof.a, | ||
| mockProof.b, | ||
| mockProof.c, | ||
| pubSignals, | ||
| passportRoots, | ||
| registryHashes, | ||
| ), | ||
| ).to.emit(deployedActors.registry, "OfacRootsUpdatedWithProof"); | ||
|
|
||
| // Update Aadhaar (index 1, 2 roots) | ||
| await expect( | ||
| deployedActors.registryAadhaar.updateOfacRootsWithProof( | ||
| mockProof.a, | ||
| mockProof.b, | ||
| mockProof.c, | ||
| pubSignals, | ||
| aadhaarRoots, | ||
| registryHashes, | ||
| ), | ||
| ).to.emit(deployedActors.registryAadhaar, "OfacRootsUpdatedWithProof"); | ||
|
|
||
| // Update ID Card (index 2, 2 roots) | ||
| await expect( | ||
| deployedActors.registryId.updateOfacRootsWithProof( | ||
| mockProof.a, | ||
| mockProof.b, | ||
| mockProof.c, | ||
| pubSignals, | ||
| idCardRoots, | ||
| registryHashes, | ||
| ), | ||
| ).to.emit(deployedActors.registryId, "OfacRootsUpdatedWithProof"); | ||
|
|
||
| // Update KYC (index 3, 2 roots) | ||
| await expect( | ||
| deployedActors.registryKyc.updateOfacRootsWithProof( | ||
| mockProof.a, | ||
| mockProof.b, | ||
| mockProof.c, | ||
| pubSignals, | ||
| kycRoots, | ||
| registryHashes, | ||
| ), | ||
| ).to.emit(deployedActors.registryKyc, "OfacRootsUpdatedWithProof"); | ||
|
|
||
| // Verify Passport roots | ||
| expect(await deployedActors.registry.getNameAndDobOfacRoot()).to.equal(passportRoots[0]); | ||
| expect(await deployedActors.registry.getNameAndYobOfacRoot()).to.equal(passportRoots[1]); | ||
| expect(await deployedActors.registry.getPassportNoOfacRoot()).to.equal(passportRoots[2]); | ||
|
|
||
| // Verify Aadhaar roots | ||
| expect(await deployedActors.registryAadhaar.getNameAndDobOfacRoot()).to.equal(aadhaarRoots[0]); | ||
| expect(await deployedActors.registryAadhaar.getNameAndYobOfacRoot()).to.equal(aadhaarRoots[1]); | ||
|
|
||
| // Verify ID Card roots | ||
| expect(await deployedActors.registryId.getNameAndDobOfacRoot()).to.equal(idCardRoots[0]); | ||
| expect(await deployedActors.registryId.getNameAndYobOfacRoot()).to.equal(idCardRoots[1]); | ||
|
|
||
| // Verify KYC roots | ||
| expect(await deployedActors.registryKyc.getNameAndDobOfacRoot()).to.equal(kycRoots[0]); | ||
| expect(await deployedActors.registryKyc.getNameAndYobOfacRoot()).to.equal(kycRoots[1]); | ||
| }); |
Contributor
There was a problem hiding this comment.
The proof updater still isn't proving the rolling window.
These happy-path cases only assert the new current roots. If updateOfacRootsWithProof overwrites the current roots without moving them into prev*, the main availability guarantee of this PR breaks and the suite still passes. Add a second proof-based update, or seed initial roots first, then assert getPrev* and checkOfacRoots for the old roots.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/test/v2/ofacProofUpdate.test.ts` around lines 174 - 242, The test
only verifies current roots after one update and misses asserting that previous
roots are preserved; modify the test around updateOfacRootsWithProof (for
deployedActors.registry, registryAadhaar, registryId, registryKyc) to either
seed initial roots first and call updateOfacRootsWithProof twice or perform an
additional proof-based update so you can assert the older values moved into prev
slots (use getPrev* accessors or the existing
getNameAndDobOfacRoot/getNameAndYobOfacRoot/getPassportNoOfacRoot for current
values and call checkOfacRoots to validate prior roots are still available);
ensure you add assertions for prev* getters (or checkOfacRoots) for each
registry after the second update to confirm the rolling window behavior.
Comment on lines
+93
to
+107
| it("should accept previous roots after one update", async () => { | ||
| await registryKyc.updateNameAndDobOfacRoot(ROOT_A_DOB); | ||
| await registryKyc.updateNameAndYobOfacRoot(ROOT_A_YOB); | ||
|
|
||
| await registryKyc.updateNameAndDobOfacRoot(ROOT_B_DOB); | ||
| await registryKyc.updateNameAndYobOfacRoot(ROOT_B_YOB); | ||
|
|
||
| // Current | ||
| expect(await registryKyc.checkOfacRoots(ROOT_B_DOB, ROOT_B_YOB)).to.be.true; | ||
| // Previous (rolling window) | ||
| expect(await registryKyc.checkOfacRoots(ROOT_A_DOB, ROOT_A_YOB)).to.be.true; | ||
| // Mixed current+previous | ||
| expect(await registryKyc.checkOfacRoots(ROOT_A_DOB, ROOT_B_YOB)).to.be.true; | ||
| expect(await registryKyc.checkOfacRoots(ROOT_B_DOB, ROOT_A_YOB)).to.be.true; | ||
| }); |
Contributor
There was a problem hiding this comment.
Exercise the transient window, not just the post-rotation state.
Each "after one update" case finishes rotating all roots before it asserts. The regression this change is meant to prevent happens after the first root moves and before the companion roots do, so a bug in that gap would still pass here. Passport also only checks one mixed permutation of its three OFAC roots.
🧪 Minimal assertion pattern
await registryKyc.updateNameAndDobOfacRoot(ROOT_B_DOB);
+ expect(await registryKyc.checkOfacRoots(ROOT_A_DOB, ROOT_A_YOB)).to.be.true;
+ expect(await registryKyc.checkOfacRoots(ROOT_B_DOB, ROOT_A_YOB)).to.be.true;
await registryKyc.updateNameAndYobOfacRoot(ROOT_B_YOB);Also applies to: 169-182, 237-246, 295-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/test/v2/ofacRollingWindow.test.ts` around lines 93 - 107, The test
"should accept previous roots after one update" only asserts after both DOB and
YOB roots are rotated; update the test to exercise the transient window by
adding assertions immediately after the first root update (i.e., call
registryKyc.updateNameAndDobOfacRoot(ROOT_B_DOB) then assert mixed and previous
combinations via registryKyc.checkOfacRoots before calling
updateNameAndYobOfacRoot(ROOT_B_YOB)), and mirror this pattern for the other
affected tests (lines referenced around 169-182, 237-246, 295-304) so that each
single-root rotation state (only DOB or only YOB changed) is validated using
registryKyc.checkOfacRoots for current, previous, and mixed permutations.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 19414827 | Triggered | Generic Password | 6c40ed7 | app/ios/PassportReaderCore.swift | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/pull_request_template.md:
- Around line 1-7: Update the PR template to require explicit notes for
security, native/platform, and performance impacts by adding new required
sections under the existing "## Summary" and "## Test plan" headings: add a "##
Security considerations" section where authors must declare if the change is
security-sensitive and list mitigations, a "## Platform / native changes"
section to document any platform-specific code paths or native module changes,
and a "## Performance impact" section to report benchmarks or expected perf
regressions/improvements; ensure these headings are mandatory in the template so
reviewers can triage appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0da1eccf-906e-4613-a4fe-8ddacc976bfe
📒 Files selected for processing (1)
.github/pull_request_template.md
Comment on lines
+1
to
+7
| ## Summary | ||
|
|
||
| _A brief description of the changes, what and how is being changed._ | ||
| <!-- Brief description of changes --> | ||
|
|
||
| ### Tested | ||
| ## Test plan | ||
|
|
||
| _Explain how the change has been tested (for example by manual testing, unit tests etc) or why it's not necessary (for example version bump)._ | ||
| <!-- How was this tested? --> |
Contributor
There was a problem hiding this comment.
Add explicit security/native/performance prompts to the template.
Line 3 and Line 7 are too generic for security-sensitive/native PRs. Please require authors to explicitly document: security-sensitive changes, platform-specific code paths, and performance impact so reviewers can triage correctly.
Proposed template update
## Summary
-<!-- Brief description of changes -->
+<!-- Brief description of changes -->
+<!-- Security-sensitive changes (required if applicable): -->
+<!-- Platform-specific/native code paths changed (required if applicable): -->
+<!-- Performance implications (required if applicable): -->
## Test plan
-<!-- How was this tested? -->
+<!-- How was this tested? -->
+<!-- Include contract/native/mobile test coverage and any manual verification steps -->Based on learnings: "Flag security-sensitive changes for special review in PR description" and "Document complex native module changes, explain platform-specific code paths, flag security-sensitive operations, and note performance implications in PR for AI review preparation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Summary | |
| _A brief description of the changes, what and how is being changed._ | |
| <!-- Brief description of changes --> | |
| ### Tested | |
| ## Test plan | |
| _Explain how the change has been tested (for example by manual testing, unit tests etc) or why it's not necessary (for example version bump)._ | |
| <!-- How was this tested? --> | |
| ## Summary | |
| <!-- Brief description of changes --> | |
| <!-- Security-sensitive changes (required if applicable): --> | |
| <!-- Platform-specific/native code paths changed (required if applicable): --> | |
| <!-- Performance implications (required if applicable): --> | |
| ## Test plan | |
| <!-- How was this tested? --> | |
| <!-- Include contract/native/mobile test coverage and any manual verification steps --> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/pull_request_template.md around lines 1 - 7, Update the PR template
to require explicit notes for security, native/platform, and performance impacts
by adding new required sections under the existing "## Summary" and "## Test
plan" headings: add a "## Security considerations" section where authors must
declare if the change is security-sensitive and list mitigations, a "## Platform
/ native changes" section to document any platform-specific code paths or native
module changes, and a "## Performance impact" section to report benchmarks or
expected perf regressions/improvements; ensure these headings are mandatory in
the template so reviewers can triage appropriately.
- Add onlyProxy + onlyRole(DEFAULT_ADMIN_ROLE) guard to initializeOfacProof() on Passport, Aadhaar, and IdCard registries to prevent front-running during the window between upgradeToAndCall and the separate initializer call - Fix checkOfacRoots() across all 4 registries to use atomic snapshot comparison instead of per-root matching — prevents accepting Frankenstein pairs like (new DOB root, old YOB root) that were never attested together - Add IdentityRegistryKycImplV1 to PoseidonT3 linking branch in prepare.ts so upgrade:prepare works correctly for KYC - Add prev* slot assertions and mixed-pair rejection test to ofacUpgradePath.test.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ripts - Add getGcpJwtVerifier() and getPcr0Manager() getters to KYC and Aadhaar - Fix initializeOfacProof guard: SECURITY_ROLE instead of DEFAULT_ADMIN_ROLE (DEFAULT_ADMIN_ROLE is never granted in governance setup) - Add Ignition upgrade scripts for KYC and Aadhaar registries - Comment out registry deploy in deployKycRegistry (verifier-only redeploy) - Update deployed_addresses.json with sepolia upgrade artifacts - Bump KYC to v1.2.1, Aadhaar to v1.3.1 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
contracts/ignition/modules/upgrade/upgradeKycRegistry.ts (1)
6-15: Good error handling, butchainIdcould be undefined.The module correctly throws if the KYC proxy address is missing from deployed addresses. However,
hre.network.config.chainIdcan beundefinedin certain Hardhat configurations (e.g., when usinglocalhostwithout explicit chainId). This would result in looking forchain-undefineddirectory.🛡️ Add chainId validation
export default buildModule("UpgradeKycRegistryModule", (m) => { const networkName = hre.network.config.chainId; + if (!networkName) { + throw new Error("Network chainId not configured"); + } const deployedAddressesPath = path.join(__dirname, `../../deployments/chain-${networkName}/deployed_addresses.json`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/ignition/modules/upgrade/upgradeKycRegistry.ts` around lines 6 - 15, The code reads hre.network.config.chainId into networkName and uses it to build deployedAddressesPath, but chainId can be undefined; update the buildModule block to validate the value returned by hre.network.config.chainId (the networkName variable) before constructing deployedAddressesPath — if undefined, either derive a safe fallback (e.g., use hre.network.name or a configured default) or throw a clear error; ensure the check happens prior to path.join and reference the networkName variable, deployedAddressesPath and kycProxyAddress so the error handling prevents creating a path like "chain-undefined" and fails fast with a descriptive message.contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol (1)
596-643: Solid implementation of proof-verified OFAC root updates.The
updateOfacRootsWithProoffunction has comprehensive validation:
- Groth16 proof verification
- Root CA pubkey hash check
- PCR0/image hash validation via manager
- ±1 hour timestamp tolerance
- Roots hash verification against proof signals
One minor observation: there's no explicit check that
_gcpJwtVerifierand_pcr0Managerare non-zero before calling them. IfinitializeOfacProofwas never called, this would revert with a low-level call failure rather than a descriptive error. Consider adding explicit zero-address checks for clearer error messages.🛡️ Optional: Add explicit address validation
function updateOfacRootsWithProof( ... ) external onlyProxy onlyTEE { if (roots.length != 2) revert InvalidRootsCount(); + if (_gcpJwtVerifier == address(0)) revert HUB_NOT_SET(); // Or add a new GCP_VERIFIER_NOT_SET error + if (_pcr0Manager == address(0)) revert HUB_NOT_SET(); // Or add a new PCR0_MANAGER_NOT_SET error // Verify Groth16 proof if (!IGCPJWTVerifier(_gcpJwtVerifier).verifyProof(pA, pB, pC, pubSignals)) revert INVALID_PROOF();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol` around lines 596 - 643, The function updateOfacRootsWithProof calls IGCPJWTVerifier(_gcpJwtVerifier).verifyProof and IPCR0Manager(_pcr0Manager).isPCR0Set without checking those addresses; add explicit non-zero address guards for _gcpJwtVerifier and _pcr0Manager at the start of updateOfacRootsWithProof (or factor into a private validator called by initializeOfacProof) so you revert with clear custom errors like MissingGCPJWTVerifier() and MissingPCR0Manager() instead of low-level call failures; reference the storage vars _gcpJwtVerifier and _pcr0Manager and the public function updateOfacRootsWithProof when adding the checks.contracts/ignition/modules/registry/updateRegistries.ts (1)
140-149: ConditionaladdPCR0relies onpcr0Managerpresence but doesn't validate the manager contract.When
pcr0Manageris set, the code callsaddPCR0on it. If thepcr0Manageraddress is incorrect or the contract doesn't implementaddPCR0, this will fail at runtime. Consider adding a check that thepcr0Manageraddress is non-zero before proceeding, or document that this is expected to fail fast.Additionally,
m.contractAt("PCR0Manager", registryData.pcr0Manager)doesn't have anidparameter which could cause issues if the same address is used across multiple registries.♻️ Suggested improvement
if (registryData.pcr0Manager) { const callOptions = { after: [currentOperation], id: ids() }; currentOperation = m.call(registryContract, "updatePCR0Manager", [registryData.pcr0Manager], callOptions); if (registryData.imageDigest) { const callOptions = { after: [currentOperation], id: ids() }; - const pcr0Manager = m.contractAt("PCR0Manager", registryData.pcr0Manager); + const pcr0Manager = m.contractAt("PCR0Manager", registryData.pcr0Manager, { id: ids() }); currentOperation = m.call(pcr0Manager, "addPCR0", [registryData.imageDigest], callOptions); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/ignition/modules/registry/updateRegistries.ts` around lines 140 - 149, The code calls updatePCR0Manager and then m.contractAt("PCR0Manager", registryData.pcr0Manager) followed by addPCR0 without validating the manager address or providing an id for contractAt; update the block to (1) guard that registryData.pcr0Manager is not a zero/empty address before creating the contract call (validate registryData.pcr0Manager !== zeroAddress / truthy), (2) when calling m.contractAt("PCR0Manager", registryData.pcr0Manager) pass a unique id (e.g. ids()) so multiple uses of the same address are tracked, and (3) only call addPCR0 when registryData.imageDigest is present and after the non-zero check; optionally wrap the contract call creation in a try/catch or otherwise ensure failures are handled via currentOperation as done for updatePCR0Manager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/ignition/modules/registry/deployKycRegistry.ts`:
- Around line 5-36: The registries object still contains
"DeployKycRegistryModule#IdentityRegistry" even though deployKycRegistry.ts only
returns gcpKycVerifier; remove that registries entry from updateRegistries.ts
(or alternatively add a guard in handleRegistryDeployment to skip entries whose
module returned address is falsy before calling m.contractAt) so
handleRegistryDeployment no longer receives undefined and attempts to call
m.contractAt with a null address.
In `@contracts/ignition/modules/upgrade/upgradeAadhaarRegistry.ts`:
- Around line 7-10: The code uses hre.network.config.chainId (assigned to
networkName) without checking for undefined, causing deployedAddressesPath and
readFileSync to fail; update the top of upgradeAadhaarRegistry.ts to validate
that hre.network.config.chainId is present (check the networkName variable),
throw a clear error if missing, and only build deployedAddressesPath and call
readFileSync/JSON.parse when chainId is defined (references: networkName,
deployedAddressesPath, deployedAddresses, hre.network.config.chainId).
In `@contracts/test/v2/ofacUpgradePath.test.ts`:
- Around line 107-110: The test currently uses the privileged deployer signer
for both setup and the "non-owner callable" assertion so it doesn't actually
verify non-owner access; modify the test to obtain a second, non-privileged
signer from ethers.getSigners() (e.g., const [, nonOwner] = await
ethers.getSigners()) and call updateOfacRootsWithProof (the non-owner call under
test) with nonOwner instead of deployer while keeping privileged setup calls
(registryKyc.updateGCPJWTVerifier, updateGCPRootCAPubkeyHash, updateTEE)
performed by the owner/deployer; ensure the call that should be rejected uses
the nonOwner signer so the test truly asserts non-owner behavior.
- Around line 155-159: Tests currently only assert one mixed snapshot pair is
rejected; add the inverse mixed-pair assertion to ensure symmetry by calling
actors.registryKyc.checkOfacRoots(initialDobRoot, 900n) and asserting it equals
false (with a message like 'mixed pair must be rejected (inverse)'). Place this
alongside the existing checkOfacRoots(800n, initialYobRoot) assertion so both
Frankenstein combinations are validated.
- Around line 23-30: The packUint256ToHexFields function is decoding the hex
string as UTF-8 characters (Buffer.from(hexStr, "utf8")) which yields ASCII byte
values instead of raw bytes; update packUint256ToHexFields to use
Buffer.from(hexStr, "hex") so the hex string is interpreted as binary data, then
keep the existing bit-shifting logic that builds p0, p1, p2 from the resulting
bytes (refer to the packUint256ToHexFields function and its p0/p1/p2 assignment
loops) and apply the same change in the other affected test files
(registerKyc.test.ts and ofacProofUpdate.test.ts) where the same pattern
appears.
---
Nitpick comments:
In `@contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol`:
- Around line 596-643: The function updateOfacRootsWithProof calls
IGCPJWTVerifier(_gcpJwtVerifier).verifyProof and
IPCR0Manager(_pcr0Manager).isPCR0Set without checking those addresses; add
explicit non-zero address guards for _gcpJwtVerifier and _pcr0Manager at the
start of updateOfacRootsWithProof (or factor into a private validator called by
initializeOfacProof) so you revert with clear custom errors like
MissingGCPJWTVerifier() and MissingPCR0Manager() instead of low-level call
failures; reference the storage vars _gcpJwtVerifier and _pcr0Manager and the
public function updateOfacRootsWithProof when adding the checks.
In `@contracts/ignition/modules/registry/updateRegistries.ts`:
- Around line 140-149: The code calls updatePCR0Manager and then
m.contractAt("PCR0Manager", registryData.pcr0Manager) followed by addPCR0
without validating the manager address or providing an id for contractAt; update
the block to (1) guard that registryData.pcr0Manager is not a zero/empty address
before creating the contract call (validate registryData.pcr0Manager !==
zeroAddress / truthy), (2) when calling m.contractAt("PCR0Manager",
registryData.pcr0Manager) pass a unique id (e.g. ids()) so multiple uses of the
same address are tracked, and (3) only call addPCR0 when
registryData.imageDigest is present and after the non-zero check; optionally
wrap the contract call creation in a try/catch or otherwise ensure failures are
handled via currentOperation as done for updatePCR0Manager.
In `@contracts/ignition/modules/upgrade/upgradeKycRegistry.ts`:
- Around line 6-15: The code reads hre.network.config.chainId into networkName
and uses it to build deployedAddressesPath, but chainId can be undefined; update
the buildModule block to validate the value returned by
hre.network.config.chainId (the networkName variable) before constructing
deployedAddressesPath — if undefined, either derive a safe fallback (e.g., use
hre.network.name or a configured default) or throw a clear error; ensure the
check happens prior to path.join and reference the networkName variable,
deployedAddressesPath and kycProxyAddress so the error handling prevents
creating a path like "chain-undefined" and fails fast with a descriptive
message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4af6796-3761-4d44-9ecc-4f50975de96a
📒 Files selected for processing (11)
contracts/contracts/registry/IdentityRegistryAadhaarImplV1.solcontracts/contracts/registry/IdentityRegistryIdCardImplV1.solcontracts/contracts/registry/IdentityRegistryImplV1.solcontracts/contracts/registry/IdentityRegistryKycImplV1.solcontracts/ignition/deployments/chain-11142220/deployed_addresses.jsoncontracts/ignition/modules/registry/deployKycRegistry.tscontracts/ignition/modules/registry/updateRegistries.tscontracts/ignition/modules/upgrade/upgradeAadhaarRegistry.tscontracts/ignition/modules/upgrade/upgradeKycRegistry.tscontracts/tasks/upgrade/prepare.tscontracts/test/v2/ofacUpgradePath.test.ts
✅ Files skipped from review due to trivial changes (1)
- contracts/contracts/registry/IdentityRegistryImplV1.sol
Comment on lines
5
to
36
| export default buildModule("DeployKycRegistryModule", (m) => { | ||
| // Deploy PoseidonT3 | ||
| console.log("📚 Deploying PoseidonT3 library..."); | ||
| const poseidonT3 = m.library("PoseidonT3"); | ||
| // // Deploy PoseidonT3 | ||
| // console.log("📚 Deploying PoseidonT3 library..."); | ||
| // const poseidonT3 = m.library("PoseidonT3"); | ||
|
|
||
| console.log("🏗️ Deploying IdentityRegistryKycImplV1 implementation..."); | ||
| // Deploy IdentityRegistryImplV1 | ||
| const identityRegistryKycImpl = m.contract("IdentityRegistryKycImplV1", [], { | ||
| libraries: { PoseidonT3: poseidonT3 }, | ||
| }); | ||
| // console.log("🏗️ Deploying IdentityRegistryKycImplV1 implementation..."); | ||
| // // Deploy IdentityRegistryImplV1 | ||
| // const identityRegistryKycImpl = m.contract("IdentityRegistryKycImplV1", [], { | ||
| // libraries: { PoseidonT3: poseidonT3 }, | ||
| // }); | ||
|
|
||
| console.log("⚙️ Preparing registry initialization data..."); | ||
| // Get the interface and encode the initialize function call | ||
| const registryInterface = getRegistryInitializeData(); | ||
| // console.log("⚙️ Preparing registry initialization data..."); | ||
| // // Get the interface and encode the initialize function call | ||
| // const registryInterface = getRegistryInitializeData(); | ||
|
|
||
| const registryInitData = registryInterface.encodeFunctionData("initialize", [ethers.ZeroAddress, ethers.ZeroAddress]); | ||
| console.log(" Init data:", registryInitData); | ||
| // const registryInitData = registryInterface.encodeFunctionData("initialize", [ethers.ZeroAddress, ethers.ZeroAddress]); | ||
| // console.log(" Init data:", registryInitData); | ||
|
|
||
| console.log("🚀 Deploying IdentityRegistry proxy..."); | ||
| // Deploy the proxy contract with the implementation address and initialization data | ||
| const registry = m.contract("IdentityRegistry", [identityRegistryKycImpl, registryInitData]); | ||
| // console.log("🚀 Deploying IdentityRegistry proxy..."); | ||
| // // Deploy the proxy contract with the implementation address and initialization data | ||
| // const registry = m.contract("IdentityRegistry", [identityRegistryKycImpl, registryInitData]); | ||
|
|
||
| // Redeploy verifier — circuit changed due to new trusted setup contributions | ||
| const gcpKycVerifier = m.contract("Verifier_gcp_jwt", []); | ||
|
|
||
| // PCR0Manager not deployed - using existing mainnet PCR0Manager at 0x9743fe2C1c3D2b068c56dE314e9B10DA9c904717 | ||
| // const pcr0Manager = m.contract("PCR0Manager", []); | ||
|
|
||
| console.log("✅ Registry deployment module setup complete!"); | ||
| console.log(" 📋 Summary:"); | ||
| console.log(" - PoseidonT3: Library"); | ||
| console.log(" - IdentityRegistryKycImplV1: Implementation contract"); | ||
| console.log(" - IdentityRegistry: Proxy contract"); | ||
| console.log(" - Verifier_gcp_jwt: GCP JWT verifier contract"); | ||
|
|
||
| return { | ||
| poseidonT3, | ||
| identityRegistryKycImpl, | ||
| registry, | ||
| gcpKycVerifier, | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if updateRegistries.ts still references the KYC registry module
rg -n "DeployKycRegistryModule#IdentityRegistry" contracts/ignition/modules/Repository: selfxyz/self
Length of output: 566
🏁 Script executed:
#!/bin/bash
# Examine updateRegistries.ts to see how it handles the registry deployment
head -100 contracts/ignition/modules/registry/updateRegistries.ts | cat -nRepository: selfxyz/self
Length of output: 5455
🏁 Script executed:
#!/bin/bash
# Check the deployKycRegistry.ts file to see if it exports IdentityRegistry under a different reference
cat -n contracts/ignition/modules/registry/deployKycRegistry.tsRepository: selfxyz/self
Length of output: 2166
🏁 Script executed:
#!/bin/bash
# Look for other modules that might deploy IdentityRegistry for fresh deployments
rg -n "IdentityRegistry" contracts/ignition/modules/ --type ts | grep -i "deploy\|contract" | head -20Repository: selfxyz/self
Length of output: 3028
🏁 Script executed:
#!/bin/bash
# Search for where updateRegistries module is imported/used
rg -n "updateRegistries" contracts/ignition/modules/ --type ts | grep -i "import\|require\|from" | head -15Repository: selfxyz/self
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there's a main deployment orchestration file
fd -e ts -e js contracts/ignition/modules/ | grep -i "main\|index\|deploy" | head -20Repository: selfxyz/self
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Look at upgradeKycRegistry.ts to see if it handles the missing address
cat -n contracts/ignition/modules/upgrade/upgradeKycRegistry.tsRepository: selfxyz/self
Length of output: 1657
🏁 Script executed:
#!/bin/bash
# Check if updateRegistries module is referenced in parent modules or deployment orchestration
rg -n "updateRegistries" contracts/ignition/ --type ts -B 2 -A 2Repository: selfxyz/self
Length of output: 38
🏁 Script executed:
#!/bin/bash
# List all modules in the hub directory to understand the orchestration
fd . contracts/ignition/modules/hub --type f -name "*.ts" | head -20Repository: selfxyz/self
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Check the hub/index or main deployment file
cat -n contracts/ignition/modules/hub/index.ts 2>/dev/null || cat -n contracts/ignition/modules/hub/deployHub.ts 2>/dev/null || ls -la contracts/ignition/modules/hub/Repository: selfxyz/self
Length of output: 1247
Remove "DeployKycRegistryModule#IdentityRegistry" from the registries object in updateRegistries.ts since this deployment module no longer creates the proxy.
Since deployKycRegistry.ts now only deploys gcpKycVerifier, the KYC registry entry in the registries object at line 35 of updateRegistries.ts creates an inconsistency. If this module is invoked, handleRegistryDeployment will receive undefined at line 76 and fail when calling m.contractAt with a null address. Either remove the entry from the registries object or add a guard in handleRegistryDeployment to skip registries with missing addresses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ignition/modules/registry/deployKycRegistry.ts` around lines 5 -
36, The registries object still contains
"DeployKycRegistryModule#IdentityRegistry" even though deployKycRegistry.ts only
returns gcpKycVerifier; remove that registries entry from updateRegistries.ts
(or alternatively add a guard in handleRegistryDeployment to skip entries whose
module returned address is falsy before calling m.contractAt) so
handleRegistryDeployment no longer receives undefined and attempts to call
m.contractAt with a null address.
Comment on lines
+7
to
+10
| const networkName = hre.network.config.chainId; | ||
|
|
||
| const deployedAddressesPath = path.join(__dirname, `../../deployments/chain-${networkName}/deployed_addresses.json`); | ||
| const deployedAddresses = JSON.parse(readFileSync(deployedAddressesPath, "utf8")); |
Contributor
There was a problem hiding this comment.
Guard chainId before resolving deployment path.
Line 7 can be undefined on some network configs; then Line 9 resolves chain-undefined and Line 10 fails with a generic file read error. Fail fast with an explicit chainId/path check.
Suggested fix
import { readFileSync } from "fs";
+import { existsSync } from "fs";
import path from "path";
export default buildModule("UpgradeAadhaarRegistryModule", (m) => {
- const networkName = hre.network.config.chainId;
+ const chainId = hre.network.config.chainId;
+ if (chainId == null) {
+ throw new Error("Missing network chainId in Hardhat config");
+ }
- const deployedAddressesPath = path.join(__dirname, `../../deployments/chain-${networkName}/deployed_addresses.json`);
+ const deployedAddressesPath = path.join(
+ __dirname,
+ `../../deployments/chain-${chainId}/deployed_addresses.json`,
+ );
+ if (!existsSync(deployedAddressesPath)) {
+ throw new Error(`deployed_addresses.json not found for chain-${chainId}`);
+ }
const deployedAddresses = JSON.parse(readFileSync(deployedAddressesPath, "utf8"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const networkName = hre.network.config.chainId; | |
| const deployedAddressesPath = path.join(__dirname, `../../deployments/chain-${networkName}/deployed_addresses.json`); | |
| const deployedAddresses = JSON.parse(readFileSync(deployedAddressesPath, "utf8")); | |
| const chainId = hre.network.config.chainId; | |
| if (chainId == null) { | |
| throw new Error("Missing network chainId in Hardhat config"); | |
| } | |
| const deployedAddressesPath = path.join( | |
| __dirname, | |
| `../../deployments/chain-${chainId}/deployed_addresses.json`, | |
| ); | |
| if (!existsSync(deployedAddressesPath)) { | |
| throw new Error(`deployed_addresses.json not found for chain-${chainId}`); | |
| } | |
| const deployedAddresses = JSON.parse(readFileSync(deployedAddressesPath, "utf8")); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ignition/modules/upgrade/upgradeAadhaarRegistry.ts` around lines 7
- 10, The code uses hre.network.config.chainId (assigned to networkName) without
checking for undefined, causing deployedAddressesPath and readFileSync to fail;
update the top of upgradeAadhaarRegistry.ts to validate that
hre.network.config.chainId is present (check the networkName variable), throw a
clear error if missing, and only build deployedAddressesPath and call
readFileSync/JSON.parse when chainId is defined (references: networkName,
deployedAddressesPath, deployedAddresses, hre.network.config.chainId).
Comment on lines
+23
to
+30
| function packUint256ToHexFields(value: bigint): [bigint, bigint, bigint] { | ||
| const hexStr = value.toString(16).padStart(64, "0"); | ||
| const bytes = Buffer.from(hexStr, "utf8"); | ||
| let p0 = 0n, p1 = 0n, p2 = 0n; | ||
| for (let i = 0; i < Math.min(31, bytes.length); i++) p0 |= BigInt(bytes[i]) << BigInt(i * 8); | ||
| for (let i = 31; i < Math.min(62, bytes.length); i++) p1 |= BigInt(bytes[i]) << BigInt((i - 31) * 8); | ||
| for (let i = 62; i < Math.min(93, bytes.length); i++) p2 |= BigInt(bytes[i]) << BigInt((i - 62) * 8); | ||
| return [p0, p1, p2]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate packing helpers and related signal decoding =="
rg -n -C3 'packUint256ToHexFields|p0|p1|p2|pubSignals|rootsHash|updateOfacRootsWithProof'
echo
echo "== Compare utf8-vs-hex packing outputs for one sample hash =="
python - << 'PY'
value = int("0x" + "12"*32, 16)
hex_str = format(value, "064x")
def pack(b):
p0 = p1 = p2 = 0
for i in range(min(31, len(b))):
p0 |= b[i] << (i * 8)
for i in range(31, min(62, len(b))):
p1 |= b[i] << ((i - 31) * 8)
for i in range(62, min(93, len(b))):
p2 |= b[i] << ((i - 62) * 8)
return p0, p1, p2, len(b)
utf8_bytes = hex_str.encode("utf-8")
hex_bytes = bytes.fromhex(hex_str)
print("utf8_len:", len(utf8_bytes), "hex_len:", len(hex_bytes))
print("utf8_pack:", pack(utf8_bytes)[:3])
print("hex_pack :", pack(hex_bytes)[:3])
print("equal? :", pack(utf8_bytes)[:3] == pack(hex_bytes)[:3])
PYRepository: selfxyz/self
Length of output: 50371
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and examine the function
cd /repo || exit 1
echo "=== Checking file existence and reading the function ==="
if [ -f "contracts/test/v2/ofacUpgradePath.test.ts" ]; then
echo "File found. Reading lines 20-35:"
sed -n '20,35p' "contracts/test/v2/ofacUpgradePath.test.ts"
else
echo "File not found at contracts/test/v2/ofacUpgradePath.test.ts"
echo "Searching for ofacUpgradePath.test.ts:"
find . -name "ofacUpgradePath.test.ts" -type f
fi
echo
echo "=== Searching for usage of packUint256ToHexFields ==="
rg -n "packUint256ToHexFields" --max-count=20Repository: selfxyz/self
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Check file existence and examine the function
echo "=== Checking file existence and reading the function ==="
if [ -f "contracts/test/v2/ofacUpgradePath.test.ts" ]; then
echo "File found. Reading lines 20-35:"
sed -n '20,35p' "contracts/test/v2/ofacUpgradePath.test.ts"
else
echo "File not found at contracts/test/v2/ofacUpgradePath.test.ts"
echo "Searching for ofacUpgradePath.test.ts:"
find . -name "ofacUpgradePath.test.ts" -type f
fi
echo
echo "=== Searching for usage of packUint256ToHexFields ==="
rg -n "packUint256ToHexFields" --max-count=20Repository: selfxyz/self
Length of output: 2056
🏁 Script executed:
#!/bin/bash
# Verify the encoding issue with Python
python3 << 'PYTHON_EOF'
import hashlib
# Show the difference between utf8 and hex encoding
test_value = int("0x" + "12" * 32, 16)
hex_str = format(test_value, "064x")
print(f"Test hex string: {hex_str}")
print(f"Length of hex string: {len(hex_str)}")
# UTF-8 encoding: encode characters themselves
utf8_bytes = hex_str.encode("utf-8")
print(f"\nUTF-8 encoded length: {len(utf8_bytes)} bytes")
print(f"First 10 UTF-8 bytes: {list(utf8_bytes[:10])}")
print(f"Character '1' -> byte {ord('1')} (0x{ord('1'):02x})")
print(f"Character '2' -> byte {ord('2')} (0x{ord('2'):02x})")
# HEX encoding: decode hex string to raw bytes
hex_bytes = bytes.fromhex(hex_str)
print(f"\nHEX decoded length: {len(hex_bytes)} bytes")
print(f"First 10 HEX bytes: {list(hex_bytes[:10])}")
print(f"'12' (hex) -> byte 0x12")
print(f"'12' (hex) -> decimal {0x12}")
print(f"\n=== PACKING RESULTS ===")
def pack(b):
p0 = p1 = p2 = 0
for i in range(min(31, len(b))):
p0 |= b[i] << (i * 8)
for i in range(31, min(62, len(b))):
p1 |= b[i] << ((i - 31) * 8)
for i in range(62, min(93, len(b))):
p2 |= b[i] << ((i - 62) * 8)
return p0, p1, p2
utf8_result = pack(utf8_bytes)
hex_result = pack(hex_bytes)
print(f"UTF-8 pack (WRONG): p0={hex(utf8_result[0])}, p1={hex(utf8_result[1])}, p2={hex(utf8_result[2])}")
print(f"HEX pack (RIGHT): p0={hex(hex_result[0])}, p1={hex(hex_result[1])}, p2={hex(hex_result[2])}")
print(f"Results equal? {utf8_result == hex_result}")
PYTHON_EOF
echo
echo "=== Checking usage context in ofacUpgradePath.test.ts ==="
sed -n '110,125p' "contracts/test/v2/ofacUpgradePath.test.ts"Repository: selfxyz/self
Length of output: 1404
🏁 Script executed:
#!/bin/bash
# Check what the mock verifier does with these signals
echo "=== Searching for verifier mock or proof verification logic ==="
rg -n "verifier|updateOfacRootsWithProof|pubSignals" contracts/test/v2/ofacUpgradePath.test.ts | head -30Repository: selfxyz/self
Length of output: 422
🏁 Script executed:
#!/bin/bash
# Find mock verifier setup or implementation
echo "=== Searching for mock verifier setup ==="
sed -n '80,115p' "contracts/test/v2/ofacUpgradePath.test.ts"
echo
echo "=== Searching for deployments or mocks directory ==="
fd "verifier|mock" -t f --max-depth 3 contracts/test/ | head -20Repository: selfxyz/self
Length of output: 1901
🏁 Script executed:
#!/bin/bash
# Look for the mock verifier in the test setup
echo "=== Search for mockVerifier or verifier definition ==="
rg -n "mockVerifier|MockVerifier" contracts/test/v2/ofacUpgradePath.test.ts -A 5 -B 2 | head -40
echo
echo "=== Check for common test fixtures/setup ==="
fd "setup|fixture" -t f contracts/test/ | head -10Repository: selfxyz/self
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Find and examine the MockGCPJWTVerifier implementation
echo "=== Finding MockGCPJWTVerifier ==="
find . -name "*MockGCPJWTVerifier*" -o -name "*verifier*" -type f | grep -i mock | head -10
echo
echo "=== Search for MockGCPJWTVerifier contract ==="
rg -l "MockGCPJWTVerifier" contracts/ --type solRepository: selfxyz/self
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Examine the MockGCPJWTVerifier contract
echo "=== Reading MockGCPJWTVerifier.sol ==="
cat contracts/contracts/tests/MockGCPJWTVerifier.sol
echo
echo "=== Check what parameters it expects for verification ==="
rg -n "verify|pubSignals" contracts/contracts/tests/MockGCPJWTVerifier.sol -A 3 -B 3Repository: selfxyz/self
Length of output: 3178
🏁 Script executed:
#!/bin/bash
# Verify this is the only place this function is defined and used similarly
echo "=== Check all other instances in registerKyc.test.ts ==="
sed -n '28,35p' "contracts/test/v2/registerKyc.test.ts"
echo
echo "=== Check all other instances in ofacProofUpdate.test.ts ==="
sed -n '25,32p' "contracts/test/v2/ofacProofUpdate.test.ts"Repository: selfxyz/self
Length of output: 610
Fix packUint256ToHexFields encoding in three test files: change Buffer.from(hexStr, "utf8") to Buffer.from(hexStr, "hex").
The function currently encodes the hex string's ASCII characters instead of decoding the hex string to raw bytes. This produces wrong signal values: UTF-8 yields character codes (0x31 for '1', 0x32 for '2') while hex decoding yields binary data (0x12 for hex '12'). The mock verifier masks this bug by ignoring pubSignals entirely and always returning true by default.
This affects at least these files:
- contracts/test/v2/ofacUpgradePath.test.ts (line 25)
- contracts/test/v2/registerKyc.test.ts (line 31)
- contracts/test/v2/ofacProofUpdate.test.ts (line 28)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/test/v2/ofacUpgradePath.test.ts` around lines 23 - 30, The
packUint256ToHexFields function is decoding the hex string as UTF-8 characters
(Buffer.from(hexStr, "utf8")) which yields ASCII byte values instead of raw
bytes; update packUint256ToHexFields to use Buffer.from(hexStr, "hex") so the
hex string is interpreted as binary data, then keep the existing bit-shifting
logic that builds p0, p1, p2 from the resulting bytes (refer to the
packUint256ToHexFields function and its p0/p1/p2 assignment loops) and apply the
same change in the other affected test files (registerKyc.test.ts and
ofacProofUpdate.test.ts) where the same pattern appears.
Comment on lines
+107
to
+110
| const [deployer] = await ethers.getSigners(); | ||
| await actors.registryKyc.updateGCPJWTVerifier(mockVerifier.target); | ||
| await actors.registryKyc.updateGCPRootCAPubkeyHash(GCP_ROOT_CA_PUBKEY_HASH); | ||
| await actors.registryKyc.updateTEE(deployer.address); |
Contributor
There was a problem hiding this comment.
The “non-owner callable” claim is not actually being tested.
Line 107 selects deployer, and Lines 134-136 call updateOfacRootsWithProof using that same signer. In this fixture, that signer is privileged, so this test can still pass even if non-owner access is broken.
Suggested fix
- const [deployer] = await ethers.getSigners();
+ const [deployer, nonOwnerTee] = await ethers.getSigners();
await actors.registryKyc.updateGCPJWTVerifier(mockVerifier.target);
await actors.registryKyc.updateGCPRootCAPubkeyHash(GCP_ROOT_CA_PUBKEY_HASH);
- await actors.registryKyc.updateTEE(deployer.address);
+ await actors.registryKyc.updateTEE(nonOwnerTee.address);
@@
const proofTx = await actors.registryKyc
- .connect(deployer)
+ .connect(nonOwnerTee)
.updateOfacRootsWithProof(Also applies to: 133-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/test/v2/ofacUpgradePath.test.ts` around lines 107 - 110, The test
currently uses the privileged deployer signer for both setup and the "non-owner
callable" assertion so it doesn't actually verify non-owner access; modify the
test to obtain a second, non-privileged signer from ethers.getSigners() (e.g.,
const [, nonOwner] = await ethers.getSigners()) and call
updateOfacRootsWithProof (the non-owner call under test) with nonOwner instead
of deployer while keeping privileged setup calls
(registryKyc.updateGCPJWTVerifier, updateGCPRootCAPubkeyHash, updateTEE)
performed by the owner/deployer; ensure the call that should be rejected uses
the nonOwner signer so the test truly asserts non-owner behavior.
Comment on lines
+155
to
+159
| // --- Phase 6c: Verify checkOfacRoots accepts current and previous snapshots (not mixed) --- | ||
| expect(await actors.registryKyc.checkOfacRoots(800n, 900n)).to.equal(true); | ||
| expect(await actors.registryKyc.checkOfacRoots(initialDobRoot, initialYobRoot)).to.equal(true); | ||
| expect(await actors.registryKyc.checkOfacRoots(800n, initialYobRoot)).to.equal(false, 'mixed pair must be rejected'); | ||
|
|
Contributor
There was a problem hiding this comment.
Add the inverse mixed-pair rejection assertion.
Right now only one Frankenstein pair is checked. If logic is asymmetric, the opposite mix can still slip through.
Suggested fix
expect(await actors.registryKyc.checkOfacRoots(800n, 900n)).to.equal(true);
expect(await actors.registryKyc.checkOfacRoots(initialDobRoot, initialYobRoot)).to.equal(true);
expect(await actors.registryKyc.checkOfacRoots(800n, initialYobRoot)).to.equal(false, 'mixed pair must be rejected');
+ expect(await actors.registryKyc.checkOfacRoots(initialDobRoot, 900n)).to.equal(false, 'mixed pair must be rejected');As per coding guidelines: **/*.{test,spec}.{ts,js,tsx,jsx} requires reviewing test coverage completeness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/test/v2/ofacUpgradePath.test.ts` around lines 155 - 159, Tests
currently only assert one mixed snapshot pair is rejected; add the inverse
mixed-pair assertion to ensure symmetry by calling
actors.registryKyc.checkOfacRoots(initialDobRoot, 900n) and asserting it equals
false (with a message like 'mixed pair must be rejected (inverse)'). Place this
alongside the existing checkOfacRoots(800n, initialYobRoot) assertion so both
Frankenstein combinations are validated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/ignition/modules/upgrade/upgradeIdCardRegistry.ts`:
- Around line 30-33: The upgrade currently calls idCardProxy.upgradeToAndCall
with empty calldata, leaving `_gcpJwtVerifier`, `_pcr0Manager`,
`_gcpRootCAPPubkeyHash`/`_gcpRootCAPubkeyHash`, and `_tee` uninitialized;
replace the empty calldata with ABI-encoded calldata that calls the new
implementation's initializeOfacProof(...) (reinitializer(3)) so all OFAC proof
fields are set atomically during the upgrade; locate the upgrade call to
`upgradeToAndCall(idCardProxy, "0x")` and change the calldata to the encoded
`initializeOfacProof` invocation targeting `newIdCardImpl` so the four fields
are initialized in the same transaction.
In `@contracts/ignition/modules/upgrade/upgradeRegistry.ts`:
- Around line 30-33: The upgrade currently calls m.call(registryProxy,
"upgradeToAndCall", [newRegistryImpl, "0x"], ...) which skips OFAC proof
initialization; change the upgrade to include the proper initializer calldata
for IdentityRegistryImplV1.initializeOfacProof(...) (or add a reinitializer
call) so the proof/TEE config is set during the upgrade; locate the upgrade
module where m.call is invoked with registryProxy and newRegistryImpl and
replace the "0x" calldata with the ABI-encoded parameters for
initializeOfacProof (or invoke initializeOfacProof via upgradeToAndCall) to
ensure OFAC proof dependencies are initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87fb3e34-92db-4941-b94d-d6a35d6bbb6c
📒 Files selected for processing (6)
contracts/contracts/registry/IdentityRegistryIdCardImplV1.solcontracts/contracts/registry/IdentityRegistryImplV1.solcontracts/error-selectors.jsoncontracts/ignition/deployments/chain-11142220/deployed_addresses.jsoncontracts/ignition/modules/upgrade/upgradeIdCardRegistry.tscontracts/ignition/modules/upgrade/upgradeRegistry.ts
✅ Files skipped from review due to trivial changes (1)
- contracts/error-selectors.json
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/contracts/registry/IdentityRegistryImplV1.sol
- contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol
Comment on lines
+30
to
+33
| // Code-only upgrade — no new reinitializer needed | ||
| m.call(idCardProxy, "upgradeToAndCall", [newIdCardImpl, "0x"], { | ||
| after: [newIdCardImpl], | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether OFAC initializer is invoked anywhere in ignition upgrade flows.
rg -n -C3 'initializeOfacProof\s*\(' contracts/ignition/modules
# Verify this module currently uses code-only upgrade calldata.
rg -n -C3 'upgradeToAndCall' contracts/ignition/modules/upgrade/upgradeIdCardRegistry.ts
# Confirm initializer signature/role gate in implementation.
rg -n -C5 'function initializeOfacProof' contracts/contracts/registry/IdentityRegistryIdCardImplV1.solRepository: selfxyz/self
Length of output: 38
🏁 Script executed:
# Search for the upgrade file
find . -name "upgradeIdCardRegistry.ts" -o -name "upgradeIdCardRegistry.js" 2>/dev/null
# Search for the implementation file
find . -name "IdentityRegistryIdCardImplV1.sol" 2>/dev/null
# Explore the contracts directory structure
fd "contracts" -t d | head -20Repository: selfxyz/self
Length of output: 263
🏁 Script executed:
# Read the upgrade file
wc -l ./contracts/ignition/modules/upgrade/upgradeIdCardRegistry.ts
# Get content around lines 30-33
cat -n ./contracts/ignition/modules/upgrade/upgradeIdCardRegistry.ts | sed -n '1,50p'Repository: selfxyz/self
Length of output: 1776
🏁 Script executed:
# Read IdentityRegistryIdCardImplV1.sol to find initializeOfacProof
wc -l ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol
# Search for initializeOfacProof in the file
rg -n 'initializeOfacProof' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol
# Get context around it
rg -n -B5 -A15 'function initializeOfacProof' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.solRepository: selfxyz/self
Length of output: 1126
🏁 Script executed:
# Search for all calls to initializeOfacProof in ignition modules
rg -n 'initializeOfacProof' ./contracts/ignition/
# Broader search across entire contracts directory
rg -n 'initializeOfacProof' ./contracts/ --type ts --type sol
# List all ignition upgrade modules
find ./contracts/ignition/modules -name "*.ts" -type fRepository: selfxyz/self
Length of output: 1559
🏁 Script executed:
# Check initial deployment module for OFAC proof initialization
cat -n ./contracts/ignition/modules/registry/deployIdCardRegistry.ts | head -100
# Look for any script calls
rg -n 'initializeOfacProof\|_gcpJwtVerifier\|_pcr0Manager' ./contracts/ignition/modules/registry/deployIdCardRegistry.tsRepository: selfxyz/self
Length of output: 1275
🏁 Script executed:
# Check state var declarations for OFAC proof variables
rg -n '_gcpJwtVerifier\|_pcr0Manager\|_gcpRootCAPubkeyHash\|_tee' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol | head -20
# Look for any scripts that might handle post-deployment OFAC setup
find ./contracts -type f \( -name "*.ts" -o -name "*.js" \) -path "*/script*" | xargs grep -l "initializeOfacProof" 2>/dev/null || echo "No scripts found"
# Check if there are any hardhat task files or deployment helpers
find ./contracts -type f -name "*.ts" | grep -i "task\|script\|helper" | head -10Repository: selfxyz/self
Length of output: 565
🏁 Script executed:
# Look at state var declarations in IdentityRegistryIdCardImplV1.sol
rg -n -B2 -A2 '_gcpJwtVerifier\|_pcr0Manager\|_gcpRootCAPubkeyHash\|_tee' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol | grep -E '^\s*(address|uint256|private|public)' -A1 -B1
# Get all state vars section
head -250 ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol | tail -150Repository: selfxyz/self
Length of output: 6978
🏁 Script executed:
# Check if OFAC proof initialization happens in any separate module or step
find ./contracts/ignition/modules -name "*.ts" -exec grep -l "gcpJwtVerifier\|pcr0Manager\|gcpRootCAPubkeyHash" {} \;
# Look for any hardhat scripts that might handle post-deployment setup
cat ./contracts/scripts/setHubV2.ts
# Check if there's documentation about OFAC proof setup
find ./contracts -name "*.md" -exec grep -l "OFAC\|initializeOfacProof" {} \; 2>/dev/null || echo "No docs found"Repository: selfxyz/self
Length of output: 3317
🏁 Script executed:
# Check deployKycRegistry.ts for OFAC initialization
cat -n ./contracts/ignition/modules/registry/deployKycRegistry.ts
# Check updateRegistries.ts
cat -n ./contracts/ignition/modules/registry/updateRegistries.ts | head -100
# Check deployPCR0.ts
cat -n ./contracts/ignition/modules/utils/deployPCR0.tsRepository: selfxyz/self
Length of output: 7840
🏁 Script executed:
# Continue reading updateRegistries.ts to see full implementation
cat -n ./contracts/ignition/modules/registry/updateRegistries.ts | sed -n '100,200p'
# Check if updateOfacProof or initializeOfacProof is used elsewhere
rg -n 'updateOfacProof\|initializeOfacProof' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.solRepository: selfxyz/self
Length of output: 3714
🏁 Script executed:
# Check if updateGCPJWTVerifier, updatePCR0Manager, updateTEE, updateGCPRootCAPubkeyHash exist
rg -n 'function updateGCPJWTVerifier\|function updatePCR0Manager\|function updateTEE\|function updateGCPRootCAPubkeyHash' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol
# Check what functions are available for OFAC configuration
rg -n 'function.*GCP\|function.*PCR0\|function.*TEE\|function.*OFAC' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol | head -30Repository: selfxyz/self
Length of output: 38
🏁 Script executed:
# Search more broadly for any function that sets GCP or PCR0 or TEE
rg -n 'function update\|function initialize' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol | grep -i 'gcp\|pcr\|tee\|ofac'
# Get all function signatures in the contract
rg -n '^\s*function ' ./contracts/contracts/registry/IdentityRegistryIdCardImplV1.sol | head -50Repository: selfxyz/self
Length of output: 3995
🏁 Script executed:
# Check other upgrade modules to see if they initialize anything
cat -n ./contracts/ignition/modules/upgrade/upgradeRegistry.ts
# Check if upgradeIdCardRegistry.ts is the only one being used or if updateRegistries is meant to follow
cat -n ./contracts/ignition/modules/upgrade/upgradeAadhaarRegistry.ts | head -50
# Look at the comment in upgradeIdCardRegistry more carefully - what does "Code-only upgrade — no new reinitializer needed" mean?
# Check git history or README for context
cat ./contracts/UPGRADE_GUIDE.mdRepository: selfxyz/self
Length of output: 9190
🏁 Script executed:
# Check if there's documentation showing deployment sequence
find ./contracts -name "README*" -o -name "*DEPLOY*" | xargs ls -la 2>/dev/null
# Check the package.json scripts to see deployment workflow
cat ./contracts/package.json | grep -A 20 '"scripts"'Repository: selfxyz/self
Length of output: 2896
Initialize OFAC proof configuration during upgrade to prevent functionality gap.
Lines 31–33 upgrade the implementation with empty calldata ("0x"), leaving _gcpJwtVerifier, _pcr0Manager, _gcpRootCAPubkeyHash, and _tee uninitialized. Although individual updateGCPJWTVerifier(), updatePCR0Manager(), updateGCPRootCAPubkeyHash(), and updateTEE() functions exist, they require separate transactions. The contract provides initializeOfacProof(...) with reinitializer(3) specifically to atomically set all four fields during upgrade—using it here ensures OFAC proof dependencies are ready immediately after the upgrade completes.
Suggested implementation
// Code-only upgrade — no new reinitializer needed
m.call(idCardProxy, "upgradeToAndCall", [newIdCardImpl, "0x"], {
after: [newIdCardImpl],
});
+
+ // Initialize OFAC proof dependencies immediately after upgrade
+ const gcpJwtVerifier = m.getParameter("gcpJwtVerifier");
+ const pcr0Manager = m.getParameter("pcr0Manager");
+ const gcpRootCAPubkeyHash = m.getParameter("gcpRootCAPubkeyHash");
+ const teeAddress = m.getParameter("teeAddress");
+
+ m.call(
+ idCardProxy,
+ "initializeOfacProof",
+ [gcpJwtVerifier, pcr0Manager, gcpRootCAPubkeyHash, teeAddress],
+ { after: [newIdCardImpl] }
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ignition/modules/upgrade/upgradeIdCardRegistry.ts` around lines 30
- 33, The upgrade currently calls idCardProxy.upgradeToAndCall with empty
calldata, leaving `_gcpJwtVerifier`, `_pcr0Manager`,
`_gcpRootCAPPubkeyHash`/`_gcpRootCAPubkeyHash`, and `_tee` uninitialized;
replace the empty calldata with ABI-encoded calldata that calls the new
implementation's initializeOfacProof(...) (reinitializer(3)) so all OFAC proof
fields are set atomically during the upgrade; locate the upgrade call to
`upgradeToAndCall(idCardProxy, "0x")` and change the calldata to the encoded
`initializeOfacProof` invocation targeting `newIdCardImpl` so the four fields
are initialized in the same transaction.
Comment on lines
+30
to
+33
| // Code-only upgrade — no new reinitializer needed | ||
| m.call(registryProxy, "upgradeToAndCall", [newRegistryImpl, "0x"], { | ||
| after: [newRegistryImpl], | ||
| }); |
Contributor
There was a problem hiding this comment.
Same upgrade gap here: OFAC proof initializer is skipped.
Line 31 uses "0x" calldata, so OFAC proof dependencies are not initialized in this upgrade module. Per contracts/contracts/registry/IdentityRegistryImplV1.sol (Line 294-Line 304), initializeOfacProof(...) is required to set the proof/TEE config used by proof-based OFAC root updates.
Suggested patch
// Code-only upgrade — no new reinitializer needed
m.call(registryProxy, "upgradeToAndCall", [newRegistryImpl, "0x"], {
after: [newRegistryImpl],
});
+
+ const gcpJwtVerifier = m.getParameter("gcpJwtVerifier");
+ const pcr0Manager = m.getParameter("pcr0Manager");
+ const gcpRootCAPubkeyHash = m.getParameter("gcpRootCAPubkeyHash");
+ const teeAddress = m.getParameter("teeAddress");
+
+ m.call(
+ registryProxy,
+ "initializeOfacProof",
+ [gcpJwtVerifier, pcr0Manager, gcpRootCAPubkeyHash, teeAddress],
+ { after: [newRegistryImpl] }
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ignition/modules/upgrade/upgradeRegistry.ts` around lines 30 - 33,
The upgrade currently calls m.call(registryProxy, "upgradeToAndCall",
[newRegistryImpl, "0x"], ...) which skips OFAC proof initialization; change the
upgrade to include the proper initializer calldata for
IdentityRegistryImplV1.initializeOfacProof(...) (or add a reinitializer call) so
the proof/TEE config is set during the upgrade; locate the upgrade module where
m.call is invoked with registryProxy and newRegistryImpl and replace the "0x"
calldata with the ABI-encoded parameters for initializeOfacProof (or invoke
initializeOfacProof via upgradeToAndCall) to ensure OFAC proof dependencies are
initialized.
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
2 participants
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
Test plan
Native Consolidation Checklist
cd app && yarn jest:run/yarn workspace @selfxyz/rn-sdk-test-app test)Summary by CodeRabbit
New Features
Tests
Documentation
Chores