From 3420a3103733e04ad9b152c7ed284acef5230b19 Mon Sep 17 00:00:00 2001 From: ksatyarth2 Date: Thu, 7 Nov 2024 14:13:23 +0530 Subject: [PATCH 1/5] feat: wip- acl cleanup script and other cleanup --- l2-contracts/test/unit/L2RewardManager.t.sol | 2 - .../07_AccessManagerCleanup.s.sol | 111 ++++++++++++++++++ mainnet-contracts/script/DeployerHelper.s.sol | 9 ++ mainnet-contracts/script/Roles.sol | 11 +- mainnet-contracts/script/UpgradePufETH.s.sol | 2 +- .../ValidatorTicketMainnetTest.fork.t.sol | 4 +- .../test/helpers/UnitTestHelper.sol | 7 +- .../test/unit/ValidatorTicket.t.sol | 2 +- 8 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol diff --git a/l2-contracts/test/unit/L2RewardManager.t.sol b/l2-contracts/test/unit/L2RewardManager.t.sol index 3aa043a1..ded10c5e 100644 --- a/l2-contracts/test/unit/L2RewardManager.t.sol +++ b/l2-contracts/test/unit/L2RewardManager.t.sol @@ -19,7 +19,6 @@ import { ROLE_ID_BRIDGE, PUBLIC_ROLE, ROLE_ID_DAO, - ROLE_ID_REWARD_WATCHER, ROLE_ID_OPERATIONS_PAYMASTER } from "mainnet-contracts/script/Roles.sol"; import { XERC20Lockbox } from "mainnet-contracts/src/XERC20Lockbox.sol"; @@ -162,7 +161,6 @@ contract L2RewardManagerTest is Test { (s,) = address(accessManager).call(cd); require(s, "failed access manager 2"); - accessManager.grantRole(ROLE_ID_REWARD_WATCHER, address(this), 0); accessManager.grantRole(ROLE_ID_DAO, address(this), 0); accessManager.grantRole(ROLE_ID_OPERATIONS_PAYMASTER, address(this), 0); diff --git a/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol b/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol new file mode 100644 index 00000000..fd6944f2 --- /dev/null +++ b/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity >=0.8.0 <0.9.0; + +// - Guardian role 88 is unused +// - Puffer Oracle role 999 is unused +// - Reward Watcher 21 role is unused +// - Conflicting role ID 25::Granted VT pricer role 25 to MergedAdapterWithoutRoundsPufStakingV1 proxy - 0xf9dfbf71f2d9c8a4e565e1346aeb2c3e1dc765de +// - Currently Active on ValidatorTicketPricer - 0x9830ad1bd5cf73640e253edf97dee3791c4a53c3 + +// - Add PufferOracleV2 - 0x8eFd1Dc43AD073232F3e2924e22F173879119489 to ACL? +// - There is another PufferOracle 0x0be2ae0edbebb517541df217ef0074fc9a9e994f also +// - PufferOracleV2 has PufferProtocol 1234 role for few functions like provisionNode and exitValidators +// - Granted function role on v2 on setMintPrice + +// - DUPLICATE OperationsCoordinator 0xe6d798165a32f37927af20d7ccb1f80fb731a3c0 +// - Original OperationsCoordinator- 0x3fee92765f5cf8f9909a3c89f4907ea5e1cd9bf7 +// - Granted Operations Coordinator 24 role to duplicate one +// - Granted function role setValidatorTicketMintPrice on duplicate one + +// - Upgrader role 1 is granted to 0x0000000000000000000000000000000000000000 contract for upgradeToAndCall(address,bytes) function +// - Lockbox no address on L1? + +// TODO: + +// create a PR to do cleanup & remove unused roles from the code +// DONE - fix the conflict for role 25 by creating a new role +// DONE - create a migration script like 06_AccessManagerCleanup.s.sol +// DONE - that script should revoke any roles from the contracts that we don't use anymore (oracle, operations coordinator) + +// DONE - ROLE_ID_LOCKBOX seems unused as well. +// on xPufETH there is `setLockbox` which is restricted to DAO, we don't need this role anymore. + +// Upgrader role is useless as well, we should revoke it + +import { Script } from "forge-std/Script.sol"; +import { AccessManager } from "@openzeppelin/contracts/access/manager/AccessManager.sol"; +import { Multicall } from "@openzeppelin/contracts/utils/Multicall.sol"; +import { ROLE_ID_OPERATIONS_COORDINATOR } from "../../script/Roles.sol"; +import { DeployerHelper } from "../../script/DeployerHelper.s.sol"; +import { console } from "forge-std/console.sol"; +import { PufferWithdrawalManager } from "../../src/PufferWithdrawalManager.sol"; +import { ROLE_ID_VT_PRICER, ROLE_ID_WITHDRAWAL_FINALIZER } from "../../script/Roles.sol"; + +contract AccessManagerCleanup is DeployerHelper { + function run() public view { + address duplicateOperationsCoordinator = 0xe6d798165A32F37927AF20D7Ccb1f80fB731a3C0; + address withdrawalManagerProxy = 0xDdA0483184E75a5579ef9635ED14BacCf9d50283; + + address paymaster = _getPaymaster(); + address withdrawalFinalizer = _getOPSMultisig(); + address communityMultisig = _getCommunityMultisig(); + + uint64 ROLE_ID_UPGRADER = 1; + + // ------------ 1. Revoke existing ROLE_ID_WITHDRAWAL_FINALIZER roles and re-grant as we have changed the role ID which was 25 earlier and now 27 ------------ + bytes[] memory calldatas = new bytes[](9); + + // 1.1 set target role for finalizeWithdrawals + bytes4[] memory paymasterSelectors = new bytes4[](1); + paymasterSelectors[0] = PufferWithdrawalManager.finalizeWithdrawals.selector; + + calldatas[0] = abi.encodeWithSelector( + AccessManager.setTargetFunctionRole.selector, + withdrawalManagerProxy, + paymasterSelectors, + ROLE_ID_WITHDRAWAL_FINALIZER + ); + // 1.2 revoke existing roles + calldatas[1] = + abi.encodeWithSelector(AccessManager.revokeRole.selector, ROLE_ID_WITHDRAWAL_FINALIZER, paymaster); + calldatas[2] = + abi.encodeWithSelector(AccessManager.revokeRole.selector, ROLE_ID_WITHDRAWAL_FINALIZER, withdrawalFinalizer); + + // 1.3 grant finalizer roles to paymaster and withdrawalFinalizer addresses + calldatas[3] = + abi.encodeWithSelector(AccessManager.grantRole.selector, ROLE_ID_WITHDRAWAL_FINALIZER, paymaster, 0); + + calldatas[4] = abi.encodeWithSelector( + AccessManager.grantRole.selector, ROLE_ID_WITHDRAWAL_FINALIZER, withdrawalFinalizer, 0 + ); + + // 1.4 label role + calldatas[5] = abi.encodeWithSelector( + AccessManager.labelRole.selector, ROLE_ID_WITHDRAWAL_FINALIZER, "Withdrawal Finalizer" + ); + + // 1.5 label role 25 as VT pricer + calldatas[6] = + abi.encodeWithSelector(AccessManager.labelRole.selector, ROLE_ID_VT_PRICER, "Validator Ticket Pricer"); + // ------------------- + + // 2. Revoke Operations Coordinator role from duplicate contract + // 2.1 revoke role from duplicate contract + calldatas[7] = abi.encodeWithSelector( + AccessManager.revokeRole.selector, ROLE_ID_OPERATIONS_COORDINATOR, duplicateOperationsCoordinator + ); + // NOTE: this duplicate contract has setValidatorTicketMintPrice function which has been granted Operations Paymaster (ID: 23) role + + // 3. Revoke PufferOracle role from PufferOracleV2 + // NOTE: PufferOracleV2 has PufferProtocol 1234 role for few functions like provisionNode and exitValidators, so we can skip it, or can change it some unaccessible role? + + // 4. revoke uint64 constant ROLE_ID_UPGRADER = 1; from Community Multisig 0x446d4d6b26815f9ba78b5d454e303315d586cb2a + calldatas[8] = abi.encodeWithSelector(AccessManager.revokeRole.selector, ROLE_ID_UPGRADER, communityMultisig); + + bytes memory encodedMulticall = abi.encodeCall(Multicall.multicall, (calldatas)); + + console.log("Timelock -> AccessManager"); + console.log("AccessManagerCleanup calldatas:"); + console.logBytes(encodedMulticall); + } +} diff --git a/mainnet-contracts/script/DeployerHelper.s.sol b/mainnet-contracts/script/DeployerHelper.s.sol index d8c847eb..d0189a4b 100644 --- a/mainnet-contracts/script/DeployerHelper.s.sol +++ b/mainnet-contracts/script/DeployerHelper.s.sol @@ -433,6 +433,15 @@ abstract contract DeployerHelper is Script { revert("OPSMultisig not available for this chain"); } + function _getCommunityMultisig() internal view returns (address) { + if (block.chainid == mainnet) { + // https://etherscan.io/address/0x446d4d6b26815f9bA78B5D454E303315D586Cb2a + return 0x446d4d6b26815f9bA78B5D454E303315D586Cb2a; + } + + revert("CommunityMultisig not available for this chain"); + } + function _getAeraVault() internal view returns (address) { if (block.chainid == mainnet) { // https://etherscan.io/address/0x6c25aE178aC3466A63A552d4D6509c3d7385A0b8 diff --git a/mainnet-contracts/script/Roles.sol b/mainnet-contracts/script/Roles.sol index f969b0b8..326a972f 100644 --- a/mainnet-contracts/script/Roles.sol +++ b/mainnet-contracts/script/Roles.sol @@ -7,15 +7,15 @@ pragma solidity >=0.8.0 <0.9.0; uint64 constant ROLE_ID_UPGRADER = 1; uint64 constant ROLE_ID_L1_REWARD_MANAGER = 20; -uint64 constant ROLE_ID_REWARD_WATCHER = 21; + uint64 constant ROLE_ID_OPERATIONS_MULTISIG = 22; uint64 constant ROLE_ID_OPERATIONS_PAYMASTER = 23; uint64 constant ROLE_ID_OPERATIONS_COORDINATOR = 24; -uint64 constant ROLE_ID_WITHDRAWAL_FINALIZER = 25; -uint64 constant ROLE_ID_REVENUE_DEPOSITOR = 26; // Role assigned to validator ticket price setter uint64 constant ROLE_ID_VT_PRICER = 25; +uint64 constant ROLE_ID_REVENUE_DEPOSITOR = 26; +uint64 constant ROLE_ID_WITHDRAWAL_FINALIZER = 27; // Role assigned to the Puffer Protocol uint64 constant ROLE_ID_PUFFER_PROTOCOL = 1234; @@ -23,8 +23,6 @@ uint64 constant ROLE_ID_VAULT_WITHDRAWER = 1235; uint64 constant ROLE_ID_PUFETH_BURNER = 1236; uint64 constant ROLE_ID_DAO = 77; -uint64 constant ROLE_ID_GUARDIANS = 88; -uint64 constant ROLE_ID_PUFFER_ORACLE = 999; // Public role (defined in AccessManager.sol) uint64 constant PUBLIC_ROLE = type(uint64).max; @@ -34,8 +32,5 @@ uint64 constant ADMIN_ROLE = 0; // Allowlister role for AVSContractsRegistry uint64 constant ROLE_ID_AVS_COORDINATOR_ALLOWLISTER = 5; -// Lockbox role for ETH Mainnet -uint64 constant ROLE_ID_LOCKBOX = 7; - // Bridge role for L2RewardManager uint64 constant ROLE_ID_BRIDGE = 8; diff --git a/mainnet-contracts/script/UpgradePufETH.s.sol b/mainnet-contracts/script/UpgradePufETH.s.sol index f07ddbcd..a2c2cc6a 100644 --- a/mainnet-contracts/script/UpgradePufETH.s.sol +++ b/mainnet-contracts/script/UpgradePufETH.s.sol @@ -54,7 +54,7 @@ contract UpgradePufETH is BaseScript { function run( PufferDeployment memory deployment, - BridgingDeployment memory bridgingDeployment, + BridgingDeployment memory, address pufferOracle, address revenueDepositor ) public broadcast { diff --git a/mainnet-contracts/test/fork-tests/ValidatorTicketMainnetTest.fork.t.sol b/mainnet-contracts/test/fork-tests/ValidatorTicketMainnetTest.fork.t.sol index 059aafbe..41fd8d77 100644 --- a/mainnet-contracts/test/fork-tests/ValidatorTicketMainnetTest.fork.t.sol +++ b/mainnet-contracts/test/fork-tests/ValidatorTicketMainnetTest.fork.t.sol @@ -49,7 +49,7 @@ contract ValidatorTicketMainnetTest is MainnetForkTestHelper { vm.stopPrank(); } - function test_initial_state() public { + function test_initial_state() public view { assertEq(validatorTicket.name(), "Puffer Validator Ticket"); assertEq(validatorTicket.symbol(), "VT"); assertEq(validatorTicket.getProtocolFeeRate(), INITIAL_PROTOCOL_FEE); @@ -184,7 +184,7 @@ contract ValidatorTicketMainnetTest is MainnetForkTestHelper { uint256 initialTreasuryBalance, uint256 initialGuardianBalance, uint256 initialVaultBalance - ) internal { + ) internal view { address treasury = validatorTicket.TREASURY(); address guardianModule = validatorTicket.GUARDIAN_MODULE(); address vault = validatorTicket.PUFFER_VAULT(); diff --git a/mainnet-contracts/test/helpers/UnitTestHelper.sol b/mainnet-contracts/test/helpers/UnitTestHelper.sol index 6442a7c6..524fed86 100644 --- a/mainnet-contracts/test/helpers/UnitTestHelper.sol +++ b/mainnet-contracts/test/helpers/UnitTestHelper.sol @@ -32,12 +32,7 @@ import { L1RewardManager } from "src/L1RewardManager.sol"; import { PufferRevenueDepositor } from "src/PufferRevenueDepositor.sol"; import { L2RewardManager } from "l2-contracts/src/L2RewardManager.sol"; import { ConnextMock } from "../mocks/ConnextMock.sol"; -import { - ROLE_ID_DAO, - ROLE_ID_OPERATIONS_PAYMASTER, - ROLE_ID_OPERATIONS_MULTISIG, - ROLE_ID_LOCKBOX -} from "../../script/Roles.sol"; +import { ROLE_ID_DAO, ROLE_ID_OPERATIONS_PAYMASTER, ROLE_ID_OPERATIONS_MULTISIG } from "../../script/Roles.sol"; contract UnitTestHelper is Test, BaseScript { bytes32 private constant _PERMIT_TYPEHASH = diff --git a/mainnet-contracts/test/unit/ValidatorTicket.t.sol b/mainnet-contracts/test/unit/ValidatorTicket.t.sol index 4f9447e7..1e0d2013 100644 --- a/mainnet-contracts/test/unit/ValidatorTicket.t.sol +++ b/mainnet-contracts/test/unit/ValidatorTicket.t.sol @@ -243,7 +243,7 @@ contract ValidatorTicketTest is UnitTestHelper { deal(address(pufferVault), recipient, pufEthAmount); } - function _signPermit(bytes32 structHash, bytes32 domainSeparator) internal view returns (Permit memory permit) { + function _signPermit(bytes32, bytes32) internal view returns (Permit memory permit) { // TODO: Implement signing logic here permit = Permit({ amount: 10 ether, deadline: block.timestamp + 1 hours, v: 27, r: bytes32(0), s: bytes32(0) }); } From 8904af52cc9059d8090be9280c46fea76f288cdf Mon Sep 17 00:00:00 2001 From: ksatyarth2 Date: Thu, 7 Nov 2024 08:44:24 +0000 Subject: [PATCH 2/5] forge fmt --- l2-contracts/test/unit/L2RewardManager.t.sol | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/l2-contracts/test/unit/L2RewardManager.t.sol b/l2-contracts/test/unit/L2RewardManager.t.sol index ded10c5e..bb0a54b9 100644 --- a/l2-contracts/test/unit/L2RewardManager.t.sol +++ b/l2-contracts/test/unit/L2RewardManager.t.sol @@ -16,10 +16,7 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils import { BridgeMock } from "../mocks/BridgeMock.sol"; import { Merkle } from "murky/Merkle.sol"; import { - ROLE_ID_BRIDGE, - PUBLIC_ROLE, - ROLE_ID_DAO, - ROLE_ID_OPERATIONS_PAYMASTER + ROLE_ID_BRIDGE, PUBLIC_ROLE, ROLE_ID_DAO, ROLE_ID_OPERATIONS_PAYMASTER } from "mainnet-contracts/script/Roles.sol"; import { XERC20Lockbox } from "mainnet-contracts/src/XERC20Lockbox.sol"; import { xPufETH } from "mainnet-contracts/src/l2/xPufETH.sol"; From 1bf020873dcb72f0648f15b3397ebee973d33b5a Mon Sep 17 00:00:00 2001 From: ksatyarth2 Date: Thu, 17 Apr 2025 18:33:19 +0530 Subject: [PATCH 3/5] fix: replace new paymaster --- mainnet-contracts/script/DeployerHelper.s.sol | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/mainnet-contracts/script/DeployerHelper.s.sol b/mainnet-contracts/script/DeployerHelper.s.sol index 7451881a..b6bfd43a 100644 --- a/mainnet-contracts/script/DeployerHelper.s.sol +++ b/mainnet-contracts/script/DeployerHelper.s.sol @@ -488,6 +488,16 @@ abstract contract DeployerHelper is Script { } function _getPaymaster() internal view returns (address) { + if (block.chainid == mainnet) { + // https://etherscan.io/address/0xb4F1f4d59bd7590e92c386083Aa260C1e09cC58b + return 0xb4F1f4d59bd7590e92c386083Aa260C1e09cC58b; + } + + revert("Paymaster not available for this chain"); + } + // Keep this address for revoking old paymaster in future + + function _getDeprecatedPaymaster() internal view returns (address) { if (block.chainid == mainnet) { // https://etherscan.io/address/0x65d2dd7A66a2733a36559fE900A236280A05FBD6 return 0x65d2dd7A66a2733a36559fE900A236280A05FBD6; @@ -547,15 +557,6 @@ abstract contract DeployerHelper is Script { revert("OPSMultisig not available for this chain"); } - function _getCommunityMultisig() internal view returns (address) { - if (block.chainid == mainnet) { - // https://etherscan.io/address/0x446d4d6b26815f9bA78B5D454E303315D586Cb2a - return 0x446d4d6b26815f9bA78B5D454E303315D586Cb2a; - } - - revert("CommunityMultisig not available for this chain"); - } - function _getAeraVault() internal view returns (address) { if (block.chainid == mainnet) { // https://etherscan.io/address/0x6c25aE178aC3466A63A552d4D6509c3d7385A0b8 From 16ef9736f04b54d03d610e967a2c6c67e2a991e2 Mon Sep 17 00:00:00 2001 From: ksatyarth2 Date: Thu, 17 Apr 2025 18:36:02 +0530 Subject: [PATCH 4/5] fix: holesky paymaster --- mainnet-contracts/script/DeployerHelper.s.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mainnet-contracts/script/DeployerHelper.s.sol b/mainnet-contracts/script/DeployerHelper.s.sol index b6bfd43a..4d155321 100644 --- a/mainnet-contracts/script/DeployerHelper.s.sol +++ b/mainnet-contracts/script/DeployerHelper.s.sol @@ -491,6 +491,9 @@ abstract contract DeployerHelper is Script { if (block.chainid == mainnet) { // https://etherscan.io/address/0xb4F1f4d59bd7590e92c386083Aa260C1e09cC58b return 0xb4F1f4d59bd7590e92c386083Aa260C1e09cC58b; + } else if (block.chainid == holesky) { + // https://holesky.etherscan.io/address/0xDDDeAfB492752FC64220ddB3E7C9f1d5CcCdFdF0 + return 0xDDDeAfB492752FC64220ddB3E7C9f1d5CcCdFdF0; } revert("Paymaster not available for this chain"); From 54b3fadfeeffcd60f471a1e13c44b237b2204fe0 Mon Sep 17 00:00:00 2001 From: ksatyarth2 Date: Thu, 17 Apr 2025 19:53:10 +0530 Subject: [PATCH 5/5] chore: no hardcoded addy --- .../07_AccessManagerCleanup.s.sol | 2 +- mainnet-contracts/script/DeployerHelper.s.sol | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol b/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol index fd6944f2..cc97a4cd 100644 --- a/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol +++ b/mainnet-contracts/script/AccessManagerMigrations/07_AccessManagerCleanup.s.sol @@ -44,7 +44,7 @@ import { ROLE_ID_VT_PRICER, ROLE_ID_WITHDRAWAL_FINALIZER } from "../../script/Ro contract AccessManagerCleanup is DeployerHelper { function run() public view { address duplicateOperationsCoordinator = 0xe6d798165A32F37927AF20D7Ccb1f80fB731a3C0; - address withdrawalManagerProxy = 0xDdA0483184E75a5579ef9635ED14BacCf9d50283; + address withdrawalManagerProxy = _getPufferWithdrawalManager(); address paymaster = _getPaymaster(); address withdrawalFinalizer = _getOPSMultisig(); diff --git a/mainnet-contracts/script/DeployerHelper.s.sol b/mainnet-contracts/script/DeployerHelper.s.sol index 4d155321..ac7faaf1 100644 --- a/mainnet-contracts/script/DeployerHelper.s.sol +++ b/mainnet-contracts/script/DeployerHelper.s.sol @@ -595,4 +595,13 @@ abstract contract DeployerHelper is Script { revert("RevenueDepositor not available for this chain"); } + + function _getPufferWithdrawalManager() internal view returns (address) { + if (block.chainid == mainnet) { + // https://etherscan.io/address/0xDdA0483184E75a5579ef9635ED14BacCf9d50283 + return 0xDdA0483184E75a5579ef9635ED14BacCf9d50283; + } + + revert("PufferWithdrawalManager not available for this chain"); + } }