[FEAT] Role-based access control across host-chain and registry-chain contracts

[liorbond]

Replaces Ownable/Ownable2Step with AccessControlDefaultAdminRules on TaskManager, ACL, PlaintextsStorage, and CommitmentRegistry, binding each previously onlyOwner entry point to a narrow role.

Roles

Contract	Roles
TaskManager	UPGRADER, PAUSER, SECURITY_ZONE_MANAGER, AGGREGATOR_MANAGER, ACCESS_LIST_MANAGER, VERIFIER_SIGNER_MANAGER, DECRYPT_SIGNER_MANAGER, CONFIG_MANAGER
ACL	UPGRADER
PlaintextsStorage	UPGRADER
CommitmentRegistry	UPGRADER, POSTER_MANAGER, VERSION_MANAGER

No admin entry point is left ungated.

Deployment: admin wallet receives every role

initialize grants only DEFAULT_ADMIN_ROLE, and _authorizeUpgrade requires UPGRADER_ROLE — so without an explicit grant every proxy would have been permanently un-upgradeable from the moment it was deployed.

Rather than maintain a hand-written list (which had also omitted ACCESS_LIST_MANAGER_ROLE), grantAllRoles() discovers every *_ROLE constant from the contract ABI and grants it to the admin wallet. A role added to a contract can no longer silently miss its grant. DEFAULT_ADMIN_ROLE is skipped — AccessControlDefaultAdminRules reverts on granting it directly.

Applied in deploy/deploy.ts (TaskManager, ACL, PlaintextsStorage), tasks/upgradeTM.ts, registry-chain/scripts/deploy.ts, and estimateGasArbitrum.ts.

Deploy script repairs

Three separate breakages on the localfhenix path:

• deployDeterministicTM.ts threw before sending a transaction — it encoded initialize from the TaskManager ABI (now 2-arg) while the implementation behind the bootstrap proxy is DeterministicTM.initialize(address). Now encoded from the DeterministicTM ABI. This matters beyond the arity error: the proxy is deployed via CREATE2, so its init data feeds its address. The encoding is byte-identical to pre-PR (0xc4d66de8…), keeping the proxy at 0xeA30c4… — the address compiled into FHE.sol and into ACL/PlaintextsStorage as a constant.
• upgradeTM called defaultAdmin() on a proxy still running the Ownable implementation → revert, before the upgrade started.
• The upgrade passed "0x", never running initializeV2, leaving AccessControl storage empty; the incVersion() call two lines later then reverted for lack of CONFIG_MANAGER_ROLE.

Security: initializeV2 is unauthenticated

On a migrating proxy, defaultAdmin() is zero between the upgrade and the migration call — anyone could claim DEFAULT_ADMIN_ROLE in that window. initializeV2 is now passed as the data argument of upgradeToAndCall so it executes atomically, in both deploy.ts and the standalone task:upgradeTM. Fresh deploys were already safe: a second call reverts with AccessControlEnforcedDefaultAdminRules.

registry-chain migration

CommitmentRegistry.sol had been migrated but nothing around it: scripts/deploy.ts and estimateGasArbitrum.ts passed the old 2-arg initializer and called setVersionStatus without VERSION_MANAGER_ROLE, and all 95 tests failed at the fixture. Scripts, fixture, and behavior tests migrated (owner() → defaultAdmin(), transferOwnership/acceptOwnership → beginDefaultAdminTransfer/acceptDefaultAdminTransfer, OwnableUnauthorizedAccount → AccessControlUnauthorizedAccount).

Storage layout

pnpm storage-layout:check (enforced by .github/workflows/checks.yml) rejected all three host-chain contracts: dropping Ownable/Ownable2Step deletes their ERC-7201 namespaces, leaving orphaned owner data a future upgrade could reuse. Applied the validator's own recommendation — retaining the namespace structs — rather than re-baselining storage-layout-snapshot.json, which would have silenced a real warning.

Tests

New test/roles/Roles.ts (13 tests) asserts the deploy invariant directly: the admin holds every declared role on all three contracts, UPGRADER_ROLE gates upgrades, DEFAULT_ADMIN_ROLE cannot stand in for an operational role, and initializeV2 cannot hijack an initialized proxy. Host-chain fixtures now use the same ABI-driven helper, so a future role added without a grant fails tests rather than a deployment.

	host-chain	registry-chain
Tests	65 passing	95 passing
Storage layout	3/3 compatible	n/a
solhint	clean¹	—
tsc	no errors in changed files²	clean

¹ one pre-existing unused-Strings warning, identical on master. ² the package has pre-existing typechain/Contract typing errors throughout; none added.

Breaking changes

• initialize signatures: TaskManager/ACL/PlaintextsStorage take (address initialAdmin, uint48 initialDelay); CommitmentRegistry takes (address initialAdmin, uint48 initialDelay, address initialPoster).
• owner() → defaultAdmin(); transferOwnership/acceptOwnership → beginDefaultAdminTransfer/acceptDefaultAdminTransfer.
• Deployments must grant operational roles explicitly, including UPGRADER_ROLE.
• Existing proxies require initializeV2 via upgradeToAndCall.

Follow-ups (deliberately not in scope)

• The Deterministic* bootstrap contracts stay on Ownable — changing their bytecode would shift the canonical TaskManager address.
• initialDelay is 0 everywhere, matching current behavior. Worth revisiting for mainnet, where a non-zero default-admin handover delay is the point of using AccessControlDefaultAdminRules.

🤖 Generated with Claude Code

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[roeezolantz]

Reviewed the whole diff locally, plus the OZ 5.2.0 sources and live chain state. The role mapping itself is solid — every onlyOwner maps to exactly one onlyRole, nothing is left ungated, sequential storage is preserved, and the CREATE2 byte-identity fix is correct and necessary. Nice catch on that one.

Severity tags on each comment. Blocking items are the Critical on initializeV2 and the missing test for the migration path.

On initializeV2 (the Critical): the deploy scripts are correct — upgradeToAndCall really is one atomic tx, so that path is safe. The problem is that the safety is a property of one ternary in TypeScript, not of the contract. Anything that upgrades without going through it (a Safe, a manual cast send, upgradeProxy with no call:, or the three contracts that have no such script) reopens the window. Verified on Sepolia that the canonical TM proxy 0xeA30c4...48D9 currently has _initialized = 1 and a zero default-admin slot, so both preconditions are live today.

A few things that couldn't be anchored inline:

• High — no test for the migration. Every upgradeToAndCall in the suite passes "0x", and the only initializeV2 test is the already-initialized case where OZ reverts for you. Worth one that deploys DeterministicTM, does upgradeToAndCall(TMImpl, "0x"), then has a non-admin call initializeV2 and win — so the fix has something to prove.
• Medium — storage-layout-snapshot.json should be regenerated in this PR. upgrades-core only walks namespaces present in the original layout, so the new AccessControl / AccessControlDefaultAdminRules namespaces aren't tracked yet and a later PR could drop them with CI green. Safe order: run storage-layout:check against the current baseline first (it passes, which proves compatibility), then generate and eyeball the diff — should be additions only. Not re-baselining blindly was the right call; re-baselining with that evidence is better than deferring.
• Low — deploy/deploy.ts:230, upgrades.upgradeProxy(...) with no call: sends empty calldata. Marked not-currently-used, but it's generic over ACL/PT/TM and one use on a pre-roles proxy opens the window. Delete it or make it pass the migration data.
• Low — registry-chain has no storage-layout check and no CI job at all (its 95 tests don't run either). Filed as PRO-528, not for this PR. Related: the CHANGELOG.md:16 claim that all four contracts retain the Ownable namespaces isn't accurate — worth fixing here.

Not flagging test/verifyInput/InputVerified.ts since I hear it's already being handled on another branch — just noting the suite is currently red on it (77 passing / 1 failing), so the numbers in the description are stale.

[liorbond]

Non-anchored items, all in fd6e0f7:

• Migration test — added. Bootstraps DeterministicTM, does upgradeToAndCall(TM, "0x"), then a stranger's initializeV2 must revert. Fails without the gate. Same for CommitmentRegistry.
• storage-layout-snapshot.json — re-baselined in your order: checked against the old baseline first, then regenerated. Sequential storage is byte-identical apart from the two intentional renames; AccessControl / AccessControlDefaultAdminRules namespaces now tracked. Heads-up: b378414's aggregators → _aggregators rename would have failed storage-layout:check against the committed baseline — this re-baseline is what makes it pass.
• deploy.ts:230 upgradeProxy with no call: — deleted, nothing called it.
• registry-chain CI — left to PRO-528. The CHANGELOG half is fixed.

Suite is green now: host-chain 87 passing / 0 failing (the InputVerified.ts red was this PR's own initialize signature change — fixed here, flagging in case it collides with the other branch), registry-chain 99 passing.

Not run: Slither. My only new assembly is an sload (Informational; CI fails on medium) with a disable comment.

[roeezolantz]

Re-reviewed at fd6e0f7. The critical one is properly closed — legacy-owner gate on all four initializeV2s, slot constant re-derived and matching OZ 5.2.0, fail-closed on a zero legacy owner (which also kills the renounce-then-reclaim edge), and _pendingOwner is a different namespace so a pending 2-step owner can't sneak through. The Roles.ts migration test is the real thing too: real DeterministicTM, real "0x" gap, asserts the stranger is rejected with state unchanged. Ran both suites locally — 87 and 99 passing, storage-layout check OK on all three.

Two follow-ups below, both on the bootstrap migration path. Neither is an auth hole; they're about the DTM→TaskManager upgrade being layout-incompatible in a way that was previously hidden by the swallowed validation error.

Not blocking, noted for later: registry-chain still hardcodes DEFAULT_ADMIN_DELAY = 0 on every network while host-chain now refuses exactly that — worth aligning when the admin moves to a Safe, since that's when the cancellation window starts being worth anything.

[liorbond]

All 12 threads addressed in 8abbdc8, plus the non-anchored one: registry-chain no longer hardcodes DEFAULT_ADMIN_DELAY = 0 — it takes REGISTRY_ADMIN_DELAY with the same non-local guard host-chain has.

Two notes worth surfacing rather than leaving in a thread:

• The DTM→TM slot read was fail-open, not fail-closed. I dumped both layouts: acl picks up the old plaintextsStorage, and verifierSigner/decryptResultSigner read slots the stub never wrote — so both were 0, the verification-disabled sentinel. The only thing holding a freshly-migrated proxy shut was isEnabled, and verifyInput was the one intake path not behind that switch. So it is now onlyIfEnabled, and initializeV2 seeds address(1) for both signers when it reads zero. This is breaking: a proxy migrated via initializeV2 starts disabled, so enable() is required before input verification works.

• initializeV2 seeds conditionally, on purpose. Doing it unconditionally would clobber a live pre-roles proxy — master's initialize sets both signers to address(1), so they hold real values on staging, and overwriting them would reject every genuine input until an operator re-ran the setters. There is a test asserting the configured-proxy case is left untouched.

I also considered fixing this structurally by making DeterministicTM's layout a compatible prefix of TaskManager's, which would delete the whole problem class. It does not work: TM inserts randomCounter at slot 1 and moves the aggregator to slot 2, so the stub's acl/verifierSigner/plaintextsStorage would all have to move, slot indices are bytecode immediates, and the stub's CREATE2 address (and therefore the proxy's, and therefore the constant compiled into FHE.sol) would shift.

Suites: host-chain 91 passing, registry-chain 100 passing, storage-layout OK ×3, solhint 0 errors.

One thing to watch: TaskManager is now 22567 bytes, 91.8% of the 24KB limit — past the 22KB warn line, ~2KB of headroom left. check:size still exits 0. The role grants cost most of it (already looped rather than inlined, which saved ~350 bytes).

[haimbj1]

initialize never runs on a TaskManager proxy (it's initialized as DeterministicTM), and TaskManagerSetup allows VERIFIER_ADDRESS=0 on local networks — so the reasoning here doesn't hold, though the code does.

Suggested change

	// A proxy arriving from the pre-roles TaskManager already holds real signers in those
	// slots and is left untouched: its `initialize` set both to address(1), so neither can
	// legitimately be zero there. `isEnabled`, `acl` and `plaintextsStorage` are deliberately
	// not touched - the first is already true on a live proxy (migrating must not pause it),
	// and the latter two have no safe default and must be set via CONFIG_MANAGER_ROLE.
	// Zero is also a legitimate configured state - the debug bypass at L789/L861 - so a proxy
	// deliberately running with verification off is flipped fail-closed here and has to re-set
	// it after migrating. `isEnabled`, `acl` and `plaintextsStorage` are deliberately not
	// touched: the first is already true on a live proxy (migrating must not pause it), and the
	// latter two have no safe default and must be set via CONFIG_MANAGER_ROLE.