From 446d391d34bdb48255d5f8fef8a8248925fc98b9 Mon Sep 17 00:00:00 2001 From: Stanislav Bezkorovainyi Date: Wed, 21 Aug 2024 18:43:27 +0200 Subject: [PATCH] Add admin role to shared bridge (#727) --- .../contracts/bridge/L1SharedBridge.sol | 53 ++++++++++++++++++- .../bridge/interfaces/IL1SharedBridge.sol | 15 ++++++ .../L1SharedBridge/L1SharedBridgeAdmin.t.sol | 26 +++++++++ .../L1SharedBridge/L1SharedBridgeFails.t.sol | 6 +-- .../_L1SharedBridge_Shared.t.sol | 8 ++- 5 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index f74a771b0..42058fe33 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -89,6 +89,12 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// NOTE: this function may be removed in the future, don't rely on it! mapping(uint256 chainId => mapping(address l1Token => uint256 balance)) public chainBalance; + /// @dev Admin has the ability to register new chains within the shared bridge. + address public admin; + + /// @dev The pending admin, i.e. the candidate to the admin role. + address public pendingAdmin; + /// @notice Checks that the message sender is the bridgehub. modifier onlyBridgehub() { require(msg.sender == address(BRIDGE_HUB), "ShB not BH"); @@ -116,6 +122,12 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _; } + /// @notice Checks that the message sender is either the owner or admin. + modifier onlyOwnerOrAdmin() { + require(msg.sender == owner() || msg.sender == admin, "ShB not owner or admin"); + _; + } + /// @dev Contract is expected to be used as proxy implementation. /// @dev Initialize the implementation to prevent Parity hack. constructor( @@ -139,6 +151,31 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _transferOwnership(_owner); } + /// @inheritdoc IL1SharedBridge + /// @dev Please note, if the owner wants to enforce the admin change it must execute both `setPendingAdmin` and + /// `acceptAdmin` atomically. Otherwise `admin` can set different pending admin and so fail to accept the admin rights. + function setPendingAdmin(address _newPendingAdmin) external onlyOwnerOrAdmin { + // Save previous value into the stack to put it into the event later + address oldPendingAdmin = pendingAdmin; + // Change pending admin + pendingAdmin = _newPendingAdmin; + emit NewPendingAdmin(oldPendingAdmin, _newPendingAdmin); + } + + /// @inheritdoc IL1SharedBridge + /// @notice Accepts transfer of admin rights. Only pending admin can accept the role. + function acceptAdmin() external { + address currentPendingAdmin = pendingAdmin; + require(msg.sender == currentPendingAdmin, "ShB not pending admin"); // Only proposed by current admin address can claim the admin rights + + address previousAdmin = admin; + admin = currentPendingAdmin; + delete pendingAdmin; + + emit NewPendingAdmin(currentPendingAdmin, address(0)); + emit NewAdmin(previousAdmin, currentPendingAdmin); + } + /// @dev This sets the first post diamond upgrade batch for era, used to check old eth withdrawals /// @param _eraPostDiamondUpgradeFirstBatch The first batch number on the zkSync Era Diamond Proxy that was settled after diamond proxy upgrade. function setEraPostDiamondUpgradeFirstBatch(uint256 _eraPostDiamondUpgradeFirstBatch) external onlyOwner { @@ -207,7 +244,21 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } /// @dev Initializes the l2Bridge address by governance for a specific chain. - function initializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwner { + /// @param _chainId The chain ID for which the l2Bridge address is being initialized. + /// @param _l2BridgeAddress The address of the L2 bridge contract. + function initializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwnerOrAdmin { + require(l2BridgeAddress[_chainId] == address(0), "ShB: l2 bridge already set"); + require(_l2BridgeAddress != address(0), "ShB: l2 bridge 0"); + l2BridgeAddress[_chainId] = _l2BridgeAddress; + } + + /// @dev Reinitializes the l2Bridge address by governance for a specific chain. + /// @dev Only accessible to the owner of the bridge to prevent malicious admin from changing the bridge address for + /// an existing chain. + /// @param _chainId The chain ID for which the l2Bridge address is being initialized. + /// @param _l2BridgeAddress The address of the L2 bridge contract. + function reinitializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwner { + require(l2BridgeAddress[_chainId] != address(0), "ShB: l2 bridge not yet set"); l2BridgeAddress[_chainId] = _l2BridgeAddress; } diff --git a/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol b/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol index 45db2d02a..ef7e74165 100644 --- a/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol @@ -10,6 +10,13 @@ import {IL1ERC20Bridge} from "./IL1ERC20Bridge.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev interface IL1SharedBridge { + /// @notice pendingAdmin is changed + /// @dev Also emitted when new admin is accepted and in this case, `newPendingAdmin` would be zero address + event NewPendingAdmin(address indexed oldPendingAdmin, address indexed newPendingAdmin); + + /// @notice Admin changed + event NewAdmin(address indexed oldAdmin, address indexed newAdmin); + event LegacyDepositInitiated( uint256 indexed chainId, bytes32 indexed l2DepositTxHash, @@ -151,4 +158,12 @@ interface IL1SharedBridge { function bridgehubConfirmL2Transaction(uint256 _chainId, bytes32 _txDataHash, bytes32 _txHash) external; function receiveEth(uint256 _chainId) external payable; + + /// @notice Starts the transfer of admin rights. Only the current admin can propose a new pending one. + /// @notice New admin can accept admin rights by calling `acceptAdmin` function. + /// @param _newPendingAdmin Address of the new admin + function setPendingAdmin(address _newPendingAdmin) external; + + /// @notice Accepts transfer of admin rights. Only pending admin can accept the role. + function acceptAdmin() external; } diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol new file mode 100644 index 000000000..af97e3ed2 --- /dev/null +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +import {L1SharedBridgeTest} from "./_L1SharedBridge_Shared.t.sol"; + +/// We are testing all the specified revert and require cases. +contract L1SharedBridgeAdminTest is L1SharedBridgeTest { + uint256 internal randomChainId = 123456; + + function testAdminCanInitializeChainGovernance() public { + address randomL2Bridge = makeAddr("randomL2Bridge"); + + vm.prank(admin); + sharedBridge.initializeChainGovernance(randomChainId, randomL2Bridge); + + assertEq(sharedBridge.l2BridgeAddress(randomChainId), randomL2Bridge); + } + + function testAdminCanNotReinitializeChainGovernance() public { + address randomNewBridge = makeAddr("randomNewBridge"); + + vm.expectRevert("Ownable: caller is not the owner"); + vm.prank(admin); + sharedBridge.reinitializeChainGovernance(randomChainId, randomNewBridge); + } +} diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol index 15ef94080..97bbe2ec2 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol @@ -21,7 +21,7 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { vm.expectRevert("ShB owner 0"); new TransparentUpgradeableProxy( address(sharedBridgeImpl), - admin, + proxyAdmin, // solhint-disable-next-line func-named-parameters abi.encodeWithSelector(L1SharedBridge.initialize.selector, address(0), eraPostUpgradeFirstBatch) ); @@ -59,7 +59,7 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { function test_bridgehubDeposit_Eth_l2BridgeNotDeployed() public { vm.prank(owner); - sharedBridge.initializeChainGovernance(chainId, address(0)); + sharedBridge.reinitializeChainGovernance(chainId, address(0)); vm.deal(bridgehubAddress, amount); vm.prank(bridgehubAddress); vm.mockCall( @@ -551,7 +551,7 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { address refundRecipient = address(0); vm.prank(owner); - sharedBridge.initializeChainGovernance(eraChainId, address(0)); + sharedBridge.reinitializeChainGovernance(eraChainId, address(0)); vm.expectRevert("ShB b. n dep"); vm.prank(l1ERC20BridgeAddress); diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol index cad8482fb..4554e4a36 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol @@ -69,6 +69,7 @@ contract L1SharedBridgeTest is Test { address owner; address admin; + address proxyAdmin; address zkSync; address alice; address bob; @@ -90,6 +91,7 @@ contract L1SharedBridgeTest is Test { function setUp() public { owner = makeAddr("owner"); admin = makeAddr("admin"); + proxyAdmin = makeAddr("proxyAdmin"); // zkSync = makeAddr("zkSync"); bridgehubAddress = makeAddr("bridgehub"); alice = makeAddr("alice"); @@ -119,7 +121,7 @@ contract L1SharedBridgeTest is Test { }); TransparentUpgradeableProxy sharedBridgeProxy = new TransparentUpgradeableProxy( address(sharedBridgeImpl), - admin, + proxyAdmin, abi.encodeWithSelector(L1SharedBridge.initialize.selector, owner) ); sharedBridge = L1SharedBridge(payable(sharedBridgeProxy)); @@ -135,6 +137,10 @@ contract L1SharedBridgeTest is Test { sharedBridge.initializeChainGovernance(chainId, l2SharedBridge); vm.prank(owner); sharedBridge.initializeChainGovernance(eraChainId, l2SharedBridge); + vm.prank(owner); + sharedBridge.setPendingAdmin(admin); + vm.prank(admin); + sharedBridge.acceptAdmin(); } function _setSharedBridgeDepositHappened(uint256 _chainId, bytes32 _txHash, bytes32 _txDataHash) internal {