From fb0e770bf882580d00dd98ee3ca74aacca3bcb04 Mon Sep 17 00:00:00 2001 From: liorbond Date: Thu, 16 Jul 2026 14:12:37 +0300 Subject: [PATCH 01/11] [FEAT] PlaintextsStorage: role-based access control Co-Authored-By: Claude Opus 4.8 (1M context) --- .../contracts/PlaintextsStorage.sol | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol index cb9c1f9..9ec8185 100644 --- a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol +++ b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol @@ -2,9 +2,11 @@ pragma solidity >=0.8.25 <0.9.0; import {taskManagerAddress} from "./addresses/TaskManagerAddress.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol"; + +contract PlaintextsStorage is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable { + bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); -contract PlaintextsStorage is UUPSUpgradeable, OwnableUpgradeable { struct PlaintextResult { bool existenceIndicator; uint256 result; @@ -40,10 +42,18 @@ contract PlaintextsStorage is UUPSUpgradeable, OwnableUpgradeable { _disableInitializers(); } - function initialize(address initialOwner) public initializer { - __Ownable_init(initialOwner); + function initialize(address initialAdmin, uint48 initialDelay) public initializer { + __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); __UUPSUpgradeable_init(); } - function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable + /// implementation. Do not call on a freshly `initialize`d proxy: it would + /// grant a second DEFAULT_ADMIN_ROLE holder, breaking the single-admin invariant. + /// @custom:oz-upgrades-validate-as-initializer + function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + } + + function _authorizeUpgrade(address newImplementation) internal override onlyRole(UPGRADER_ROLE) {} } \ No newline at end of file From 70d9b506a7baf3268f7c96c987ac52c1abbe5264 Mon Sep 17 00:00:00 2001 From: liorbond Date: Thu, 16 Jul 2026 16:20:59 +0300 Subject: [PATCH 02/11] [FEAT] ACL: role-based access control Co-Authored-By: Claude Opus 4.8 (1M context) --- .../internal/host-chain/contracts/ACL.sol | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/contracts/internal/host-chain/contracts/ACL.sol b/contracts/internal/host-chain/contracts/ACL.sol index c6b0d52..2bd82b8 100644 --- a/contracts/internal/host-chain/contracts/ACL.sol +++ b/contracts/internal/host-chain/contracts/ACL.sol @@ -3,7 +3,7 @@ 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 {PermissionedUpgradeable, Permission} from "./Permissioned.sol"; @@ -14,7 +14,9 @@ import {PermissionedUpgradeable, Permission} from "./Permissioned.sol"; * 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"); + /// @notice Returned if the delegatee contract is already delegatee for sender & delegator addresses. error AlreadyDelegated(); @@ -72,13 +74,22 @@ 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. Do not call on a freshly `initialize`d proxy: it would + /// grant a second DEFAULT_ADMIN_ROLE holder, breaking the single-admin invariant. + /// @custom:oz-upgrades-validate-as-initializer + function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + __AccessControlDefaultAdminRules_init(initialDelay, 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. @@ -328,7 +339,7 @@ contract ACL is UUPSUpgradeable, Ownable2StepUpgradeable, PermissionedUpgradeabl * Empty implementation since authorization is handled by onlyOwner 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. From 9642f37d8117373352d64ea2928672664a55afb2 Mon Sep 17 00:00:00 2001 From: liorbond Date: Mon, 20 Jul 2026 11:26:28 +0300 Subject: [PATCH 03/11] [FIX] initializeV2 NatSpec: reflect fail-closed default-admin behavior Co-Authored-By: Claude Opus 4.8 (1M context) --- contracts/internal/host-chain/contracts/ACL.sol | 6 +++--- .../internal/host-chain/contracts/PlaintextsStorage.sol | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/internal/host-chain/contracts/ACL.sol b/contracts/internal/host-chain/contracts/ACL.sol index 2bd82b8..6f3d0fb 100644 --- a/contracts/internal/host-chain/contracts/ACL.sol +++ b/contracts/internal/host-chain/contracts/ACL.sol @@ -83,8 +83,8 @@ contract ACL is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable, Perm } /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable - /// implementation. Do not call on a freshly `initialize`d proxy: it would - /// grant a second DEFAULT_ADMIN_ROLE holder, breaking the single-admin invariant. + /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the + /// proxy already has a default admin, so it is safe against accidental reuse. /// @custom:oz-upgrades-validate-as-initializer function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); @@ -336,7 +336,7 @@ contract ACL is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable, Perm /** * @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 onlyRole(UPGRADER_ROLE) {} diff --git a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol index 9ec8185..0ff5354 100644 --- a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol +++ b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol @@ -48,8 +48,8 @@ contract PlaintextsStorage is UUPSUpgradeable, AccessControlDefaultAdminRulesUpg } /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable - /// implementation. Do not call on a freshly `initialize`d proxy: it would - /// grant a second DEFAULT_ADMIN_ROLE holder, breaking the single-admin invariant. + /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the + /// proxy already has a default admin, so it is safe against accidental reuse. /// @custom:oz-upgrades-validate-as-initializer function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); From 1345232477bf5196d774325d2c8f9b6946bb4e76 Mon Sep 17 00:00:00 2001 From: liorbond Date: Mon, 20 Jul 2026 11:41:46 +0300 Subject: [PATCH 04/11] [FEAT] TaskManager: granular role-based access control Co-Authored-By: Claude Opus 4.8 (1M context) --- .../host-chain/contracts/TaskManager.sol | 62 ++++++++++++------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/contracts/internal/host-chain/contracts/TaskManager.sol b/contracts/internal/host-chain/contracts/TaskManager.sol index f48449e..d24341e 100644 --- a/contracts/internal/host-chain/contracts/TaskManager.sol +++ b/contracts/internal/host-chain/contracts/TaskManager.sol @@ -5,7 +5,7 @@ import {ACL, Permission} from "./ACL.sol"; import {PlaintextsStorage} from "./PlaintextsStorage.sol"; 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 {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {ITaskManager, FunctionId, Utils, EncryptedInput} from "@fhenixprotocol/cofhe-contracts/ICofhe.sol"; @@ -152,7 +152,16 @@ library TMCommon { } } -contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2StepUpgradeable { +contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable { + bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); + bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); + bytes32 public constant SECURITY_ZONE_MANAGER_ROLE = keccak256("SECURITY_ZONE_MANAGER_ROLE"); + bytes32 public constant AGGREGATOR_MANAGER_ROLE = keccak256("AGGREGATOR_MANAGER_ROLE"); + bytes32 public constant ACCESS_LIST_MANAGER_ROLE = keccak256("ACCESS_LIST_MANAGER_ROLE"); + bytes32 public constant VERIFIER_SIGNER_MANAGER_ROLE = keccak256("VERIFIER_SIGNER_MANAGER_ROLE"); + bytes32 public constant DECRYPT_SIGNER_MANAGER_ROLE = keccak256("DECRYPT_SIGNER_MANAGER_ROLE"); + bytes32 public constant CONFIG_MANAGER_ROLE = keccak256("CONFIG_MANAGER_ROLE"); + bool private initialized; /// @custom:oz-upgrades-unsafe-allow constructor @@ -162,11 +171,12 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St /** * @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); + address initialAdmin, uint48 initialDelay) public initializer { + __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); __UUPSUpgradeable_init(); initialized = true; verifierSigner = address(1); @@ -174,7 +184,15 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St isEnabled = true; } - function setSecurityZones(int32 minSZ, int32 maxSZ) external onlyOwner { + /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable + /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the + /// proxy already has a default admin, so it is safe against accidental reuse. + /// @custom:oz-upgrades-validate-as-initializer + function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + } + + function setSecurityZones(int32 minSZ, int32 maxSZ) external onlyRole(SECURITY_ZONE_MANAGER_ROLE) { securityZoneMin = minSZ; securityZoneMax = maxSZ; } @@ -187,13 +205,13 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St return version; } - function incVersion() public onlyOwner { + function incVersion() public onlyRole(CONFIG_MANAGER_ROLE) { version++; } function _authorizeUpgrade( address newImplementation - ) internal override onlyOwner {} + ) internal override onlyRole(UPGRADER_ROLE) {} // Errors // Returned when the handle is not allowed in the ACL for the account. @@ -272,25 +290,25 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St _; } - function enable() external onlyOwner { + function enable() external onlyRole(PAUSER_ROLE) { isEnabled = true; } - function disable() external onlyOwner { + function disable() external onlyRole(PAUSER_ROLE) { isEnabled = false; } - function enableAccessList() external onlyOwner { + function enableAccessList() external onlyRole(ACCESS_LIST_MANAGER_ROLE) { accessListEnabled = true; emit AccessListEnabledSet(true); } - function disableAccessList() external onlyOwner { + function disableAccessList() external onlyRole(ACCESS_LIST_MANAGER_ROLE) { accessListEnabled = false; emit AccessListEnabledSet(false); } - function addToAccessList(address[] calldata accounts) external onlyOwner { + function addToAccessList(address[] calldata accounts) external onlyRole(ACCESS_LIST_MANAGER_ROLE) { for (uint256 i = 0; i < accounts.length; i++) { if (accounts[i] == address(0)) { revert InvalidAddress(); @@ -300,7 +318,7 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St } } - function removeFromAccessList(address[] calldata accounts) external onlyOwner { + function removeFromAccessList(address[] calldata accounts) external onlyRole(ACCESS_LIST_MANAGER_ROLE) { for (uint256 i = 0; i < accounts.length; i++) { if (accounts[i] == address(0)) { revert InvalidAddress(); @@ -829,7 +847,7 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St return signer; } - function setVerifierSigner(address signer) external onlyOwner { + function setVerifierSigner(address signer) external onlyRole(VERIFIER_SIGNER_MANAGER_ROLE) { address oldSigner = verifierSigner; verifierSigner = signer; emit VerifierSignerChanged(oldSigner, signer); @@ -837,41 +855,41 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St /// @notice Set the authorized signer for decrypt results /// @param signer The new signer address (address(0) disables verification) - function setDecryptResultSigner(address signer) external onlyOwner { + function setDecryptResultSigner(address signer) external onlyRole(DECRYPT_SIGNER_MANAGER_ROLE) { address oldSigner = decryptResultSigner; decryptResultSigner = signer; emit DecryptResultSignerChanged(oldSigner, signer); } - function setSecurityZoneMax(int32 securityZone) external onlyOwner { + function setSecurityZoneMax(int32 securityZone) external onlyRole(SECURITY_ZONE_MANAGER_ROLE) { if (securityZone < securityZoneMin) { revert InvalidSecurityZone(securityZone, securityZoneMin, securityZoneMax); } securityZoneMax = securityZone; } - function setSecurityZoneMin(int32 securityZone) external onlyOwner { + function setSecurityZoneMin(int32 securityZone) external onlyRole(SECURITY_ZONE_MANAGER_ROLE) { if (securityZone > securityZoneMax) { revert InvalidSecurityZone(securityZone, securityZoneMin, securityZoneMax); } securityZoneMin = securityZone; } - function setACLContract(address _aclAddress) external onlyOwner { + function setACLContract(address _aclAddress) external onlyRole(CONFIG_MANAGER_ROLE) { if (_aclAddress == address(0)) { revert InvalidAddress(); } acl = ACL(_aclAddress); } - function setPlaintextsStorage(address _plaintextsStorageAddress) external onlyOwner { + function setPlaintextsStorage(address _plaintextsStorageAddress) external onlyRole(CONFIG_MANAGER_ROLE) { if (_plaintextsStorageAddress == address(0)) { revert InvalidAddress(); } plaintextsStorage = PlaintextsStorage(_plaintextsStorageAddress); } - function addAggregator(address _aggregatorAddress) external onlyOwner { + function addAggregator(address _aggregatorAddress) external onlyRole(AGGREGATOR_MANAGER_ROLE) { if (_aggregatorAddress == address(0)) { revert InvalidAddress(); } @@ -879,7 +897,7 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, Ownable2St aggregators[_aggregatorAddress] = true; } - function removeAggregator(address _aggregatorAddress) external onlyOwner { + function removeAggregator(address _aggregatorAddress) external onlyRole(AGGREGATOR_MANAGER_ROLE) { if (_aggregatorAddress == address(0)) { revert InvalidAddress(); } From f89e6a56626db6da060e61510961988ef5efe4f3 Mon Sep 17 00:00:00 2001 From: liorbond Date: Sat, 25 Jul 2026 17:29:16 +0300 Subject: [PATCH 05/11] [FEAT] host-chain: wire tests and deploy to role-based access Co-Authored-By: Claude Opus 4.8 (1M context) --- .../internal/host-chain/deploy/deploy.ts | 25 +++++- .../internal/host-chain/tasks/upgradeTM.ts | 2 +- .../host-chain/test/accessList/AccessList.ts | 15 +++- .../decryptResult/DecryptResult.behavior.ts | 4 +- .../decryptResult/DecryptResult.fixture.ts | 80 +++++++++---------- .../test/onChain/OnChain.fixture.ts | 74 +++++++++-------- .../test/publiclyAllowed/PubliclyAllowed.ts | 68 +++++++++------- 7 files changed, 155 insertions(+), 113 deletions(-) diff --git a/contracts/internal/host-chain/deploy/deploy.ts b/contracts/internal/host-chain/deploy/deploy.ts index 39ec4d4..4ae5c6f 100644 --- a/contracts/internal/host-chain/deploy/deploy.ts +++ b/contracts/internal/host-chain/deploy/deploy.ts @@ -23,7 +23,7 @@ async function getProxyContract(adminAddress: string, contractName: string) { const TaskManager = await ethers.getContractFactory(contractName); const ProxyContract = await upgrades.deployProxy( TaskManager, - [adminAddress], + [adminAddress, 0], { kind: "uups", initializer: "initialize" }, ); const deployedImpl = await ProxyContract.waitForDeployment(); @@ -52,7 +52,7 @@ async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) TMProxyContract, ); const isInitialized = await TMProxyContract.isInitialized(); - const owner = await TMProxyContract.owner(); + const owner = await TMProxyContract.defaultAdmin(); console.log( "Implementation address:", currentImplementation, @@ -66,6 +66,25 @@ async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) return e; } + // Grant the setup signer the operational roles it needs before using them. + try { + const admin = aggregatorSigners[0]; + const tm = TMProxyContract.connect(admin); + for (const role of [ + await TMProxyContract.AGGREGATOR_MANAGER_ROLE(), + await TMProxyContract.PAUSER_ROLE(), + await TMProxyContract.SECURITY_ZONE_MANAGER_ROLE(), + await TMProxyContract.VERIFIER_SIGNER_MANAGER_ROLE(), + await TMProxyContract.DECRYPT_SIGNER_MANAGER_ROLE(), + await TMProxyContract.CONFIG_MANAGER_ROLE(), + ]) { + await (await tm.grantRole(role, admin.address)).wait(); + } + } catch (e) { + console.error(chalk.red(`Failed granting setup roles: ${e}`)); + return e; + } + // Set the aggregator address try { const connectedImplementation = TMProxyContract.connect(aggregatorSigners[0]); @@ -285,7 +304,7 @@ async function getImplementationAddress(proxy: any) { async function upgradeTM(TMProxyContract: any, TMFactory: any, aggregatorSigner: any) { console.log(chalk.bold.blue("-----------------------Upgrading TaskManager--------------------------")); console.log(chalk.green("Aggregator signer:", aggregatorSigner.address)); - console.log(chalk.green("owner:", await TMProxyContract.owner())); + console.log(chalk.green("owner:", await TMProxyContract.defaultAdmin())); const connectedImplementation = TMProxyContract.connect(aggregatorSigner); const oldImplementationAddress = await getImplementationAddress(connectedImplementation); console.log(chalk.green("Old implementation address:", oldImplementationAddress)); diff --git a/contracts/internal/host-chain/tasks/upgradeTM.ts b/contracts/internal/host-chain/tasks/upgradeTM.ts index 76c935c..4c2faf9 100644 --- a/contracts/internal/host-chain/tasks/upgradeTM.ts +++ b/contracts/internal/host-chain/tasks/upgradeTM.ts @@ -46,7 +46,7 @@ async function validateUpgrade(upgrades: any, TMProxyContract: any, TMFactory: a async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFactory: any, adminSigner: any) { const connectedImplementation = TMProxyContract.connect(adminSigner); - console.log(chalk.green("TMProxyContract owner:", await TMProxyContract.owner())); + console.log(chalk.green("TMProxyContract owner:", await TMProxyContract.defaultAdmin())); const oldImplementationAddress = await getImplementationAddress(ethers, connectedImplementation); console.log(chalk.green("Old implementation address:", oldImplementationAddress)); diff --git a/contracts/internal/host-chain/test/accessList/AccessList.ts b/contracts/internal/host-chain/test/accessList/AccessList.ts index 376062c..ce26fe3 100644 --- a/contracts/internal/host-chain/test/accessList/AccessList.ts +++ b/contracts/internal/host-chain/test/accessList/AccessList.ts @@ -74,13 +74,20 @@ describe("TaskManager access list", function () { it("restricts every admin function to the owner", async function () { await expect(taskManager.connect(other).enableAccessList()) - .to.be.revertedWithCustomError(taskManager, "OwnableUnauthorizedAccount"); + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount"); await expect(taskManager.connect(other).disableAccessList()) - .to.be.revertedWithCustomError(taskManager, "OwnableUnauthorizedAccount"); + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount"); await expect(taskManager.connect(other).addToAccessList([other.address])) - .to.be.revertedWithCustomError(taskManager, "OwnableUnauthorizedAccount"); + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount"); await expect(taskManager.connect(other).removeFromAccessList([other.address])) - .to.be.revertedWithCustomError(taskManager, "OwnableUnauthorizedAccount"); + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount"); + }); + + it("names the ACCESS_LIST_MANAGER_ROLE in the revert for non-holders", async function () { + const role = await taskManager.ACCESS_LIST_MANAGER_ROLE(); + await expect(taskManager.connect(other).enableAccessList()) + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount") + .withArgs(other.address, role); }); it("rejects the zero address when adding or removing", async function () { diff --git a/contracts/internal/host-chain/test/decryptResult/DecryptResult.behavior.ts b/contracts/internal/host-chain/test/decryptResult/DecryptResult.behavior.ts index 455c0c0..0dda2fd 100644 --- a/contracts/internal/host-chain/test/decryptResult/DecryptResult.behavior.ts +++ b/contracts/internal/host-chain/test/decryptResult/DecryptResult.behavior.ts @@ -708,7 +708,7 @@ export function shouldBehaveLikeDecryptResult(): void { await expect( taskManager.connect(otherAccount).setDecryptResultSigner(newSigner) - ).to.be.revertedWithCustomError(taskManager, "OwnableUnauthorizedAccount"); + ).to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount"); }); }); @@ -736,7 +736,7 @@ export function shouldBehaveLikeDecryptResult(): void { await expect( taskManager.connect(otherAccount).setVerifierSigner(newSigner) - ).to.be.revertedWithCustomError(taskManager, "OwnableUnauthorizedAccount"); + ).to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount"); }); }); diff --git a/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts b/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts index b222ec6..251a417 100644 --- a/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts +++ b/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts @@ -24,58 +24,45 @@ function getTestSignerWallet(): Wallet { } /** - * Deploy a proxy at a specific address using hardhat_setCode + * Install a UUPS proxy's runtime bytecode at a fixed address and initialize it + * in place. We can't `deployProxy` at an arbitrary address, and AccessControl + * stores role membership in computed mapping slots (not one fixed slot), so we + * initialize through the real proxy rather than copying storage slots. */ async function deployProxyAtAddress( targetAddress: string, implementationAddress: string, initData: string ): Promise { - // Get the proxy bytecode by deploying one temporarily const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); - const tempProxy = await ERC1967Proxy.deploy(implementationAddress, initData); + // Deploy a throwaway proxy only to capture the proxy runtime bytecode. + const tempProxy = await ERC1967Proxy.deploy(implementationAddress, "0x"); await tempProxy.waitForDeployment(); - - // Get the runtime bytecode from the deployed proxy const proxyBytecode = await ethers.provider.getCode(await tempProxy.getAddress()); - // Set the bytecode at our target address + // Install proxy code at the fixed address. await ethers.provider.send("hardhat_setCode", [targetAddress, proxyBytecode]); - // Storage slots to copy (ERC1967 + OZ v5 namespaced storage) - const storageSlots = [ - // ERC1967 implementation slot - "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc", - // OZ Initializable slot: keccak256("openzeppelin.storage.Initializable") - 1 - "0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00", - // OZ Ownable slot: keccak256("openzeppelin.storage.Ownable") - 1 - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300", - // OZ Ownable2Step pending owner slot (next slot after owner) - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199301", - // TaskManager storage slots (slot 0-10 for custom state variables) - "0x0000000000000000000000000000000000000000000000000000000000000000", - "0x0000000000000000000000000000000000000000000000000000000000000001", - "0x0000000000000000000000000000000000000000000000000000000000000002", - "0x0000000000000000000000000000000000000000000000000000000000000003", - "0x0000000000000000000000000000000000000000000000000000000000000004", - "0x0000000000000000000000000000000000000000000000000000000000000005", - "0x0000000000000000000000000000000000000000000000000000000000000006", - "0x0000000000000000000000000000000000000000000000000000000000000007", - "0x0000000000000000000000000000000000000000000000000000000000000008", - "0x0000000000000000000000000000000000000000000000000000000000000009", - "0x000000000000000000000000000000000000000000000000000000000000000a", - ]; - - const tempAddress = await tempProxy.getAddress(); - for (const slot of storageSlots) { - const value = await ethers.provider.getStorage(tempAddress, slot); - if (value !== "0x0000000000000000000000000000000000000000000000000000000000000000") { - await ethers.provider.send("hardhat_setStorageAt", [targetAddress, slot, value]); - } - } + // Point the ERC1967 implementation slot at our implementation. + const IMPL_SLOT = "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc"; + await ethers.provider.send("hardhat_setStorageAt", [ + targetAddress, + IMPL_SLOT, + ethers.zeroPadValue(implementationAddress, 32), + ]); + + // Initialize the proxy in place (fresh storage at the target address). + const [signer] = await ethers.getSigners(); + const tx = await signer.sendTransaction({ to: targetAddress, data: initData }); + await tx.wait(); } export async function deployDecryptResultFixture(): Promise { + // Multiple test files deploy TaskManager at the same hardcoded address within + // the same Hardhat network process; reset so `initialize` sees fresh storage + // (hardhat_setCode/hardhat_setStorageAt below only work on the Hardhat network). + await ethers.provider.send("hardhat_reset", []); + const [owner, otherAccount] = await ethers.getSigners(); // Deploy TaskManager implementation @@ -84,7 +71,7 @@ export async function deployDecryptResultFixture(): Promise { const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); - const tempProxy = await ERC1967Proxy.deploy(implementationAddress, initData); + // Deploy a throwaway proxy only to capture the proxy runtime bytecode. + const tempProxy = await ERC1967Proxy.deploy(implementationAddress, "0x"); await tempProxy.waitForDeployment(); - const proxyBytecode = await ethers.provider.getCode(await tempProxy.getAddress()); + + // Install proxy code at the fixed address. await ethers.provider.send("hardhat_setCode", [targetAddress, proxyBytecode]); - // Storage slots to copy (ERC1967 + OZ v5 namespaced storage) - const storageSlots = [ - "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc", // ERC1967 implementation - "0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00", // OZ Initializable - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300", // OZ Ownable - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199301", // OZ Ownable2Step pending - // TaskManager storage slots - "0x0000000000000000000000000000000000000000000000000000000000000000", - "0x0000000000000000000000000000000000000000000000000000000000000001", - "0x0000000000000000000000000000000000000000000000000000000000000002", - "0x0000000000000000000000000000000000000000000000000000000000000003", - "0x0000000000000000000000000000000000000000000000000000000000000004", - "0x0000000000000000000000000000000000000000000000000000000000000005", - "0x0000000000000000000000000000000000000000000000000000000000000006", - "0x0000000000000000000000000000000000000000000000000000000000000007", - "0x0000000000000000000000000000000000000000000000000000000000000008", - "0x0000000000000000000000000000000000000000000000000000000000000009", - "0x000000000000000000000000000000000000000000000000000000000000000a", - ]; - - const tempAddress = await tempProxy.getAddress(); - for (const slot of storageSlots) { - const value = await ethers.provider.getStorage(tempAddress, slot); - if (value !== "0x0000000000000000000000000000000000000000000000000000000000000000") { - await ethers.provider.send("hardhat_setStorageAt", [targetAddress, slot, value]); - } - } + // Point the ERC1967 implementation slot at our implementation. + const IMPL_SLOT = "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc"; + await ethers.provider.send("hardhat_setStorageAt", [ + targetAddress, + IMPL_SLOT, + ethers.zeroPadValue(implementationAddress, 32), + ]); + + // Initialize the proxy in place (fresh storage at the target address). + const [signer] = await ethers.getSigners(); + const tx = await signer.sendTransaction({ to: targetAddress, data: initData }); + await tx.wait(); } export async function deployOnChainFixture(): Promise<{ @@ -55,6 +45,11 @@ export async function deployOnChainFixture(): Promise<{ address: string; address2: string; }> { + // Multiple test files deploy TaskManager at the same hardcoded address within + // the same Hardhat network process; reset so `initialize` sees fresh storage + // (hardhat_setCode/hardhat_setStorageAt below only work on the Hardhat network). + await ethers.provider.send("hardhat_reset", []); + const [owner] = await ethers.getSigners(); // Deploy TaskManager implementation @@ -63,7 +58,7 @@ export async function deployOnChainFixture(): Promise<{ await taskManagerImpl.waitForDeployment(); // Prepare init data and deploy at hardcoded address - const initData = TaskManager.interface.encodeFunctionData("initialize", [owner.address]); + const initData = TaskManager.interface.encodeFunctionData("initialize", [owner.address, 0]); await deployProxyAtAddress(TASK_MANAGER_ADDRESS, await taskManagerImpl.getAddress(), initData); // Get TaskManager at the hardcoded address @@ -75,7 +70,7 @@ export async function deployOnChainFixture(): Promise<{ await aclImpl.waitForDeployment(); const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); - const aclInitData = ACL.interface.encodeFunctionData("initialize", [owner.address]); + const aclInitData = ACL.interface.encodeFunctionData("initialize", [owner.address, 0]); const aclProxy = await ERC1967Proxy.deploy(await aclImpl.getAddress(), aclInitData); await aclProxy.waitForDeployment(); @@ -84,10 +79,23 @@ export async function deployOnChainFixture(): Promise<{ const psImpl = await PlaintextsStorage.deploy(); await psImpl.waitForDeployment(); - const psInitData = PlaintextsStorage.interface.encodeFunctionData("initialize", [owner.address]); + const psInitData = PlaintextsStorage.interface.encodeFunctionData("initialize", [owner.address, 0]); const psProxy = await ERC1967Proxy.deploy(await psImpl.getAddress(), psInitData); await psProxy.waitForDeployment(); + // Owner holds DEFAULT_ADMIN_ROLE from init; grant the operational roles it exercises. + for (const role of [ + await taskManager.CONFIG_MANAGER_ROLE(), + await taskManager.SECURITY_ZONE_MANAGER_ROLE(), + await taskManager.PAUSER_ROLE(), + await taskManager.ACCESS_LIST_MANAGER_ROLE(), + await taskManager.AGGREGATOR_MANAGER_ROLE(), + await taskManager.VERIFIER_SIGNER_MANAGER_ROLE(), + await taskManager.DECRYPT_SIGNER_MANAGER_ROLE(), + ]) { + await taskManager.grantRole(role, owner.address); + } + // Configure TaskManager await taskManager.setACLContract(await aclProxy.getAddress()); await taskManager.setPlaintextsStorage(await psProxy.getAddress()); diff --git a/contracts/internal/host-chain/test/publiclyAllowed/PubliclyAllowed.ts b/contracts/internal/host-chain/test/publiclyAllowed/PubliclyAllowed.ts index a21fb2f..67782f3 100644 --- a/contracts/internal/host-chain/test/publiclyAllowed/PubliclyAllowed.ts +++ b/contracts/internal/host-chain/test/publiclyAllowed/PubliclyAllowed.ts @@ -5,43 +5,38 @@ const { ethers } = hre; const TASK_MANAGER_ADDRESS = "0xeA30c4B8b44078Bbf8a6ef5b9f1eC1626C7848D9"; +/** + * Install a UUPS proxy's runtime bytecode at a fixed address and initialize it + * in place. We can't `deployProxy` at an arbitrary address, and AccessControl + * stores role membership in computed mapping slots (not one fixed slot), so we + * initialize through the real proxy rather than copying storage slots. + */ async function deployProxyAtAddress( targetAddress: string, implementationAddress: string, initData: string ): Promise { const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); - const tempProxy = await ERC1967Proxy.deploy(implementationAddress, initData); + // Deploy a throwaway proxy only to capture the proxy runtime bytecode. + const tempProxy = await ERC1967Proxy.deploy(implementationAddress, "0x"); await tempProxy.waitForDeployment(); - const proxyBytecode = await ethers.provider.getCode(await tempProxy.getAddress()); + + // Install proxy code at the fixed address. await ethers.provider.send("hardhat_setCode", [targetAddress, proxyBytecode]); - const storageSlots = [ - "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc", - "0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00", - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300", - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199301", - "0x0000000000000000000000000000000000000000000000000000000000000000", - "0x0000000000000000000000000000000000000000000000000000000000000001", - "0x0000000000000000000000000000000000000000000000000000000000000002", - "0x0000000000000000000000000000000000000000000000000000000000000003", - "0x0000000000000000000000000000000000000000000000000000000000000004", - "0x0000000000000000000000000000000000000000000000000000000000000005", - "0x0000000000000000000000000000000000000000000000000000000000000006", - "0x0000000000000000000000000000000000000000000000000000000000000007", - "0x0000000000000000000000000000000000000000000000000000000000000008", - "0x0000000000000000000000000000000000000000000000000000000000000009", - "0x000000000000000000000000000000000000000000000000000000000000000a", - ]; - - const tempAddress = await tempProxy.getAddress(); - for (const slot of storageSlots) { - const value = await ethers.provider.getStorage(tempAddress, slot); - if (value !== "0x0000000000000000000000000000000000000000000000000000000000000000") { - await ethers.provider.send("hardhat_setStorageAt", [targetAddress, slot, value]); - } - } + // Point the ERC1967 implementation slot at our implementation. + const IMPL_SLOT = "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc"; + await ethers.provider.send("hardhat_setStorageAt", [ + targetAddress, + IMPL_SLOT, + ethers.zeroPadValue(implementationAddress, 32), + ]); + + // Initialize the proxy in place (fresh storage at the target address). + const [signer] = await ethers.getSigners(); + const tx = await signer.sendTransaction({ to: targetAddress, data: initData }); + await tx.wait(); } describe("PubliclyAllowed Tests", function () { @@ -49,13 +44,18 @@ describe("PubliclyAllowed Tests", function () { let testContract: any; before(async function () { + // Multiple test files deploy TaskManager at the same hardcoded address within + // the same Hardhat network process; reset so `initialize` sees fresh storage + // (hardhat_setCode/hardhat_setStorageAt below only work on the Hardhat network). + await ethers.provider.send("hardhat_reset", []); + const [owner] = await ethers.getSigners(); const TaskManager = await ethers.getContractFactory("TaskManager"); const taskManagerImpl = await TaskManager.deploy(); await taskManagerImpl.waitForDeployment(); - const initData = TaskManager.interface.encodeFunctionData("initialize", [owner.address]); + const initData = TaskManager.interface.encodeFunctionData("initialize", [owner.address, 0]); await deployProxyAtAddress(TASK_MANAGER_ADDRESS, await taskManagerImpl.getAddress(), initData); taskManager = TaskManager.attach(TASK_MANAGER_ADDRESS); @@ -64,17 +64,25 @@ describe("PubliclyAllowed Tests", function () { await aclImpl.waitForDeployment(); const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); - const aclInitData = ACL.interface.encodeFunctionData("initialize", [owner.address]); + const aclInitData = ACL.interface.encodeFunctionData("initialize", [owner.address, 0]); const aclProxy = await ERC1967Proxy.deploy(await aclImpl.getAddress(), aclInitData); await aclProxy.waitForDeployment(); const PlaintextsStorage = await ethers.getContractFactory("PlaintextsStorage"); const psImpl = await PlaintextsStorage.deploy(); await psImpl.waitForDeployment(); - const psInitData = PlaintextsStorage.interface.encodeFunctionData("initialize", [owner.address]); + const psInitData = PlaintextsStorage.interface.encodeFunctionData("initialize", [owner.address, 0]); const psProxy = await ERC1967Proxy.deploy(await psImpl.getAddress(), psInitData); await psProxy.waitForDeployment(); + // Owner holds DEFAULT_ADMIN_ROLE from init; grant the operational roles it exercises. + for (const role of [ + await taskManager.CONFIG_MANAGER_ROLE(), + await taskManager.SECURITY_ZONE_MANAGER_ROLE(), + ]) { + await taskManager.grantRole(role, owner.address); + } + await taskManager.setACLContract(await aclProxy.getAddress()); await taskManager.setPlaintextsStorage(await psProxy.getAddress()); await taskManager.setSecurityZones(-128, 127); From c22db618a08e064f6aa903d5fed9002f455c4f7b Mon Sep 17 00:00:00 2001 From: liorbond Date: Mon, 10 Aug 2026 16:02:05 +0300 Subject: [PATCH 06/11] [FIX] roles: grant all roles, repair deploy Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +- .../internal/host-chain/contracts/ACL.sol | 13 ++ .../contracts/PlaintextsStorage.sol | 8 + .../host-chain/contracts/TaskManager.sol | 13 ++ .../internal/host-chain/deploy/deploy.ts | 53 +++--- .../host-chain/tasks/deployDeterministicTM.ts | 10 +- .../internal/host-chain/tasks/upgradeTM.ts | 19 +- .../decryptResult/DecryptResult.fixture.ts | 19 +- .../test/onChain/OnChain.fixture.ts | 19 +- .../test/publiclyAllowed/PubliclyAllowed.ts | 14 +- .../internal/host-chain/test/roles/Roles.ts | 176 ++++++++++++++++++ contracts/internal/host-chain/utils/roles.ts | 55 ++++++ .../CommitmentRegistry.sol | 29 ++- .../internal/registry-chain/scripts/deploy.ts | 14 +- .../scripts/estimateGasArbitrum.ts | 6 +- .../CommitmentRegistry.behavior.ts | 147 ++++++++++++--- .../CommitmentRegistry.fixture.ts | 13 +- .../commitmentRegistry/CommitmentRegistry.ts | 2 +- .../internal/registry-chain/utils/deploy.ts | 41 ++++ 19 files changed, 550 insertions(+), 110 deletions(-) create mode 100644 contracts/internal/host-chain/test/roles/Roles.ts create mode 100644 contracts/internal/host-chain/utils/roles.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ecb0664..9759022 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,14 @@ ## [Unreleased] ### Added -- **TaskManager access list** — optional, owner-controlled allowlist that gates task intake (`createTask`, `createRandomTask`, `verifyInput`) to approved callers. Off by default, so behavior is unchanged on upgrade; the owner turns it on with `enableAccessList()` / off with `disableAccessList()`, and manages members via batch `addToAccessList` / `removeFromAccessList`. Intended for controlled early-mainnet rollout. ACL `allow*` and decrypt-result publishing are intentionally not gated (ACL is reachable only through gated intake, and decrypt publishing is signature-gated). New storage is appended (the toggle packs into an existing slot, the mapping takes the next), keeping UUPS upgrades storage-layout-compatible. +- **TaskManager access list** — optional allowlist that gates task intake (`createTask`, `createRandomTask`, `verifyInput`) to approved callers. Off by default, so behavior is unchanged on upgrade; a holder of `ACCESS_LIST_MANAGER_ROLE` turns it on with `enableAccessList()` / off with `disableAccessList()`, and manages members via batch `addToAccessList` / `removeFromAccessList`. Intended for controlled early-mainnet rollout. ACL `allow*` and decrypt-result publishing are intentionally not gated (ACL is reachable only through gated intake, and decrypt publishing is signature-gated). New storage is appended (the toggle packs into an existing slot, the mapping takes the next), keeping UUPS upgrades storage-layout-compatible. + +### Changed +- **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 narrow role: TaskManager splits into `UPGRADER_ROLE`, `PAUSER_ROLE`, `SECURITY_ZONE_MANAGER_ROLE`, `AGGREGATOR_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`. + + 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 replaced by `defaultAdmin()`, and `transferOwnership`/`acceptOwnership` by `beginDefaultAdminTransfer`/`acceptDefaultAdminTransfer`. + + Migration: proxies already deployed on the `Ownable` implementation have no AccessControl storage. `initializeV2(uint48 initialDelay, address initialAdmin)` seeds it, and must be passed as the `data` argument of `upgradeToAndCall` so it executes atomically with the upgrade — it is unauthenticated, so any gap would let a third party claim `DEFAULT_ADMIN_ROLE`. The abandoned `openzeppelin.storage.Ownable` / `Ownable2Step` ERC-7201 namespaces are retained as struct declarations so the orphaned owner data stays reserved and cannot be reused by a later upgrade. ### Fixed - `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. diff --git a/contracts/internal/host-chain/contracts/ACL.sol b/contracts/internal/host-chain/contracts/ACL.sol index 6f3d0fb..8816512 100644 --- a/contracts/internal/host-chain/contracts/ACL.sol +++ b/contracts/internal/host-chain/contracts/ACL.sol @@ -17,6 +17,19 @@ import {PermissionedUpgradeable, Permission} from "./Permissioned.sol"; 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(); diff --git a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol index 0ff5354..8c95570 100644 --- a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol +++ b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol @@ -7,6 +7,14 @@ import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts contract PlaintextsStorage is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable { bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); + /// @dev Reserves the namespace this contract used while it inherited OwnableUpgradeable. + /// Already-deployed proxies still hold an owner there; keeping the declaration marks + /// that storage as taken so a later upgrade cannot silently reuse it. + /// @custom:storage-location erc7201:openzeppelin.storage.Ownable + struct OwnableStorage { + address _owner; + } + struct PlaintextResult { bool existenceIndicator; uint256 result; diff --git a/contracts/internal/host-chain/contracts/TaskManager.sol b/contracts/internal/host-chain/contracts/TaskManager.sol index d24341e..77ed546 100644 --- a/contracts/internal/host-chain/contracts/TaskManager.sol +++ b/contracts/internal/host-chain/contracts/TaskManager.sol @@ -162,6 +162,19 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont bytes32 public constant DECRYPT_SIGNER_MANAGER_ROLE = keccak256("DECRYPT_SIGNER_MANAGER_ROLE"); bytes32 public constant CONFIG_MANAGER_ROLE = keccak256("CONFIG_MANAGER_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; + } + bool private initialized; /// @custom:oz-upgrades-unsafe-allow constructor diff --git a/contracts/internal/host-chain/deploy/deploy.ts b/contracts/internal/host-chain/deploy/deploy.ts index 4ae5c6f..4396156 100644 --- a/contracts/internal/host-chain/deploy/deploy.ts +++ b/contracts/internal/host-chain/deploy/deploy.ts @@ -8,6 +8,7 @@ import fs from "fs"; import { deployCreateX } from "../utils/deployCreateX"; import { fundAccount } from "../utils/fund"; +import { getDefaultAdmin, grantAllRoles } from "../utils/roles"; // DOTENV_CONFIG_PATH is used to specify the path to the .env file for example in the CI const dotenvConfigPath: string = process.env.DOTENV_CONFIG_PATH || "../.env"; @@ -15,15 +16,15 @@ dotenvConfig({ path: resolve(__dirname, dotenvConfigPath) }); /** * Deploys a proxy contract for a given contract name - * @param adminAddress The address of the admin account + * @param adminSigner The admin account, which becomes the default admin and holds every role * @param contractName The name of the contract to deploy * @returns The proxy contract and its address */ -async function getProxyContract(adminAddress: string, contractName: string) { +async function getProxyContract(adminSigner: any, contractName: string) { const TaskManager = await ethers.getContractFactory(contractName); const ProxyContract = await upgrades.deployProxy( TaskManager, - [adminAddress, 0], + [adminSigner.address, 0], { kind: "uups", initializer: "initialize" }, ); const deployedImpl = await ProxyContract.waitForDeployment(); @@ -36,6 +37,9 @@ async function getProxyContract(adminAddress: string, contractName: string) { ProxyAddress, ), ); + // `initialize` only grants DEFAULT_ADMIN_ROLE; without this the admin could not even + // upgrade the contract it just deployed. + await grantAllRoles(ProxyContract, adminSigner); return { ProxyContract, ProxyAddress }; } @@ -66,25 +70,6 @@ async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) return e; } - // Grant the setup signer the operational roles it needs before using them. - try { - const admin = aggregatorSigners[0]; - const tm = TMProxyContract.connect(admin); - for (const role of [ - await TMProxyContract.AGGREGATOR_MANAGER_ROLE(), - await TMProxyContract.PAUSER_ROLE(), - await TMProxyContract.SECURITY_ZONE_MANAGER_ROLE(), - await TMProxyContract.VERIFIER_SIGNER_MANAGER_ROLE(), - await TMProxyContract.DECRYPT_SIGNER_MANAGER_ROLE(), - await TMProxyContract.CONFIG_MANAGER_ROLE(), - ]) { - await (await tm.grantRole(role, admin.address)).wait(); - } - } catch (e) { - console.error(chalk.red(`Failed granting setup roles: ${e}`)); - return e; - } - // Set the aggregator address try { const connectedImplementation = TMProxyContract.connect(aggregatorSigners[0]); @@ -304,7 +289,8 @@ async function getImplementationAddress(proxy: any) { async function upgradeTM(TMProxyContract: any, TMFactory: any, aggregatorSigner: any) { console.log(chalk.bold.blue("-----------------------Upgrading TaskManager--------------------------")); console.log(chalk.green("Aggregator signer:", aggregatorSigner.address)); - console.log(chalk.green("owner:", await TMProxyContract.defaultAdmin())); + const currentDefaultAdmin = await getDefaultAdmin(TMProxyContract, ethers.ZeroAddress); + console.log(chalk.green("Default admin before upgrade:", currentDefaultAdmin ?? "none (pre-roles implementation)")); const connectedImplementation = TMProxyContract.connect(aggregatorSigner); const oldImplementationAddress = await getImplementationAddress(connectedImplementation); console.log(chalk.green("Old implementation address:", oldImplementationAddress)); @@ -313,9 +299,24 @@ async function upgradeTM(TMProxyContract: any, TMFactory: any, aggregatorSigner: await newIplDeployment.waitForDeployment(); const newIplAddress = await newIplDeployment.getAddress(); console.log(chalk.green("Before upgrade, new implementation address:", newIplAddress)); - const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, "0x"); + + // The deterministic bootstrap implementation behind this proxy is Ownable, so the + // AccessControl storage is still empty. Seed it via initializeV2 in the *same* + // transaction as the upgrade: initializeV2 is unauthenticated, so any gap between the + // two calls would let anyone claim DEFAULT_ADMIN_ROLE. + const migrationData = + currentDefaultAdmin === null + ? TMFactory.interface.encodeFunctionData("initializeV2", [0, aggregatorSigner.address]) + : "0x"; + const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, migrationData); await tx.wait(); console.log(chalk.green("Successfully upgraded TaskManager contract")); + console.log(chalk.green("Default admin after upgrade:", await TMProxyContract.defaultAdmin())); + + // initialize/initializeV2 only grant DEFAULT_ADMIN_ROLE; incVersion below and the whole + // of TaskManagerSetup need the operational roles. + await grantAllRoles(TMProxyContract, aggregatorSigner); + const incTx = await connectedImplementation.incVersion(); await incTx.wait(); const newImplementationAddress = await getImplementationAddress(connectedImplementation); @@ -384,12 +385,12 @@ const func: DeployFunction = async function () { console.log(chalk.bold.blue("---------------------------ACL------------------------------")); // Deploy and upgrade ACL contract - const {ProxyContract: aclContract} = await getProxyContract(aggregatorSigners[0].address, "ACL"); + const {ProxyContract: aclContract} = await getProxyContract(aggregatorSigners[0], "ACL"); await ACLSetup(TMProxyContract, aggregatorSigners[0], aclContract); // Deploy new PlaintextsStorage contract console.log(chalk.bold.blue("---------------------PlaintextsStorage----------------------")); - const {ProxyAddress: ptStorageAddress} = await getProxyContract(aggregatorSigners[0].address, "PlaintextsStorage"); + const {ProxyAddress: ptStorageAddress} = await getProxyContract(aggregatorSigners[0], "PlaintextsStorage"); await PlaintextsStorageSetup(TMProxyContract, ptStorageAddress, aggregatorSigners[0]); }; diff --git a/contracts/internal/host-chain/tasks/deployDeterministicTM.ts b/contracts/internal/host-chain/tasks/deployDeterministicTM.ts index bc2bb5e..2f1de16 100644 --- a/contracts/internal/host-chain/tasks/deployDeterministicTM.ts +++ b/contracts/internal/host-chain/tasks/deployDeterministicTM.ts @@ -65,7 +65,15 @@ async function getDeterministicProxyContract( // using the ERC1967ProxyModule, in the constructor we pass the implementation address and the data // where the data is the initialization data for the implementation contract const proxyAddress = "0xeA30c4B8b44078Bbf8a6ef5b9f1eC1626C7848D9"; - const proxyInitData = factory.interface.encodeFunctionData("initialize", [admin]); + // The proxy is deployed with CREATE2, so its address depends on its constructor args - + // implementation and init data included. The init data must therefore stay byte-identical + // to keep the proxy at `proxyAddress`, which is compiled into FHE.sol and into ACL / + // PlaintextsStorage as a constant. It has to encode DeterministicTM's `initialize(address)`, + // the implementation actually behind the proxy at this point, and not TaskManager's + // role-based `initialize(address,uint48)`. deploy.ts later upgrades this proxy to + // TaskManager and migrates it to AccessControl via initializeV2. + const deterministicFactory = await hre.ethers.getContractFactory("DeterministicTM"); + const proxyInitData = deterministicFactory.interface.encodeFunctionData("initialize", [admin]); const dummyAddress = await getDeterministicDummyContract(admin, hre); const deployedAddress = await deployDeterministic( hre, diff --git a/contracts/internal/host-chain/tasks/upgradeTM.ts b/contracts/internal/host-chain/tasks/upgradeTM.ts index 4c2faf9..46a715e 100644 --- a/contracts/internal/host-chain/tasks/upgradeTM.ts +++ b/contracts/internal/host-chain/tasks/upgradeTM.ts @@ -4,6 +4,8 @@ import type { TaskArguments } from "hardhat/types"; import { Contract, Wallet } from "ethers"; import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers"; +import { getDefaultAdmin, grantAllRoles } from "../utils/roles"; + async function getImplementationAddress(ethers: any, proxy: any) { const IMPLEMENTATION_SLOT = "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc"; @@ -46,7 +48,8 @@ async function validateUpgrade(upgrades: any, TMProxyContract: any, TMFactory: a async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFactory: any, adminSigner: any) { const connectedImplementation = TMProxyContract.connect(adminSigner); - console.log(chalk.green("TMProxyContract owner:", await TMProxyContract.defaultAdmin())); + const currentDefaultAdmin = await getDefaultAdmin(TMProxyContract, ethers.ZeroAddress); + console.log(chalk.green("TMProxyContract default admin:", currentDefaultAdmin ?? "none (pre-roles implementation)")); const oldImplementationAddress = await getImplementationAddress(ethers, connectedImplementation); console.log(chalk.green("Old implementation address:", oldImplementationAddress)); @@ -54,9 +57,21 @@ async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFac await newIplDeployment.waitForDeployment(); const newIplAddress = await newIplDeployment.getAddress(); console.log(chalk.green("Before upgrade, new implementation address:", newIplAddress)); - const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, "0x"); + + // A proxy still on the pre-roles (Ownable) implementation has no AccessControl storage. + // Seed it via initializeV2 atomically with the upgrade: initializeV2 is unauthenticated, + // so any gap between the two calls would let anyone claim DEFAULT_ADMIN_ROLE. + const migrationData = + currentDefaultAdmin === null + ? TMFactory.interface.encodeFunctionData("initializeV2", [0, adminSigner.address]) + : "0x"; + const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, migrationData); await tx.wait(); console.log(chalk.green("Successfully upgraded TaskManager contract")); + + // initialize/initializeV2 only grant DEFAULT_ADMIN_ROLE; incVersion needs CONFIG_MANAGER_ROLE. + await grantAllRoles(TMProxyContract, adminSigner); + const incTx = await connectedImplementation.incVersion(); await incTx.wait(); const newImplementationAddress = await getImplementationAddress(ethers, connectedImplementation); diff --git a/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts b/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts index 251a417..a050108 100644 --- a/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts +++ b/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts @@ -2,6 +2,8 @@ import hre from "hardhat"; const { ethers } = hre; import { Wallet, BaseContract } from "ethers"; +import { grantAllRoles } from "../../utils/roles"; + // The hardcoded TaskManager address that ACL and PlaintextsStorage expect const TASK_MANAGER_ADDRESS = "0xeA30c4B8b44078Bbf8a6ef5b9f1eC1626C7848D9"; @@ -104,18 +106,11 @@ export async function deployDecryptResultFixture(): Promise + fragment.type === "function" && + fragment.inputs.length === 0 && + /^[A-Z0-9_]+_ROLE$/.test(fragment.name) && + fragment.name !== "DEFAULT_ADMIN_ROLE", + ) + .map((fragment: any) => (fragment as any).name); +} + +describe("Role-based access control", function () { + let owner: HardhatEthersSigner; + let other: HardhatEthersSigner; + let taskManager: any; + let acl: any; + let plaintextsStorage: any; + + before(async function () { + await deployOnChainFixture(); + [owner, other] = await ethers.getSigners(); + taskManager = await ethers.getContractAt("TaskManager", TASK_MANAGER_ADDRESS); + acl = await ethers.getContractAt("ACL", await taskManager.acl()); + plaintextsStorage = await ethers.getContractAt( + "PlaintextsStorage", + await taskManager.plaintextsStorage(), + ); + }); + + // The deploy scripts grant every declared role to the admin wallet. Asserting it here means a + // role added to a contract without a matching grant fails the suite rather than the deployment. + describe("admin wallet holds every declared role", function () { + it("on TaskManager", async function () { + const roleNames = declaredRoleNames(taskManager); + expect(roleNames.length).to.be.greaterThan(0); + for (const roleName of roleNames) { + expect(await taskManager.hasRole(await taskManager[roleName](), owner.address), roleName) + .to.equal(true); + } + }); + + it("on ACL", async function () { + for (const roleName of declaredRoleNames(acl)) { + expect(await acl.hasRole(await acl[roleName](), owner.address), roleName).to.equal(true); + } + }); + + it("on PlaintextsStorage", async function () { + for (const roleName of declaredRoleNames(plaintextsStorage)) { + expect( + await plaintextsStorage.hasRole(await plaintextsStorage[roleName](), owner.address), + roleName, + ).to.equal(true); + } + }); + }); + + describe("default admin", function () { + it("is the admin wallet on every contract", async function () { + expect(await taskManager.defaultAdmin()).to.equal(owner.address); + expect(await acl.defaultAdmin()).to.equal(owner.address); + expect(await plaintextsStorage.defaultAdmin()).to.equal(owner.address); + }); + + it("does not grant operational roles to anyone else", async function () { + for (const roleName of declaredRoleNames(taskManager)) { + expect(await taskManager.hasRole(await taskManager[roleName](), other.address), roleName) + .to.equal(false); + } + }); + }); + + // UPGRADER_ROLE is the one role with no other caller in the system, so it is the easiest to + // forget to grant - and forgetting it leaves the proxy permanently un-upgradeable. + describe("UPGRADER_ROLE gates upgrades", function () { + it("lets a holder upgrade TaskManager", async function () { + const TaskManager = await ethers.getContractFactory("TaskManager"); + const newImpl = await TaskManager.deploy(); + await newImpl.waitForDeployment(); + + await expect(taskManager.connect(owner).upgradeToAndCall(await newImpl.getAddress(), "0x")) + .to.not.be.reverted; + }); + + it("rejects a non-holder", async function () { + const TaskManager = await ethers.getContractFactory("TaskManager"); + const newImpl = await TaskManager.deploy(); + await newImpl.waitForDeployment(); + + await expect(taskManager.connect(other).upgradeToAndCall(await newImpl.getAddress(), "0x")) + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount") + .withArgs(other.address, await taskManager.UPGRADER_ROLE()); + }); + + it("gates ACL and PlaintextsStorage upgrades too", async function () { + const ACL = await ethers.getContractFactory("ACL"); + const newAclImpl = await ACL.deploy(); + await newAclImpl.waitForDeployment(); + await expect(acl.connect(other).upgradeToAndCall(await newAclImpl.getAddress(), "0x")) + .to.be.revertedWithCustomError(acl, "AccessControlUnauthorizedAccount") + .withArgs(other.address, await acl.UPGRADER_ROLE()); + await expect(acl.connect(owner).upgradeToAndCall(await newAclImpl.getAddress(), "0x")) + .to.not.be.reverted; + + const PlaintextsStorage = await ethers.getContractFactory("PlaintextsStorage"); + const newPsImpl = await PlaintextsStorage.deploy(); + await newPsImpl.waitForDeployment(); + await expect( + plaintextsStorage.connect(other).upgradeToAndCall(await newPsImpl.getAddress(), "0x"), + ) + .to.be.revertedWithCustomError(plaintextsStorage, "AccessControlUnauthorizedAccount") + .withArgs(other.address, await plaintextsStorage.UPGRADER_ROLE()); + await expect( + plaintextsStorage.connect(owner).upgradeToAndCall(await newPsImpl.getAddress(), "0x"), + ).to.not.be.reverted; + }); + }); + + // Each setter is bound to its own role, so revoking one must not disturb the others. + describe("roles are independent", function () { + it("revoking PAUSER_ROLE does not affect CONFIG_MANAGER_ROLE", async function () { + const pauserRole = await taskManager.PAUSER_ROLE(); + await taskManager.connect(owner).revokeRole(pauserRole, owner.address); + + await expect(taskManager.connect(owner).disable()) + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount") + .withArgs(owner.address, pauserRole); + await expect(taskManager.connect(owner).incVersion()).to.not.be.reverted; + + await taskManager.connect(owner).grantRole(pauserRole, owner.address); + }); + + it("does not let DEFAULT_ADMIN_ROLE stand in for an operational role", async function () { + const securityZoneRole = await taskManager.SECURITY_ZONE_MANAGER_ROLE(); + await taskManager.connect(owner).revokeRole(securityZoneRole, owner.address); + + expect(await taskManager.defaultAdmin()).to.equal(owner.address); + await expect(taskManager.connect(owner).setSecurityZones(-1, 1)) + .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount") + .withArgs(owner.address, securityZoneRole); + + await taskManager.connect(owner).grantRole(securityZoneRole, owner.address); + }); + }); + + // initializeV2 exists to migrate proxies coming from the pre-roles Ownable implementation. It + // is unauthenticated, so it must be impossible to re-run against a proxy that already has an + // admin - the deploy scripts rely on this plus an atomic upgradeToAndCall. + describe("initializeV2 cannot hijack an initialized proxy", function () { + it("reverts on TaskManager", async function () { + await expect(taskManager.connect(other).initializeV2(0, other.address)) + .to.be.revertedWithCustomError(taskManager, "AccessControlEnforcedDefaultAdminRules"); + }); + + it("reverts on ACL", async function () { + await expect(acl.connect(other).initializeV2(0, other.address)) + .to.be.revertedWithCustomError(acl, "AccessControlEnforcedDefaultAdminRules"); + }); + + it("reverts on PlaintextsStorage", async function () { + await expect(plaintextsStorage.connect(other).initializeV2(0, other.address)) + .to.be.revertedWithCustomError(plaintextsStorage, "AccessControlEnforcedDefaultAdminRules"); + }); + }); +}); diff --git a/contracts/internal/host-chain/utils/roles.ts b/contracts/internal/host-chain/utils/roles.ts new file mode 100644 index 0000000..f0e62bb --- /dev/null +++ b/contracts/internal/host-chain/utils/roles.ts @@ -0,0 +1,55 @@ +import chalk from "chalk"; + +/** + * Grants every role a contract declares (any `*_ROLE` public constant in its ABI) to `adminSigner`. + * + * Discovering the roles from the ABI rather than listing them keeps deployments in sync with the + * contracts: a role added to a contract is granted here without touching this file. + * + * DEFAULT_ADMIN_ROLE is skipped on purpose - AccessControlDefaultAdminRules reverts on granting it + * directly, and the initial admin already holds it from `initialize`. + * + * @param contract An AccessControl contract instance. + * @param adminSigner Signer holding DEFAULT_ADMIN_ROLE, and the grantee. + * @param log Whether to print each grant. Off for test fixtures. + */ +export async function grantAllRoles(contract: any, adminSigner: any, log = true) { + const grantee = adminSigner.address; + const connectedContract = contract.connect(adminSigner); + const defaultAdminRole = await contract.DEFAULT_ADMIN_ROLE(); + + const roleNames: string[] = contract.interface.fragments + .filter( + (fragment: any) => + fragment.type === "function" && + fragment.inputs.length === 0 && + /^[A-Z0-9_]+_ROLE$/.test(fragment.name), + ) + .map((fragment: any) => fragment.name); + + for (const roleName of roleNames) { + const role = await contract[roleName](); + if (role === defaultAdminRole || (await contract.hasRole(role, grantee))) { + continue; + } + const tx = await connectedContract.grantRole(role, grantee); + await tx.wait(); + if (log) { + console.log(chalk.green(`Granted ${roleName} to ${grantee}`)); + } + } +} + +/** + * Returns the proxy's current default admin, or null when the proxy has no AccessControl storage + * yet - either because it still runs a pre-roles (Ownable) implementation, which has no + * `defaultAdmin()` selector at all, or because it was upgraded without running initializeV2. + */ +export async function getDefaultAdmin(proxy: any, zeroAddress: string): Promise { + try { + const defaultAdmin = await proxy.defaultAdmin(); + return defaultAdmin === zeroAddress ? null : defaultAdmin; + } catch { + return null; + } +} diff --git a/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol b/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol index c4bb9a6..19c7235 100644 --- a/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol +++ b/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol @@ -2,9 +2,12 @@ pragma solidity >=0.8.25 <0.9.0; 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"; -contract CommitmentRegistry is UUPSUpgradeable, Ownable2StepUpgradeable { +contract CommitmentRegistry is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable { + bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); + bytes32 public constant POSTER_MANAGER_ROLE = keccak256("POSTER_MANAGER_ROLE"); + bytes32 public constant VERSION_MANAGER_ROLE = keccak256("VERSION_MANAGER_ROLE"); enum VersionStatus { Unset, Active, Deprecated, Revoked } @@ -72,17 +75,25 @@ contract CommitmentRegistry is UUPSUpgradeable, Ownable2StepUpgradeable { _disableInitializers(); } - function initialize(address initialOwner, address initialPoster) public initializer { - if (initialOwner == address(0) || initialPoster == address(0)) { + function initialize(address initialAdmin, uint48 initialDelay, address initialPoster) public initializer { + if (initialAdmin == address(0) || initialPoster == address(0)) { revert InvalidAddress(); } - __Ownable_init(initialOwner); + __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); __UUPSUpgradeable_init(); CommitmentRegistryStorage storage $ = _getStorage(); $.posters[initialPoster] = true; emit PosterAdded(initialPoster); } + /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable + /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the + /// proxy already has a default admin, so it is safe against accidental reuse. + /// @custom:oz-upgrades-validate-as-initializer + function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + } + function postCommitments( bytes32 version, bytes32[] calldata handles, @@ -155,7 +166,7 @@ contract CommitmentRegistry is UUPSUpgradeable, Ownable2StepUpgradeable { emit CommitmentsPostedSafe(version, newlyPosted, len - newlyPosted); } - function addPoster(address poster) external onlyOwner { + function addPoster(address poster) external onlyRole(POSTER_MANAGER_ROLE) { if (poster == address(0)) revert InvalidAddress(); CommitmentRegistryStorage storage $ = _getStorage(); if ($.posters[poster]) revert PosterAlreadyExists(poster); @@ -163,7 +174,7 @@ contract CommitmentRegistry is UUPSUpgradeable, Ownable2StepUpgradeable { emit PosterAdded(poster); } - function removePoster(address poster) external onlyOwner { + function removePoster(address poster) external onlyRole(POSTER_MANAGER_ROLE) { if (poster == address(0)) revert InvalidAddress(); CommitmentRegistryStorage storage $ = _getStorage(); if (!$.posters[poster]) revert PosterNotFound(poster); @@ -171,7 +182,7 @@ contract CommitmentRegistry is UUPSUpgradeable, Ownable2StepUpgradeable { emit PosterRemoved(poster); } - function setVersionStatus(bytes32 version, VersionStatus newStatus) external onlyOwner { + function setVersionStatus(bytes32 version, VersionStatus newStatus) external onlyRole(VERSION_MANAGER_ROLE) { CommitmentRegistryStorage storage $ = _getStorage(); VersionStatus current = $.versionStatus[version]; @@ -229,7 +240,7 @@ contract CommitmentRegistry is UUPSUpgradeable, Ownable2StepUpgradeable { return _getStorage().posters[account]; } - function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + function _authorizeUpgrade(address newImplementation) internal override onlyRole(UPGRADER_ROLE) {} function _getStorage() private pure returns (CommitmentRegistryStorage storage $) { bytes32 slot = STORAGE_SLOT; diff --git a/contracts/internal/registry-chain/scripts/deploy.ts b/contracts/internal/registry-chain/scripts/deploy.ts index 6aec5ef..332e302 100644 --- a/contracts/internal/registry-chain/scripts/deploy.ts +++ b/contracts/internal/registry-chain/scripts/deploy.ts @@ -1,5 +1,9 @@ import hre from "hardhat"; -import { deployUUPSProxy } from "../utils/deploy"; +import { deployUUPSProxy, grantAllRoles } from "../utils/deploy"; + +// Delay enforced on default-admin handover, matching the host-chain deployment. 0 makes transfers +// take effect immediately, which suits dev/test; production should deploy with a non-zero delay. +const DEFAULT_ADMIN_DELAY = 0; // OZ Relayer signer address (deterministic from dev keystore) const DEFAULT_POSTER_ADDRESS = "0x53118C97bD4b7FdDb68244D788Ce7b2946ECd327"; @@ -17,12 +21,16 @@ async function main() { const { proxy: registry, address: proxyAddress } = await deployUUPSProxy( "CommitmentRegistry", - [deployer.address, OZ_RELAYER_ADDRESS], + [deployer.address, DEFAULT_ADMIN_DELAY, OZ_RELAYER_ADDRESS], ); - console.log("Owner:", deployer.address); + console.log("Default admin:", deployer.address); console.log("Poster:", OZ_RELAYER_ADDRESS); + // `initialize` only grants DEFAULT_ADMIN_ROLE. The deployer needs VERSION_MANAGER_ROLE for + // the activation below, and UPGRADER_ROLE / POSTER_MANAGER_ROLE to operate the registry. + await grantAllRoles(registry, deployer); + // Activate initial version const tx = await registry.setVersionStatus(INITIAL_VERSION, 1); // 1 = Active await tx.wait(); diff --git a/contracts/internal/registry-chain/scripts/estimateGasArbitrum.ts b/contracts/internal/registry-chain/scripts/estimateGasArbitrum.ts index 6624ca1..5cdfd5b 100644 --- a/contracts/internal/registry-chain/scripts/estimateGasArbitrum.ts +++ b/contracts/internal/registry-chain/scripts/estimateGasArbitrum.ts @@ -1,4 +1,5 @@ import hre from "hardhat"; +import { grantAllRoles } from "../utils/deploy"; const { ethers, upgrades } = hre; const NODE_INTERFACE_ADDRESS = "0x00000000000000000000000000000000000000C8"; @@ -24,7 +25,7 @@ async function main() { // Deploy console.log("\n--- Deploying CommitmentRegistry ---"); const Factory = await ethers.getContractFactory("CommitmentRegistry"); - const proxy = await upgrades.deployProxy(Factory, [signer.address, signer.address], { + const proxy = await upgrades.deployProxy(Factory, [signer.address, 0, signer.address], { kind: "uups", initializer: "initialize", }); @@ -32,6 +33,9 @@ async function main() { const registryAddress = await proxy.getAddress(); console.log(`Deployed at: ${registryAddress}`); + // `initialize` only grants DEFAULT_ADMIN_ROLE; setVersionStatus needs VERSION_MANAGER_ROLE. + await grantAllRoles(proxy, signer); + // Activate a version const version = ethers.keccak256(ethers.toUtf8Bytes("gas-test-v1")); const tx = await proxy.setVersionStatus(version, 1); // Active diff --git a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts index 15f85ef..71ff073 100644 --- a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts +++ b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts @@ -21,8 +21,21 @@ export function shouldBehaveLikeCommitmentRegistry(): void { // ── Initialization ────────────────────────────────────────────────── describe("Initialization", function () { - it("should set the correct owner", async function () { - expect(await this.registry.owner()).to.equal(this.owner.address); + it("should set the correct default admin", async function () { + expect(await this.registry.defaultAdmin()).to.equal(this.admin.address); + }); + + it("should grant every declared role to the admin", async function () { + for (const roleName of ["UPGRADER_ROLE", "POSTER_MANAGER_ROLE", "VERSION_MANAGER_ROLE"]) { + const role = await this.registry[roleName](); + expect(await this.registry.hasRole(role, this.admin.address), roleName).to.equal(true); + } + }); + + it("should not grant operational roles to anyone else", async function () { + const role = await this.registry.POSTER_MANAGER_ROLE(); + expect(await this.registry.hasRole(role, this.poster.address)).to.equal(false); + expect(await this.registry.hasRole(role, this.otherAccount.address)).to.equal(false); }); it("should set the initial poster", async function () { @@ -35,7 +48,7 @@ export function shouldBehaveLikeCommitmentRegistry(): void { it("should not be re-initializable", async function () { await expect( - this.registry.initialize(this.owner.address, this.poster.address) + this.registry.initialize(this.admin.address, 0, this.poster.address) ).to.be.reverted; }); @@ -45,16 +58,16 @@ export function shouldBehaveLikeCommitmentRegistry(): void { await impl.waitForDeployment(); await expect( - impl.initialize(this.owner.address, this.poster.address) + impl.initialize(this.admin.address, 0, this.poster.address) ).to.be.reverted; }); - it("should revert when initializing with zero owner", async function () { + it("should revert when initializing with zero admin", async function () { const CommitmentRegistry = await ethers.getContractFactory("CommitmentRegistry"); await expect( upgrades.deployProxy( CommitmentRegistry, - [ethers.ZeroAddress, this.poster.address], + [ethers.ZeroAddress, 0, this.poster.address], { kind: "uups", initializer: "initialize" }, ) ).to.be.reverted; @@ -65,11 +78,18 @@ export function shouldBehaveLikeCommitmentRegistry(): void { await expect( upgrades.deployProxy( CommitmentRegistry, - [this.owner.address, ethers.ZeroAddress], + [this.admin.address, 0, ethers.ZeroAddress], { kind: "uups", initializer: "initialize" }, ) ).to.be.reverted; }); + + it("should not let anyone re-seed the admin through initializeV2", async function () { + const registryAsOther = this.registry.connect(this.otherAccount); + await expect( + registryAsOther.initializeV2(0, this.otherAccount.address) + ).to.be.revertedWithCustomError(this.registry, "AccessControlEnforcedDefaultAdminRules"); + }); }); // ── Version Lifecycle ─────────────────────────────────────────────── @@ -167,25 +187,26 @@ export function shouldBehaveLikeCommitmentRegistry(): void { .withArgs(VERSION_1, VersionStatus.Active, VersionStatus.Active); }); - it("should revert when non-owner sets version status", async function () { + it("should revert when a non-VERSION_MANAGER sets version status", async function () { const registryAsPoster = this.registry.connect(this.poster); await expect( registryAsPoster.setVersionStatus(VERSION_1, VersionStatus.Active) - ).to.be.revertedWithCustomError(this.registry, "OwnableUnauthorizedAccount"); + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.poster.address, await this.registry.VERSION_MANAGER_ROLE()); }); }); // ── Poster Management ────────────────────────────────────────────── describe("Poster Management", function () { - it("should allow owner to add a poster", async function () { + it("should allow a POSTER_MANAGER to add a poster", async function () { await expect(this.registry.addPoster(this.otherAccount.address)) .to.emit(this.registry, "PosterAdded") .withArgs(this.otherAccount.address); expect(await this.registry.isPoster(this.otherAccount.address)).to.equal(true); }); - it("should allow owner to remove a poster", async function () { + it("should allow a POSTER_MANAGER to remove a poster", async function () { await expect(this.registry.removePoster(this.poster.address)) .to.emit(this.registry, "PosterRemoved") .withArgs(this.poster.address); @@ -246,18 +267,20 @@ export function shouldBehaveLikeCommitmentRegistry(): void { ).to.be.revertedWithCustomError(this.registry, "InvalidAddress"); }); - it("should revert when non-owner adds poster", async function () { + it("should revert when a non-POSTER_MANAGER adds poster", async function () { const registryAsPoster = this.registry.connect(this.poster); await expect( registryAsPoster.addPoster(this.otherAccount.address) - ).to.be.revertedWithCustomError(this.registry, "OwnableUnauthorizedAccount"); + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.poster.address, await this.registry.POSTER_MANAGER_ROLE()); }); - it("should revert when non-owner removes poster", async function () { + it("should revert when a non-POSTER_MANAGER removes poster", async function () { const registryAsPoster = this.registry.connect(this.poster); await expect( registryAsPoster.removePoster(this.poster.address) - ).to.be.revertedWithCustomError(this.registry, "OwnableUnauthorizedAccount"); + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.poster.address, await this.registry.POSTER_MANAGER_ROLE()); }); it("should allow re-adding a previously removed poster", async function () { @@ -279,30 +302,76 @@ export function shouldBehaveLikeCommitmentRegistry(): void { }); }); - // ── Ownership Transfer (Two-Step) ────────────────────────────────── + // ── Default Admin Transfer (Two-Step) ────────────────────────────── - describe("Ownership Transfer", function () { - it("should not change owner immediately on transferOwnership", async function () { - await this.registry.transferOwnership(this.otherAccount.address); - expect(await this.registry.owner()).to.equal(this.owner.address); + describe("Default Admin Transfer", function () { + it("should not change the default admin immediately on begin", async function () { + await this.registry.beginDefaultAdminTransfer(this.otherAccount.address); + expect(await this.registry.defaultAdmin()).to.equal(this.admin.address); }); - it("should change owner after acceptOwnership", async function () { - await this.registry.transferOwnership(this.otherAccount.address); + it("should change the default admin after accept", async function () { + await this.registry.beginDefaultAdminTransfer(this.otherAccount.address); const registryAsOther = this.registry.connect(this.otherAccount); - await registryAsOther.acceptOwnership(); - expect(await this.registry.owner()).to.equal(this.otherAccount.address); + await registryAsOther.acceptDefaultAdminTransfer(); + expect(await this.registry.defaultAdmin()).to.equal(this.otherAccount.address); + }); + + it("should revert when someone other than the pending admin accepts", async function () { + await this.registry.beginDefaultAdminTransfer(this.otherAccount.address); + const registryAsPoster = this.registry.connect(this.poster); + await expect( + registryAsPoster.acceptDefaultAdminTransfer() + ).to.be.revertedWithCustomError(this.registry, "AccessControlInvalidDefaultAdmin"); + }); + + it("should revert when a non-admin begins a transfer", async function () { + const registryAsPoster = this.registry.connect(this.poster); + await expect( + registryAsPoster.beginDefaultAdminTransfer(this.poster.address) + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount"); }); - it("should allow new owner to call protected functions", async function () { - await this.registry.transferOwnership(this.otherAccount.address); + // Operational roles are held independently of DEFAULT_ADMIN_ROLE, so handing over the + // admin does not hand over the ability to operate the registry - the new admin has to + // grant itself the roles it wants. + it("should not carry operational roles over to the new default admin", async function () { + await this.registry.beginDefaultAdminTransfer(this.otherAccount.address); const registryAsOther = this.registry.connect(this.otherAccount); - await registryAsOther.acceptOwnership(); + await registryAsOther.acceptDefaultAdminTransfer(); + + await expect( + registryAsOther.setVersionStatus(VERSION_1, VersionStatus.Active) + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.otherAccount.address, await this.registry.VERSION_MANAGER_ROLE()); + }); + + it("should let the new default admin grant itself the operational roles", async function () { + await this.registry.beginDefaultAdminTransfer(this.otherAccount.address); + const registryAsOther = this.registry.connect(this.otherAccount); + await registryAsOther.acceptDefaultAdminTransfer(); + + const versionManagerRole = await this.registry.VERSION_MANAGER_ROLE(); + await registryAsOther.grantRole(versionManagerRole, this.otherAccount.address); await expect( registryAsOther.setVersionStatus(VERSION_1, VersionStatus.Active) ).to.not.be.reverted; }); + + it("should let the new default admin revoke the previous admin's roles", async function () { + await this.registry.beginDefaultAdminTransfer(this.otherAccount.address); + const registryAsOther = this.registry.connect(this.otherAccount); + await registryAsOther.acceptDefaultAdminTransfer(); + + const posterManagerRole = await this.registry.POSTER_MANAGER_ROLE(); + await registryAsOther.revokeRole(posterManagerRole, this.admin.address); + + await expect( + this.registry.addPoster(this.otherAccount.address) + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.admin.address, posterManagerRole); + }); }); // ── Post Commitments ─────────────────────────────────────────────── @@ -621,7 +690,7 @@ export function shouldBehaveLikeCommitmentRegistry(): void { .withArgs(this.otherAccount.address); }); - it("should revert when owner (non-poster) posts commitments", async function () { + it("should revert when the admin (non-poster) posts commitments", async function () { await expect( this.registry.postCommitments(VERSION_1, [randomBytes32()], [randomBytes32()]) ).to.be.revertedWithCustomError(this.registry, "OnlyPosterAllowed"); @@ -912,7 +981,7 @@ export function shouldBehaveLikeCommitmentRegistry(): void { // ── Upgrade ──────────────────────────────────────────────────────── describe("Upgrade", function () { - it("should allow owner to upgrade", async function () { + it("should allow an UPGRADER to upgrade", async function () { const CommitmentRegistry = await ethers.getContractFactory("CommitmentRegistry"); const newImpl = await CommitmentRegistry.deploy(); await newImpl.waitForDeployment(); @@ -922,7 +991,7 @@ export function shouldBehaveLikeCommitmentRegistry(): void { ).to.not.be.reverted; }); - it("should revert when non-owner upgrades", async function () { + it("should revert when a non-UPGRADER upgrades", async function () { const CommitmentRegistry = await ethers.getContractFactory("CommitmentRegistry"); const newImpl = await CommitmentRegistry.deploy(); await newImpl.waitForDeployment(); @@ -930,7 +999,23 @@ export function shouldBehaveLikeCommitmentRegistry(): void { const registryAsPoster = this.registry.connect(this.poster); await expect( registryAsPoster.upgradeToAndCall(await newImpl.getAddress(), "0x") - ).to.be.revertedWithCustomError(this.registry, "OwnableUnauthorizedAccount"); + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.poster.address, await this.registry.UPGRADER_ROLE()); + }); + + // DEFAULT_ADMIN_ROLE on its own does not authorize an upgrade; UPGRADER_ROLE does. + it("should revert when the default admin upgrades without UPGRADER_ROLE", async function () { + const CommitmentRegistry = await ethers.getContractFactory("CommitmentRegistry"); + const newImpl = await CommitmentRegistry.deploy(); + await newImpl.waitForDeployment(); + + const upgraderRole = await this.registry.UPGRADER_ROLE(); + await this.registry.revokeRole(upgraderRole, this.admin.address); + + await expect( + this.registry.upgradeToAndCall(await newImpl.getAddress(), "0x") + ).to.be.revertedWithCustomError(this.registry, "AccessControlUnauthorizedAccount") + .withArgs(this.admin.address, upgraderRole); }); it("should preserve state after upgrade", async function () { diff --git a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.fixture.ts b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.fixture.ts index 13a5bb2..8aca178 100644 --- a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.fixture.ts +++ b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.fixture.ts @@ -3,23 +3,28 @@ const { ethers } = hre; import { upgrades } from "hardhat"; import { BaseContract } from "ethers"; +import { grantAllRoles } from "../../utils/deploy"; + export interface CommitmentRegistryFixture { registry: BaseContract; - owner: any; + admin: any; poster: any; otherAccount: any; } export async function deployCommitmentRegistryFixture(): Promise { - const [owner, poster, otherAccount] = await ethers.getSigners(); + const [admin, poster, otherAccount] = await ethers.getSigners(); const CommitmentRegistry = await ethers.getContractFactory("CommitmentRegistry"); const deployed = await upgrades.deployProxy( CommitmentRegistry, - [owner.address, poster.address], + [admin.address, 0, poster.address], { kind: "uups", initializer: "initialize" }, ); const registry = await deployed.waitForDeployment(); - return { registry, owner, poster, otherAccount }; + // `initialize` only grants DEFAULT_ADMIN_ROLE, mirroring what the deploy script does. + await grantAllRoles(registry, admin); + + return { registry, admin, poster, otherAccount }; } diff --git a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.ts b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.ts index bf00cc8..e118f06 100644 --- a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.ts +++ b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.ts @@ -6,7 +6,7 @@ describe("CommitmentRegistry Tests", function () { beforeEach(async function () { const fixture = await loadFixture(deployCommitmentRegistryFixture); this.registry = fixture.registry; - this.owner = fixture.owner; + this.admin = fixture.admin; this.poster = fixture.poster; this.otherAccount = fixture.otherAccount; }); diff --git a/contracts/internal/registry-chain/utils/deploy.ts b/contracts/internal/registry-chain/utils/deploy.ts index 127d84b..b7c8311 100644 --- a/contracts/internal/registry-chain/utils/deploy.ts +++ b/contracts/internal/registry-chain/utils/deploy.ts @@ -21,3 +21,44 @@ export async function deployUUPSProxy( console.log(`Deployed ${contractName} proxy to: ${address}`); return { proxy, address }; } + +/** + * Grants every role a contract declares (any `*_ROLE` public constant in its ABI) to `account`. + * + * Discovering the roles from the ABI rather than listing them keeps the deployment in sync + * with the contracts: a role added to a contract is granted here without touching this file. + * + * DEFAULT_ADMIN_ROLE is skipped on purpose - AccessControlDefaultAdminRules reverts on granting + * it directly, and the initial admin already holds it from `initialize`. + * + * @param contract An AccessControl contract instance. + * @param adminSigner Signer holding DEFAULT_ADMIN_ROLE; also the grantee unless `account` is set. + * @param account Optional grantee, defaults to `adminSigner.address`. + */ +export async function grantAllRoles(contract: any, adminSigner: any, account?: string) { + const grantee = account ?? adminSigner.address; + const connectedContract = contract.connect(adminSigner); + const defaultAdminRole = await contract.DEFAULT_ADMIN_ROLE(); + + const roleNames: string[] = contract.interface.fragments + .filter( + (fragment: any) => + fragment.type === "function" && + fragment.inputs.length === 0 && + /^[A-Z0-9_]+_ROLE$/.test(fragment.name), + ) + .map((fragment: any) => fragment.name); + + for (const roleName of roleNames) { + const role = await contract[roleName](); + if (role === defaultAdminRole) { + continue; + } + if (await contract.hasRole(role, grantee)) { + continue; + } + const tx = await connectedContract.grantRole(role, grantee); + await tx.wait(); + console.log(`Granted ${roleName} to ${grantee}`); + } +} From b3784141067965f28e73dafa0ea6cb7dd2fb6fe4 Mon Sep 17 00:00:00 2001 From: liorbond Date: Mon, 17 Aug 2026 23:00:35 +0300 Subject: [PATCH 07/11] Remove redundant code --- .../host-chain/contracts/TaskManager.sol | 45 +++---------------- 1 file changed, 6 insertions(+), 39 deletions(-) diff --git a/contracts/internal/host-chain/contracts/TaskManager.sol b/contracts/internal/host-chain/contracts/TaskManager.sol index 9ff00a3..6d6b2e7 100644 --- a/contracts/internal/host-chain/contracts/TaskManager.sol +++ b/contracts/internal/host-chain/contracts/TaskManager.sol @@ -30,7 +30,6 @@ error LengthMismatch(); // Access control errors error InvalidAddress(); error OnlyOwnerAllowed(address caller); -error OnlyAggregatorAllowed(address caller); error CofheIsUnavailable(); error NotOnAccessList(address caller); @@ -157,7 +156,6 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); bytes32 public constant SECURITY_ZONE_MANAGER_ROLE = keccak256("SECURITY_ZONE_MANAGER_ROLE"); - bytes32 public constant AGGREGATOR_MANAGER_ROLE = keccak256("AGGREGATOR_MANAGER_ROLE"); bytes32 public constant ACCESS_LIST_MANAGER_ROLE = keccak256("ACCESS_LIST_MANAGER_ROLE"); bytes32 public constant VERIFIER_SIGNER_MANAGER_ROLE = keccak256("VERIFIER_SIGNER_MANAGER_ROLE"); bytes32 public constant DECRYPT_SIGNER_MANAGER_ROLE = keccak256("DECRYPT_SIGNER_MANAGER_ROLE"); @@ -219,7 +217,7 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont return version; } - function incVersion() public onlyRole(CONFIG_MANAGER_ROLE) { + function incVersion() public onlyRole(UPGRADER_ROLE) { version++; } @@ -255,7 +253,8 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont // Random counter uint256 private randomCounter; - address private unusedAggregator; // Should never be used / deleted present only for storage layout + // Deprecated: this address is no longer used + address private _aggregator; // Access-Control contract ACL public acl; @@ -267,7 +266,8 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont // Storage contract for plaintext results of decrypt operations PlaintextsStorage public plaintextsStorage; - mapping(address aggregator => bool isActiveAggregator) public aggregators; + // Deprecated: this mapping is no longer used + mapping(address aggregator => bool isActiveAggregator) public _aggregators; // Master kill-switch for coprocessor intake. // When disabled, task creation (createTask, createRandomTask) and @@ -282,13 +282,6 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont bool public accessListEnabled; mapping(address account => bool isAllowed) public accessList; - modifier onlyAggregator() { - if (!aggregators[msg.sender]) { - revert OnlyAggregatorAllowed(msg.sender); - } - _; - } - modifier onlyIfEnabled() { if (!isEnabled) { revert CofheIsUnavailable(); @@ -633,13 +626,6 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont return ctHash; } - function handleDecryptResult(uint256 ctHash, uint256 result, address[] calldata requestors) external onlyAggregator { - plaintextsStorage.storeResult(ctHash, result); - for (uint8 i = 0; i < requestors.length; i++) { - emit DecryptionResult(ctHash, result, requestors[i]); - } - } - /// @notice Publish a signed decrypt result to the chain /// @dev Anyone with a valid signature from the decrypt network can call this /// @param ctHash The ciphertext hash @@ -787,10 +773,6 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont } } - function handleError(uint256 ctHash, string memory operation, string memory errorMessage) external onlyAggregator { - emit ProtocolNotification(ctHash, operation, errorMessage); - } - // slither-disable-next-line unused-return function getDecryptResultSafe(uint256 ctHash) external view returns (uint256, bool) { return plaintextsStorage.getResult(ctHash); @@ -915,22 +897,7 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont plaintextsStorage = PlaintextsStorage(_plaintextsStorageAddress); } - function addAggregator(address _aggregatorAddress) external onlyRole(AGGREGATOR_MANAGER_ROLE) { - if (_aggregatorAddress == address(0)) { - revert InvalidAddress(); - } - - aggregators[_aggregatorAddress] = true; - } - - function removeAggregator(address _aggregatorAddress) external onlyRole(AGGREGATOR_MANAGER_ROLE) { - if (_aggregatorAddress == address(0)) { - revert InvalidAddress(); - } - aggregators[_aggregatorAddress] = false; - } - function isAllowedWithPermission(Permission memory permission, uint256 handle) public view returns (bool) { return acl.isAllowedWithPermission(permission, handle); } -} \ No newline at end of file +} From fd6e0f7c0d2216d32dd6cbb937fc30ac35f72437 Mon Sep 17 00:00:00 2001 From: liorbond Date: Tue, 18 Aug 2026 11:23:56 +0300 Subject: [PATCH 08/11] [FIX] gate initializeV2, harden deploy scripts Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +- .../internal/host-chain/contracts/ACL.sol | 8 +- .../host-chain/contracts/LegacyOwnable.sol | 50 +++ .../contracts/PlaintextsStorage.sol | 8 +- .../host-chain/contracts/TaskManager.sol | 57 +++- .../internal/host-chain/deploy/deploy.ts | 238 ++++++++------ .../host-chain/storage-layout-snapshot.json | 300 +++++++++++++++--- .../internal/host-chain/tasks/upgradeTM.ts | 46 ++- .../decryptResult/DecryptResult.fixture.ts | 6 +- .../test/onChain/OnChain.fixture.ts | 6 +- .../test/publiclyAllowed/PubliclyAllowed.ts | 6 +- .../internal/host-chain/test/roles/Roles.ts | 106 ++++++- .../test/verifyInput/InputVerified.ts | 68 ++-- contracts/internal/host-chain/utils/roles.ts | 43 ++- .../CommitmentRegistry.sol | 23 +- .../commitment-registry/LegacyOwnable.sol | 49 +++ .../contracts/mocks/MigrationMocks.sol | 34 ++ .../internal/registry-chain/scripts/deploy.ts | 39 ++- .../CommitmentRegistry.behavior.ts | 64 +++- .../internal/registry-chain/utils/deploy.ts | 20 +- 20 files changed, 935 insertions(+), 245 deletions(-) create mode 100644 contracts/internal/host-chain/contracts/LegacyOwnable.sol create mode 100644 contracts/internal/registry-chain/contracts/commitment-registry/LegacyOwnable.sol create mode 100644 contracts/internal/registry-chain/contracts/mocks/MigrationMocks.sol diff --git a/CHANGELOG.md b/CHANGELOG.md index 5325566..5e8ab8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,14 @@ - **TaskManager access list** — optional allowlist that gates task intake (`createTask`, `createRandomTask`, `verifyInput`) to approved callers. Off by default, so behavior is unchanged on upgrade; a holder of `ACCESS_LIST_MANAGER_ROLE` turns it on with `enableAccessList()` / off with `disableAccessList()`, and manages members via batch `addToAccessList` / `removeFromAccessList`. Intended for controlled early-mainnet rollout. ACL `allow*` and decrypt-result publishing are intentionally not gated (ACL is reachable only through gated intake, and decrypt publishing is signature-gated). New storage is appended (the toggle packs into an existing slot, the mapping takes the next), keeping UUPS upgrades storage-layout-compatible. ### Changed -- **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 narrow role: TaskManager splits into `UPGRADER_ROLE`, `PAUSER_ROLE`, `SECURITY_ZONE_MANAGER_ROLE`, `AGGREGATOR_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`. +- **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`. - 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 replaced by `defaultAdmin()`, and `transferOwnership`/`acceptOwnership` by `beginDefaultAdminTransfer`/`acceptDefaultAdminTransfer`. + 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. - Migration: proxies already deployed on the `Ownable` implementation have no AccessControl storage. `initializeV2(uint48 initialDelay, address initialAdmin)` seeds it, and must be passed as the `data` argument of `upgradeToAndCall` so it executes atomically with the upgrade — it is unauthenticated, so any gap would let a third party claim `DEFAULT_ADMIN_ROLE`. The abandoned `openzeppelin.storage.Ownable` / `Ownable2Step` ERC-7201 namespaces are retained as struct declarations so the orphaned owner data stays reserved and cannot be reused by a later upgrade. + 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(uint48 initialDelay, address initialAdmin)` seeds it. 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). - **TaskManager access list** — optional, owner-controlled allowlist that gates task intake (`createTask`, `createRandomTask`, `verifyInput`) to approved callers. Off by default, so behavior is unchanged on upgrade; the owner turns it on with `enableAccessList()` / off with `disableAccessList()`, and manages members via batch `addToAccessList` / `removeFromAccessList`. Intended for controlled early-mainnet rollout. ACL `allow*` and decrypt-result publishing are intentionally not gated (ACL is reachable only through gated intake, and decrypt publishing is signature-gated). New storage is appended (the toggle packs into an existing slot, the mapping takes the next), keeping UUPS upgrades storage-layout-compatible. diff --git a/contracts/internal/host-chain/contracts/ACL.sol b/contracts/internal/host-chain/contracts/ACL.sol index 8816512..f53e7ca 100644 --- a/contracts/internal/host-chain/contracts/ACL.sol +++ b/contracts/internal/host-chain/contracts/ACL.sol @@ -5,6 +5,7 @@ import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol"; import {taskManagerAddress} from "./addresses/TaskManagerAddress.sol"; +import {LegacyOwnable} from "./LegacyOwnable.sol"; import {PermissionedUpgradeable, Permission} from "./Permissioned.sol"; /** @@ -95,11 +96,12 @@ contract ACL is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable, Perm __PermissionedUpgradeable_init(); } - /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable - /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the - /// proxy already has a default admin, so it is safe against accidental reuse. + /// @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. /// @custom:oz-upgrades-validate-as-initializer function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); } diff --git a/contracts/internal/host-chain/contracts/LegacyOwnable.sol b/contracts/internal/host-chain/contracts/LegacyOwnable.sol new file mode 100644 index 0000000..9deb92f --- /dev/null +++ b/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); + } + } +} diff --git a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol index 8c95570..6337f4d 100644 --- a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol +++ b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: BSD-3-Clause-Clear pragma solidity >=0.8.25 <0.9.0; import {taskManagerAddress} from "./addresses/TaskManagerAddress.sol"; +import {LegacyOwnable} from "./LegacyOwnable.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol"; @@ -55,11 +56,12 @@ contract PlaintextsStorage is UUPSUpgradeable, AccessControlDefaultAdminRulesUpg __UUPSUpgradeable_init(); } - /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable - /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the - /// proxy already has a default admin, so it is safe against accidental reuse. + /// @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. /// @custom:oz-upgrades-validate-as-initializer function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); } diff --git a/contracts/internal/host-chain/contracts/TaskManager.sol b/contracts/internal/host-chain/contracts/TaskManager.sol index 6d6b2e7..8e1aac9 100644 --- a/contracts/internal/host-chain/contracts/TaskManager.sol +++ b/contracts/internal/host-chain/contracts/TaskManager.sol @@ -2,6 +2,7 @@ /* solhint-disable one-contract-per-file */ pragma solidity >=0.8.25 <0.9.0; import {ACL, Permission} from "./ACL.sol"; +import {LegacyOwnable} from "./LegacyOwnable.sol"; import {PlaintextsStorage} from "./PlaintextsStorage.sol"; import {Strings} from "@openzeppelin/contracts/utils/Strings.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; @@ -153,12 +154,30 @@ library TMCommon { } contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable { + // --------------------------------------------------------------------------------------- + // Roles. Splitting `onlyOwner` into capabilities limits *who has to hold* each key, not how + // much damage each key can do. Four of these are protocol-critical - a single holder can + // break confidentiality or integrity outright, without ever touching the implementation: + // + // UPGRADER_ROLE arbitrary implementation, i.e. everything. + // CONFIG_MANAGER_ROLE repoints `acl` (permissive `isAllowed` -> unrestricted + // ciphertext access) and `plaintextsStorage` (arbitrary + // plaintext for any handle). See setACLContract below. + // VERIFIER_SIGNER_MANAGER_ROLE forges encrypted inputs; `address(0)` skips verification. + // DECRYPT_SIGNER_MANAGER_ROLE forges decrypt results; `address(0)` skips verification. + // + // Treat those four as admin-equivalent: they belong on the same governance as + // DEFAULT_ADMIN_ROLE, not on an operational hot key. Only PAUSER_ROLE, + // SECURITY_ZONE_MANAGER_ROLE and ACCESS_LIST_MANAGER_ROLE are genuinely narrow - their worst + // case is availability (halting intake, or gating it to an allowlist), not disclosure. + // --------------------------------------------------------------------------------------- bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE"); bytes32 public constant SECURITY_ZONE_MANAGER_ROLE = keccak256("SECURITY_ZONE_MANAGER_ROLE"); bytes32 public constant ACCESS_LIST_MANAGER_ROLE = keccak256("ACCESS_LIST_MANAGER_ROLE"); bytes32 public constant VERIFIER_SIGNER_MANAGER_ROLE = keccak256("VERIFIER_SIGNER_MANAGER_ROLE"); bytes32 public constant DECRYPT_SIGNER_MANAGER_ROLE = keccak256("DECRYPT_SIGNER_MANAGER_ROLE"); + /// @dev Admin-equivalent despite the name - see the role notes above. bytes32 public constant CONFIG_MANAGER_ROLE = keccak256("CONFIG_MANAGER_ROLE"); /// @dev Reserves the namespaces this contract used while it inherited Ownable2StepUpgradeable. @@ -196,11 +215,15 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont isEnabled = true; } - /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable - /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the - /// proxy already has a default admin, so it is safe against accidental reuse. + /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable implementation. + /// Callable only by the owner the pre-roles implementation left behind - the same account + /// its `_authorizeUpgrade` required - so the migration does not depend on being bundled + /// into `upgradeToAndCall`. Without that check `reinitializer(2)` passes on any proxy + /// whose `_initialized == 1`, and the inherited `_grantRole` guard does not fire while + /// `defaultAdmin()` is still zero, leaving DEFAULT_ADMIN_ROLE free for the taking. /// @custom:oz-upgrades-validate-as-initializer function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); } @@ -266,7 +289,9 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont // Storage contract for plaintext results of decrypt operations PlaintextsStorage public plaintextsStorage; - // Deprecated: this mapping is no longer used + // Deprecated: the aggregator allowlist and the unsigned `handleDecryptResult` / `handleError` + // entry points it gated are gone. Decrypt results are now published only through the + // signature-checked `publishDecryptResult*`. Kept so the slot stays reserved. mapping(address aggregator => bool isActiveAggregator) public _aggregators; // Master kill-switch for coprocessor intake. @@ -278,7 +303,8 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont // When set to address(0), signature verification is skipped (debug mode) address public decryptResultSigner; - // Optional, owner-controlled access list, off by default (no behavior change until enabled). + // Optional access list managed by ACCESS_LIST_MANAGER_ROLE, off by default (no behavior + // change until enabled). bool public accessListEnabled; mapping(address account => bool isAllowed) public accessList; @@ -855,6 +881,11 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont return signer; } + /// @notice Set the authorized signer for encrypted inputs + /// @dev Admin-equivalent. The holder can point this at a key it controls and forge + /// encrypted inputs; `address(0)` skips input verification entirely (debug mode, + /// see `verifyInput`). Grant only to whoever holds DEFAULT_ADMIN_ROLE. + /// @param signer The new signer address (address(0) disables verification) function setVerifierSigner(address signer) external onlyRole(VERIFIER_SIGNER_MANAGER_ROLE) { address oldSigner = verifierSigner; verifierSigner = signer; @@ -862,6 +893,11 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont } /// @notice Set the authorized signer for decrypt results + /// @dev Admin-equivalent. The holder can point this at a key it controls and forge + /// decrypt results; `address(0)` makes `_verifyDecryptResult` return true for every + /// signature, so any caller can publish arbitrary plaintext for any handle. Deploy + /// scripts refuse to set zero on non-local networks. Grant only to whoever holds + /// DEFAULT_ADMIN_ROLE. /// @param signer The new signer address (address(0) disables verification) function setDecryptResultSigner(address signer) external onlyRole(DECRYPT_SIGNER_MANAGER_ROLE) { address oldSigner = decryptResultSigner; @@ -883,6 +919,12 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont securityZoneMin = securityZone; } + /// @notice Point the TaskManager at an ACL contract + /// @dev Admin-equivalent, not narrow config. Every confidentiality check funnels through + /// `acl.isAllowed*`, so an ACL whose `isAllowed` returns true grants unrestricted + /// access to every ciphertext - no implementation upgrade required. Grant + /// CONFIG_MANAGER_ROLE only to whoever holds DEFAULT_ADMIN_ROLE. + /// @param _aclAddress The ACL contract address function setACLContract(address _aclAddress) external onlyRole(CONFIG_MANAGER_ROLE) { if (_aclAddress == address(0)) { revert InvalidAddress(); @@ -890,6 +932,11 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont acl = ACL(_aclAddress); } + /// @notice Point the TaskManager at a PlaintextsStorage contract + /// @dev Admin-equivalent, same reasoning as setACLContract: decrypt results are read back + /// from here, so a storage contract that returns attacker-chosen values yields + /// arbitrary plaintext for any handle. + /// @param _plaintextsStorageAddress The PlaintextsStorage contract address function setPlaintextsStorage(address _plaintextsStorageAddress) external onlyRole(CONFIG_MANAGER_ROLE) { if (_plaintextsStorageAddress == address(0)) { revert InvalidAddress(); diff --git a/contracts/internal/host-chain/deploy/deploy.ts b/contracts/internal/host-chain/deploy/deploy.ts index 4396156..c4c55a7 100644 --- a/contracts/internal/host-chain/deploy/deploy.ts +++ b/contracts/internal/host-chain/deploy/deploy.ts @@ -8,7 +8,7 @@ import fs from "fs"; import { deployCreateX } from "../utils/deployCreateX"; import { fundAccount } from "../utils/fund"; -import { getDefaultAdmin, grantAllRoles } from "../utils/roles"; +import { getDefaultAdmin, grantAllRoles, requireDefaultAdminIsSignerOrUnset } from "../utils/roles"; // DOTENV_CONFIG_PATH is used to specify the path to the .env file for example in the CI const dotenvConfigPath: string = process.env.DOTENV_CONFIG_PATH || "../.env"; @@ -17,14 +17,15 @@ dotenvConfig({ path: resolve(__dirname, dotenvConfigPath) }); /** * Deploys a proxy contract for a given contract name * @param adminSigner The admin account, which becomes the default admin and holds every role + * @param adminDelay The default-admin transfer delay to initialize with * @param contractName The name of the contract to deploy * @returns The proxy contract and its address */ -async function getProxyContract(adminSigner: any, contractName: string) { +async function getProxyContract(adminSigner: any, adminDelay: number, contractName: string) { const TaskManager = await ethers.getContractFactory(contractName); const ProxyContract = await upgrades.deployProxy( TaskManager, - [adminSigner.address, 0], + [adminSigner.address, adminDelay], { kind: "uups", initializer: "initialize" }, ); const deployedImpl = await ProxyContract.waitForDeployment(); @@ -37,19 +38,40 @@ async function getProxyContract(adminSigner: any, contractName: string) { ProxyAddress, ), ); - // `initialize` only grants DEFAULT_ADMIN_ROLE; without this the admin could not even - // upgrade the contract it just deployed. + // `initialize` grants only DEFAULT_ADMIN_ROLE, so grant every role the contract declares to the + // deployer - including UPGRADER_ROLE, without which this proxy could never be upgraded again. await grantAllRoles(ProxyContract, adminSigner); return { ProxyContract, ProxyAddress }; } +/** + * Returns true when the network being deployed to is a local dev chain. + * Used to keep dev-only defaults (zero signers, committed keys) from reaching a public network. + */ +function isLocalNetwork() { + const networkName = hre?.network?.name; + const networkUrl = (hre?.network?.config as any)?.url; + if (networkName === "hardhat" || networkName?.startsWith("localfhenix")) { + return true; + } + return Boolean( + networkUrl && + (networkUrl.includes("localhost") || networkUrl.includes("127.0.0.1")), + ); +} + /** * Sets up the TaskManager contract - * Sets the aggregator address and verifies the contract is initialized + * Enables intake, sets the security zones and the verifier / decrypt-result signers. + * + * Every step here is role-gated, so a missing grant surfaces as a revert. These used to be caught + * and returned to a caller that ignored the return value, which turned a half-configured + * TaskManager into a successful-looking deploy - rethrow so the deploy exits non-zero instead. + * * @param TMProxyContract The TaskManager proxy contract - * @param aggregatorSigner The signer with permissions to call TaskManager functions + * @param adminSigner The signer holding the operational roles on the TaskManager */ -async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) { +async function TaskManagerSetup(TMProxyContract: any, adminSigner: any) { // Get the implementation address using ERC1967 storage slot try { const currentImplementation = await getImplementationAddress( @@ -67,59 +89,40 @@ async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) ); } catch (e) { console.error(chalk.red(`Failed isInitialized transaction: ${e}`)); - return e; + throw e; } - // Set the aggregator address + // Open the coprocessor intake kill-switch try { - const connectedImplementation = TMProxyContract.connect(aggregatorSigners[0]); - for (const aggregatorSigner of aggregatorSigners) { - const tx = await connectedImplementation.addAggregator( - aggregatorSigner.address, - ); - - await tx.wait(); - - const enableTx = await connectedImplementation.enable(); - await enableTx.wait(); - - console.log( - chalk.green("Successfully added Aggregator address ", aggregatorSigner.address, " in TaskManager"), - ); - } + const connectedImplementation = TMProxyContract.connect(adminSigner); + const enableTx = await connectedImplementation.enable(); + await enableTx.wait(); + console.log(chalk.green("Successfully enabled TaskManager")); } catch (e) { - console.error(chalk.red(`Failed addAggregator transaction: ${e}`)); - return e; + console.error(chalk.red(`Failed enable transaction: ${e}`)); + throw e; } // Set the security zones try { const minSZ = 0; const maxSZ = 0; - const connectedImplementation = TMProxyContract.connect(aggregatorSigners[0]); + const connectedImplementation = TMProxyContract.connect(adminSigner); const tx = await connectedImplementation.setSecurityZones(minSZ, maxSZ); await tx.wait(); console.log(chalk.green("Successfully set Security Zones in TaskManager")); } catch (e) { console.error(chalk.red(`Failed setSecurityZones transaction: ${e}`)); - return e; + throw e; } try { - const connectedImplementation = TMProxyContract.connect(aggregatorSigners[0]); - if (process.env.VERIFIER_ADDRESS === "0x0000000000000000000000000000000000000000") { - const networkName = hre?.network?.name; - const networkConfig = hre?.network?.config as any; - const networkUrl = networkConfig?.url; - if ( - networkUrl && - !networkUrl.includes("localhost") && - !networkUrl.includes("127.0.0.1") && - !networkName?.startsWith("localfhenix") - ) { - console.error(chalk.red("refusing to set VERIFIER_ADDRESS to 0 on a non-local network!")); - return; - } + const connectedImplementation = TMProxyContract.connect(adminSigner); + if ( + process.env.VERIFIER_ADDRESS === "0x0000000000000000000000000000000000000000" && + !isLocalNetwork() + ) { + throw new Error("refusing to set VERIFIER_ADDRESS to 0 on a non-local network!"); } const tx = await connectedImplementation.setVerifierSigner( @@ -129,25 +132,17 @@ async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) console.log(chalk.green(`Successfully set verifier signer address: ${process.env.VERIFIER_ADDRESS}`)); } catch (e) { console.error(chalk.red(`Failed setVerifierSigner transaction: ${e}`)); - return e; + throw e; } // Set the decrypt result signer (dispatcher's signing key) try { - const connectedImplementation = TMProxyContract.connect(aggregatorSigners[0]); - if (process.env.DECRYPT_RESULT_SIGNER === "0x0000000000000000000000000000000000000000") { - const networkName = hre?.network?.name; - const networkConfig = hre?.network?.config as any; - const networkUrl = networkConfig?.url; - if ( - networkUrl && - !networkUrl.includes("localhost") && - !networkUrl.includes("127.0.0.1") && - !networkName?.startsWith("localfhenix") - ) { - console.error(chalk.red("refusing to set DECRYPT_RESULT_SIGNER to 0 on a non-local network!")); - return; - } + const connectedImplementation = TMProxyContract.connect(adminSigner); + if ( + process.env.DECRYPT_RESULT_SIGNER === "0x0000000000000000000000000000000000000000" && + !isLocalNetwork() + ) { + throw new Error("refusing to set DECRYPT_RESULT_SIGNER to 0 on a non-local network!"); } const tx = await connectedImplementation.setDecryptResultSigner( @@ -157,7 +152,7 @@ async function TaskManagerSetup(TMProxyContract: any, aggregatorSigners: any[]) console.log(chalk.green(`Successfully set decrypt result signer address: ${process.env.DECRYPT_RESULT_SIGNER}`)); } catch (e) { console.error(chalk.red(`Failed setDecryptResultSigner transaction: ${e}`)); - return e; + throw e; } console.log("\n"); } @@ -186,7 +181,7 @@ async function ACLSetup( ); } catch (e) { console.error(chalk.red(`Failed setACL transaction: ${e}`)); - return e; + throw e; } console.log("\n"); } @@ -212,32 +207,6 @@ async function ExampleSetup(deploy: any, deployer: string) { console.log("\n"); } -/** - * Upgrades a proxy contract to a new implementation - * Currently not used, but can be used to upgrade the contracts - mainly for testing - * @param proxy The proxy contract that will be upgraded (must be connected to admin) - * @param admin The admin account that has upgrade permissions - * @param newFactory The contract factory for the new implementation (must be connected to admin) - */ -async function upgradeContract(proxy: any, admin: any, newFactory: any) { - const connectedProxy = proxy.connect(admin); - const connectedNewFactory = newFactory.connect(admin); - // Get the implementation address of the old ACL contract - const oldImplementationAddress = await getImplementationAddress( - connectedProxy, - ); - - const rec = await upgrades.upgradeProxy(connectedProxy, connectedNewFactory); - // Get the implementation address of the new ACL contract - const newImplementationAddress = await getImplementationAddress(rec); - - if (oldImplementationAddress === newImplementationAddress) { - console.log(chalk.red("WARNING: Implementation address did not change!")); - } else { - console.log(chalk.green("Implementation address changed successfully!")); - } -} - /** * Sets up the PlaintextsStorage contract in the TaskManager * @param TMProxyContract The TaskManager proxy contract @@ -283,15 +252,22 @@ async function getImplementationAddress(proxy: any) { * Upgrades the TaskManager contract * @param TMProxyContract The TaskManager proxy contract * @param TMFactory The factory for the TaskManager contract - * @param implementationAddress The address of the implementation contract - * @param aggregatorSigner The signer with permissions to call TaskManager functions + * @param adminSigner The signer that is (or becomes) the proxy's default admin + * @param adminDelay The default-admin transfer delay to seed on migration */ -async function upgradeTM(TMProxyContract: any, TMFactory: any, aggregatorSigner: any) { +async function upgradeTM(TMProxyContract: any, TMFactory: any, adminSigner: any, adminDelay: number) { console.log(chalk.bold.blue("-----------------------Upgrading TaskManager--------------------------")); - console.log(chalk.green("Aggregator signer:", aggregatorSigner.address)); + console.log(chalk.green("Admin signer:", adminSigner.address)); const currentDefaultAdmin = await getDefaultAdmin(TMProxyContract, ethers.ZeroAddress); console.log(chalk.green("Default admin before upgrade:", currentDefaultAdmin ?? "none (pre-roles implementation)")); - const connectedImplementation = TMProxyContract.connect(aggregatorSigner); + + // `_authorizeUpgrade` needs only UPGRADER_ROLE, but `grantAllRoles` below needs + // DEFAULT_ADMIN_ROLE. Once the admin moves to a Safe and this key holds only UPGRADER_ROLE, the + // upgrade would land and the grants would then revert, leaving the proxy on the new + // implementation with no operational roles, no version bump and no TaskManagerSetup. + requireDefaultAdminIsSignerOrUnset(currentDefaultAdmin, adminSigner); + + const connectedImplementation = TMProxyContract.connect(adminSigner); const oldImplementationAddress = await getImplementationAddress(connectedImplementation); console.log(chalk.green("Old implementation address:", oldImplementationAddress)); @@ -300,13 +276,13 @@ async function upgradeTM(TMProxyContract: any, TMFactory: any, aggregatorSigner: const newIplAddress = await newIplDeployment.getAddress(); console.log(chalk.green("Before upgrade, new implementation address:", newIplAddress)); - // The deterministic bootstrap implementation behind this proxy is Ownable, so the - // AccessControl storage is still empty. Seed it via initializeV2 in the *same* - // transaction as the upgrade: initializeV2 is unauthenticated, so any gap between the - // two calls would let anyone claim DEFAULT_ADMIN_ROLE. + // The deterministic bootstrap implementation behind this proxy is Ownable, so the AccessControl + // storage is still empty. Seed it via initializeV2 in the *same* transaction as the upgrade. + // initializeV2 is gated on the legacy Ownable owner, so a gap is no longer exploitable, but + // keeping it atomic means the proxy is never observable in a half-migrated state. const migrationData = currentDefaultAdmin === null - ? TMFactory.interface.encodeFunctionData("initializeV2", [0, aggregatorSigner.address]) + ? TMFactory.interface.encodeFunctionData("initializeV2", [adminDelay, adminSigner.address]) : "0x"; const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, migrationData); await tx.wait(); @@ -315,7 +291,7 @@ async function upgradeTM(TMProxyContract: any, TMFactory: any, aggregatorSigner: // initialize/initializeV2 only grant DEFAULT_ADMIN_ROLE; incVersion below and the whole // of TaskManagerSetup need the operational roles. - await grantAllRoles(TMProxyContract, aggregatorSigner); + await grantAllRoles(TMProxyContract, adminSigner); const incTx = await connectedImplementation.incVersion(); await incTx.wait(); @@ -346,6 +322,60 @@ function getAggregatorWallets(ethers: any) { ); } +/** + * Picks the signer that becomes DEFAULT_ADMIN_ROLE on every proxy this script touches, and the + * default-admin transfer delay to seed. + * + * The fallback is `wallets.json[0]` with a zero delay - a key committed to this repository. That is + * fine for a local stack and unacceptable anywhere else, so on a non-local network both values must + * be stated explicitly via TM_ADMIN_ADDRESS / TM_ADMIN_DELAY. TM_ADMIN_ADDRESS is matched against + * the candidate signers rather than merely recorded: this script has to hold DEFAULT_ADMIN_ROLE to + * run `grantAllRoles`, so an admin it cannot sign for could not be honoured anyway. + */ +function resolveAdmin(candidateSigners: any[]) { + const local = isLocalNetwork(); + const requestedAdmin = process.env.TM_ADMIN_ADDRESS; + const requestedDelay = process.env.TM_ADMIN_DELAY; + + if (!local && !requestedAdmin) { + throw new Error( + "TM_ADMIN_ADDRESS must be set on a non-local network. Refusing to make the committed " + + "wallets.json key the DEFAULT_ADMIN of these proxies.", + ); + } + if (!local && requestedDelay === undefined) { + throw new Error( + "TM_ADMIN_DELAY must be set on a non-local network. A zero delay makes default-admin " + + "transfers take effect immediately, removing the timelock this contract exists to enforce.", + ); + } + + const adminSigner = requestedAdmin + ? candidateSigners.find( + (candidate) => candidate.address.toLowerCase() === requestedAdmin.toLowerCase(), + ) + : candidateSigners[0]; + + if (!adminSigner) { + throw new Error( + `TM_ADMIN_ADDRESS is ${requestedAdmin}, but this deployment has no signer for it. ` + + `Available: ${candidateSigners.map((c) => c.address).join(", ")}. This script must sign ` + + `as the default admin to grant the operational roles.`, + ); + } + + const adminDelay = requestedDelay === undefined ? 0 : Number(requestedDelay); + if (!Number.isInteger(adminDelay) || adminDelay < 0) { + throw new Error(`TM_ADMIN_DELAY must be a non-negative integer number of seconds, got "${requestedDelay}"`); + } + + console.log(chalk.green("Default admin:", adminSigner.address, "delay:", adminDelay)); + if (local && !requestedAdmin) { + console.log(chalk.yellow("TM_ADMIN_ADDRESS not set - using the committed dev key (local network only)")); + } + return { adminSigner, adminDelay }; +} + const func: DeployFunction = async function () { console.log(chalk.bold.blue("-----------------------Network-----------------------------")); console.log(chalk.green("Network name:", hre.network.name)); @@ -373,6 +403,8 @@ const func: DeployFunction = async function () { console.log(chalk.dim("Successfully funded aggregator and deployer accounts")); console.log("\n"); + const { adminSigner, adminDelay } = resolveAdmin([...aggregatorSigners, signer]); + const TMProxyAddress = "0xeA30c4B8b44078Bbf8a6ef5b9f1eC1626C7848D9"; // Headline in chalk blue, with length of 60 @@ -380,18 +412,18 @@ const func: DeployFunction = async function () { const TMFactory = await ethers.getContractFactory("TaskManager"); const TMProxyContract = TMFactory.attach(TMProxyAddress) as Contract; console.log(chalk.green("TMProxyContract attached to:", await TMProxyContract.getAddress())); - await upgradeTM(TMProxyContract, TMFactory, aggregatorSigners[0]); - await TaskManagerSetup(TMProxyContract, aggregatorSigners); + await upgradeTM(TMProxyContract, TMFactory, adminSigner, adminDelay); + await TaskManagerSetup(TMProxyContract, adminSigner); console.log(chalk.bold.blue("---------------------------ACL------------------------------")); // Deploy and upgrade ACL contract - const {ProxyContract: aclContract} = await getProxyContract(aggregatorSigners[0], "ACL"); - await ACLSetup(TMProxyContract, aggregatorSigners[0], aclContract); + const {ProxyContract: aclContract} = await getProxyContract(adminSigner, adminDelay, "ACL"); + await ACLSetup(TMProxyContract, adminSigner, aclContract); // Deploy new PlaintextsStorage contract console.log(chalk.bold.blue("---------------------PlaintextsStorage----------------------")); - const {ProxyAddress: ptStorageAddress} = await getProxyContract(aggregatorSigners[0], "PlaintextsStorage"); - await PlaintextsStorageSetup(TMProxyContract, ptStorageAddress, aggregatorSigners[0]); + const {ProxyAddress: ptStorageAddress} = await getProxyContract(adminSigner, adminDelay, "PlaintextsStorage"); + await PlaintextsStorageSetup(TMProxyContract, ptStorageAddress, adminSigner); }; export default func; diff --git a/contracts/internal/host-chain/storage-layout-snapshot.json b/contracts/internal/host-chain/storage-layout-snapshot.json index f605ff5..4263fdb 100644 --- a/contracts/internal/host-chain/storage-layout-snapshot.json +++ b/contracts/internal/host-chain/storage-layout-snapshot.json @@ -10,7 +10,7 @@ "slot": "0", "type": "t_bool", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:156" + "src": "contracts/TaskManager.sol:196" }, { "label": "securityZoneMax", @@ -18,7 +18,7 @@ "slot": "0", "type": "t_int32", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:219" + "src": "contracts/TaskManager.sol:273" }, { "label": "securityZoneMin", @@ -26,7 +26,7 @@ "slot": "0", "type": "t_int32", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:220" + "src": "contracts/TaskManager.sol:274" }, { "label": "randomCounter", @@ -34,23 +34,23 @@ "slot": "1", "type": "t_uint256", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:223" + "src": "contracts/TaskManager.sol:277" }, { - "label": "unusedAggregator", + "label": "_aggregator", "offset": 0, "slot": "2", "type": "t_address", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:225" + "src": "contracts/TaskManager.sol:280" }, { "label": "acl", "offset": 0, "slot": "3", - "type": "t_contract(ACL)27249", + "type": "t_contract(ACL)28768", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:228" + "src": "contracts/TaskManager.sol:283" }, { "label": "verifierSigner", @@ -58,7 +58,7 @@ "slot": "4", "type": "t_address", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:230" + "src": "contracts/TaskManager.sol:285" }, { "label": "version", @@ -66,23 +66,23 @@ "slot": "4", "type": "t_uint8", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:232" + "src": "contracts/TaskManager.sol:287" }, { "label": "plaintextsStorage", "offset": 0, "slot": "5", - "type": "t_contract(PlaintextsStorage)27679", + "type": "t_contract(PlaintextsStorage)29293", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:235" + "src": "contracts/TaskManager.sol:290" }, { - "label": "aggregators", + "label": "_aggregators", "offset": 0, "slot": "6", "type": "t_mapping(t_address,t_bool)", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:237" + "src": "contracts/TaskManager.sol:295" }, { "label": "isEnabled", @@ -90,7 +90,7 @@ "slot": "7", "type": "t_bool", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:241" + "src": "contracts/TaskManager.sol:300" }, { "label": "decryptResultSigner", @@ -98,7 +98,7 @@ "slot": "7", "type": "t_address", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:245" + "src": "contracts/TaskManager.sol:304" }, { "label": "accessListEnabled", @@ -106,7 +106,7 @@ "slot": "7", "type": "t_bool", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:250" + "src": "contracts/TaskManager.sol:308" }, { "label": "accessList", @@ -114,7 +114,7 @@ "slot": "8", "type": "t_mapping(t_address,t_bool)", "contract": "TaskManager", - "src": "contracts/TaskManager.sol:251" + "src": "contracts/TaskManager.sol:309" } ], "types": { @@ -126,11 +126,11 @@ "label": "bool", "numberOfBytes": "1" }, - "t_contract(ACL)27249": { + "t_contract(ACL)28768": { "label": "contract ACL", "numberOfBytes": "20" }, - "t_contract(PlaintextsStorage)27679": { + "t_contract(PlaintextsStorage)29293": { "label": "contract PlaintextsStorage", "numberOfBytes": "20" }, @@ -150,25 +150,93 @@ "label": "uint8", "numberOfBytes": "1" }, + "t_uint48": { + "label": "uint48" + }, + "t_mapping(t_bytes32,t_struct(RoleData)19325_storage)": { + "label": "mapping(bytes32 => struct AccessControlUpgradeable.RoleData)" + }, + "t_bytes32": { + "label": "bytes32" + }, + "t_struct(RoleData)19325_storage": { + "label": "struct AccessControlUpgradeable.RoleData", + "members": [ + { + "label": "hasRole", + "type": "t_mapping(t_address,t_bool)" + }, + { + "label": "adminRole", + "type": "t_bytes32" + } + ] + }, "t_uint64": { "label": "uint64" } }, "namespaces": { + "erc7201:openzeppelin.storage.Ownable": [ + { + "contract": "TaskManager", + "label": "_owner", + "type": "t_address", + "src": "contracts/TaskManager.sol:188" + } + ], "erc7201:openzeppelin.storage.Ownable2Step": [ { - "contract": "Ownable2StepUpgradeable", + "contract": "TaskManager", "label": "_pendingOwner", "type": "t_address", - "src": "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol:29" + "src": "contracts/TaskManager.sol:193" } ], - "erc7201:openzeppelin.storage.Ownable": [ + "erc7201:openzeppelin.storage.AccessControlDefaultAdminRules": [ { - "contract": "OwnableUpgradeable", - "label": "_owner", + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDefaultAdmin", + "type": "t_address", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:45" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDefaultAdminSchedule", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:46" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_currentDelay", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:48" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_currentDefaultAdmin", "type": "t_address", - "src": "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol:24" + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:49" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDelay", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:52" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDelaySchedule", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:53" + } + ], + "erc7201:openzeppelin.storage.AccessControl": [ + { + "contract": "AccessControlUpgradeable", + "label": "_roles", + "type": "t_mapping(t_bytes32,t_struct(RoleData)19325_storage)", + "src": "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol:61" } ], "erc7201:openzeppelin.storage.Initializable": [ @@ -191,6 +259,9 @@ "solcVersion": "0.8.25", "storage": [], "types": { + "t_address": { + "label": "address" + }, "t_mapping(t_uint256,t_bool)": { "label": "mapping(uint256 => bool)" }, @@ -206,9 +277,6 @@ "t_mapping(t_address,t_bool)": { "label": "mapping(address => bool)" }, - "t_address": { - "label": "address" - }, "t_mapping(t_address,t_mapping(t_address,t_mapping(t_address,t_bool)))": { "label": "mapping(address => mapping(address => mapping(address => bool)))" }, @@ -221,35 +289,70 @@ "t_string_storage": { "label": "string" }, + "t_uint48": { + "label": "uint48" + }, + "t_mapping(t_bytes32,t_struct(RoleData)19325_storage)": { + "label": "mapping(bytes32 => struct AccessControlUpgradeable.RoleData)" + }, + "t_struct(RoleData)19325_storage": { + "label": "struct AccessControlUpgradeable.RoleData", + "members": [ + { + "label": "hasRole", + "type": "t_mapping(t_address,t_bool)" + }, + { + "label": "adminRole", + "type": "t_bytes32" + } + ] + }, "t_uint64": { "label": "uint64" } }, "namespaces": { + "erc7201:openzeppelin.storage.Ownable": [ + { + "contract": "ACL", + "label": "_owner", + "type": "t_address", + "src": "contracts/ACL.sol:26" + } + ], + "erc7201:openzeppelin.storage.Ownable2Step": [ + { + "contract": "ACL", + "label": "_pendingOwner", + "type": "t_address", + "src": "contracts/ACL.sol:31" + } + ], "erc7201:cofhe.storage.ACL": [ { "contract": "ACL", "label": "globalHandles", "type": "t_mapping(t_uint256,t_bool)", - "src": "contracts/ACL.sol:44" + "src": "contracts/ACL.sol:60" }, { "contract": "ACL", "label": "persistedAllowedPairs", "type": "t_mapping(t_uint256,t_mapping(t_address,t_bool))", - "src": "contracts/ACL.sol:45" + "src": "contracts/ACL.sol:61" }, { "contract": "ACL", "label": "allowedForDecryption", "type": "t_mapping(t_uint256,t_bool)", - "src": "contracts/ACL.sol:46" + "src": "contracts/ACL.sol:62" }, { "contract": "ACL", "label": "delegates", "type": "t_mapping(t_address,t_mapping(t_address,t_mapping(t_address,t_bool)))", - "src": "contracts/ACL.sol:47" + "src": "contracts/ACL.sol:63" } ], "erc7201:openzeppelin.storage.EIP712": [ @@ -278,20 +381,50 @@ "src": "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol:44" } ], - "erc7201:openzeppelin.storage.Ownable2Step": [ + "erc7201:openzeppelin.storage.AccessControlDefaultAdminRules": [ { - "contract": "Ownable2StepUpgradeable", - "label": "_pendingOwner", + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDefaultAdmin", "type": "t_address", - "src": "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol:29" + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:45" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDefaultAdminSchedule", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:46" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_currentDelay", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:48" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_currentDefaultAdmin", + "type": "t_address", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:49" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDelay", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:52" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDelaySchedule", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:53" } ], - "erc7201:openzeppelin.storage.Ownable": [ + "erc7201:openzeppelin.storage.AccessControl": [ { - "contract": "OwnableUpgradeable", - "label": "_owner", - "type": "t_address", - "src": "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol:24" + "contract": "AccessControlUpgradeable", + "label": "_roles", + "type": "t_mapping(t_bytes32,t_struct(RoleData)19325_storage)", + "src": "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol:61" } ], "erc7201:openzeppelin.storage.Initializable": [ @@ -317,9 +450,9 @@ "label": "plaintextResults", "offset": 0, "slot": "0", - "type": "t_mapping(t_uint256,t_struct(PlaintextResult)27580_storage)", + "type": "t_mapping(t_uint256,t_struct(PlaintextResult)29166_storage)", "contract": "PlaintextsStorage", - "src": "contracts/PlaintextsStorage.sol:13" + "src": "contracts/PlaintextsStorage.sol:24" } ], "types": { @@ -327,11 +460,11 @@ "label": "bool", "numberOfBytes": "1" }, - "t_mapping(t_uint256,t_struct(PlaintextResult)27580_storage)": { + "t_mapping(t_uint256,t_struct(PlaintextResult)29166_storage)": { "label": "mapping(uint256 => struct PlaintextsStorage.PlaintextResult)", "numberOfBytes": "32" }, - "t_struct(PlaintextResult)27580_storage": { + "t_struct(PlaintextResult)29166_storage": { "label": "struct PlaintextsStorage.PlaintextResult", "members": [ { @@ -356,6 +489,31 @@ "t_address": { "label": "address" }, + "t_uint48": { + "label": "uint48" + }, + "t_mapping(t_bytes32,t_struct(RoleData)19325_storage)": { + "label": "mapping(bytes32 => struct AccessControlUpgradeable.RoleData)" + }, + "t_bytes32": { + "label": "bytes32" + }, + "t_struct(RoleData)19325_storage": { + "label": "struct AccessControlUpgradeable.RoleData", + "members": [ + { + "label": "hasRole", + "type": "t_mapping(t_address,t_bool)" + }, + { + "label": "adminRole", + "type": "t_bytes32" + } + ] + }, + "t_mapping(t_address,t_bool)": { + "label": "mapping(address => bool)" + }, "t_uint64": { "label": "uint64" } @@ -363,10 +521,56 @@ "namespaces": { "erc7201:openzeppelin.storage.Ownable": [ { - "contract": "OwnableUpgradeable", + "contract": "PlaintextsStorage", "label": "_owner", "type": "t_address", - "src": "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol:24" + "src": "contracts/PlaintextsStorage.sol:16" + } + ], + "erc7201:openzeppelin.storage.AccessControlDefaultAdminRules": [ + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDefaultAdmin", + "type": "t_address", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:45" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDefaultAdminSchedule", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:46" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_currentDelay", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:48" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_currentDefaultAdmin", + "type": "t_address", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:49" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDelay", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:52" + }, + { + "contract": "AccessControlDefaultAdminRulesUpgradeable", + "label": "_pendingDelaySchedule", + "type": "t_uint48", + "src": "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol:53" + } + ], + "erc7201:openzeppelin.storage.AccessControl": [ + { + "contract": "AccessControlUpgradeable", + "label": "_roles", + "type": "t_mapping(t_bytes32,t_struct(RoleData)19325_storage)", + "src": "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol:61" } ], "erc7201:openzeppelin.storage.Initializable": [ diff --git a/contracts/internal/host-chain/tasks/upgradeTM.ts b/contracts/internal/host-chain/tasks/upgradeTM.ts index 46a715e..4a17268 100644 --- a/contracts/internal/host-chain/tasks/upgradeTM.ts +++ b/contracts/internal/host-chain/tasks/upgradeTM.ts @@ -4,7 +4,7 @@ import type { TaskArguments } from "hardhat/types"; import { Contract, Wallet } from "ethers"; import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers"; -import { getDefaultAdmin, grantAllRoles } from "../utils/roles"; +import { getDefaultAdmin, grantAllRoles, requireDefaultAdminIsSignerOrUnset } from "../utils/roles"; async function getImplementationAddress(ethers: any, proxy: any) { const IMPLEMENTATION_SLOT = @@ -20,29 +20,40 @@ async function getImplementationAddress(ethers: any, proxy: any) { ); } -async function validateUpgrade(upgrades: any, TMProxyContract: any, TMFactory: any) { +// Registering the proxy with the OpenZeppelin plugin has to use the implementation that is +// *currently* behind it, not the one we are upgrading to - importing with the new factory makes +// `validateUpgrade` compare the new layout against itself, which can never fail. The deterministic +// bootstrap proxy runs DeterministicTM; anything already migrated runs TaskManager. +async function currentImplementationFactory(ethers: any, TMProxyContract: any) { + const defaultAdmin = await getDefaultAdmin(TMProxyContract, ethers.ZeroAddress); + const contractName = defaultAdmin === null ? "DeterministicTM" : "TaskManager"; + console.log(chalk.dim(`Current implementation assumed to be ${contractName}`)); + return ethers.getContractFactory(contractName); +} + +async function validateUpgrade(ethers: any, upgrades: any, TMProxyContract: any, TMFactory: any) { + const proxyAddress = await TMProxyContract.getAddress(); try { console.log("Importing implementation contract..."); - // First, force import the implementation to register it with OpenZeppelin plugin await upgrades.forceImport( - await TMProxyContract.getAddress(), - TMFactory, + proxyAddress, + await currentImplementationFactory(ethers, TMProxyContract), { kind: 'uups' } ); console.log("Validating storage layout..."); // Now validate the upgrade await upgrades.validateUpgrade( - await TMProxyContract.getAddress(), - TMFactory, + proxyAddress, + TMFactory, { kind: 'uups' } ); console.log(chalk.green("✅ Storage layout is compatible with the previous implementation")); } catch (error: any) { console.log(chalk.red("❌ Storage layout validation failed:")); - console.error(chalk.red(error.stack || error.message || error)); - console.log(chalk.yellow("Upgrade aborted")); - return; + // Rethrow: `return` here only exits this function, and the caller would go on to upgrade + // anyway right after printing "Upgrade aborted". + throw error; } } @@ -53,14 +64,21 @@ async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFac const oldImplementationAddress = await getImplementationAddress(ethers, connectedImplementation); console.log(chalk.green("Old implementation address:", oldImplementationAddress)); + // `_authorizeUpgrade` needs only UPGRADER_ROLE, but `grantAllRoles` below needs + // DEFAULT_ADMIN_ROLE. Once the admin moves to a Safe and this key holds only UPGRADER_ROLE, + // the upgrade would land and the grants would then revert, leaving the proxy on the new + // implementation with no operational roles and no version bump. Refuse up front instead. + requireDefaultAdminIsSignerOrUnset(currentDefaultAdmin, adminSigner); + const newIplDeployment = await TMFactory.deploy(); await newIplDeployment.waitForDeployment(); const newIplAddress = await newIplDeployment.getAddress(); console.log(chalk.green("Before upgrade, new implementation address:", newIplAddress)); // A proxy still on the pre-roles (Ownable) implementation has no AccessControl storage. - // Seed it via initializeV2 atomically with the upgrade: initializeV2 is unauthenticated, - // so any gap between the two calls would let anyone claim DEFAULT_ADMIN_ROLE. + // Seed it via initializeV2 in the same transaction as the upgrade. initializeV2 is gated on + // the legacy Ownable owner, so a gap is no longer exploitable, but keeping it atomic means + // the proxy is never observable in a half-migrated state. const migrationData = currentDefaultAdmin === null ? TMFactory.interface.encodeFunctionData("initializeV2", [0, adminSigner.address]) @@ -69,7 +87,7 @@ async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFac await tx.wait(); console.log(chalk.green("Successfully upgraded TaskManager contract")); - // initialize/initializeV2 only grant DEFAULT_ADMIN_ROLE; incVersion needs CONFIG_MANAGER_ROLE. + // initialize/initializeV2 only grant DEFAULT_ADMIN_ROLE; incVersion needs UPGRADER_ROLE. await grantAllRoles(TMProxyContract, adminSigner); const incTx = await connectedImplementation.incVersion(); @@ -116,7 +134,7 @@ task("task:upgradeTM") console.log(chalk.green("TMProxyContract:", await TMProxyContract.getAddress())); - await validateUpgrade(upgrades, TMProxyContract, TMFactory); + await validateUpgrade(ethers, upgrades, TMProxyContract, TMFactory); if (!taskArguments.onlyvalidate) { await upgradeTM(ethers, upgrades, TMProxyContract, TMFactory, signer); diff --git a/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts b/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts index a050108..bfeecad 100644 --- a/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts +++ b/contracts/internal/host-chain/test/decryptResult/DecryptResult.fixture.ts @@ -108,9 +108,9 @@ export async function deployDecryptResultFixture(): Promise requireDefaultAdminIsSignerOrUnset(currentDefaultAdmin, owner)).to.not.throw(); + }); + + it("passes when the proxy has no default admin yet", function () { + expect(() => requireDefaultAdminIsSignerOrUnset(null, other)).to.not.throw(); + }); + + it("throws when the default admin is someone else", async function () { + const currentDefaultAdmin = await getDefaultAdmin(taskManager, ethers.ZeroAddress); + expect(() => requireDefaultAdminIsSignerOrUnset(currentDefaultAdmin, other)) + .to.throw(/Refusing to upgrade: default admin is/); + }); + + it("compares addresses case-insensitively", function () { + expect(() => + requireDefaultAdminIsSignerOrUnset(owner.address.toLowerCase(), { + address: owner.address.toUpperCase().replace("0X", "0x"), + }), + ).to.not.throw(); }); }); }); diff --git a/contracts/internal/host-chain/test/verifyInput/InputVerified.ts b/contracts/internal/host-chain/test/verifyInput/InputVerified.ts index 3bd0d77..eb845fe 100644 --- a/contracts/internal/host-chain/test/verifyInput/InputVerified.ts +++ b/contracts/internal/host-chain/test/verifyInput/InputVerified.ts @@ -1,6 +1,8 @@ import hre from "hardhat"; import { expect } from "chai"; +import { grantAllRoles } from "../../utils/roles"; + const { ethers } = hre; // ACL.allowTransient (and DeterministicACL.allowTransient) require msg.sender to equal @@ -8,67 +10,69 @@ const { ethers } = hre; // this fixed address. Mirrors deployProxyAtAddress from test/publiclyAllowed/PubliclyAllowed.ts. const TASK_MANAGER_ADDRESS = "0xeA30c4B8b44078Bbf8a6ef5b9f1eC1626C7848D9"; +/** + * Install a UUPS proxy's runtime bytecode at a fixed address and initialize it in place. + * + * Copying storage slots out of a throwaway proxy is not enough under AccessControl: role + * membership lives in mapping slots computed from the role and the account, not at a fixed + * offset, so a copied proxy ends up with a default admin that holds no roles. Initialize through + * the real proxy instead, exactly as test/onChain/OnChain.fixture.ts does. + */ async function deployProxyAtAddress( targetAddress: string, implementationAddress: string, initData: string ): Promise { const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); - const tempProxy = await ERC1967Proxy.deploy(implementationAddress, initData); + // Deploy a throwaway proxy only to capture the proxy runtime bytecode. + const tempProxy = await ERC1967Proxy.deploy(implementationAddress, "0x"); await tempProxy.waitForDeployment(); - const proxyBytecode = await ethers.provider.getCode(await tempProxy.getAddress()); + await ethers.provider.send("hardhat_setCode", [targetAddress, proxyBytecode]); - const storageSlots = [ - "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc", - "0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00", - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300", - "0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199301", - "0x0000000000000000000000000000000000000000000000000000000000000000", - "0x0000000000000000000000000000000000000000000000000000000000000001", - "0x0000000000000000000000000000000000000000000000000000000000000002", - "0x0000000000000000000000000000000000000000000000000000000000000003", - "0x0000000000000000000000000000000000000000000000000000000000000004", - "0x0000000000000000000000000000000000000000000000000000000000000005", - "0x0000000000000000000000000000000000000000000000000000000000000006", - "0x0000000000000000000000000000000000000000000000000000000000000007", - "0x0000000000000000000000000000000000000000000000000000000000000008", - "0x0000000000000000000000000000000000000000000000000000000000000009", - "0x000000000000000000000000000000000000000000000000000000000000000a", - ]; - - const tempAddress = await tempProxy.getAddress(); - for (const slot of storageSlots) { - const value = await ethers.provider.getStorage(tempAddress, slot); - if (value !== "0x0000000000000000000000000000000000000000000000000000000000000000") { - await ethers.provider.send("hardhat_setStorageAt", [targetAddress, slot, value]); - } - } + const IMPL_SLOT = "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc"; + await ethers.provider.send("hardhat_setStorageAt", [ + targetAddress, + IMPL_SLOT, + ethers.zeroPadValue(implementationAddress, 32), + ]); + + const [signer] = await ethers.getSigners(); + const tx = await signer.sendTransaction({ to: targetAddress, data: initData }); + await tx.wait(); } async function deployTm(factoryName: string) { + // Other test files deploy a TaskManager at this same hardcoded address inside the same Hardhat + // network process; reset so `initialize` sees fresh storage. + await ethers.provider.send("hardhat_reset", []); + const [owner] = await ethers.getSigners(); const TM = await ethers.getContractFactory(factoryName); const impl = await TM.deploy(); await impl.waitForDeployment(); - const initData = TM.interface.encodeFunctionData("initialize", [owner.address]); + const initData = TM.interface.encodeFunctionData("initialize", [owner.address, 0]); await deployProxyAtAddress(TASK_MANAGER_ADDRESS, await impl.getAddress(), initData); - const tm = TM.attach(TASK_MANAGER_ADDRESS); + const tm = TM.attach(TASK_MANAGER_ADDRESS) as any; const ERC1967Proxy = await ethers.getContractFactory("ERC1967Proxy"); const ACL = await ethers.getContractFactory("ACL"); const aclImpl = await ACL.deploy(); await aclImpl.waitForDeployment(); - const aclInit = ACL.interface.encodeFunctionData("initialize", [owner.address]); + const aclInit = ACL.interface.encodeFunctionData("initialize", [owner.address, 0]); const aclProxy = await ERC1967Proxy.deploy(await aclImpl.getAddress(), aclInit); await aclProxy.waitForDeployment(); + // `initialize` grants only DEFAULT_ADMIN_ROLE, and each setter below is bound to its own role; + // mirror the deploy script and grant them all. + await grantAllRoles(tm, owner, undefined, false); + await tm.setACLContract(await aclProxy.getAddress()); await tm.setSecurityZones(0, 1); - // TaskManager.initialize() sets verifierSigner to address(1) (DeterministicTM uses - // address(0)); reset it here so both variants take the debug path this test targets. + // TaskManager.initialize() sets verifierSigner to address(1); reset it here so this test takes + // the debug path it targets. await tm.setVerifierSigner(ethers.ZeroAddress); return { tm, owner }; } diff --git a/contracts/internal/host-chain/utils/roles.ts b/contracts/internal/host-chain/utils/roles.ts index f0e62bb..2f42dcd 100644 --- a/contracts/internal/host-chain/utils/roles.ts +++ b/contracts/internal/host-chain/utils/roles.ts @@ -1,7 +1,7 @@ import chalk from "chalk"; /** - * Grants every role a contract declares (any `*_ROLE` public constant in its ABI) to `adminSigner`. + * Grants every role a contract declares (any `*_ROLE` public constant in its ABI) to `account`. * * Discovering the roles from the ABI rather than listing them keeps deployments in sync with the * contracts: a role added to a contract is granted here without touching this file. @@ -9,12 +9,21 @@ import chalk from "chalk"; * DEFAULT_ADMIN_ROLE is skipped on purpose - AccessControlDefaultAdminRules reverts on granting it * directly, and the initial admin already holds it from `initialize`. * + * Keep the signature in sync with the sibling copy in `registry-chain/utils/deploy.ts` - the two + * hardhat projects have no shared package, so this is duplicated on purpose. + * * @param contract An AccessControl contract instance. - * @param adminSigner Signer holding DEFAULT_ADMIN_ROLE, and the grantee. + * @param adminSigner Signer holding DEFAULT_ADMIN_ROLE; also the grantee unless `account` is set. + * @param account Optional grantee, defaults to `adminSigner.address`. * @param log Whether to print each grant. Off for test fixtures. */ -export async function grantAllRoles(contract: any, adminSigner: any, log = true) { - const grantee = adminSigner.address; +export async function grantAllRoles( + contract: any, + adminSigner: any, + account?: string, + log = true, +) { + const grantee = account ?? adminSigner.address; const connectedContract = contract.connect(adminSigner); const defaultAdminRole = await contract.DEFAULT_ADMIN_ROLE(); @@ -53,3 +62,29 @@ export async function getDefaultAdmin(proxy: any, zeroAddress: string): Promise< return null; } } + +/** + * Throws unless the proxy's default admin is unset (pre-roles proxy, about to be migrated) or is + * `signer` itself. + * + * Upgrading and granting roles need different roles - UPGRADER_ROLE and DEFAULT_ADMIN_ROLE - so a + * signer holding only the former gets halfway: the implementation swaps, then `grantAllRoles` + * reverts. That leaves the proxy on new code with no operational roles. Check before the swap. + */ +export function requireDefaultAdminIsSignerOrUnset( + currentDefaultAdmin: string | null, + signer: { address: string }, +) { + if ( + currentDefaultAdmin !== null && + currentDefaultAdmin.toLowerCase() !== signer.address.toLowerCase() + ) { + throw new Error( + `Refusing to upgrade: default admin is ${currentDefaultAdmin}, but the signer is ` + + `${signer.address}. The upgrade would succeed and the subsequent role grants would ` + + `revert, leaving the proxy on the new implementation without operational roles. Run ` + + `this from the default admin, or split the upgrade and the grants into separate ` + + `admin-signed steps.`, + ); + } +} diff --git a/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol b/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol index 19c7235..8fdaf4a 100644 --- a/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol +++ b/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol @@ -3,12 +3,26 @@ pragma solidity >=0.8.25 <0.9.0; import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import {AccessControlDefaultAdminRulesUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlDefaultAdminRulesUpgradeable.sol"; +import {LegacyOwnable} from "./LegacyOwnable.sol"; contract CommitmentRegistry is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable { bytes32 public constant UPGRADER_ROLE = keccak256("UPGRADER_ROLE"); bytes32 public constant POSTER_MANAGER_ROLE = keccak256("POSTER_MANAGER_ROLE"); bytes32 public constant VERSION_MANAGER_ROLE = keccak256("VERSION_MANAGER_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; + } + enum VersionStatus { Unset, Active, Deprecated, Revoked } /// @notice Returned when a non-poster address attempts to post commitments. @@ -86,11 +100,14 @@ contract CommitmentRegistry is UUPSUpgradeable, AccessControlDefaultAdminRulesUp emit PosterAdded(initialPoster); } - /// @dev Upgrade-only re-initializer for proxies migrating from the Ownable - /// implementation. Reverts with AccessControlEnforcedDefaultAdminRules if the - /// proxy already has a default admin, so it is safe against accidental reuse. + /// @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. There is + /// no upgrade script for this proxy, so the migration cannot rely on being bundled into + /// `upgradeToAndCall`; whoever performs it must send this call from the legacy owner. /// @custom:oz-upgrades-validate-as-initializer function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); } diff --git a/contracts/internal/registry-chain/contracts/commitment-registry/LegacyOwnable.sol b/contracts/internal/registry-chain/contracts/commitment-registry/LegacyOwnable.sol new file mode 100644 index 0000000..397fada --- /dev/null +++ b/contracts/internal/registry-chain/contracts/commitment-registry/LegacyOwnable.sol @@ -0,0 +1,49 @@ +// 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 `host-chain/contracts/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); + } + } +} diff --git a/contracts/internal/registry-chain/contracts/mocks/MigrationMocks.sol b/contracts/internal/registry-chain/contracts/mocks/MigrationMocks.sol new file mode 100644 index 0000000..eb54008 --- /dev/null +++ b/contracts/internal/registry-chain/contracts/mocks/MigrationMocks.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: BSD-3-Clause-Clear +/* solhint-disable one-contract-per-file */ +pragma solidity >=0.8.25 <0.9.0; + +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; + +/** + * @notice Test-only stand-in for the pre-roles `Ownable2Step` CommitmentRegistry implementation. + * @dev Lets the suite reproduce the state a real migration passes through: a proxy whose + * `_initialized == 1` and whose AccessControl namespace is still zero, with an owner + * recorded in `openzeppelin.storage.Ownable`. Only the parts `initializeV2` depends on are + * reproduced - the registry's own storage is irrelevant to the migration gate. + */ +contract OwnableCommitmentRegistryMock is UUPSUpgradeable, Ownable2StepUpgradeable { + /// @custom:oz-upgrades-unsafe-allow constructor + constructor() { + _disableInitializers(); + } + + function initialize(address initialOwner) public initializer { + __Ownable_init(initialOwner); + __UUPSUpgradeable_init(); + } + + function _authorizeUpgrade(address) internal override onlyOwner {} +} + +/// @notice Test-only ERC-1967 proxy, so the suite can bootstrap on an implementation the +/// OpenZeppelin upgrades plugin would refuse to `deployProxy`. +contract ERC1967ProxyMock is ERC1967Proxy { + constructor(address implementation, bytes memory data) ERC1967Proxy(implementation, data) {} +} diff --git a/contracts/internal/registry-chain/scripts/deploy.ts b/contracts/internal/registry-chain/scripts/deploy.ts index 963bf82..fb26fcc 100644 --- a/contracts/internal/registry-chain/scripts/deploy.ts +++ b/contracts/internal/registry-chain/scripts/deploy.ts @@ -7,25 +7,52 @@ const DEFAULT_ADMIN_DELAY = 0; // OZ Relayer signer address (deterministic from dev keystore) const DEFAULT_POSTER_ADDRESS = "0x53118C97bD4b7FdDb68244D788Ce7b2946ECd327"; -const OZ_RELAYER_ADDRESS = process.env.POSTER_ADDRESS || DEFAULT_POSTER_ADDRESS; // Commitment version to activate (must match COMMITMENT_VERSION in fhe-engine) const INITIAL_VERSION = "0x0000000000000000000000000000000000000000000000000000000000000002"; +/** True when deploying to a local dev chain, where the committed dev defaults are acceptable. */ +function isLocalNetwork() { + const name = hre.network.name; + const url = (hre.network.config as any)?.url; + if (name === "hardhat" || name === "localhost" || name.startsWith("localfhenix")) { + return true; + } + return Boolean(url && (url.includes("localhost") || url.includes("127.0.0.1"))); +} + +/** + * Resolves the initial poster. `DEFAULT_POSTER_ADDRESS` is derivable from the committed dev + * keystore, so falling back to it on a public network would silently hand commitment-posting + * rights to an account anyone can reconstruct. Require POSTER_ADDRESS off local networks. + */ +function resolvePosterAddress() { + if (process.env.POSTER_ADDRESS) { + return process.env.POSTER_ADDRESS; + } + if (!isLocalNetwork()) { + throw new Error( + `POSTER_ADDRESS must be set on network "${hre.network.name}". The default ` + + `${DEFAULT_POSTER_ADDRESS} is derived from the dev keystore and must never hold ` + + `poster rights outside a local stack.`, + ); + } + console.warn(`WARNING: POSTER_ADDRESS not set, using default dev address: ${DEFAULT_POSTER_ADDRESS}`); + return DEFAULT_POSTER_ADDRESS; +} + async function main() { const [deployer] = await hre.ethers.getSigners(); - if (!process.env.POSTER_ADDRESS) { - console.warn(`WARNING: POSTER_ADDRESS not set, using default dev address: ${DEFAULT_POSTER_ADDRESS}`); - } + const posterAddress = resolvePosterAddress(); console.log("Deploying CommitmentRegistry with account:", deployer.address); const { proxy: registry, address: proxyAddress } = await deployUUPSProxy( "CommitmentRegistry", - [deployer.address, DEFAULT_ADMIN_DELAY, OZ_RELAYER_ADDRESS], + [deployer.address, DEFAULT_ADMIN_DELAY, posterAddress], ); console.log("Default admin:", deployer.address); - console.log("Poster:", OZ_RELAYER_ADDRESS); + console.log("Poster:", posterAddress); // `initialize` only grants DEFAULT_ADMIN_ROLE. The deployer needs VERSION_MANAGER_ROLE for // the activation below, and UPGRADER_ROLE / POSTER_MANAGER_ROLE to operate the registry. diff --git a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts index 71ff073..6f1631e 100644 --- a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts +++ b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts @@ -84,11 +84,73 @@ export function shouldBehaveLikeCommitmentRegistry(): void { ).to.be.reverted; }); + // A proxy deployed through `initialize` never ran an Ownable implementation, so its legacy + // owner slot is zero and the migration path is closed to everyone. it("should not let anyone re-seed the admin through initializeV2", async function () { const registryAsOther = this.registry.connect(this.otherAccount); await expect( registryAsOther.initializeV2(0, this.otherAccount.address) - ).to.be.revertedWithCustomError(this.registry, "AccessControlEnforcedDefaultAdminRules"); + ) + .to.be.revertedWithCustomError(this.registry, "NotLegacyOwner") + .withArgs(this.otherAccount.address, ethers.ZeroAddress); + }); + + // There is no upgrade script for this proxy, so a real migration off the Ownable + // implementation would be hand-rolled and would not necessarily bundle the migration call + // into `upgradeToAndCall`. Reproduce that gap and assert nobody but the legacy owner can + // close it: `reinitializer(2)` passes (the Ownable implementation left `_initialized == 1`) + // and the inherited `_grantRole` guard does not fire while `defaultAdmin()` is zero. + describe("during a non-atomic migration from Ownable", function () { + let migrating: any; + let legacyOwner: any; + + beforeEach(async function () { + legacyOwner = this.admin; + + const LegacyRegistry = await ethers.getContractFactory("OwnableCommitmentRegistryMock"); + const legacyImpl = await LegacyRegistry.deploy(); + await legacyImpl.waitForDeployment(); + + const ERC1967Proxy = await ethers.getContractFactory("ERC1967ProxyMock"); + const proxy = await ERC1967Proxy.deploy( + await legacyImpl.getAddress(), + LegacyRegistry.interface.encodeFunctionData("initialize", [legacyOwner.address]), + ); + await proxy.waitForDeployment(); + + const CommitmentRegistry = await ethers.getContractFactory("CommitmentRegistry"); + const newImpl = await CommitmentRegistry.deploy(); + await newImpl.waitForDeployment(); + + // Deliberately no migration calldata - this is the gap being tested. + const legacyProxy = LegacyRegistry.attach(await proxy.getAddress()) as any; + await legacyProxy.connect(legacyOwner).upgradeToAndCall(await newImpl.getAddress(), "0x"); + + migrating = CommitmentRegistry.attach(await proxy.getAddress()); + }); + + it("leaves the proxy with no default admin", async function () { + expect(await migrating.defaultAdmin()).to.equal(ethers.ZeroAddress); + }); + + it("rejects a stranger claiming DEFAULT_ADMIN_ROLE", async function () { + await expect(migrating.connect(this.otherAccount).initializeV2(0, this.otherAccount.address)) + .to.be.revertedWithCustomError(migrating, "NotLegacyOwner") + .withArgs(this.otherAccount.address, legacyOwner.address); + expect(await migrating.defaultAdmin()).to.equal(ethers.ZeroAddress); + }); + + it("lets the legacy owner complete the migration", async function () { + await expect(migrating.connect(legacyOwner).initializeV2(0, legacyOwner.address)) + .to.not.be.reverted; + expect(await migrating.defaultAdmin()).to.equal(legacyOwner.address); + }); + + it("cannot be replayed once migrated", async function () { + await migrating.connect(legacyOwner).initializeV2(0, legacyOwner.address); + await expect(migrating.connect(legacyOwner).initializeV2(0, this.otherAccount.address)) + .to.be.revertedWithCustomError(migrating, "InvalidInitialization"); + }); }); }); diff --git a/contracts/internal/registry-chain/utils/deploy.ts b/contracts/internal/registry-chain/utils/deploy.ts index b7c8311..5e5211b 100644 --- a/contracts/internal/registry-chain/utils/deploy.ts +++ b/contracts/internal/registry-chain/utils/deploy.ts @@ -31,11 +31,20 @@ export async function deployUUPSProxy( * DEFAULT_ADMIN_ROLE is skipped on purpose - AccessControlDefaultAdminRules reverts on granting * it directly, and the initial admin already holds it from `initialize`. * + * Keep the signature in sync with the sibling copy in `host-chain/utils/roles.ts` - the two + * hardhat projects have no shared package, so this is duplicated on purpose. + * * @param contract An AccessControl contract instance. * @param adminSigner Signer holding DEFAULT_ADMIN_ROLE; also the grantee unless `account` is set. * @param account Optional grantee, defaults to `adminSigner.address`. + * @param log Whether to print each grant. Off for test fixtures. */ -export async function grantAllRoles(contract: any, adminSigner: any, account?: string) { +export async function grantAllRoles( + contract: any, + adminSigner: any, + account?: string, + log = true, +) { const grantee = account ?? adminSigner.address; const connectedContract = contract.connect(adminSigner); const defaultAdminRole = await contract.DEFAULT_ADMIN_ROLE(); @@ -51,14 +60,13 @@ export async function grantAllRoles(contract: any, adminSigner: any, account?: s for (const roleName of roleNames) { const role = await contract[roleName](); - if (role === defaultAdminRole) { - continue; - } - if (await contract.hasRole(role, grantee)) { + if (role === defaultAdminRole || (await contract.hasRole(role, grantee))) { continue; } const tx = await connectedContract.grantRole(role, grantee); await tx.wait(); - console.log(`Granted ${roleName} to ${grantee}`); + if (log) { + console.log(`Granted ${roleName} to ${grantee}`); + } } } From cb52f5126f52c34fb220f885344b3c117204b03c Mon Sep 17 00:00:00 2001 From: liorbond Date: Tue, 18 Aug 2026 11:42:57 +0300 Subject: [PATCH 09/11] changelog --- CHANGELOG.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e8ab8d..1bdbbd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,6 @@ Migration: proxies already deployed on the `Ownable` implementation have no AccessControl storage. `initializeV2(uint48 initialDelay, address initialAdmin)` seeds it. 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). -- **TaskManager access list** — optional, owner-controlled allowlist that gates task intake (`createTask`, `createRandomTask`, `verifyInput`) to approved callers. Off by default, so behavior is unchanged on upgrade; the owner turns it on with `enableAccessList()` / off with `disableAccessList()`, and manages members via batch `addToAccessList` / `removeFromAccessList`. Intended for controlled early-mainnet rollout. ACL `allow*` and decrypt-result publishing are intentionally not gated (ACL is reachable only through gated intake, and decrypt publishing is signature-gated). New storage is appended (the toggle packs into an existing slot, the mapping takes the next), keeping UUPS upgrades storage-layout-compatible. - ### Fixed - **`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. From 8abbdc87bb3c5af50f04fe7e6163570685981c05 Mon Sep 17 00:00:00 2001 From: liorbond Date: Wed, 19 Aug 2026 17:29:38 +0300 Subject: [PATCH 10/11] [FIX] fail-closed migration, unblock bootstrap upgrade Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 +- .../internal/host-chain/contracts/ACL.sol | 7 +- .../contracts/PlaintextsStorage.sol | 7 +- .../host-chain/contracts/TaskManager.sol | 45 ++++++++- .../internal/host-chain/deploy/deploy.ts | 49 +++------- .../internal/host-chain/tasks/upgradeTM.ts | 78 ++++++++++----- .../internal/host-chain/test/roles/Roles.ts | 98 ++++++++++++++++--- contracts/internal/host-chain/utils/roles.ts | 63 ++++++++++++ .../CommitmentRegistry.sol | 9 +- .../internal/registry-chain/scripts/deploy.ts | 47 +++++++-- .../CommitmentRegistry.behavior.ts | 22 ++++- .../internal/registry-chain/utils/deploy.ts | 10 ++ 12 files changed, 352 insertions(+), 91 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bdbbd7..f41d27c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,16 @@ 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(uint48 initialDelay, address initialAdmin)` seeds it. 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. + 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). +- **`verifyInput` is now behind the `isEnabled` kill-switch** (BREAKING) — it was the only intake path not gated by `onlyIfEnabled`, so a disabled TaskManager still verified inputs and still emitted `InputVerified`, which off-chain services relay to the CommitmentRegistry. It now reverts with `CofheIsUnavailable` while disabled, consistent with `createTask`, `createRandomTask` and decrypt-result publishing. **Deployment Requirement:** a proxy migrated via `initializeV2` starts disabled, so `enable()` must be called before input verification works. + ### 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. diff --git a/contracts/internal/host-chain/contracts/ACL.sol b/contracts/internal/host-chain/contracts/ACL.sol index f53e7ca..be2d1ee 100644 --- a/contracts/internal/host-chain/contracts/ACL.sol +++ b/contracts/internal/host-chain/contracts/ACL.sol @@ -99,10 +99,15 @@ contract ACL is UUPSUpgradeable, AccessControlDefaultAdminRulesUpgradeable, Perm /// @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(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + function initializeV2(address initialAdmin, uint48 initialDelay) public reinitializer(2) { LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + _grantRole(UPGRADER_ROLE, initialAdmin); } /** diff --git a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol index 6337f4d..28f2424 100644 --- a/contracts/internal/host-chain/contracts/PlaintextsStorage.sol +++ b/contracts/internal/host-chain/contracts/PlaintextsStorage.sol @@ -59,10 +59,15 @@ contract PlaintextsStorage is UUPSUpgradeable, AccessControlDefaultAdminRulesUpg /// @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(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + function initializeV2(address initialAdmin, uint48 initialDelay) public reinitializer(2) { LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + _grantRole(UPGRADER_ROLE, initialAdmin); } function _authorizeUpgrade(address newImplementation) internal override onlyRole(UPGRADER_ROLE) {} diff --git a/contracts/internal/host-chain/contracts/TaskManager.sol b/contracts/internal/host-chain/contracts/TaskManager.sol index 8e1aac9..ff201ea 100644 --- a/contracts/internal/host-chain/contracts/TaskManager.sol +++ b/contracts/internal/host-chain/contracts/TaskManager.sol @@ -221,10 +221,48 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont /// into `upgradeToAndCall`. Without that check `reinitializer(2)` passes on any proxy /// whose `_initialized == 1`, and the inherited `_grantRole` guard does not fire while /// `defaultAdmin()` is still zero, leaving DEFAULT_ADMIN_ROLE free for the taking. + /// + /// Grants the operational roles to `initialAdmin` as well as DEFAULT_ADMIN_ROLE. The admin + /// can grant them to itself anyway, so this is no extra power - it just means 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 (which would brick it permanently). + /// Revoke afterwards to re-establish separation. + /// @param initialAdmin Address receiving DEFAULT_ADMIN_ROLE and the operational roles. + /// @param initialDelay Delay enforced on subsequent default-admin transfers. /// @custom:oz-upgrades-validate-as-initializer - function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + function initializeV2(address initialAdmin, uint48 initialDelay) public reinitializer(2) { LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + + // Looped rather than seven inlined calls: `_grantRole` is large enough that inlining it + // seven times costs ~1KB of the 24KB limit, and TaskManager is already the contract closest + // to it. + bytes32[7] memory roles = [ + UPGRADER_ROLE, + PAUSER_ROLE, + SECURITY_ZONE_MANAGER_ROLE, + ACCESS_LIST_MANAGER_ROLE, + VERIFIER_SIGNER_MANAGER_ROLE, + DECRYPT_SIGNER_MANAGER_ROLE, + CONFIG_MANAGER_ROLE + ]; + for (uint256 i = 0; i < roles.length; i++) { + _grantRole(roles[i], initialAdmin); + } + + // A proxy arriving from the deterministic bootstrap stub reinterprets slots: that stub + // 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. Seed the fail-closed value so a migrated proxy is safe by + // construction rather than by whatever the deploy script gets around to setting. + // + // 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. + if (verifierSigner == address(0)) verifierSigner = address(1); + if (decryptResultSigner == address(0)) decryptResultSigner = address(1); } function setSecurityZones(int32 minSZ, int32 maxSZ) external onlyRole(SECURITY_ZONE_MANAGER_ROLE) { @@ -812,7 +850,10 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont return result; } - function verifyInput(EncryptedInput memory input, address sender) external onlyAccessListed returns (uint256) { + /// @dev `onlyIfEnabled` for the same reason as the other intake paths: this is the only one that + /// was not behind the kill-switch, so a disabled TaskManager still accepted inputs - and + /// still emitted `InputVerified`, which off-chain services relay to the CommitmentRegistry. + function verifyInput(EncryptedInput memory input, address sender) external onlyIfEnabled onlyAccessListed returns (uint256) { int32 securityZone = int32(uint32(input.securityZone)); // When signer is set to 0 address we skip this logic to be able to support debug use cases. diff --git a/contracts/internal/host-chain/deploy/deploy.ts b/contracts/internal/host-chain/deploy/deploy.ts index c4c55a7..e2e6643 100644 --- a/contracts/internal/host-chain/deploy/deploy.ts +++ b/contracts/internal/host-chain/deploy/deploy.ts @@ -8,7 +8,13 @@ import fs from "fs"; import { deployCreateX } from "../utils/deployCreateX"; import { fundAccount } from "../utils/fund"; -import { getDefaultAdmin, grantAllRoles, requireDefaultAdminIsSignerOrUnset } from "../utils/roles"; +import { + getDefaultAdmin, + grantAllRoles, + isLocalNetwork, + requireDefaultAdminIsSignerOrUnset, + resolveAdminDelay, +} from "../utils/roles"; // DOTENV_CONFIG_PATH is used to specify the path to the .env file for example in the CI const dotenvConfigPath: string = process.env.DOTENV_CONFIG_PATH || "../.env"; @@ -44,22 +50,6 @@ async function getProxyContract(adminSigner: any, adminDelay: number, contractNa return { ProxyContract, ProxyAddress }; } -/** - * Returns true when the network being deployed to is a local dev chain. - * Used to keep dev-only defaults (zero signers, committed keys) from reaching a public network. - */ -function isLocalNetwork() { - const networkName = hre?.network?.name; - const networkUrl = (hre?.network?.config as any)?.url; - if (networkName === "hardhat" || networkName?.startsWith("localfhenix")) { - return true; - } - return Boolean( - networkUrl && - (networkUrl.includes("localhost") || networkUrl.includes("127.0.0.1")), - ); -} - /** * Sets up the TaskManager contract * Enables intake, sets the security zones and the verifier / decrypt-result signers. @@ -120,7 +110,7 @@ async function TaskManagerSetup(TMProxyContract: any, adminSigner: any) { const connectedImplementation = TMProxyContract.connect(adminSigner); if ( process.env.VERIFIER_ADDRESS === "0x0000000000000000000000000000000000000000" && - !isLocalNetwork() + !isLocalNetwork(hre) ) { throw new Error("refusing to set VERIFIER_ADDRESS to 0 on a non-local network!"); } @@ -140,7 +130,7 @@ async function TaskManagerSetup(TMProxyContract: any, adminSigner: any) { const connectedImplementation = TMProxyContract.connect(adminSigner); if ( process.env.DECRYPT_RESULT_SIGNER === "0x0000000000000000000000000000000000000000" && - !isLocalNetwork() + !isLocalNetwork(hre) ) { throw new Error("refusing to set DECRYPT_RESULT_SIGNER to 0 on a non-local network!"); } @@ -282,7 +272,7 @@ async function upgradeTM(TMProxyContract: any, TMFactory: any, adminSigner: any, // keeping it atomic means the proxy is never observable in a half-migrated state. const migrationData = currentDefaultAdmin === null - ? TMFactory.interface.encodeFunctionData("initializeV2", [adminDelay, adminSigner.address]) + ? TMFactory.interface.encodeFunctionData("initializeV2", [adminSigner.address, adminDelay]) : "0x"; const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, migrationData); await tx.wait(); @@ -333,9 +323,8 @@ function getAggregatorWallets(ethers: any) { * run `grantAllRoles`, so an admin it cannot sign for could not be honoured anyway. */ function resolveAdmin(candidateSigners: any[]) { - const local = isLocalNetwork(); - const requestedAdmin = process.env.TM_ADMIN_ADDRESS; - const requestedDelay = process.env.TM_ADMIN_DELAY; + const local = isLocalNetwork(hre); + const requestedAdmin = process.env.TM_ADMIN_ADDRESS?.trim(); if (!local && !requestedAdmin) { throw new Error( @@ -343,12 +332,9 @@ function resolveAdmin(candidateSigners: any[]) { "wallets.json key the DEFAULT_ADMIN of these proxies.", ); } - if (!local && requestedDelay === undefined) { - throw new Error( - "TM_ADMIN_DELAY must be set on a non-local network. A zero delay makes default-admin " + - "transfers take effect immediately, removing the timelock this contract exists to enforce.", - ); - } + + // Throws on a blank or zero delay off a local network - see resolveAdminDelay. + const adminDelay = resolveAdminDelay(hre); const adminSigner = requestedAdmin ? candidateSigners.find( @@ -364,11 +350,6 @@ function resolveAdmin(candidateSigners: any[]) { ); } - const adminDelay = requestedDelay === undefined ? 0 : Number(requestedDelay); - if (!Number.isInteger(adminDelay) || adminDelay < 0) { - throw new Error(`TM_ADMIN_DELAY must be a non-negative integer number of seconds, got "${requestedDelay}"`); - } - console.log(chalk.green("Default admin:", adminSigner.address, "delay:", adminDelay)); if (local && !requestedAdmin) { console.log(chalk.yellow("TM_ADMIN_ADDRESS not set - using the committed dev key (local network only)")); diff --git a/contracts/internal/host-chain/tasks/upgradeTM.ts b/contracts/internal/host-chain/tasks/upgradeTM.ts index 4a17268..8f146cf 100644 --- a/contracts/internal/host-chain/tasks/upgradeTM.ts +++ b/contracts/internal/host-chain/tasks/upgradeTM.ts @@ -4,7 +4,12 @@ import type { TaskArguments } from "hardhat/types"; import { Contract, Wallet } from "ethers"; import { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers"; -import { getDefaultAdmin, grantAllRoles, requireDefaultAdminIsSignerOrUnset } from "../utils/roles"; +import { + getDefaultAdmin, + grantAllRoles, + requireDefaultAdminIsSignerOrUnset, + resolveAdminDelay, +} from "../utils/roles"; async function getImplementationAddress(ethers: any, proxy: any) { const IMPLEMENTATION_SLOT = @@ -20,34 +25,59 @@ async function getImplementationAddress(ethers: any, proxy: any) { ); } -// Registering the proxy with the OpenZeppelin plugin has to use the implementation that is -// *currently* behind it, not the one we are upgrading to - importing with the new factory makes -// `validateUpgrade` compare the new layout against itself, which can never fail. The deterministic -// bootstrap proxy runs DeterministicTM; anything already migrated runs TaskManager. -async function currentImplementationFactory(ethers: any, TMProxyContract: any) { - const defaultAdmin = await getDefaultAdmin(TMProxyContract, ethers.ZeroAddress); - const contractName = defaultAdmin === null ? "DeterministicTM" : "TaskManager"; - console.log(chalk.dim(`Current implementation assumed to be ${contractName}`)); - return ethers.getContractFactory(contractName); +/** + * Identifies the implementation currently behind the proxy. + * + * `defaultAdmin() == null` is NOT a proxy for "this is the deterministic stub" - the pre-roles + * Ownable TaskManager, which is what is actually deployed on staging/testnet, has no + * `defaultAdmin()` selector either and so also reads as null. The two have different layouts + * (`DeterministicTM` packs `aggregator` into slot 0 and has no `randomCounter`), so guessing wrong + * makes `validateUpgrade` reject the one migration that is genuinely safe. + * + * Probe instead: `aggregator()` is a public getter only `DeterministicTM` declares. + */ +async function detectCurrentImplementation(ethers: any, proxyAddress: string) { + const stub = (await ethers.getContractFactory("DeterministicTM")).attach(proxyAddress); + try { + await stub.aggregator(); + return "DeterministicTM" as const; + } catch { + return "TaskManager" as const; + } } +/** + * Validates the storage layout of the pending upgrade, and throws if it is incompatible. + * + * Skipped for the deterministic bootstrap: DeterministicTM -> TaskManager is knowingly + * layout-incompatible (TaskManager inserts `randomCounter` at slot 1 and moves the aggregator + * address to slot 2), so the reinterpreted slots are deliberate, not an accident. `initializeV2` + * reseeds the ones that matter to fail-closed values. Validation stays strict on every other path, + * which is where an accidental layout break would actually show up. + */ async function validateUpgrade(ethers: any, upgrades: any, TMProxyContract: any, TMFactory: any) { const proxyAddress = await TMProxyContract.getAddress(); + const current = await detectCurrentImplementation(ethers, proxyAddress); + console.log(chalk.dim(`Current implementation detected as ${current}`)); + + if (current === "DeterministicTM") { + console.log( + chalk.yellow( + "⚠ Skipping storage-layout validation: the deterministic bootstrap stub is intentionally " + + "layout-incompatible with TaskManager. initializeV2 reseeds the reinterpreted slots.", + ), + ); + return; + } + try { console.log("Importing implementation contract..."); - await upgrades.forceImport( - proxyAddress, - await currentImplementationFactory(ethers, TMProxyContract), - { kind: 'uups' } - ); + await upgrades.forceImport(proxyAddress, await ethers.getContractFactory(current), { + kind: "uups", + }); console.log("Validating storage layout..."); - // Now validate the upgrade - await upgrades.validateUpgrade( - proxyAddress, - TMFactory, - { kind: 'uups' } - ); + await upgrades.validateUpgrade(proxyAddress, TMFactory, { kind: "uups" }); console.log(chalk.green("✅ Storage layout is compatible with the previous implementation")); } catch (error: any) { console.log(chalk.red("❌ Storage layout validation failed:")); @@ -57,7 +87,7 @@ async function validateUpgrade(ethers: any, upgrades: any, TMProxyContract: any, } } -async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFactory: any, adminSigner: any) { +async function upgradeTM(ethers: any, TMProxyContract: any, TMFactory: any, adminSigner: any, adminDelay: number) { const connectedImplementation = TMProxyContract.connect(adminSigner); const currentDefaultAdmin = await getDefaultAdmin(TMProxyContract, ethers.ZeroAddress); console.log(chalk.green("TMProxyContract default admin:", currentDefaultAdmin ?? "none (pre-roles implementation)")); @@ -81,7 +111,7 @@ async function upgradeTM(ethers: any, upgrades: any, TMProxyContract: any, TMFac // the proxy is never observable in a half-migrated state. const migrationData = currentDefaultAdmin === null - ? TMFactory.interface.encodeFunctionData("initializeV2", [0, adminSigner.address]) + ? TMFactory.interface.encodeFunctionData("initializeV2", [adminSigner.address, adminDelay]) : "0x"; const tx = await connectedImplementation.upgradeToAndCall(newIplAddress, migrationData); await tx.wait(); @@ -137,6 +167,6 @@ task("task:upgradeTM") await validateUpgrade(ethers, upgrades, TMProxyContract, TMFactory); if (!taskArguments.onlyvalidate) { - await upgradeTM(ethers, upgrades, TMProxyContract, TMFactory, signer); + await upgradeTM(ethers, TMProxyContract, TMFactory, signer, resolveAdminDelay(hre)); } }); diff --git a/contracts/internal/host-chain/test/roles/Roles.ts b/contracts/internal/host-chain/test/roles/Roles.ts index ee99db1..9c08a78 100644 --- a/contracts/internal/host-chain/test/roles/Roles.ts +++ b/contracts/internal/host-chain/test/roles/Roles.ts @@ -8,9 +8,15 @@ const { ethers } = hre; const TASK_MANAGER_ADDRESS = "0xeA30c4B8b44078Bbf8a6ef5b9f1eC1626C7848D9"; -/** Every `*_ROLE` constant the contract declares, other than DEFAULT_ADMIN_ROLE. */ +/** + * Every `*_ROLE` constant the contract declares, other than DEFAULT_ADMIN_ROLE. + * + * Asserts the list is non-empty here rather than at each call site: a caller that loops over an + * empty list runs zero assertions and passes, so an ABI regression would turn these tests green + * instead of red. + */ function declaredRoleNames(contract: any): string[] { - return contract.interface.fragments + const names = contract.interface.fragments .filter( (fragment: any) => fragment.type === "function" && @@ -19,6 +25,8 @@ function declaredRoleNames(contract: any): string[] { fragment.name !== "DEFAULT_ADMIN_ROLE", ) .map((fragment: any) => (fragment as any).name); + expect(names.length, "declaredRoleNames found no *_ROLE constants").to.be.greaterThan(0); + return names; } describe("Role-based access control", function () { @@ -43,9 +51,7 @@ describe("Role-based access control", function () { // role added to a contract without a matching grant fails the suite rather than the deployment. describe("admin wallet holds every declared role", function () { it("on TaskManager", async function () { - const roleNames = declaredRoleNames(taskManager); - expect(roleNames.length).to.be.greaterThan(0); - for (const roleName of roleNames) { + for (const roleName of declaredRoleNames(taskManager)) { expect(await taskManager.hasRole(await taskManager[roleName](), owner.address), roleName) .to.equal(true); } @@ -75,9 +81,11 @@ describe("Role-based access control", function () { }); it("does not grant operational roles to anyone else", async function () { - for (const roleName of declaredRoleNames(taskManager)) { - expect(await taskManager.hasRole(await taskManager[roleName](), other.address), roleName) - .to.equal(false); + for (const contract of [taskManager, acl, plaintextsStorage]) { + for (const roleName of declaredRoleNames(contract)) { + expect(await contract.hasRole(await contract[roleName](), other.address), roleName) + .to.equal(false); + } } }); }); @@ -137,7 +145,10 @@ describe("Role-based access control", function () { await expect(taskManager.connect(owner).disable()) .to.be.revertedWithCustomError(taskManager, "AccessControlUnauthorizedAccount") .withArgs(owner.address, pauserRole); - await expect(taskManager.connect(owner).incVersion()).to.not.be.reverted; + // setACLContract is the CONFIG_MANAGER_ROLE call - incVersion is UPGRADER_ROLE, so using it + // here would leave CONFIG_MANAGER_ROLE untested despite the name. + await expect(taskManager.connect(owner).setACLContract(await acl.getAddress())) + .to.not.be.reverted; await taskManager.connect(owner).grantRole(pauserRole, owner.address); }); @@ -161,19 +172,19 @@ describe("Role-based access control", function () { // owner and rejects the call outright. describe("initializeV2 cannot hijack an initialized proxy", function () { it("reverts on TaskManager", async function () { - await expect(taskManager.connect(other).initializeV2(0, other.address)) + await expect(taskManager.connect(other).initializeV2(other.address, 0)) .to.be.revertedWithCustomError(taskManager, "NotLegacyOwner") .withArgs(other.address, ethers.ZeroAddress); }); it("reverts on ACL", async function () { - await expect(acl.connect(other).initializeV2(0, other.address)) + await expect(acl.connect(other).initializeV2(other.address, 0)) .to.be.revertedWithCustomError(acl, "NotLegacyOwner") .withArgs(other.address, ethers.ZeroAddress); }); it("reverts on PlaintextsStorage", async function () { - await expect(plaintextsStorage.connect(other).initializeV2(0, other.address)) + await expect(plaintextsStorage.connect(other).initializeV2(other.address, 0)) .to.be.revertedWithCustomError(plaintextsStorage, "NotLegacyOwner") .withArgs(other.address, ethers.ZeroAddress); }); @@ -219,7 +230,7 @@ describe("Role-based access control", function () { }); it("rejects a stranger claiming DEFAULT_ADMIN_ROLE", async function () { - await expect(migrating.connect(other).initializeV2(0, other.address)) + await expect(migrating.connect(other).initializeV2(other.address, 0)) .to.be.revertedWithCustomError(migrating, "NotLegacyOwner") .withArgs(other.address, legacyOwner.address); expect(await migrating.defaultAdmin()).to.equal(ethers.ZeroAddress); @@ -228,18 +239,73 @@ describe("Role-based access control", function () { }); it("lets the legacy owner complete the migration", async function () { - await expect(migrating.connect(legacyOwner).initializeV2(0, legacyOwner.address)) + await expect(migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0)) .to.not.be.reverted; expect(await migrating.defaultAdmin()).to.equal(legacyOwner.address); }); + // The bootstrap stub's layout stops at slot 3, so TaskManager's signer slots read as zero - + // which is the verification-*disabled* sentinel. initializeV2 must reseed them, otherwise a + // migrated-but-not-yet-configured proxy accepts unsigned inputs and unsigned decrypt results. + // Pinning it here means a future layout shift fails CI rather than a testnet. + it("leaves both signers fail-closed, not in debug mode", async function () { + expect(await migrating.verifierSigner()).to.equal(ethers.ZeroAddress); + expect(await migrating.decryptResultSigner()).to.equal(ethers.ZeroAddress); + + await migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0); + + expect(await migrating.verifierSigner()).to.equal("0x0000000000000000000000000000000000000001"); + expect(await migrating.decryptResultSigner()).to.equal("0x0000000000000000000000000000000000000001"); + }); + + // Intake stays shut until an operator explicitly enables it, and the unconfigured contract + // addresses stay zero - they have no safe default and must come from CONFIG_MANAGER_ROLE. + it("does not auto-enable intake or invent contract addresses", async function () { + await migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0); + expect(await migrating.isEnabled()).to.equal(false); + expect(await migrating.acl()).to.equal(ethers.ZeroAddress); + expect(await migrating.plaintextsStorage()).to.equal(ethers.ZeroAddress); + }); + + // A Safe or a manual `cast send` performs the migration with no follow-up grant script, so + // initializeV2 has to leave a usable contract - above all an UPGRADER_ROLE holder, without + // which the proxy is bricked permanently. + it("grants the operational roles, so the proxy is not bricked", async function () { + await migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0); + for (const roleName of declaredRoleNames(migrating)) { + expect( + await migrating.hasRole(await migrating[roleName](), legacyOwner.address), + roleName, + ).to.equal(true); + } + }); + it("cannot be replayed once migrated", async function () { - await migrating.connect(legacyOwner).initializeV2(0, legacyOwner.address); - await expect(migrating.connect(legacyOwner).initializeV2(0, other.address)) + await migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0); + await expect(migrating.connect(legacyOwner).initializeV2(other.address, 0)) .to.be.revertedWithCustomError(migrating, "InvalidInitialization"); }); }); + // The seeding above must not touch a proxy migrating from the pre-roles TaskManager: those slots + // hold real, live values there (its `initialize` set both signers to address(1)), and overwriting + // them would reject every genuine input until an operator re-ran the setters. + describe("initializeV2 does not clobber a configured proxy", function () { + it("leaves an already-migrated proxy's signers and ACL untouched", async function () { + const verifier = await taskManager.verifierSigner(); + const decrypt = await taskManager.decryptResultSigner(); + const aclAddress = await taskManager.acl(); + const enabled = await taskManager.isEnabled(); + + await expect(taskManager.connect(owner).initializeV2(owner.address, 0)).to.be.reverted; + + expect(await taskManager.verifierSigner()).to.equal(verifier); + expect(await taskManager.decryptResultSigner()).to.equal(decrypt); + expect(await taskManager.acl()).to.equal(aclAddress); + expect(await taskManager.isEnabled()).to.equal(enabled); + }); + }); + // The deploy scripts upgrade and then grant roles, which need UPGRADER_ROLE and // DEFAULT_ADMIN_ROLE respectively. A signer holding only the former would land the // implementation swap and then revert on the grants, leaving the proxy on new code with no diff --git a/contracts/internal/host-chain/utils/roles.ts b/contracts/internal/host-chain/utils/roles.ts index 2f42dcd..0a28b6f 100644 --- a/contracts/internal/host-chain/utils/roles.ts +++ b/contracts/internal/host-chain/utils/roles.ts @@ -36,6 +36,16 @@ export async function grantAllRoles( ) .map((fragment: any) => fragment.name); + // Discovering roles from the ABI means an ABI that no longer exposes them - a stale typechain + // build, the wrong factory, a renamed constant - silently grants nothing and returns success, + // leaving the proxy with no UPGRADER_ROLE holder and a clean deploy log. Fail loudly instead. + if (roleNames.length === 0) { + throw new Error( + `grantAllRoles found no *_ROLE constants on this contract's ABI, so it would grant nothing. ` + + `Expected at least UPGRADER_ROLE. Recompile, or check the factory being passed in.`, + ); + } + for (const roleName of roleNames) { const role = await contract[roleName](); if (role === defaultAdminRole || (await contract.hasRole(role, grantee))) { @@ -63,6 +73,59 @@ export async function getDefaultAdmin(proxy: any, zeroAddress: string): Promise< } } +/** + * True when the target network is a local dev chain, where the committed dev defaults (zero + * signers, zero admin delay, `wallets.json` keys) are acceptable. + */ +export function isLocalNetwork(hre: any) { + const name: string | undefined = hre?.network?.name; + const url: string | undefined = (hre?.network?.config as any)?.url; + if (name === "hardhat" || name === "localhost" || name?.startsWith("localfhenix")) { + return true; + } + return Boolean(url && (url.includes("localhost") || url.includes("127.0.0.1"))); +} + +/** + * Resolves the default-admin transfer delay from `TM_ADMIN_DELAY`. + * + * A zero delay makes default-admin transfers take effect immediately, removing the timelock the + * contract exists to enforce, so off a local network the value must be stated explicitly. Blank and + * whitespace-only are treated as unset rather than as zero: `TM_ADMIN_DELAY=""` is common in CI and + * docker-compose, and `Number("")` is `0`, so accepting it would hand a production deploy exactly + * the delay this guard exists to prevent. + */ +export function resolveAdminDelay(hre: any): number { + const raw = process.env.TM_ADMIN_DELAY; + const provided = raw !== undefined && raw.trim() !== ""; + + if (!provided) { + if (!isLocalNetwork(hre)) { + throw new Error( + `TM_ADMIN_DELAY must be set to a non-zero number of seconds on network ` + + `"${hre?.network?.name}" (got ${raw === undefined ? "unset" : JSON.stringify(raw)}). A ` + + `zero delay makes default-admin transfers take effect immediately, removing the ` + + `timelock AccessControlDefaultAdminRules exists to enforce.`, + ); + } + return 0; + } + + const delay = Number(raw!.trim()); + if (!Number.isInteger(delay) || delay < 0) { + throw new Error( + `TM_ADMIN_DELAY must be a non-negative integer number of seconds, got ${JSON.stringify(raw)}`, + ); + } + if (delay === 0 && !isLocalNetwork(hre)) { + throw new Error( + `TM_ADMIN_DELAY is 0 on network "${hre?.network?.name}". Refusing - that removes the ` + + `default-admin transfer timelock. Set a non-zero delay, or deploy to a local network.`, + ); + } + return delay; +} + /** * Throws unless the proxy's default admin is unset (pre-roles proxy, about to be migrated) or is * `signer` itself. diff --git a/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol b/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol index 8fdaf4a..3f486b6 100644 --- a/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol +++ b/contracts/internal/registry-chain/contracts/commitment-registry/CommitmentRegistry.sol @@ -105,10 +105,17 @@ contract CommitmentRegistry is UUPSUpgradeable, AccessControlDefaultAdminRulesUp /// {LegacyOwnable} for why that is the only authority available in this window. There is /// no upgrade script for this proxy, so the migration cannot rely on being bundled into /// `upgradeToAndCall`; whoever performs it must send this call from the legacy owner. + /// Grants the operational roles too, so a hand-rolled migration cannot leave the proxy + /// without an UPGRADER_ROLE holder. Revoke afterwards to re-establish separation. + /// @param initialAdmin Address receiving DEFAULT_ADMIN_ROLE and the operational roles. + /// @param initialDelay Delay enforced on subsequent default-admin transfers. /// @custom:oz-upgrades-validate-as-initializer - function initializeV2(uint48 initialDelay, address initialAdmin) public reinitializer(2) { + function initializeV2(address initialAdmin, uint48 initialDelay) public reinitializer(2) { LegacyOwnable.requireLegacyOwner(msg.sender); __AccessControlDefaultAdminRules_init(initialDelay, initialAdmin); + _grantRole(UPGRADER_ROLE, initialAdmin); + _grantRole(POSTER_MANAGER_ROLE, initialAdmin); + _grantRole(VERSION_MANAGER_ROLE, initialAdmin); } function postCommitments( diff --git a/contracts/internal/registry-chain/scripts/deploy.ts b/contracts/internal/registry-chain/scripts/deploy.ts index fb26fcc..3776e6f 100644 --- a/contracts/internal/registry-chain/scripts/deploy.ts +++ b/contracts/internal/registry-chain/scripts/deploy.ts @@ -1,10 +1,6 @@ import hre from "hardhat"; import { deployUUPSProxy, grantAllRoles } from "../utils/deploy"; -// Delay enforced on default-admin handover, matching the host-chain deployment. 0 makes transfers -// take effect immediately, which suits dev/test; production should deploy with a non-zero delay. -const DEFAULT_ADMIN_DELAY = 0; - // OZ Relayer signer address (deterministic from dev keystore) const DEFAULT_POSTER_ADDRESS = "0x53118C97bD4b7FdDb68244D788Ce7b2946ECd327"; @@ -21,6 +17,44 @@ function isLocalNetwork() { return Boolean(url && (url.includes("localhost") || url.includes("127.0.0.1"))); } +/** + * Resolves the delay enforced on default-admin handover, from `REGISTRY_ADMIN_DELAY`. + * + * Mirrors the host-chain guard: a zero delay makes default-admin transfers take effect + * immediately, removing the timelock. Fine on a local stack, refused anywhere else. Blank and + * whitespace-only count as unset rather than as zero, since `Number("")` is `0` and a set-but-empty + * env var is common in CI and docker-compose. + */ +function resolveAdminDelay() { + const raw = process.env.REGISTRY_ADMIN_DELAY; + const provided = raw !== undefined && raw.trim() !== ""; + + if (!provided) { + if (!isLocalNetwork()) { + throw new Error( + `REGISTRY_ADMIN_DELAY must be set to a non-zero number of seconds on network ` + + `"${hre.network.name}" (got ${raw === undefined ? "unset" : JSON.stringify(raw)}). A ` + + `zero delay removes the default-admin transfer timelock.`, + ); + } + return 0; + } + + const delay = Number(raw!.trim()); + if (!Number.isInteger(delay) || delay < 0) { + throw new Error( + `REGISTRY_ADMIN_DELAY must be a non-negative integer number of seconds, got ${JSON.stringify(raw)}`, + ); + } + if (delay === 0 && !isLocalNetwork()) { + throw new Error( + `REGISTRY_ADMIN_DELAY is 0 on network "${hre.network.name}". Refusing - that removes the ` + + `default-admin transfer timelock.`, + ); + } + return delay; +} + /** * Resolves the initial poster. `DEFAULT_POSTER_ADDRESS` is derivable from the committed dev * keystore, so falling back to it on a public network would silently hand commitment-posting @@ -44,14 +78,15 @@ function resolvePosterAddress() { async function main() { const [deployer] = await hre.ethers.getSigners(); const posterAddress = resolvePosterAddress(); + const adminDelay = resolveAdminDelay(); console.log("Deploying CommitmentRegistry with account:", deployer.address); const { proxy: registry, address: proxyAddress } = await deployUUPSProxy( "CommitmentRegistry", - [deployer.address, DEFAULT_ADMIN_DELAY, posterAddress], + [deployer.address, adminDelay, posterAddress], ); - console.log("Default admin:", deployer.address); + console.log("Default admin:", deployer.address, "delay:", adminDelay); console.log("Poster:", posterAddress); // `initialize` only grants DEFAULT_ADMIN_ROLE. The deployer needs VERSION_MANAGER_ROLE for diff --git a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts index 6f1631e..ecd5ed3 100644 --- a/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts +++ b/contracts/internal/registry-chain/test/commitmentRegistry/CommitmentRegistry.behavior.ts @@ -89,7 +89,7 @@ export function shouldBehaveLikeCommitmentRegistry(): void { it("should not let anyone re-seed the admin through initializeV2", async function () { const registryAsOther = this.registry.connect(this.otherAccount); await expect( - registryAsOther.initializeV2(0, this.otherAccount.address) + registryAsOther.initializeV2(this.otherAccount.address, 0) ) .to.be.revertedWithCustomError(this.registry, "NotLegacyOwner") .withArgs(this.otherAccount.address, ethers.ZeroAddress); @@ -134,21 +134,33 @@ export function shouldBehaveLikeCommitmentRegistry(): void { }); it("rejects a stranger claiming DEFAULT_ADMIN_ROLE", async function () { - await expect(migrating.connect(this.otherAccount).initializeV2(0, this.otherAccount.address)) + await expect(migrating.connect(this.otherAccount).initializeV2(this.otherAccount.address, 0)) .to.be.revertedWithCustomError(migrating, "NotLegacyOwner") .withArgs(this.otherAccount.address, legacyOwner.address); expect(await migrating.defaultAdmin()).to.equal(ethers.ZeroAddress); }); it("lets the legacy owner complete the migration", async function () { - await expect(migrating.connect(legacyOwner).initializeV2(0, legacyOwner.address)) + await expect(migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0)) .to.not.be.reverted; expect(await migrating.defaultAdmin()).to.equal(legacyOwner.address); }); + // This proxy has no upgrade script at all, so the migration will be hand-rolled with no + // follow-up grant. Without the operational roles it would come out permanently bricked. + it("grants the operational roles, so the proxy is not bricked", async function () { + await migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0); + for (const role of ["UPGRADER_ROLE", "POSTER_MANAGER_ROLE", "VERSION_MANAGER_ROLE"]) { + expect( + await migrating.hasRole(await migrating[role](), legacyOwner.address), + role, + ).to.equal(true); + } + }); + it("cannot be replayed once migrated", async function () { - await migrating.connect(legacyOwner).initializeV2(0, legacyOwner.address); - await expect(migrating.connect(legacyOwner).initializeV2(0, this.otherAccount.address)) + await migrating.connect(legacyOwner).initializeV2(legacyOwner.address, 0); + await expect(migrating.connect(legacyOwner).initializeV2(this.otherAccount.address, 0)) .to.be.revertedWithCustomError(migrating, "InvalidInitialization"); }); }); diff --git a/contracts/internal/registry-chain/utils/deploy.ts b/contracts/internal/registry-chain/utils/deploy.ts index 5e5211b..15a37a5 100644 --- a/contracts/internal/registry-chain/utils/deploy.ts +++ b/contracts/internal/registry-chain/utils/deploy.ts @@ -58,6 +58,16 @@ export async function grantAllRoles( ) .map((fragment: any) => fragment.name); + // Discovering roles from the ABI means an ABI that no longer exposes them - a stale typechain + // build, the wrong factory, a renamed constant - silently grants nothing and returns success, + // leaving the proxy with no UPGRADER_ROLE holder and a clean deploy log. Fail loudly instead. + if (roleNames.length === 0) { + throw new Error( + `grantAllRoles found no *_ROLE constants on this contract's ABI, so it would grant nothing. ` + + `Expected at least UPGRADER_ROLE. Recompile, or check the factory being passed in.`, + ); + } + for (const roleName of roleNames) { const role = await contract[roleName](); if (role === defaultAdminRole || (await contract.hasRole(role, grantee))) { From 673eeb716abf53db557b15c565b363a57b0f2f28 Mon Sep 17 00:00:00 2001 From: liorbond Date: Mon, 24 Aug 2026 11:24:03 +0300 Subject: [PATCH 11/11] Update contracts/internal/host-chain/contracts/TaskManager.sol Co-authored-by: haimbj1 <64969413+haimbj1@users.noreply.github.com> --- .../internal/host-chain/contracts/TaskManager.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/internal/host-chain/contracts/TaskManager.sol b/contracts/internal/host-chain/contracts/TaskManager.sol index ff201ea..e710b46 100644 --- a/contracts/internal/host-chain/contracts/TaskManager.sol +++ b/contracts/internal/host-chain/contracts/TaskManager.sol @@ -256,11 +256,11 @@ contract TaskManager is ITaskManager, Initializable, UUPSUpgradeable, AccessCont // sentinel, not a safe default. Seed the fail-closed value so a migrated proxy is safe by // construction rather than by whatever the deploy script gets around to setting. // - // 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. if (verifierSigner == address(0)) verifierSigner = address(1); if (decryptResultSigner == address(0)) decryptResultSigner = address(1); }