From dc65224b7e43ca6875b993571c2d88713a018563 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Mart=C3=ADn=20Alconada=20Verzini?= Date: Wed, 2 Oct 2024 09:04:37 +0100 Subject: [PATCH] chore: add fixes from reviews: rmv legacy code, modify interfaces --- .env.example | 1 - script/DeployL1SharedBridge.s.sol | 8 +- src/L1SharedBridge.sol | 127 +++--------------- src/L2SharedBridge.sol | 12 +- src/interfaces/IL1SharedBridge.sol | 102 ++++++++++++++ src/interfaces/IL2SharedBridge.sol | 2 - test/e2e/SharedBridgesTest.sol | 4 +- test/l1ShareBridge/L1SharedBridgeFails.t.sol | 5 - .../_L1SharedBridge_Shared.t.sol | 14 +- 9 files changed, 124 insertions(+), 151 deletions(-) diff --git a/.env.example b/.env.example index 0180ac6..3502fe8 100644 --- a/.env.example +++ b/.env.example @@ -11,7 +11,6 @@ SOPH_TOKEN=0x06c03F9319EBbd84065336240dcc243bda9D8896 L1_USDC_TOKEN=0xBF4FdF7BF4014EA78C0A07259FBc4315Cb10d94E # bridge addresses on ethereum sepolia -ERA_DIAMOND_PROXY=0x9A6DE0f62Aa270A8bCB1e2610078650D539B1Ef9 SEPOLIA_L1_BRIDGEHUB=0x35A54c8C757806eB6820629bc82d90E056394C92 SEPOLIA_SHARED_BRIDGE_L1=0x3E8b2fe58675126ed30d0d12dea2A9bda72D18Ae SEPOLIA_CUSTOM_SHARED_BRIDGE_L1=0x3f842b5FaD08Bac49D0517C975d393f5f466Fd3b diff --git a/script/DeployL1SharedBridge.s.sol b/script/DeployL1SharedBridge.s.sol index 8b08dc1..4b15ecd 100644 --- a/script/DeployL1SharedBridge.s.sol +++ b/script/DeployL1SharedBridge.s.sol @@ -31,12 +31,8 @@ contract DeployL1SharedBridge is Script { vm.startBroadcast(); - L1SharedBridge sharedBridgeImpl = new L1SharedBridge( - vm.envAddress("L1_USDC_TOKEN"), - IBridgehub(vm.envAddress("SEPOLIA_L1_BRIDGEHUB")), - vm.envUint("SOPHON_SEPOLIA_CHAIN_ID"), - vm.envAddress("ERA_DIAMOND_PROXY") - ); + L1SharedBridge sharedBridgeImpl = + new L1SharedBridge(vm.envAddress("L1_USDC_TOKEN"), IBridgehub(vm.envAddress("SEPOLIA_L1_BRIDGEHUB"))); TransparentUpgradeableProxy sharedBridgeProxy = new TransparentUpgradeableProxy( address(sharedBridgeImpl), diff --git a/src/L1SharedBridge.sol b/src/L1SharedBridge.sol index c4f714b..64a4d44 100644 --- a/src/L1SharedBridge.sol +++ b/src/L1SharedBridge.sol @@ -10,7 +10,7 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import {IL1ERC20Bridge} from "@era-contracts/l1-contracts/contracts/bridge/interfaces/IL1ERC20Bridge.sol"; -import {IL1SharedBridge} from "@era-contracts/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol"; +import {IL1SharedBridge} from "./interfaces/IL1SharedBridge.sol"; import {IL2Bridge} from "@era-contracts/l1-contracts/contracts/bridge/interfaces/IL2Bridge.sol"; import {IMailbox} from "@era-contracts/l1-contracts/contracts/state-transition/chain-interfaces/IMailbox.sol"; @@ -40,12 +40,6 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @dev Bridgehub smart contract that is used to operate with L2 via asynchronous L2 <-> L1 communication. IBridgehub public immutable override BRIDGE_HUB; - /// @dev Era's chainID - uint256 public immutable ERA_CHAIN_ID; - - /// @dev The address of zkSync Era diamond proxy contract. - address public immutable ERA_DIAMOND_PROXY; - /// @dev A mapping chainId => bridgeProxy. Used to store the bridge proxy's address, and to see if it has been deployed yet. mapping(uint256 chainId => address l2Bridge) public override l2BridgeAddress; @@ -80,15 +74,6 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _; } - /// @notice Checks that the message sender is the bridgehub or zkSync Era Diamond Proxy. - modifier onlyBridgehubOrEra(uint256 _chainId) { - require( - msg.sender == address(BRIDGE_HUB) || (_chainId == ERA_CHAIN_ID && msg.sender == ERA_DIAMOND_PROXY), - "L1SharedBridge: not bridgehub or era chain" - ); - _; - } - /// @notice Checks that the message sender is the shared bridge itself. modifier onlySelf() { require(msg.sender == address(this), "USDC-ShB not shared bridge"); @@ -103,14 +88,12 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @dev Contract is expected to be used as proxy implementation. /// @dev Initialize the implementation to prevent Parity hack. - constructor(address _l1UsdcAddress, IBridgehub _bridgehub, uint256 _eraChainId, address _eraDiamondProxy) + constructor(address _l1UsdcAddress, IBridgehub _bridgehub) reentrancyGuardInitializer { _disableInitializers(); L1_USDC_TOKEN = _l1UsdcAddress; BRIDGE_HUB = _bridgehub; - ERA_CHAIN_ID = _eraChainId; - ERA_DIAMOND_PROXY = _eraDiamondProxy; } /// @dev Initializes a contract bridge for later use. Expected to be used in the proxy @@ -196,7 +179,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade whenNotPaused returns (L2TransactionRequestTwoBridgesInner memory request) { - require(l2BridgeAddress[_chainId] != address(0), "USDC-ShB l2 bridge not deployed"); + address l2Bridge = l2BridgeAddress[_chainId]; + require(l2Bridge != address(0), "USDC-ShB l2 bridge not deployed"); (address _l1Token, uint256 _depositAmount, address _l2Receiver) = abi.decode(_data, (address, uint256, address)); require(_l1Token == L1_USDC_TOKEN, "USDC-ShB: Only USDC deposits supported"); @@ -212,18 +196,18 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade chainBalance[_chainId][_l1Token] += _depositAmount; } - { - // Request the finalization of the deposit on the L2 side - bytes memory l2TxCalldata = _getDepositL2Calldata(_prevMsgSender, _l2Receiver, _l1Token, _depositAmount); - - request = L2TransactionRequestTwoBridgesInner({ - magicValue: TWO_BRIDGES_MAGIC_VALUE, - l2Contract: l2BridgeAddress[_chainId], - l2Calldata: l2TxCalldata, - factoryDeps: new bytes[](0), - txDataHash: txDataHash - }); - } + // Request the finalization of the deposit on the L2 side + bytes memory l2TxCalldata = abi.encodeCall( + IL2Bridge.finalizeDeposit, (_prevMsgSender, _l2Receiver, _l1Token, _depositAmount, abi.encode("USD Coin", "USDC", 6)) + ); + + request = L2TransactionRequestTwoBridgesInner({ + magicValue: TWO_BRIDGES_MAGIC_VALUE, + l2Contract: l2Bridge, + l2Calldata: l2TxCalldata, + factoryDeps: new bytes[](0), + txDataHash: txDataHash + }); emit BridgehubDepositInitiated({ chainId: _chainId, @@ -248,20 +232,6 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade emit BridgehubDepositFinalized(_chainId, _txDataHash, _txHash); } - /// @dev Generate a calldata for calling the deposit finalization on the L2 bridge contract - function _getDepositL2Calldata(address _l1Sender, address _l2Receiver, address _l1Token, uint256 _amount) - internal - view - returns (bytes memory) - { - (, bytes memory data1) = _l1Token.staticcall(abi.encodeCall(IERC20Metadata.name, ())); - (, bytes memory data2) = _l1Token.staticcall(abi.encodeCall(IERC20Metadata.symbol, ())); - (, bytes memory data3) = _l1Token.staticcall(abi.encodeCall(IERC20Metadata.decimals, ())); - return abi.encodeCall( - IL2Bridge.finalizeDeposit, (_l1Sender, _l2Receiver, _l1Token, _amount, abi.encode(data1, data2, data3)) - ); - } - /// @dev Withdraw funds from the initiated deposit, that failed when finalizing on L2 /// @param _depositSender The address of the deposit initiator /// @param _l1Token The address of the deposited L1 ERC20 token @@ -460,71 +430,6 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } } - /*////////////////////////////////////////////////////////////// - UNUSED BUT REQUIRED BY INTERFACE - //////////////////////////////////////////////////////////////*/ - - /// @dev The address of the WETH token on L1. - address public immutable override L1_WETH_TOKEN; - - /// @dev Legacy bridge smart contract that used to hold ERC20 tokens. - IL1ERC20Bridge public override legacyBridge; // unused but interface requires it - - function setEraPostDiamondUpgradeFirstBatch(uint256) external pure { - return; - } - - function setEraPostLegacyBridgeUpgradeFirstBatch(uint256) external pure { - return; - } - - function setEraLegacyBridgeLastDepositTime(uint256, uint256) external pure { - return; - } - - function bridgehubDepositBaseToken(uint256, address, address, uint256) - external - payable - virtual - { - revert("NOT_IMPLEMENTED"); - } - - /*////////////////////////////////////////////////////////////// - ERA LEGACY FUNCTIONS - //////////////////////////////////////////////////////////////*/ - - function depositLegacyErc20Bridge(address, address, address, uint256, uint256, uint256, address) - external - payable - override - returns (bytes32) - { - revert("NOT_IMPLEMENTED"); - } - - function finalizeWithdrawalLegacyErc20Bridge(uint256, uint256, uint16, bytes calldata, bytes32[] calldata) - external - pure - override - returns (address, address, uint256) - { - revert("NOT_IMPLEMENTED"); - } - - function claimFailedDepositLegacyErc20Bridge( - address, - address, - uint256, - bytes32, - uint256, - uint256, - uint16, - bytes32[] calldata - ) external pure override { - revert("NOT_IMPLEMENTED"); - } - /*////////////////////////////////////////////////////////////// PAUSE //////////////////////////////////////////////////////////////*/ diff --git a/src/L2SharedBridge.sol b/src/L2SharedBridge.sol index c12d416..23d8af6 100644 --- a/src/L2SharedBridge.sol +++ b/src/L2SharedBridge.sol @@ -32,7 +32,6 @@ contract L2SharedBridge is IL2SharedBridge, Initializable { /// @dev Contract is expected to be used as proxy implementation. /// @dev Disable the initialization to prevent Parity hack. - // uint256 immutable ERA_CHAIN_ID; // TODO: check if i need this! /// @dev The address of the USDC token on L1. address public immutable L1_USDC_TOKEN; @@ -41,7 +40,6 @@ contract L2SharedBridge is IL2SharedBridge, Initializable { address public immutable L2_USDC_TOKEN; constructor(address _l1UsdcToken, address _l2UsdcToken) { - // ERA_CHAIN_ID = _eraChainId; // TODO: checke if we need this! L1_USDC_TOKEN = _l1UsdcToken; L2_USDC_TOKEN = _l2UsdcToken; _disableInitializers(); @@ -50,7 +48,7 @@ contract L2SharedBridge is IL2SharedBridge, Initializable { /// @notice Initializes the bridge contract for later use. Expected to be used in the proxy. /// @param _l1SharedBridge The address of the L1 Bridge contract. /// _l1Bridge The address of the legacy L1 Bridge contract. - function initialize(address _l1SharedBridge) external reinitializer(2) { + function initialize(address _l1SharedBridge) external reinitializer(1) { require(_l1SharedBridge != address(0), "bf"); l1SharedBridge = _l1SharedBridge; } @@ -99,14 +97,6 @@ contract L2SharedBridge is IL2SharedBridge, Initializable { return L2_USDC_TOKEN; } - /*////////////////////////////////////////////////////////////// - UNUSED BUT REQUIRED BY INTERFACE - //////////////////////////////////////////////////////////////*/ - - /// @dev The address of the legacy L1 erc20 bridge counterpart. - /// This is non-zero only on Era, and should not be renamed for backward compatibility with the SDKs. - address public override l1Bridge; - /*////////////////////////////////////////////////////////////// UTILS //////////////////////////////////////////////////////////////*/ diff --git a/src/interfaces/IL1SharedBridge.sol b/src/interfaces/IL1SharedBridge.sol index 21d0be2..6b090f6 100644 --- a/src/interfaces/IL1SharedBridge.sol +++ b/src/interfaces/IL1SharedBridge.sol @@ -2,10 +2,83 @@ pragma solidity 0.8.24; +import {IBridgehub, L2TransactionRequestTwoBridgesInner} from "@era-contracts/l1-contracts/contracts/bridgehub/IBridgehub.sol"; +import {IL1ERC20Bridge} from "./IL1ERC20Bridge.sol"; + /// @title L1 Bridge contract interface /// @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, + address indexed from, + address to, + address l1Token, + uint256 amount + ); + + event BridgehubDepositInitiated( + uint256 indexed chainId, + bytes32 indexed txDataHash, + address indexed from, + address to, + address l1Token, + uint256 amount + ); + + event BridgehubDepositBaseTokenInitiated( + uint256 indexed chainId, + address indexed from, + address l1Token, + uint256 amount + ); + + event BridgehubDepositFinalized( + uint256 indexed chainId, + bytes32 indexed txDataHash, + bytes32 indexed l2DepositTxHash + ); + + event WithdrawalFinalizedSharedBridge( + uint256 indexed chainId, + address indexed to, + address indexed l1Token, + uint256 amount + ); + + event ClaimedFailedDepositSharedBridge( + uint256 indexed chainId, + address indexed to, + address indexed l1Token, + uint256 amount + ); + + function isWithdrawalFinalized( + uint256 _chainId, + uint256 _l2BatchNumber, + uint256 _l2MessageIndex + ) external view returns (bool); + + function claimFailedDeposit( + uint256 _chainId, + address _depositSender, + address _l1Token, + uint256 _amount, + bytes32 _l2TxHash, + uint256 _l2BatchNumber, + uint256 _l2MessageIndex, + uint16 _l2TxNumberInBatch, + bytes32[] calldata _merkleProof + ) external; + function finalizeWithdrawal( uint256 _chainId, uint256 _l2BatchNumber, @@ -14,4 +87,33 @@ interface IL1SharedBridge { bytes calldata _message, bytes32[] calldata _merkleProof ) external; + + function BRIDGE_HUB() external view returns (IBridgehub); + + function l2BridgeAddress(uint256 _chainId) external view returns (address); + + function depositHappened(uint256 _chainId, bytes32 _l2TxHash) external view returns (bytes32); + + /// data is abi encoded : + /// address _l1Token, + /// uint256 _amount, + /// address _l2Receiver + function bridgehubDeposit( + uint256 _chainId, + address _prevMsgSender, + uint256 _l2Value, + bytes calldata _data + ) external payable returns (L2TransactionRequestTwoBridgesInner memory request); + + 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/src/interfaces/IL2SharedBridge.sol b/src/interfaces/IL2SharedBridge.sol index 9dd4365..6137485 100644 --- a/src/interfaces/IL2SharedBridge.sol +++ b/src/interfaces/IL2SharedBridge.sol @@ -26,7 +26,5 @@ interface IL2SharedBridge { function l2TokenAddress(address _l1Token) external view returns (address); - function l1Bridge() external view returns (address); - function l1SharedBridge() external view returns (address); } diff --git a/test/e2e/SharedBridgesTest.sol b/test/e2e/SharedBridgesTest.sol index 4d53119..d98b520 100644 --- a/test/e2e/SharedBridgesTest.sol +++ b/test/e2e/SharedBridgesTest.sol @@ -22,9 +22,7 @@ contract SharedBridgesTest is Test { // deploy implementation L1SharedBridge sharedBridgeImpl = new L1SharedBridge{salt: "1"}( vm.envAddress("L1_USDC_TOKEN"), - IBridgehub(vm.envAddress("SEPOLIA_L1_BRIDGEHUB")), // Sepolia L1 Bridgehub - vm.envUint("SOPHON_SEPOLIA_CHAIN_ID"), // Sophon chain ID - vm.envAddress("ERA_DIAMOND_PROXY") // Era Diamond Proxy); + IBridgehub(vm.envAddress("SEPOLIA_L1_BRIDGEHUB")) // Sepolia L1 Bridgehub ); // deploy proxy diff --git a/test/l1ShareBridge/L1SharedBridgeFails.t.sol b/test/l1ShareBridge/L1SharedBridgeFails.t.sol index 4a064d4..b99fb58 100644 --- a/test/l1ShareBridge/L1SharedBridgeFails.t.sol +++ b/test/l1ShareBridge/L1SharedBridgeFails.t.sol @@ -25,11 +25,6 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { ); } - function test_bridgehubDepositBaseToken_NotImplemented() public { - vm.expectRevert("NOT_IMPLEMENTED"); - sharedBridge.bridgehubDepositBaseToken(chainId, alice, address(token), 0); - } - function test_bridgehubDeposit_unsupportedErc() public { vm.prank(bridgehubAddress); vm.expectRevert("USDC-ShB: Only USDC deposits supported"); diff --git a/test/l1ShareBridge/_L1SharedBridge_Shared.t.sol b/test/l1ShareBridge/_L1SharedBridge_Shared.t.sol index 016a537..766aec0 100644 --- a/test/l1ShareBridge/_L1SharedBridge_Shared.t.sol +++ b/test/l1ShareBridge/_L1SharedBridge_Shared.t.sol @@ -102,23 +102,13 @@ contract L1SharedBridgeTest is Test { eraErc20BridgeAddress = makeAddr("eraErc20BridgeAddress"); token = new TestnetERC20Token("TestnetERC20Token", "TET", 18); - sharedBridgeImpl = new L1SharedBridge({ - _l1UsdcAddress: address(token), - _bridgehub: IBridgehub(bridgehubAddress), - _eraChainId: eraChainId, - _eraDiamondProxy: eraDiamondProxy - }); + sharedBridgeImpl = + new L1SharedBridge({_l1UsdcAddress: address(token), _bridgehub: IBridgehub(bridgehubAddress)}); TransparentUpgradeableProxy sharedBridgeProxy = new TransparentUpgradeableProxy( address(sharedBridgeImpl), proxyAdmin, abi.encodeWithSelector(L1SharedBridge.initialize.selector, owner) ); sharedBridge = L1SharedBridge(payable(sharedBridgeProxy)); vm.prank(owner); - sharedBridge.setEraPostDiamondUpgradeFirstBatch(eraPostUpgradeFirstBatch); - vm.prank(owner); - sharedBridge.setEraPostLegacyBridgeUpgradeFirstBatch(eraPostUpgradeFirstBatch); - vm.prank(owner); - sharedBridge.setEraLegacyBridgeLastDepositTime(1, 0); - vm.prank(owner); sharedBridge.initializeChainGovernance(chainId, l2SharedBridge); vm.prank(owner); sharedBridge.initializeChainGovernance(eraChainId, l2SharedBridge);