fix(genesis): normalize genesis identity comparisons

[pawansatoshi]

Summary

Normalize hexadecimal identities at comparison boundaries during genesis validation.

schemaAddress and schemaHex accept mixed-case hexadecimal values, but several genesis validation checks previously compared raw strings. Because hexadecimal casing does not change the represented bytes, equivalent identities with different casing could bypass uniqueness or role-separation checks.

What changed

• Normalize validator public keys before uniqueness checks.
• Normalize validator registerer addresses before uniqueness checks.
• Normalize controller addresses before cross-validator uniqueness checks.
• Normalize operator/proxy-admin comparisons.
• Normalize the hard-coded ValidatorRegistry proxy-address comparison.
• Preserve the original input representation for serialization and error messages.

Regression coverage

Added tests covering:

• Duplicate public keys with different hexadecimal casing.
• Duplicate controllers with different casing.
• Duplicate validator registerers with different casing.
• Operator colliding with the proxy admin using different casing.
• Controller colliding with the PVM proxy admin using different casing.

Security impact

This is a genesis/configuration integrity issue rather than a standalone permissionless exploit.

Without normalization, a malformed genesis configuration could represent the same underlying address/key bytes multiple times while bypassing string-based uniqueness or role-separation checks. This could result in unintended duplicate identities or role assignments during genesis construction.

The fix makes comparisons operate on the represented hexadecimal identity rather than its textual casing.

Validation

The regression tests are included in:

tests/unit/validator-manager-genesis-validation.test.ts

The full Arc toolchain was not executed in this environment. Before merging, run the repository's normal validation commands, including:

• make test-unit-hardhat
• make lint

and attach/verify the resulting CI checks.

Scope

This PR is intentionally limited to genesis identity normalization and its regression coverage. EIP-7702 transaction-pool research was kept separate from this PR.

[osr21]

Reviewed this end-to-end, including running the tests. The premise is real, the fix is correct, and the tests are genuine regression coverage. Two things need attention before merge: the fix is incomplete in one file, and CI will not execute any of it.

Verified the premise

scripts/genesis/types.ts:24 is a bare regex with no checksum enforcement and no normalizing transform:

export const schemaAddress = z.string().regex(/^0x[0-9a-fA-F]{40}$/) as z.Schema<Address>

So mixed casing is accepted and two spellings of one identity are representable. Confirmed.

Verified the tests actually catch the bug

I ran them both ways rather than taking the diff at its word:

tree	result
PR head c7189f3	9 passing
main + only the PR's test file	5 failing — exactly the 5 new cases

They're real regression tests, not tautological. Good.

Incomplete: NativeFiatToken.ts minters have the identical bug

scripts/genesis/NativeFiatToken.ts:81 still uses the raw-Set pattern this PR removes everywhere else:

const minterSet = new Set()
for (const minter of data.minters) {
  if (minterSet.has(minter.address)) { /* ... */ }
  minterSet.add(minter.address)
}

I probed it against your branch with a control case:

identical casing -> success = false   (correctly rejected)
mixed casing     -> success = true    (duplicate slips through)

The impact is concrete, not theoretical. Minter storage is written via slotForAddressMap(12n, …) and slotForAddressMap(13n, …), which routes through addressToBigInt — so both spellings resolve to the same slot. A config declaring two minters with allowances A and B does not produce two minters and does not error; it produces one minter whose minterAllowed is silently whichever entry came last. That is precisely the outcome the uniqueness check exists to prevent, and minters are a privileged role.

Same one-line change you applied elsewhere:

const minterSet = new Set<string>()
for (const minter of data.minters) {
  const normalized = minter.address.toLowerCase()
  if (minterSet.has(normalized)) { /* message keeps minter.address */ }
  minterSet.add(normalized)
}

Worth adding to the security note and to the regression suite, since it's the one case with a demonstrable silent-overwrite consequence.

The call-site casts are redundant — and the fix is broader than the description says

Because the fix also landed inside enforceOperatorsNotProxyAdmin (normalizing both operands), the four .toLowerCase() as Address casts added at the ValidatorManager.ts:131 call site have no observable effect — the helper already lowercases value. I'd drop them: they add as Address casts that assert a type the expression doesn't carry, and they make the diff look narrower than it is.

That helper change actually fixes all five call sites — Denylist.ts:60, NativeFiatToken.ts:70, ProtocolConfig.ts:96, and both ValidatorManager.ts:131,175. That's a genuine strengthening and the PR description undersells it; a reviewer scanning the diff would conclude only ValidatorManager was covered.

One thing not to "fix" while you're in there: Denylist's denylisters array has no uniqueness check at all, so casing is moot for it. Adding normalization there without adding the uniqueness check would be cosmetic.

CI will not run these tests, and will not lint these files

This matters for your own "run these before merging" note. The target exists and does cover the file — Makefile:176:

test-unit-hardhat: ## Run hardhat unit tests
	npx hardhat test ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts --no-compile

But nothing invokes it:

• .github/workflows/ci.yml is 288 lines and the only make invocation in the entire .github/workflows/ directory is make up. There is no npx hardhat test and no reference to tests/unit anywhere in it.
• make test-unit does not chain to it either — it runs make lint plus cargo nextest.
• The eslint step explicitly excludes both changed paths: npx eslint --ignore-pattern 'scripts/' --ignore-pattern 'tests/'.

So a green CI run on this PR does not mean the new regression tests passed — they will not have executed, and neither changed file is in lint scope. Since fork PRs need maintainer CI approval anyway, I'd state the local results explicitly in the PR body. The results above are reproducible with npx hardhat test ./tests/unit/validator-manager-genesis-validation.test.ts --no-compile after npm ci.

A stronger root-cause option, with evidence that it's free

Normalizing at comparison boundaries is the right tactical fix, but it's a discipline the codebase now has to maintain forever — the NativeFiatToken miss above is that discipline failing on the very first pass. Two root-cause options:

1. schemaAddress.transform(s => s.toLowerCase()) — fixes every present and future comparison at once, but changes serialization. I'd avoid it: committed genesis artifacts carry mixed-case addresses (34 of 294 in assets/testnet/genesis.json) and the testnet genesis hash is pinned by test, so this needs hash re-verification.

2. Enforce EIP-55 checksum in schemaAddress. Strictly stronger than normalization: with checksum enforced there is exactly one valid spelling per address, so casing-duplicates become unrepresentable rather than merely detected. And it is serialization-neutral — I checksummed every address in the committed configs:

   file	unique addresses	valid EIP-55
   assets/devnet/config.json	26	26
   assets/mainnet/config.json	40	40
   assets/testnet/config.json	26	26

   All 92 already pass, so enforcing it changes no committed config and no output. It also catches single-character typos, which normalization silently accepts.

I'd keep this PR as the safe, reviewable fix and treat checksum enforcement as a separate change — but it's worth recording as the durable version, because normalization-at-comparison relies on every future author remembering.

Minor

• security/ is a new top-level directory that doesn't exist on main. Worth confirming maintainers want that location rather than docs/.
• The security note ends with "Before upstream submission, run the repository's normal TypeScript formatting, linting, and unit-test commands" — that's a working note to yourself, and reads oddly in a committed file since this is the upstream submission.
• overrides: Record<string, unknown> in the test helper drops type-checking on the override, so a typo'd key would silently produce a config that passes for the wrong reason. Typing it as Partial<ReturnType<typeof configWithValidators>> keeps the tests honest.
• 9 commits including staging/unstaging churn (stage EIP-7702 research, remove unrelated finding, separate EIP-7702 research). Worth squashing so the history is just the fix, tests, and doc.

For what it's worth, AccountCreator.ts is clean — its Map/Set keys are numeric registration IDs, not hex, so it isn't affected.

---

Disclosure: I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory only. Test results above were produced locally against PR head c7189f3 and main at de76122; the EIP-55 check used a self-tested keccak256 implementation verified against the standard empty-string and abc vectors.

[pawansatoshi]

Validation update
Re-ran the validation locally after initializing the repository submodules:
Focused genesis regression tests: 14 passing
Full Hardhat unit suite (make test-unit-hardhat): 47 passing
make lint: passed
git diff --check: clean
Foundry: v1.4.4
The working tree is clean and no dependency/package changes are included.
Also verified the mixed-case NativeFiatToken minter regression coverage alongside the ValidatorManager cases.
The change remains scoped to genesis identity normalization and regression coverage.

[pawansatoshi]

Reviewed this end-to-end, including running the tests. The premise is real, the fix is correct, and the tests are genuine regression coverage. Two things need attention before merge: the fix is incomplete in one file, and CI will not execute any of it.

Verified the premise

scripts/genesis/types.ts:24 is a bare regex with no checksum enforcement and no normalizing transform:

export const schemaAddress = z.string().regex(/^0x[0-9a-fA-F]{40}$/) as z.Schema<Address>

So mixed casing is accepted and two spellings of one identity are representable. Confirmed.

Verified the tests actually catch the bug

I ran them both ways rather than taking the diff at its word:

tree result
PR head c7189f3 9 passing
main + only the PR's test file 5 failing — exactly the 5 new cases
They're real regression tests, not tautological. Good.

Incomplete: NativeFiatToken.ts minters have the identical bug

scripts/genesis/NativeFiatToken.ts:81 still uses the raw-Set pattern this PR removes everywhere else:

const minterSet = new Set()
for (const minter of data.minters) {
  if (minterSet.has(minter.address)) { /* ... */ }
  minterSet.add(minter.address)
}

I probed it against your branch with a control case:

identical casing -> success = false   (correctly rejected)
mixed casing     -> success = true    (duplicate slips through)

The impact is concrete, not theoretical. Minter storage is written via slotForAddressMap(12n, …) and slotForAddressMap(13n, …), which routes through addressToBigInt — so both spellings resolve to the same slot. A config declaring two minters with allowances A and B does not produce two minters and does not error; it produces one minter whose minterAllowed is silently whichever entry came last. That is precisely the outcome the uniqueness check exists to prevent, and minters are a privileged role.

Same one-line change you applied elsewhere:

const minterSet = new Set<string>()
for (const minter of data.minters) {
  const normalized = minter.address.toLowerCase()
  if (minterSet.has(normalized)) { /* message keeps minter.address */ }
  minterSet.add(normalized)
}

Worth adding to the security note and to the regression suite, since it's the one case with a demonstrable silent-overwrite consequence.

The call-site casts are redundant — and the fix is broader than the description says

Because the fix also landed inside enforceOperatorsNotProxyAdmin (normalizing both operands), the four .toLowerCase() as Address casts added at the ValidatorManager.ts:131 call site have no observable effect — the helper already lowercases value. I'd drop them: they add as Address casts that assert a type the expression doesn't carry, and they make the diff look narrower than it is.

That helper change actually fixes all five call sites — Denylist.ts:60, NativeFiatToken.ts:70, ProtocolConfig.ts:96, and both ValidatorManager.ts:131,175. That's a genuine strengthening and the PR description undersells it; a reviewer scanning the diff would conclude only ValidatorManager was covered.

One thing not to "fix" while you're in there: Denylist's denylisters array has no uniqueness check at all, so casing is moot for it. Adding normalization there without adding the uniqueness check would be cosmetic.

CI will not run these tests, and will not lint these files

This matters for your own "run these before merging" note. The target exists and does cover the file — Makefile:176:

test-unit-hardhat: ## Run hardhat unit tests
	npx hardhat test ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts --no-compile

But nothing invokes it:

• .github/workflows/ci.yml is 288 lines and the only make invocation in the entire .github/workflows/ directory is make up. There is no npx hardhat test and no reference to tests/unit anywhere in it.
• make test-unit does not chain to it either — it runs make lint plus cargo nextest.
• The eslint step explicitly excludes both changed paths: npx eslint --ignore-pattern 'scripts/' --ignore-pattern 'tests/'.

So a green CI run on this PR does not mean the new regression tests passed — they will not have executed, and neither changed file is in lint scope. Since fork PRs need maintainer CI approval anyway, I'd state the local results explicitly in the PR body. The results above are reproducible with npx hardhat test ./tests/unit/validator-manager-genesis-validation.test.ts --no-compile after npm ci.

A stronger root-cause option, with evidence that it's free

Normalizing at comparison boundaries is the right tactical fix, but it's a discipline the codebase now has to maintain forever — the NativeFiatToken miss above is that discipline failing on the very first pass. Two root-cause options:

1. schemaAddress.transform(s => s.toLowerCase()) — fixes every present and future comparison at once, but changes serialization. I'd avoid it: committed genesis artifacts carry mixed-case addresses (34 of 294 in assets/testnet/genesis.json) and the testnet genesis hash is pinned by test, so this needs hash re-verification.

2. Enforce EIP-55 checksum in schemaAddress. Strictly stronger than normalization: with checksum enforced there is exactly one valid spelling per address, so casing-duplicates become unrepresentable rather than merely detected. And it is serialization-neutral — I checksummed every address in the committed configs:

   file
   unique addresses
   valid EIP-55

   assets/devnet/config.json
   26
   26

   assets/mainnet/config.json
   40
   40

   assets/testnet/config.json
   26
   26

   All 92 already pass, so enforcing it changes no committed config and no output. It also catches single-character typos, which normalization silently accepts.

I'd keep this PR as the safe, reviewable fix and treat checksum enforcement as a separate change — but it's worth recording as the durable version, because normalization-at-comparison relies on every future author remembering.

Minor

• security/ is a new top-level directory that doesn't exist on main. Worth confirming maintainers want that location rather than docs/.
• The security note ends with "Before upstream submission, run the repository's normal TypeScript formatting, linting, and unit-test commands" — that's a working note to yourself, and reads oddly in a committed file since this is the upstream submission.
• overrides: Record<string, unknown> in the test helper drops type-checking on the override, so a typo'd key would silently produce a config that passes for the wrong reason. Typing it as Partial<ReturnType<typeof configWithValidators>> keeps the tests honest.
• 9 commits including staging/unstaging churn (stage EIP-7702 research, remove unrelated finding, separate EIP-7702 research). Worth squashing so the history is just the fix, tests, and doc.

For what it's worth, AccountCreator.ts is clean — its Map/Set keys are numeric registration IDs, not hex, so it isn't affected.

Disclosure: I'm an external community contributor, not affiliated with Circle, with no write access to this repository. Advisory only. Test results above were produced locally against PR head c7189f3 and main at de76122; the EIP-55 check used a self-tested keccak256 implementation verified against the standard empty-string and abc vectors.

Thanks for the detailed review. I addressed the NativeFiatToken minter finding as suggested.
Normalized minter addresses at the uniqueness-check boundary using toLowerCase().
Added regression coverage for both identical-case and mixed-case duplicate minters.
Added a positive case confirming distinct minter addresses remain valid.
Updated the security note to document the concrete storage-slot collision / last-write-wins impact.
I also removed the redundant .toLowerCase() as Address call-site casts since enforceOperatorsNotProxyAdmin() now normalizes both operands internally.
I kept EIP-55 checksum enforcement out of this PR as suggested, since that would be a broader input-validation policy change.
After the changes, I re-ran the validation:
Focused genesis regression tests: 14 passing
Full Hardhat unit suite: 47 passing
make lint: passed
git diff --check: clean
Thanks again for catching the NativeFiatToken case — it made the fix materially more complete.