From ee8f723f58b0f5974cde29120a893c373cb40495 Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Wed, 21 Aug 2024 10:54:25 +0200 Subject: [PATCH 1/6] new shared bridge --- .../contracts/bridge/L1SharedBridge.sol | 38 ++++++++++++++++++- .../bridge/interfaces/IL1SharedBridge.sol | 15 ++++++++ .../L1SharedBridge/L1SharedBridgeAdmin.t.sol | 29 ++++++++++++++ .../L1SharedBridge/L1SharedBridgeFails.t.sol | 2 +- .../_L1SharedBridge_Shared.t.sol | 8 +++- 5 files changed, 89 insertions(+), 3 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..92cab62df 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,11 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _; } + 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 +150,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 +243,7 @@ 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 { + function initializeChainGovernance(uint256 _chainId, address _l2BridgeAddress) external onlyOwnerOrAdmin { 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..a7ab09dea --- /dev/null +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +import {L1SharedBridgeTest} from "./_L1SharedBridge_Shared.t.sol"; + +import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + +import {L1SharedBridge} from "contracts/bridge/L1SharedBridge.sol"; +import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; +import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; +import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; +import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; +import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; +import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; +import {IGetters} from "contracts/state-transition/chain-interfaces/IGetters.sol"; + +/// We are testing all the specified revert and require cases. +contract L1SharedBridgeAdminTest is L1SharedBridgeTest { + function testAdminCanInitializeChainGovernance() public { + uint256 randomChainId = 123456; + address randomL2Bridge = makeAddr("randomL2Bridge"); + + vm.prank(admin); + sharedBridge.initializeChainGovernance(randomChainId, randomL2Bridge); + + assertEq(sharedBridge.l2BridgeAddress(randomChainId), randomL2Bridge); + } +} 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..f5905e258 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) ); 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 { From 3f17b69c087a00d8c1b135f16a756d402baea7af Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Wed, 21 Aug 2024 10:56:10 +0200 Subject: [PATCH 2/6] remove unused imports --- .../Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol | 12 ------------ 1 file changed, 12 deletions(-) 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 index a7ab09dea..40454b628 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol @@ -3,18 +3,6 @@ pragma solidity 0.8.24; import {L1SharedBridgeTest} from "./_L1SharedBridge_Shared.t.sol"; -import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; -import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; - -import {L1SharedBridge} from "contracts/bridge/L1SharedBridge.sol"; -import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; -import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; -import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; -import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; -import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; -import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; -import {IGetters} from "contracts/state-transition/chain-interfaces/IGetters.sol"; - /// We are testing all the specified revert and require cases. contract L1SharedBridgeAdminTest is L1SharedBridgeTest { function testAdminCanInitializeChainGovernance() public { From 77bad840c19c71e6cb9c24db50cc4613c1368d5e Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Wed, 21 Aug 2024 10:56:27 +0200 Subject: [PATCH 3/6] fmt --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 92cab62df..fdbd70209 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -153,7 +153,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @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 { + 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 From 2cf5cc026a54f01cef509dcfa4c3c9361b4c04dd Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Wed, 21 Aug 2024 10:57:53 +0200 Subject: [PATCH 4/6] add a comment --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index fdbd70209..baf97f6a9 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -122,6 +122,7 @@ 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"); _; From b242e8686c57ebc51aaea86ed8624e904c328d3e Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Wed, 21 Aug 2024 12:22:08 +0200 Subject: [PATCH 5/6] add new method and tests for it --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 14 ++++++++++++++ .../L1SharedBridge/L1SharedBridgeAdmin.t.sol | 11 ++++++++++- .../L1SharedBridge/L1SharedBridgeFails.t.sol | 4 ++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index baf97f6a9..42058fe33 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -244,7 +244,21 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } /// @dev Initializes the l2Bridge address by governance for a specific chain. + /// @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/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol index 40454b628..28472440c 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol @@ -5,8 +5,9 @@ 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 { - uint256 randomChainId = 123456; address randomL2Bridge = makeAddr("randomL2Bridge"); vm.prank(admin); @@ -14,4 +15,12 @@ contract L1SharedBridgeAdminTest is L1SharedBridgeTest { 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 f5905e258..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 @@ -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); From 881e61055c206d62a6dd58be58fefd803fc8a478 Mon Sep 17 00:00:00 2001 From: Stanislav Breadless Date: Wed, 21 Aug 2024 12:30:19 +0200 Subject: [PATCH 6/6] lint --- .../concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 28472440c..af97e3ed2 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeAdmin.t.sol @@ -6,7 +6,7 @@ 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");