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

CHANGELOG.md

@@ -14,8 +14,21 @@
 - **BREAKING — `TaskManager.verifyInput` is removed; `batchVerifyInputs` is the only input-verification entry point.** The verifier no longer issues per-input signatures (its `POST /verify` endpoint is gone), so the single-input endpoint had no way to be satisfied. The `bytes` and `external*` overloads of `FHE.asEbool`/`asEuint*`/`asEaddress` still exist and keep their signatures — internally each now submits a batch of one, so the proof they take must be the batch digest `keccak256(h_0)` rather than `h_0` itself. `EncryptedInput` and the `Utils.inputFromBytes` / `inputFromHashAndProof` helpers are unchanged. `verifyInput` is also dropped from the `ITaskManager` interface, so any contract calling `TaskManager.verifyInput` directly must move to `batchVerifyInputs`. `DeterministicTM` still exposes only the old `verifyInput` and has no `batchVerifyInputs`, so it can no longer back input verification through `FHE.sol`.
 - **BREAKING — the `InEbool` / `InEuint8`–`InEuint128` / `InEaddress` structs are removed**, along with the `FHE.asEbool(InEbool)`, `asEuint*(InEuint*)` and `asEaddress(InEaddress)` overloads and the `Utils.inputFromEbool` / `inputFromEuint*` / `inputFromEaddress` helpers that only existed to convert them. They were a third spelling of the same `(ctHash, securityZone, utype, signature)` tuple already covered by the `bytes` overload (`Utils.inputFromBytes`) and the `external*` handle + proof overload. Contracts taking an `InEuint64 memory` parameter must switch to `externalEuint64` + a `bytes` proof (preferred) or to the ABI-encoded `bytes` overload; note that `external*` overloads pin `securityZone` to `0`, so a non-zero zone needs the `bytes` form or `Utils.inputFromHashAndProof(hash, proof, utype, securityZone)`. `EncryptedInput` and `UnsignedEncryptedInput` are unaffected.
 - **BREAKING — input verification now binds the consuming contract.** `TaskManager.extractBatchSigner` folds the calling contract (`msg.sender`) into the verifier-signed message, so an encrypted input is only accepted by the contract it was signed for. Previously the signature covered only `(ctHash, utype, securityZone, sender, chainId)`, so a signed input observed on-chain could be replayed into any other contract, which then obtained an ACL allowance over the ciphertext. The signed message is now `keccak256(abi.encodePacked(ctHash, utype, securityZone, sender, chainId, contractAddress))`. Requires a lock-step verifier upgrade that appends the contract address to the signed message in the same byte order; inputs signed by an old verifier will no longer verify. Debug mode (`verifierSigner == address(0)`) is unaffected.
+- **Role-based access control** (BREAKING) — `TaskManager`, `ACL`, `PlaintextsStorage`, and `CommitmentRegistry` move from `Ownable`/`Ownable2Step` to `AccessControlDefaultAdminRules`. Each previously `onlyOwner` entry point is now bound to a capability role: TaskManager splits into `UPGRADER_ROLE`, `PAUSER_ROLE`, `SECURITY_ZONE_MANAGER_ROLE`, `ACCESS_LIST_MANAGER_ROLE`, `VERIFIER_SIGNER_MANAGER_ROLE`, `DECRYPT_SIGNER_MANAGER_ROLE`, and `CONFIG_MANAGER_ROLE`; ACL and PlaintextsStorage expose `UPGRADER_ROLE`; CommitmentRegistry exposes `UPGRADER_ROLE`, `POSTER_MANAGER_ROLE`, and `VERSION_MANAGER_ROLE`.
+
+  Blast radius: splitting `onlyOwner` limits who has to hold each key, not what each key can do. Four TaskManager roles are admin-equivalent and must sit on the same governance as `DEFAULT_ADMIN_ROLE`, never on an operational hot key: `UPGRADER_ROLE` (arbitrary implementation), `CONFIG_MANAGER_ROLE` (repoints `acl` for unrestricted ciphertext access, or `plaintextsStorage` for arbitrary plaintext — no upgrade needed), `VERIFIER_SIGNER_MANAGER_ROLE` (forges encrypted inputs) and `DECRYPT_SIGNER_MANAGER_ROLE` (forges decrypt results). Only `PAUSER_ROLE`, `SECURITY_ZONE_MANAGER_ROLE` and `ACCESS_LIST_MANAGER_ROLE` are genuinely narrow — their worst case is availability, not disclosure.
+
+  Deployment notes: `initialize` signatures changed — `TaskManager`/`ACL`/`PlaintextsStorage` take `(address initialAdmin, uint48 initialDelay)` and `CommitmentRegistry` takes `(address initialAdmin, uint48 initialDelay, address initialPoster)`. `initialize` grants only `DEFAULT_ADMIN_ROLE`, so deployments must explicitly grant the operational roles — including `UPGRADER_ROLE`, without which the proxy cannot be upgraded again. `owner()` is retained for ABI compatibility: `AccessControlDefaultAdminRules` implements ERC-5313, so `owner()` now returns `defaultAdmin()`. `transferOwnership`/`acceptOwnership` are replaced by `beginDefaultAdminTransfer`/`acceptDefaultAdminTransfer`. The host-chain deploy script requires `TM_ADMIN_ADDRESS` and `TM_ADMIN_DELAY` on any non-local network, and the registry-chain script requires `POSTER_ADDRESS`, rather than falling back to keys committed to this repository.
+
+  Migration: proxies already deployed on the `Ownable` implementation have no AccessControl storage. `initializeV2(address initialAdmin, uint48 initialDelay)` seeds it — argument order matches `initialize`, and it also grants the operational roles to `initialAdmin` (not just `DEFAULT_ADMIN_ROLE`), so a migration driven by a Safe or a manual `cast send` with no follow-up script cannot leave the proxy without an `UPGRADER_ROLE` holder and therefore permanently un-upgradeable; revoke afterwards to re-establish separation. It cannot be `onlyRole`-gated — it runs precisely when no role holder exists yet — so it is gated on the owner recorded in the abandoned `openzeppelin.storage.Ownable` namespace, which is the same account the old `_authorizeUpgrade` required. Passing it as the `data` argument of `upgradeToAndCall` is still recommended so the proxy is never observable half-migrated, but the safety no longer depends on it: a bare `upgradeTo` from a Safe or `cast send` leaves a window in which `reinitializer(2)` passes and `defaultAdmin()` is zero, and only the legacy owner can close it. `TaskManager`, `ACL`, `PlaintextsStorage` and `CommitmentRegistry` all retain the `openzeppelin.storage.Ownable` (and, where they inherited `Ownable2Step`, `Ownable2Step`) ERC-7201 namespaces as struct declarations, so the orphaned owner data stays reserved and cannot be reused by a later upgrade.
+- **Aggregator allowlist removed** — `addAggregator`, `removeAggregator`, `handleDecryptResult` and `handleError` are gone, along with `AGGREGATOR_MANAGER_ROLE` and the `onlyAggregator` modifier. `handleDecryptResult` wrote plaintext for any `ctHash` with no signature check at all, so an aggregator entry (or the role that could add one) was arbitrary-plaintext-for-any-handle. Decrypt results are now published only through the signature-checked `publishDecryptResult` / `publishDecryptResultBatch`, which need no allowlist. The mapping itself is kept as deprecated storage so its slot stays reserved, renamed to `_aggregators` — same slot, same type, so the layout is still upgrade-compatible, but the public getter is now `_aggregators(address)` instead of `aggregators(address)`. `storage-layout-snapshot.json` is re-baselined in this PR to record the rename (and, separately, to start tracking the new `AccessControl` / `AccessControlDefaultAdminRules` namespaces).
+- **`InputVerified` event** — `TaskManager.verifyInput` now emits `InputVerified(uint256 indexed ctHash, bytes32 commitment)`: the appended handle plus the raw verifier-signature-checked `ctHash` (`keccak256` of the ciphertext bytes) as the commitment; the security zone is bound by the handle's last byte, not the value. Off-chain services relay the commitment verbatim to the CommitmentRegistry so the TEE decryptor can verify user inputs before decrypting. Emitted in debug mode (`verifierSigner == address(0)`) too, so local stacks exercise the flow. Adds ~1.5k gas to `verifyInput` (event emission only).
 
 ### Fixed
+- **`initializeV2` seeds fail-closed signers on the bootstrap migration path** — the deterministic bootstrap stub's storage stops at slot 3, so TaskManager's `verifierSigner` (slot 4) and `decryptResultSigner` (slot 7) read storage it never wrote, i.e. zero — which is the verification-*disabled* sentinel, not a safe default. `initializeV2` now sets each to `address(1)` **only when it reads zero**, so a proxy migrating from the pre-roles TaskManager (where both hold real, live values) is left untouched rather than having its signers clobbered. `isEnabled`, `acl` and `plaintextsStorage` are deliberately not seeded — the first must not be flipped on a live proxy, the latter two have no safe default. The resulting end state is asserted in `test/roles/Roles.ts`, so a future layout shift fails CI rather than a testnet.
+- **`task:upgradeTM` no longer aborts on the bootstrap migration** — the storage-layout validation runs before the `onlyvalidate` check, so making it throw (correctly) also killed the `DeterministicTM` → `TaskManager` upgrade, which is knowingly layout-incompatible. Validation is now skipped for that one path with an explicit warning, and stays strict everywhere else. The current implementation is also detected by probing for `DeterministicTM.aggregator()` instead of inferring it from `defaultAdmin() == null` — the pre-roles Ownable TaskManager also has no `defaultAdmin()`, so the old heuristic force-imported the stub's layout and rejected the one migration that is actually safe.
+- **Deploy scripts reject blank admin delays** — `TM_ADMIN_DELAY=""` (and `REGISTRY_ADMIN_DELAY=""`) previously passed the `undefined` check and then became `0` via `Number("")`, handing a production deploy the zero delay the guard exists to prevent. Blank and whitespace-only now count as unset, and an explicit `0` is refused off local networks. `registry-chain` gained the same delay guard host-chain has, replacing its hardcoded `DEFAULT_ADMIN_DELAY = 0`.
+- **`grantAllRoles` fails loudly when it finds no roles** — discovering roles from the ABI meant a stale typechain build or the wrong factory granted nothing and returned success, leaving the proxy with no `UPGRADER_ROLE` holder and a clean deploy log. Both copies now throw. The test-side equivalent (`declaredRoleNames`) asserts non-empty too, so an ABI regression can no longer turn those loops into zero-assertion passes.
 - **`createRandomTask` now derives the handle with `TMCommon.calcPlaceholderKey`** instead of using the caller-supplied seed directly, so random matches every other task path. The preimage is `[seed, msg.sender]`; the sender is included so the same seed from different callers yields different handles, while a repeated `(seed, caller)` pair still yields the same handle. `TaskCreated.input3`, previously always `0` for random, now carries the caller. `seed == 0` is unchanged and still routes through `_generateSeed`, so `FHE.randomEuintNN()` behaves as before. No storage layout change. **Breaking:** the handle no longer equals the emitted seed, so fheos must be deployed in the same window and there is no partial rollback. **Deployment Requirement:** UUPS upgrade of TaskManager in every environment.
 - `createRandomTask` now respects the `isEnabled` kill-switch (`onlyIfEnabled`). Previously it emitted `TaskCreated` and granted ACL access even while the TaskManager was disabled, so the coprocessor still received random-generation intake during a halt. It now reverts with `CofheIsUnavailable` when disabled, consistent with `createTask` and decrypt-result publishing. Also corrected the `isEnabled` comment, which claimed all operations revert when disabled.
 - `Utils.inputFromHashAndProof` no longer hardcodes `securityZone: 0`. A new 4-argument overload accepts an explicit `securityZone`, bringing it in line with the other `inputFrom*` helpers. The original 3-argument signature is kept as a backward-compatible wrapper defaulting to zone `0`. Fixes `verifyInput` failures when building an `EncryptedInput` from a hash and proof for a ciphertext on a non-zero security zone.

contracts/internal/host-chain/contracts/ACL.sol

@@ -3,8 +3,9 @@ pragma solidity >=0.8.25 <0.9.0;
 
 import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
-import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
+import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol";
 import {taskManagerAddress} from "./addresses/TaskManagerAddress.sol";
+import {LegacyOwnable} from "./LegacyOwnable.sol";
 import {PermissionedUpgradeable, ACP, SCOPE_GLOBAL, SCOPE_CONTRACT, SCOPE_HANDLES} from "./Permissioned.sol";
 
 /**
@@ -14,7 +15,22 @@ import {PermissionedUpgradeable, ACP, SCOPE_GLOBAL, SCOPE_CONTRACT, SCOPE_HANDLE
  *         By defining and enforcing these permissions, the ACL ensures that encrypted data remains secure while still being usable
  *         within authorized contexts.
  */
-contract ACL is UUPSUpgradeable, Ownable2StepUpgradeable, PermissionedUpgradeable {
+contract ACL is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable, PermissionedUpgradeable {
+    bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE");
+
+    /// @dev Reserves the namespaces this contract used while it inherited Ownable2StepUpgradeable.
+    ///      Already-deployed proxies still hold an owner there; keeping the declarations marks
+    ///      that storage as taken so a later upgrade cannot silently reuse it.
+    /// @custom:storage-location erc7201:openzeppelin.storage.Ownable
+    struct OwnableStorage {
+        address _owner;
+    }
+
+    /// @custom:storage-location erc7201:openzeppelin.storage.Ownable2Step
+    struct Ownable2StepStorage {
+        address _pendingOwner;
+    }
+
     /// @notice Returned if the delegatee contract is already delegatee for sender & delegator addresses.
     error AlreadyDelegated();
 
@@ -100,13 +116,28 @@ contract ACL is UUPSUpgradeable, Ownable2StepUpgradeable, PermissionedUpgradeabl
 
     /**
      * @notice              Initializes the contract.
-     * @param initialOwner  Initial owner address.
+     * @param initialAdmin  Initial admin address.
+     * @param initialDelay  Initial delay for the default admin transfer.
      */
-    function initialize(address initialOwner) public initializer {
-        __Ownable_init(initialOwner);
+    function initialize(address initialAdmin, uint48 initialDelay) public initializer {
+        __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin);
         __PermissionedUpgradeable_init();
     }
 
+    /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable implementation.
+    ///      Callable only by the owner the pre-roles implementation left behind - see
+    ///      {LegacyOwnable} for why that is the only authority available in this window.
+    ///      Grants UPGRADER_ROLE too: there is no upgrade task for this proxy, so a hand-rolled
+    ///      migration with no follow-up grant would leave it permanently un-upgradeable.
+    /// @param initialAdmin  Address receiving DEFAULT_ADMIN_ROLE and UPGRADER_ROLE.
+    /// @param initialDelay  Delay enforced on subsequent default-admin transfers.
+    /// @custom:oz-upgrades-validate-as-initializer
+    function initializeV2(address initialAdmin, uint48 initialDelay) public reinitializer(2) {
+        LegacyOwnable.requireLegacyOwner(msg.sender);
+        __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin);
+        _grantRole(UPGRADER_ROLE, initialAdmin);
+    }
+
     /**
      * @notice              Allows the use of `handle` for the address `account`.
      * @dev                 The caller must be allowed to use `handle` for allow() to succeed. If not, allow() reverts.
@@ -482,10 +513,10 @@ contract ACL is UUPSUpgradeable, Ownable2StepUpgradeable, PermissionedUpgradeabl
 
     /**
      * @dev Should revert when `msg.sender` is not authorized to upgrade the contract.
-     *      Empty implementation since authorization is handled by onlyOwner modifier.
+     *      Empty implementation since authorization is handled by onlyRole(UPGRADER_ROLE) modifier.
      */
     /* solhint-disable-next-line no-empty-blocks */
-    function _authorizeUpgrade(address _newImplementation) internal virtual override onlyOwner {}
+    function _authorizeUpgrade(address _newImplementation) internal virtual override onlyRole(UPGRADER_ROLE) {}
 
     /**
      * @dev                         Returns the ACL storage location.
@@ -519,15 +550,15 @@ contract ACL is UUPSUpgradeable, Ownable2StepUpgradeable, PermissionedUpgradeabl
 
     /// @notice             Sets the default revoker contract address.
     /// @param newAddress   The new address (zero = unset).
-    function setDefaultRevokerContract(address newAddress) external virtual onlyOwner {
+    function setDefaultRevokerContract(address newAddress) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
         ACLStorage storage $ = _getACLStorage();
         emit DefaultRevokerContractUpdated($.defaultRevokerContract, newAddress);
         $.defaultRevokerContract = newAddress;
     }
 
     /// @notice             Sets the share registry address.
     /// @param newAddress   The new address (zero = unset).
-    function setShareRegistry(address newAddress) external virtual onlyOwner {
+    function setShareRegistry(address newAddress) external virtual onlyRole(DEFAULT_ADMIN_ROLE) {
         ACLStorage storage $ = _getACLStorage();
         emit ShareRegistryUpdated($.shareRegistry, newAddress);
         $.shareRegistry = newAddress;

contracts/internal/host-chain/contracts/LegacyOwnable.sol

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+pragma solidity >=0.8.25 <0.9.0;
+
+/// @notice Returned when a migration re-initializer is called by anyone other than the owner
+///         recorded in the abandoned Ownable storage namespace.
+/// @param caller       The caller.
+/// @param legacyOwner  The owner the pre-roles implementation left behind.
+error NotLegacyOwner(address caller, address legacyOwner);
+
+/**
+ * @title  LegacyOwnable
+ * @notice Reads the owner left behind in the `openzeppelin.storage.Ownable` ERC-7201 namespace by
+ *         a pre-roles `Ownable*Upgradeable` implementation.
+ * @dev    `initializeV2` cannot be gated with `onlyRole` - it exists precisely because the
+ *         AccessControl namespace is still empty, so there is no role holder to check against.
+ *         The inherited `AccessControlDefaultAdminRules._grantRole` guard only reverts once
+ *         `defaultAdmin() != address(0)`, which is exactly the state `initializeV2` has yet to
+ *         create. The legacy owner is the only authority that exists in that window, and it is
+ *         the same account the old `_authorizeUpgrade` (`onlyOwner`) required, so gating on it
+ *         adds no new key to the upgrade procedure.
+ *
+ *         A sibling copy of this logic lives in
+ *         `registry-chain/contracts/commitment-registry/LegacyOwnable.sol` - the two projects have
+ *         no shared package. Keep them identical.
+ */
+library LegacyOwnable {
+    /// @dev keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Ownable")) - 1)) & ~bytes32(uint256(0xff))
+    ///      Matches `OwnableUpgradeable.OwnableStorageLocation` in OpenZeppelin 5.2.0.
+    bytes32 internal constant OWNABLE_STORAGE_SLOT =
+        0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300;
+
+    /// @dev Returns the owner recorded by the pre-roles implementation, or address(0) when the
+    ///      proxy never ran one (a fresh deployment through `initialize`).
+    function owner() internal view returns (address legacyOwner) {
+        bytes32 slot = OWNABLE_STORAGE_SLOT;
+        // slither-disable-next-line assembly
+        assembly {
+            legacyOwner := sload(slot)
+        }
+    }
+
+    /// @dev Reverts unless `caller` is the legacy owner. A zero legacy owner can never be matched,
+    ///      so freshly deployed proxies reject the migration path outright.
+    function requireLegacyOwner(address caller) internal view {
+        address legacyOwner = owner();
+        if (caller != legacyOwner || legacyOwner == address(0)) {
+            revert NotLegacyOwner(caller, legacyOwner);
+        }
+    }
+}