feat: OZ fixes from (GW + Governance audit) + ZK as base token for GW…

… combined (#978) Co-authored-by: Raid Ateir <[email protected]> Co-authored-by: Stanislav Breadless <[email protected]> Co-authored-by: Vlad Bochok <[email protected]> Co-authored-by: vladbochok <[email protected]> Co-authored-by: kelemeno <[email protected]>
matter-labs · Oct 26, 2024 · 21ac083 · 21ac083
1 parent f33bcb7
commit 21ac083
Show file tree

Hide file tree

Showing 58 changed files with 538 additions and 235 deletions.
diff --git a/da-contracts/contracts/IL1DAValidator.sol b/da-contracts/contracts/IL1DAValidator.sol
@@ -17,7 +17,7 @@ struct L1DAValidatorOutput {
 interface IL1DAValidator {
     /// @notice The function that checks the data availability for the given batch input.
     /// @param _chainId The chain id of the chain that is being committed.
-    /// @param _chainId The batch number for which the data availability is being checked.
+    /// @param _batchNumber The batch number for which the data availability is being checked.
     /// @param _l2DAValidatorOutputHash The hash of that was returned by the l2DAValidator.
     /// @param _operatorDAInput The DA input by the operator provided on L1.
     /// @param _maxBlobsSupported The maximal number of blobs supported by the chain.

diff --git a/l1-contracts/.env b/l1-contracts/.env
@@ -39,8 +39,12 @@ ETH_SENDER_SENDER_OPERATOR_BLOBS_ETH_ADDR=0x000000000000000000000000000000000000
 CONTRACTS_SHARED_BRIDGE_UPGRADE_STORAGE_SWITCH=0
 CONTRACTS_MAX_NUMBER_OF_ZK_CHAINS=100
 L1_CONFIG=/script-config/config-deploy-l1.toml
+L2_CONFIG=/script-config/config-deploy-l2-contracts.toml
 L1_OUTPUT=/script-out/output-deploy-l1.toml
+L2_CONFIG=/script-config/config-deploy-l2-contracts.toml
 TOKENS_CONFIG=/script-config/config-deploy-erc20.toml
+ZK_TOKEN_CONFIG=/script-config/config-deploy-zk.toml
+ZK_TOKEN_OUTPUT=/script-out/output-deploy-zk-token.toml
 ZK_CHAIN_CONFIG=/script-config/register-zk-chain.toml
 ZK_CHAIN_OUTPUT=/script-out/output-deploy-zk-chain-era.toml
 FORCE_DEPLOYMENTS_CONFIG=/script-config/generate-force-deployments-data.toml

diff --git a/l1-contracts/contracts/bridge/BridgeHelper.sol b/l1-contracts/contracts/bridge/BridgeHelper.sol
@@ -9,7 +9,7 @@ import {DataEncoding} from "../common/libraries/DataEncoding.sol";
 /**
  * @author Matter Labs
  * @custom:security-contact [email protected]
- * @notice Helper library for working with L2 contracts on L1.
+ * @notice Helper library for working with native tokens on both L1 and L2.
  */
 library BridgeHelper {
     /// @dev Receives and parses (name, symbol, decimals) from the token contract

diff --git a/l1-contracts/contracts/bridge/BridgedStandardERC20.sol b/l1-contracts/contracts/bridge/BridgedStandardERC20.sol
@@ -95,11 +95,12 @@ contract BridgedStandardERC20 is ERC20PermitUpgradeable, IBridgedStandardToken,
 
         nativeTokenVault = msg.sender;
 
+        bytes memory nameBytes;
+        bytes memory symbolBytes;
+        bytes memory decimalsBytes;
         // We parse the data exactly as they were created on the L1 bridge
         // slither-disable-next-line unused-return
-        (, bytes memory nameBytes, bytes memory symbolBytes, bytes memory decimalsBytes) = DataEncoding.decodeTokenData(
-            _data
-        );
+        (, nameBytes, symbolBytes, decimalsBytes) = DataEncoding.decodeTokenData(_data);
 
         ERC20Getters memory getters;
         string memory decodedName;

diff --git a/l1-contracts/contracts/bridge/L1BridgeContractErrors.sol b/l1-contracts/contracts/bridge/L1BridgeContractErrors.sol
@@ -2,9 +2,6 @@
 
 pragma solidity ^0.8.21;
 
-// 0xe4efb466
-error NotNTV();
-
 // 0x6d963f88
 error EthTransferFailed();
 

diff --git a/l1-contracts/contracts/bridge/L1Nullifier.sol b/l1-contracts/contracts/bridge/L1Nullifier.sol
@@ -29,7 +29,7 @@ import {IBridgehub} from "../bridgehub/IBridgehub.sol";
 import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR, L2_ASSET_ROUTER_ADDR} from "../common/L2ContractAddresses.sol";
 import {DataEncoding} from "../common/libraries/DataEncoding.sol";
 import {Unauthorized, SharedBridgeKey, DepositExists, AddressAlreadySet, InvalidProof, DepositDoesNotExist, SharedBridgeValueNotSet, WithdrawalAlreadyFinalized, L2WithdrawalMessageWrongLength, InvalidSelector, SharedBridgeValueNotSet, ZeroAddress} from "../common/L1ContractErrors.sol";
-import {WrongL2Sender, NotNTV, NativeTokenVaultAlreadySet, EthTransferFailed, WrongMsgLength} from "./L1BridgeContractErrors.sol";
+import {WrongL2Sender, NativeTokenVaultAlreadySet, EthTransferFailed, WrongMsgLength} from "./L1BridgeContractErrors.sol";
 
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
@@ -126,14 +126,6 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
         _;
     }
 
-    /// @notice Checks that the message sender is the bridgehub or ZKsync Era Diamond Proxy.
-    modifier onlyBridgehubOrEra(uint256 _chainId) {
-        if (msg.sender != address(BRIDGE_HUB) && (_chainId != ERA_CHAIN_ID || msg.sender != ERA_DIAMOND_PROXY)) {
-            revert Unauthorized(msg.sender);
-        }
-        _;
-    }
-
     /// @notice Checks that the message sender is the legacy bridge.
     modifier onlyLegacyBridge() {
         if (msg.sender != address(legacyBridge)) {
@@ -142,14 +134,6 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
         _;
     }
 
-    /// @notice Checks that the message sender is the legacy bridge.
-    modifier onlyAssetRouterOrErc20Bridge() {
-        if (msg.sender != address(l1AssetRouter) && msg.sender != address(legacyBridge)) {
-            revert Unauthorized(msg.sender);
-        }
-        _;
-    }
-
     /// @dev Contract is expected to be used as proxy implementation.
     /// @dev Initialize the implementation to prevent Parity hack.
     constructor(IBridgehub _bridgehub, uint256 _eraChainId, address _eraDiamondProxy) reentrancyGuardInitializer {
@@ -210,10 +194,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
     /// @dev This function is part of the upgrade process used to nullify chain balances once they are credited to NTV.
     /// @param _chainId The ID of the ZK chain.
     /// @param _token The address of the token which was previously deposit to shared bridge.
-    function nullifyChainBalanceByNTV(uint256 _chainId, address _token) external {
-        if (msg.sender != address(l1NativeTokenVault)) {
-            revert NotNTV();
-        }
+    function nullifyChainBalanceByNTV(uint256 _chainId, address _token) external onlyL1NTV {
         __DEPRECATED_chainBalance[_chainId][_token] = 0;
     }
 
@@ -290,7 +271,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
         emit BridgehubDepositFinalized(_chainId, _txDataHash, _txHash);
     }
 
-    /// @dev Calls the internal `_encodeTxDataHash`. Used as a wrapped for try / catch case.
+    /// @dev Calls the library `encodeTxDataHash`. Used as a wrapped for try / catch case.
     /// @dev Encodes the transaction data hash using either the latest encoding standard or the legacy standard.
     /// @param _encodingVersion EncodingVersion.
     /// @param _originalCaller The address of the entity that initiated the deposit.
@@ -431,10 +412,9 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
         }
         isWithdrawalFinalized[chainId][l2BatchNumber][l2MessageIndex] = true;
 
-        // Handling special case for withdrawal from ZKsync Era initiated before Shared Bridge.
         (bytes32 assetId, bytes memory transferData) = _verifyWithdrawal(_finalizeWithdrawalParams);
 
-        // Handling special case for withdrawal from zkSync Era initiated before Shared Bridge.
+        // Handling special case for withdrawal from ZKsync Era initiated before Shared Bridge.
         if (_isPreSharedBridgeEraEthWithdrawal(chainId, l2BatchNumber)) {
             // Checks that the withdrawal wasn't finalized already.
             bool alreadyFinalized = IGetters(ERA_DIAMOND_PROXY).isEthWithdrawalFinalized(l2BatchNumber, l2MessageIndex);
@@ -593,8 +573,8 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
             address baseToken = BRIDGE_HUB.baseToken(_chainId);
             transferData = DataEncoding.encodeBridgeMintData({
                 _originalCaller: address(0),
-                _l2Receiver: l1Receiver,
-                _l1Token: baseToken,
+                _remoteReceiver: l1Receiver,
+                _originToken: baseToken,
                 _amount: amount,
                 _erc20Metadata: new bytes(0)
             });
@@ -618,8 +598,8 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
             assetId = DataEncoding.encodeNTVAssetId(block.chainid, l1Token);
             transferData = DataEncoding.encodeBridgeMintData({
                 _originalCaller: address(0),
-                _l2Receiver: l1Receiver,
-                _l1Token: l1Token,
+                _remoteReceiver: l1Receiver,
+                _originToken: l1Token,
                 _amount: amount,
                 _erc20Metadata: new bytes(0)
             });

diff --git a/l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol b/l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol
@@ -109,7 +109,7 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable,
             IAssetHandler(assetHandler).bridgeMint(_chainId, _assetId, _transferData);
         } else {
             assetHandlerAddress[_assetId] = _nativeTokenVault;
-            IAssetHandler(_nativeTokenVault).bridgeMint(_chainId, _assetId, _transferData); // ToDo: Maybe it's better to receive amount and receiver here? transferData may have different encoding
+            IAssetHandler(_nativeTokenVault).bridgeMint(_chainId, _assetId, _transferData);
         }
     }
 

diff --git a/l1-contracts/contracts/bridge/asset-router/IAssetRouterBase.sol b/l1-contracts/contracts/bridge/asset-router/IAssetRouterBase.sol
@@ -32,13 +32,6 @@ interface IAssetRouterBase {
         bytes bridgeMintCalldata
     );
 
-    event BridgehubWithdrawalInitiated(
-        uint256 chainId,
-        address indexed sender,
-        bytes32 indexed assetId,
-        bytes32 assetDataHash // Todo: What's the point of emitting hash?
-    );
-
     event AssetHandlerRegisteredInitial(
         bytes32 indexed assetId,
         address indexed assetHandlerAddress,

diff --git a/l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol
@@ -16,7 +16,7 @@ interface IL2AssetRouter is IAssetRouterBase {
 
     function withdraw(bytes32 _assetId, bytes calldata _transferData) external returns (bytes32);
 
-    function l1AssetRouter() external view returns (address);
+    function L1_ASSET_ROUTER() external view returns (address);
 
     function withdrawLegacyBridge(address _l1Receiver, address _l2Token, uint256 _amount, address _sender) external;
 
@@ -28,7 +28,7 @@ interface IL2AssetRouter is IAssetRouterBase {
         bytes calldata _data
     ) external;
 
-    /// @dev Used to set the assedAddress for a given assetId.
+    /// @dev Used to set the assetHandlerAddress for a given assetId.
     /// @dev Will be used by ZK Gateway
     function setAssetHandlerAddress(uint256 _originChainId, bytes32 _assetId, address _assetAddress) external;
 }
diff --git a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol
@@ -109,7 +109,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
         _transferOwnership(_owner);
     }
 
-    /// @notice Sets the L1ERC20Bridge contract address.
+    /// @notice Sets the NativeTokenVault contract address.
     /// @dev Should be called only once by the owner.
     /// @param _nativeTokenVault The address of the native token vault.
     function setNativeTokenVault(INativeTokenVault _nativeTokenVault) external onlyOwner {
@@ -144,9 +144,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
         bytes32 _assetRegistrationData,
         address _assetDeploymentTracker
     ) external onlyOwner {
-        bytes32 assetId = keccak256(
-            abi.encode(uint256(block.chainid), _assetDeploymentTracker, _assetRegistrationData)
-        );
+        bytes32 assetId = keccak256(abi.encode(block.chainid, _assetDeploymentTracker, _assetRegistrationData));
         assetDeploymentTracker[assetId] = _assetDeploymentTracker;
         emit AssetDeploymentTrackerSet(assetId, _assetDeploymentTracker, _assetRegistrationData);
     }
@@ -160,7 +158,6 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
     }
 
     /// @notice Used to set the asset handler address for a given asset ID on a remote ZK chain
-    /// @dev No access control on the caller, as msg.sender is encoded in the assetId.
     /// @param _chainId The ZK chain ID.
     /// @param _originalCaller The `msg.sender` address from the external call that initiated current one.
     /// @param _assetId The encoding of asset ID.

diff --git a/l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol
@@ -33,13 +33,13 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     bytes32 public immutable BASE_TOKEN_ASSET_ID;
 
     /// @dev The address of the L1 asset router counterpart.
-    address public override l1AssetRouter;
+    address public immutable override L1_ASSET_ROUTER;
 
     /// @notice Checks that the message sender is the L1 Asset Router.
     modifier onlyAssetRouterCounterpart(uint256 _originChainId) {
         if (_originChainId == L1_CHAIN_ID) {
             // Only the L1 Asset Router counterpart can initiate and finalize the deposit.
-            if (AddressAliasHelper.undoL1ToL2Alias(msg.sender) != l1AssetRouter) {
+            if (AddressAliasHelper.undoL1ToL2Alias(msg.sender) != L1_ASSET_ROUTER) {
                 revert InvalidCaller(msg.sender);
             }
         } else {
@@ -52,9 +52,11 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     modifier onlyAssetRouterCounterpartOrSelf(uint256 _originChainId) {
         if (_originChainId == L1_CHAIN_ID) {
             // Only the L1 Asset Router counterpart can initiate and finalize the deposit.
-            if ((AddressAliasHelper.undoL1ToL2Alias(msg.sender) != l1AssetRouter) && (msg.sender != address(this))) {
+            if ((AddressAliasHelper.undoL1ToL2Alias(msg.sender) != L1_ASSET_ROUTER) && (msg.sender != address(this))) {
                 revert InvalidCaller(msg.sender);
             }
+        } else {
+            revert InvalidCaller(msg.sender); // xL2 messaging not supported for now
         }
         _;
     }
@@ -82,7 +84,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         if (_l1AssetRouter == address(0)) {
             revert EmptyAddress();
         }
-        l1AssetRouter = _l1AssetRouter;
+        L1_ASSET_ROUTER = _l1AssetRouter;
         assetHandlerAddress[_baseTokenAssetId] = L2_NATIVE_TOKEN_VAULT_ADDR;
         BASE_TOKEN_ASSET_ID = _baseTokenAssetId;
         _disableInitializers();
@@ -93,10 +95,10 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     function setAssetHandlerAddress(
         uint256 _originChainId,
         bytes32 _assetId,
-        address _assetAddress
+        address _assetHandlerAddress
     ) external override onlyAssetRouterCounterpart(_originChainId) {
-        assetHandlerAddress[_assetId] = _assetAddress;
-        emit AssetHandlerRegistered(_assetId, _assetAddress);
+        assetHandlerAddress[_assetId] = _assetHandlerAddress;
+        emit AssetHandlerRegistered(_assetId, _assetHandlerAddress);
     }
 
     /// @inheritdoc IAssetRouterBase
@@ -128,16 +130,6 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         emit DepositFinalizedAssetRouter(L1_CHAIN_ID, _assetId, _transferData);
     }
 
-    /*//////////////////////////////////////////////////////////////
-                     Internal & Helpers
-    //////////////////////////////////////////////////////////////*/
-
-    /// @inheritdoc AssetRouterBase
-    function _ensureTokenRegisteredWithNTV(address _token) internal override returns (bytes32 assetId) {
-        IL2NativeTokenVault nativeTokenVault = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR);
-        nativeTokenVault.ensureTokenIsRegistered(_token);
-    }
-
     /*//////////////////////////////////////////////////////////////
                             LEGACY FUNCTIONS
     //////////////////////////////////////////////////////////////*/
@@ -161,6 +153,19 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         return _withdrawSender(assetId, _assetData, msg.sender, true);
     }
 
+    /*//////////////////////////////////////////////////////////////
+                     Internal & Helpers
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Ensures that token is registered with native token vault.
+    /// @dev Only used when deposit is made with legacy data encoding format.
+    /// @param _token The L2 token address which should be registered with native token vault.
+    /// @return assetId The asset ID of the token provided.
+    function _ensureTokenRegisteredWithNTV(address _token) internal override returns (bytes32 assetId) {
+        IL2NativeTokenVault nativeTokenVault = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR);
+        nativeTokenVault.ensureTokenIsRegistered(_token);
+    }
+
     /// @notice Initiates a withdrawal by burning funds on the contract and sending the message to L1
     /// where tokens would be unlocked
     /// @param _assetId The asset id of the withdrawn asset
@@ -223,6 +228,10 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         return abi.encodePacked(IL1ERC20Bridge.finalizeWithdrawal.selector, _l1Receiver, _l1Token, _amount);
     }
 
+    /*//////////////////////////////////////////////////////////////
+                            LEGACY FUNCTIONS
+    //////////////////////////////////////////////////////////////*/
+
     /// @notice Legacy finalizeDeposit.
     /// @dev Finalizes the deposit and mint funds.
     /// @param _l1Sender The address of token sender on L1.
@@ -338,6 +347,6 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     /// @notice Returns the address of the L1 asset router.
     /// @dev The old name is kept for backward compatibility.
     function l1Bridge() external view returns (address) {
-        return l1AssetRouter;
+        return L1_ASSET_ROUTER;
     }
 }
diff --git a/l1-contracts/contracts/bridge/interfaces/IAssetHandler.sol b/l1-contracts/contracts/bridge/interfaces/IAssetHandler.sol
@@ -7,9 +7,6 @@ pragma solidity 0.8.24;
 /// @custom:security-contact [email protected]
 /// @notice Used for any asset handler and called by the AssetRouter
 interface IAssetHandler {
-    /// @dev Emitted when a new token is initialized
-    event BridgeInitialize(address indexed token, string name, string symbol, uint8 decimals);
-
     /// @dev Emitted when a token is minted
     event BridgeMint(uint256 indexed chainId, bytes32 indexed assetId, address receiver, uint256 amount);
 
@@ -27,7 +24,7 @@ interface IAssetHandler {
     /// @param _data the actual data specified for the function
     function bridgeMint(uint256 _chainId, bytes32 _assetId, bytes calldata _data) external payable;
 
-    /// @notice Burns bridged tokens and returns the calldata for L2 -> L1 message.
+    /// @notice Burns bridged tokens and returns the calldata for L2 <-> L1 message.
     /// @dev In case of native token vault _data is the tuple of _depositAmount and _l2Receiver.
     /// @param _chainId the chainId that the message will be sent to
     /// @param _msgValue the msg.value of the L2 transaction. For now it is always 0.

diff --git a/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol
@@ -35,9 +35,6 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
     /// @dev L1 nullifier contract that handles legacy functions & finalize withdrawal, confirm l2 tx mappings
     IL1Nullifier public immutable override L1_NULLIFIER;
 
-    /// @dev Era's chainID
-    uint256 public immutable ERA_CHAIN_ID;
-
     /// @dev Maps token balances for each chain to prevent unauthorized spending across ZK chains.
     /// This serves as a security measure until hyperbridging is implemented.
     /// NOTE: this function may be removed in the future, don't rely on it!
@@ -47,12 +44,10 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
     /// @dev Initialize the implementation to prevent Parity hack.
     /// @param _l1WethAddress Address of WETH on deployed chain
     /// @param _l1AssetRouter Address of Asset Router on L1.
-    /// @param _eraChainId ID of Era.
     /// @param _l1Nullifier Address of the nullifier contract, which handles transaction progress between L1 and ZK chains.
     constructor(
         address _l1WethAddress,
         address _l1AssetRouter,
-        uint256 _eraChainId,
         IL1Nullifier _l1Nullifier
     )
         NativeTokenVault(
@@ -62,7 +57,6 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
             block.chainid
         )
     {
-        ERA_CHAIN_ID = _eraChainId;
         L1_NULLIFIER = _l1Nullifier;
     }
 
@@ -237,9 +231,9 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
     // get the computed address before the contract DeployWithCreate2 deployed using Bytecode of contract DeployWithCreate2 and salt specified by the sender
     function calculateCreate2TokenAddress(
         uint256 _originChainId,
-        address _l1Token
+        address _nonNativeToken
     ) public view override(INativeTokenVault, NativeTokenVault) returns (address) {
-        bytes32 salt = _getCreate2Salt(_originChainId, _l1Token);
+        bytes32 salt = _getCreate2Salt(_originChainId, _nonNativeToken);
         return
             Create2.computeAddress(
                 salt,