Add admin role to shared bridge (#727)

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;
     }
 

l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol

@@ -10,6 +10,13 @@ import {IL1ERC20Bridge} from "./IL1ERC20Bridge.sol";
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 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;
 }

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);
+    }
+}

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);

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 {