From 80171e783dad29c3c158a4c3d77a1b808a8e05b8 Mon Sep 17 00:00:00 2001 From: Szooot Date: Thu, 19 Mar 2026 11:36:22 +0100 Subject: [PATCH] fix: create function admin parameter deleted --- abis/ValidatorFactory.json | 63 +++++++++++++++++++++-------- src/ValidatorFactory.sol | 73 ++++++++++++++++++++++++++++++---- test/ValidatorFactory.t.sol | 79 +++++++++++++++++++++++++++++++------ 3 files changed, 181 insertions(+), 34 deletions(-) diff --git a/abis/ValidatorFactory.json b/abis/ValidatorFactory.json index f371bd9..b4a515a 100644 --- a/abis/ValidatorFactory.json +++ b/abis/ValidatorFactory.json @@ -42,11 +42,6 @@ "type": "function", "name": "create", "inputs": [ - { - "name": "admin", - "type": "address", - "internalType": "address" - }, { "name": "dealId", "type": "uint256", @@ -112,18 +107,18 @@ "name": "grantRole", "inputs": [ { - "name": "role", + "name": "", "type": "bytes32", "internalType": "bytes32" }, { - "name": "account", + "name": "", "type": "address", "internalType": "address" } ], "outputs": [], - "stateMutability": "nonpayable" + "stateMutability": "pure" }, { "type": "function", @@ -242,30 +237,43 @@ "name": "renounceRole", "inputs": [ { - "name": "role", + "name": "", "type": "bytes32", "internalType": "bytes32" }, { - "name": "callerConfirmation", + "name": "", "type": "address", "internalType": "address" } ], "outputs": [], - "stateMutability": "nonpayable" + "stateMutability": "pure" }, { "type": "function", "name": "revokeRole", "inputs": [ { - "name": "role", + "name": "", "type": "bytes32", "internalType": "bytes32" }, { - "name": "account", + "name": "", + "type": "address", + "internalType": "address" + } + ], + "outputs": [], + "stateMutability": "pure" + }, + { + "type": "function", + "name": "setAdmin", + "inputs": [ + { + "name": "newAdmin", "type": "address", "internalType": "address" } @@ -310,6 +318,19 @@ "outputs": [], "stateMutability": "payable" }, + { + "type": "event", + "name": "AdminChanged", + "inputs": [ + { + "name": "newAdmin", + "type": "address", + "indexed": true, + "internalType": "address" + } + ], + "anonymous": false + }, { "type": "event", "name": "Initialized", @@ -534,11 +555,21 @@ "name": "InvalidFilecoinPayAddress", "inputs": [] }, + { + "type": "error", + "name": "InvalidImplementationAddress", + "inputs": [] + }, { "type": "error", "name": "InvalidInitialization", "inputs": [] }, + { + "type": "error", + "name": "InvalidNewAdminAddress", + "inputs": [] + }, { "type": "error", "name": "InvalidPoRepMarketAddress", @@ -556,17 +587,17 @@ }, { "type": "error", - "name": "InvalidSlcAddress", + "name": "InvalidSliScorerAddress", "inputs": [] }, { "type": "error", - "name": "InvalidSliScorerAddress", + "name": "NotInitializing", "inputs": [] }, { "type": "error", - "name": "NotInitializing", + "name": "RoleManagementDisabled", "inputs": [] }, { diff --git a/src/ValidatorFactory.sol b/src/ValidatorFactory.sol index a6f5d91..8ae1872 100644 --- a/src/ValidatorFactory.sol +++ b/src/ValidatorFactory.sol @@ -33,6 +33,7 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { address _poRepMarket; address _SPRegistry; address _beacon; + address _admin; } // keccak256(abi.encode(uint256(keccak256("porepmarket.storage.ValidatorFactoryStorage")) - 1)) & ~bytes32(uint256(0xff)) @@ -59,13 +60,15 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { error InstanceAlreadyExists(); error InvalidAdminAddress(); error InvalidClientAddress(); - error InvalidSlcAddress(); error InvalidPoRepMarketAddress(); error InvalidClientSmartContractAddress(); error InvalidFilecoinPayAddress(); error InvalidPoRepServiceAddress(); error InvalidSliScorerAddress(); error InvalidSPRegistryAddress(); + error InvalidImplementationAddress(); + error InvalidNewAdminAddress(); + error RoleManagementDisabled(); /** * @notice Emitted when a new proxy is successfully created @@ -74,6 +77,12 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { */ event ProxyCreated(address indexed proxy, uint256 indexed dealId); + /** + * @notice Emitted when the admin is changed + * @param newAdmin The address of the new admin + */ + event AdminChanged(address indexed newAdmin); + /** * @notice Initializes the contract * @dev Initializes the contract by setting a default admin role and a UUPS upgradeable role @@ -81,12 +90,20 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { * @param implementation The address of the implementation contract */ function initialize(address admin, address implementation) public initializer { + if (admin == address(0)) { + revert InvalidAdminAddress(); + } + if (implementation == address(0)) { + revert InvalidImplementationAddress(); + } + __AccessControl_init(); _grantRole(DEFAULT_ADMIN_ROLE, admin); _grantRole(UPGRADER_ROLE, admin); ValidatorFactoryStorage storage $ = s(); $._beacon = address(new UpgradeableBeacon(implementation, admin)); + $._admin = admin; } /** @@ -127,12 +144,9 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { * @notice Creates a new instance of an upgradeable contract. * @dev Uses BeaconProxy to create a new proxy instance, pointing to the Beacon for the logic contract. * @dev Reverts if an instance for the given dealId already exists. - * @param admin The address of the admin responsible for the contract. * @param dealId The dealId for which the proxy was created. */ - function create(address admin, uint256 dealId) external { - if (admin == address(0)) revert InvalidAdminAddress(); - + function create(uint256 dealId) external { ValidatorFactoryStorage storage $ = s(); if ($._instances[dealId] != address(0)) revert InstanceAlreadyExists(); @@ -146,7 +160,7 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { abi.encodeCall( Validator.initialize, ( - admin, + $._admin, $._poRepService, $._filecoinPay, $._sliScorer, @@ -159,7 +173,7 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { ) ); // forge-lint: disable-next-line(asm-keccak256) - bytes32 salt = keccak256(abi.encode(admin, dealId)); + bytes32 salt = keccak256(abi.encode($._admin, dealId)); address proxy = Create2.computeAddress(salt, keccak256(initCode), address(this)); $._instances[dealId] = proxy; $._isValidatorContract[proxy] = true; @@ -168,6 +182,51 @@ contract ValidatorFactory is UUPSUpgradeable, AccessControlUpgradeable { emit ProxyCreated(proxy, dealId); } + /** + * @notice Sets a new admin for the contract and revoke the role from the old admin + * @dev Only callable by the current admin. Reverts if the new admin address is the zero address. + * @param newAdmin The new admin address + */ + function setAdmin(address newAdmin) external onlyRole(DEFAULT_ADMIN_ROLE) { + if (newAdmin == address(0)) { + revert InvalidNewAdminAddress(); + } + ValidatorFactoryStorage storage $ = s(); + + _revokeRole(DEFAULT_ADMIN_ROLE, $._admin); + _grantRole(DEFAULT_ADMIN_ROLE, newAdmin); + $._admin = newAdmin; + + emit AdminChanged(newAdmin); + } + + // solhint-disable use-natspec + /** + * @notice Disabled role management functions + * @dev This contract has a fixed admin and does not allow for dynamic role management + */ + function grantRole(bytes32, address) public pure override { + revert RoleManagementDisabled(); + } + + /** + * @notice Disabled role management functions + * @dev This contract has a fixed admin and does not allow for dynamic role management + */ + function revokeRole(bytes32, address) public pure override { + revert RoleManagementDisabled(); + } + + /** + * @notice Disabled role management functions + * @dev This contract has a fixed admin and does not allow for dynamic role management + */ + function renounceRole(bytes32, address) public pure override { + revert RoleManagementDisabled(); + } + + // solhint-enable use-natspec + /** * @notice Checks if an address is a validator contract * @param contractAddress The address to check diff --git a/test/ValidatorFactory.t.sol b/test/ValidatorFactory.t.sol index 0f7255b..b7c49c6 100644 --- a/test/ValidatorFactory.t.sol +++ b/test/ValidatorFactory.t.sol @@ -83,25 +83,25 @@ contract ValidatorFactoryTest is Test { emit ValidatorFactory.ProxyCreated(expectedProxy, dealId); vm.prank(client); - factory.create(admin, dealId); + factory.create(dealId); assertTrue(factory.isValidatorContract(expectedProxy)); } function testDeployMarksProxyAsDeployed() public { address expectedProxy = computeProxyAddress(admin, dealId); vm.prank(client); - factory.create(admin, dealId); + factory.create(dealId); assertTrue(factory.getInstance(dealId) == expectedProxy); } function testDeployRevertsIfInstanceExists() public { vm.prank(client); - factory.create(admin, dealId); + factory.create(dealId); vm.expectRevert(abi.encodeWithSelector(ValidatorFactory.InstanceAlreadyExists.selector)); vm.prank(client); - factory.create(admin, dealId); + factory.create(dealId); } function computeProxyAddress(address admin_, uint256 dealId_) private view returns (address) { @@ -144,17 +144,11 @@ contract ValidatorFactoryTest is Test { assertFalse(factory.isValidatorContract(address(0))); } - function testShouldRevertWhenAdminAddressIsZero() public { - vm.expectRevert(abi.encodeWithSelector(ValidatorFactory.InvalidAdminAddress.selector)); - vm.prank(client); - factory.create(address(0), dealId); - } - function testShouldRevertWhenIncorrectClientAddress() public { address incorrectClient = vm.addr(999); vm.expectRevert(abi.encodeWithSelector(ValidatorFactory.InvalidClientAddress.selector)); vm.prank(incorrectClient); - factory.create(admin, dealId); + factory.create(dealId); } function testInitialize2RevertsWhenPoRepServiceIsZero() public { @@ -209,4 +203,67 @@ contract ValidatorFactoryTest is Test { vm.prank(client); f.initialize2(poRepService, filecoinPay, sliScorer, clientSmartContract, poRepMarket, spRegistry); } + + function testInitializeRevertsWhenAdminIsZero() public { + address zeroAddress = address(0); + address validImplementation = address(0x1234); + ValidatorFactory factoryImplementation = new ValidatorFactory(); + vm.expectRevert(ValidatorFactory.InvalidAdminAddress.selector); + factoryImplementation.initialize(zeroAddress, validImplementation); + } + + function testInitializeRevertsWhenImplementationIsZero() public { + address validAdmin = address(0x1234); + address zeroAddress = address(0); + ValidatorFactory factoryImplementation = new ValidatorFactory(); + vm.expectRevert(ValidatorFactory.InvalidImplementationAddress.selector); + factoryImplementation.initialize(validAdmin, zeroAddress); + } + + function testSetAdminRevertsWhenNewAdminIsZero() public { + Validator validatorImplementation = new Validator(); + ValidatorFactory factoryImplementation = new ValidatorFactory(); + address validAdmin = address(0xABCD); + + factoryImplementation.initialize(validAdmin, address(validatorImplementation)); + + vm.prank(validAdmin); + vm.expectRevert(ValidatorFactory.InvalidNewAdminAddress.selector); + factoryImplementation.setAdmin(address(0)); + } + + function testSetsNewAdminCorrectly() public { + Validator validatorImplementation = new Validator(); + ValidatorFactory factoryImplementation = new ValidatorFactory(); + address validAdmin = address(0xABCD); + address newValidAdmin = address(0xDCBA); + + factoryImplementation.initialize(validAdmin, address(validatorImplementation)); + + vm.prank(validAdmin); + factoryImplementation.setAdmin(newValidAdmin); + + assertTrue(factoryImplementation.hasRole(factoryImplementation.DEFAULT_ADMIN_ROLE(), newValidAdmin)); + } + + function testGrantRoleReverts() public { + bytes32 adminRole = factory.DEFAULT_ADMIN_ROLE(); + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(ValidatorFactory.RoleManagementDisabled.selector)); + factory.grantRole(adminRole, client); + } + + function testRevokeRoleReverts() public { + bytes32 adminRole = factory.DEFAULT_ADMIN_ROLE(); + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(ValidatorFactory.RoleManagementDisabled.selector)); + factory.revokeRole(adminRole, client); + } + + function testRenounceRoleReverts() public { + bytes32 adminRole = factory.DEFAULT_ADMIN_ROLE(); + vm.prank(admin); + vm.expectRevert(abi.encodeWithSelector(ValidatorFactory.RoleManagementDisabled.selector)); + factory.renounceRole(adminRole, admin); + } }