feat: ZKChain and Gateway Upgrade Audit (#1052)

.github/workflows/l1-contracts-ci.yaml

@@ -45,9 +45,6 @@ jobs:
       - name: Build da-contracts artifacts
         run: yarn da build:foundry
 
-      - name: Build l1 artifacts
-        run: yarn l1 build
-
       - name: Create cache
         uses: actions/cache/save@v3
         with:
@@ -287,18 +284,17 @@ jobs:
       - name: Run coverage
         run: FOUNDRY_PROFILE=default yarn test:foundry && FOUNDRY_PROFILE=default yarn coverage:foundry --report summary --report lcov
 
-      # TODO: for some reason filtering directories stopped working.
-      # # To ignore coverage for certain directories modify the paths in this step as needed. The
-      # # below default ignores coverage results for the test and script directories. Alternatively,
-      # # to include coverage in all directories, comment out this step. Note that because this
-      # # filtering applies to the lcov file, the summary table generated in the previous step will
-      # # still include all files and directories.
-      # # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov
-      # # defaults to removing branch info.
-      # - name: Filter directories
-      #   run: |
-      #     sudo apt update && sudo apt install -y lcov
-      #     lcov --remove lcov.info 'test/*' 'contracts/dev-contracts/*' '../lib/forge-std/*' '../lib/murky/*' 'lib/*' '../lib/*' 'lib/' 'deploy-scripts/*' --output-file lcov.info --rc lcov_branch_coverage=1
+      # To ignore coverage for certain directories modify the paths in this step as needed. The
+      # below default ignores coverage results for the test and script directories. Alternatively,
+      # to include coverage in all directories, comment out this step. Note that because this
+      # filtering applies to the lcov file, the summary table generated in the previous step will
+      # still include all files and directories.
+      # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov
+      # defaults to removing branch info.
+      - name: Filter directories
+        run: |
+          sudo apt update && sudo apt install -y lcov
+          lcov --remove lcov.info 'test/*' 'contracts/bridge/asset-router/L1AssetRouter.sol' 'contracts/dev-contracts/*' '../lib/forge-std/*' '../lib/murky/*' 'lib/*' '../lib/*' 'lib/' --ignore-errors unused --output-file lcov.info --rc lcov_branch_coverage=1
 
       # # This step posts a detailed coverage report as a comment and deletes previous comments on
       # # each push. The below step is used to fail coverage if the specified coverage threshold is

l1-contracts/contracts/bridge/L1ERC20Bridge.sol

@@ -13,7 +13,7 @@ import {IL1AssetRouter} from "./asset-router/IL1AssetRouter.sol";
 import {L2ContractHelper} from "../common/libraries/L2ContractHelper.sol";
 import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";
 
-import {EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported, ApprovalFailed} from "../common/L1ContractErrors.sol";
+import {EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported} from "../common/L1ContractErrors.sol";
 import {ETH_TOKEN_ADDRESS} from "../common/Config.sol";
 
 /// @author Matter Labs
@@ -203,11 +203,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
             _l2TxGasPerPubdataByte: _l2TxGasPerPubdataByte,
             _refundRecipient: _refundRecipient
         });
-        // clearing approval
-        bool success = IERC20(_l1Token).approve(address(L1_ASSET_ROUTER), 0);
-        if (!success) {
-            revert ApprovalFailed();
-        }
+
         depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;
         emit DepositInitiated({
             l2DepositTxHash: l2TxHash,
@@ -227,10 +223,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
     function _approveFundsToAssetRouter(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
         uint256 balanceBefore = _token.balanceOf(address(this));
         _token.safeTransferFrom(_from, address(this), _amount);
-        bool success = _token.approve(address(L1_ASSET_ROUTER), _amount);
-        if (!success) {
-            revert ApprovalFailed();
-        }
+        _token.forceApprove(address(L1_ASSET_ROUTER), _amount);
         uint256 balanceAfter = _token.balanceOf(address(this));
 
         return balanceAfter - balanceBefore;

l1-contracts/contracts/bridge/L1Nullifier.sol

@@ -573,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)
             });
@@ -598,15 +598,15 @@ 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)
             });
         } else if (bytes4(functionSignature) == IAssetRouterBase.finalizeDeposit.selector) {
-            // The data is expected to be at least 36 bytes long to contain assetId.
-            if (_l2ToL1message.length < 36) {
-                revert WrongMsgLength(36, _l2ToL1message.length);
+            // The data is expected to be at least 68 bytes long to contain assetId.
+            if (_l2ToL1message.length < 68) {
+                revert WrongMsgLength(68, _l2ToL1message.length);
             }
             // slither-disable-next-line unused-return
             (, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset); // originChainId, not used for L2->L1 txs

l1-contracts/contracts/bridge/L2WrappedBaseToken.sol

@@ -97,7 +97,7 @@ contract L2WrappedBaseToken is ERC20PermitUpgradeable, IL2WrappedBaseToken, IBri
     /// Always reverts instead of minting anything!
     /// Note: Use `deposit`/`depositTo` methods instead.
     // solhint-disable-next-line no-unused-vars
-    function bridgeMint(address _to, uint256 _amount) external override onlyBridge {
+    function bridgeMint(address _to, uint256 _amount) external view override onlyBridge {
         revert BridgeMintNotImplemented();
     }
 

l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol

@@ -117,6 +117,11 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable,
                             Internal Functions
     //////////////////////////////////////////////////////////////*/
 
+    function _setAssetHandler(bytes32 _assetId, address _assetHandlerAddress) internal {
+        assetHandlerAddress[_assetId] = _assetHandlerAddress;
+        emit AssetHandlerRegistered(_assetId, _assetHandlerAddress);
+    }
+
     /// @dev send the burn message to the asset
     /// @notice Forwards the burn request for specific asset to respective asset handler.
     /// @param _chainId The chain ID of the ZK chain to which to deposit.

l1-contracts/contracts/bridge/asset-router/IAssetRouterBase.sol

@@ -32,6 +32,13 @@ 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,

l1-contracts/contracts/bridge/asset-router/IL1AssetRouter.sol

@@ -28,15 +28,6 @@ interface IL1AssetRouter is IAssetRouterBase, IL1SharedBridgeLegacy {
         bytes32 indexed additionalData
     );
 
-    event LegacyDepositInitiated(
-        uint256 indexed chainId,
-        bytes32 indexed l2DepositTxHash,
-        address indexed from,
-        address to,
-        address l1Asset,
-        uint256 amount
-    );
-
     /// @notice Initiates a deposit by locking funds on the contract and sending the request
     /// of processing an L2 transaction where tokens would be minted.
     /// @dev If the token is bridged for the first time, the L2 token contract will be deployed. Note however, that the

l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol

@@ -31,4 +31,9 @@ interface IL2AssetRouter is IAssetRouterBase {
     /// @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;
+
+    /// @notice Function that allows native token vault to register itself as the asset handler for
+    /// a legacy asset.
+    /// @param _assetId The assetId of the legacy token.
+    function setLegacyTokenAssetHandler(bytes32 _assetId) external;
 }

l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol

@@ -2,6 +2,8 @@
 
 pragma solidity 0.8.24;
 
+// solhint-disable reason-string, gas-custom-errors
+
 import {IERC20} from "@openzeppelin/contracts-v4/token/ERC20/IERC20.sol";
 import {SafeERC20} from "@openzeppelin/contracts-v4/token/ERC20/utils/SafeERC20.sol";
 

l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

@@ -7,7 +7,6 @@ import {IAssetRouterBase} from "./IAssetRouterBase.sol";
 import {AssetRouterBase} from "./AssetRouterBase.sol";
 
 import {IL2NativeTokenVault} from "../ntv/IL2NativeTokenVault.sol";
-import {INativeTokenVault} from "../ntv/INativeTokenVault.sol";
 import {IL2SharedBridgeLegacy} from "../interfaces/IL2SharedBridgeLegacy.sol";
 import {IAssetHandler} from "../interfaces/IAssetHandler.sol";
 import {IBridgedStandardToken} from "../interfaces/IBridgedStandardToken.sol";
@@ -19,7 +18,7 @@ import {AddressAliasHelper} from "../../vendor/AddressAliasHelper.sol";
 import {L2_NATIVE_TOKEN_VAULT_ADDR, L2_BRIDGEHUB_ADDR} from "../../common/L2ContractAddresses.sol";
 import {L2ContractHelper} from "../../common/libraries/L2ContractHelper.sol";
 import {DataEncoding} from "../../common/libraries/DataEncoding.sol";
-import {EmptyAddress, InvalidCaller, AmountMustBeGreaterThanZero, AssetIdNotSupported} from "../../common/L1ContractErrors.sol";
+import {TokenNotLegacy, EmptyAddress, InvalidCaller, AmountMustBeGreaterThanZero, AssetIdNotSupported} from "../../common/L1ContractErrors.sol";
 
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
@@ -69,6 +68,13 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         _;
     }
 
+    modifier onlyNTV() {
+        if (msg.sender != L2_NATIVE_TOKEN_VAULT_ADDR) {
+            revert InvalidCaller(msg.sender);
+        }
+        _;
+    }
+
     /// @dev Disable the initialization to prevent Parity hack.
     /// @dev this contract is deployed in the L2GenesisUpgrade, and is meant as direct deployment without a proxy.
     /// @param _l1AssetRouter The address of the L1 Bridge contract.
@@ -109,6 +115,12 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         _setAssetHandlerAddressThisChain(L2_NATIVE_TOKEN_VAULT_ADDR, _assetRegistrationData, _assetHandlerAddress);
     }
 
+    function setLegacyTokenAssetHandler(bytes32 _assetId) external override onlyNTV {
+        // Note, that it is an asset handler, but not asset deployment tracker,
+        // which is located on L1.
+        _setAssetHandler(_assetId, L2_NATIVE_TOKEN_VAULT_ADDR);
+    }
+
     /*//////////////////////////////////////////////////////////////
                             Receive transaction Functions
     //////////////////////////////////////////////////////////////*/
@@ -131,26 +143,16 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     }
 
     /*//////////////////////////////////////////////////////////////
-                            LEGACY FUNCTIONS
+                            INITIATTE DEPOSIT Functions
     //////////////////////////////////////////////////////////////*/
 
     /// @notice Initiates a withdrawal by burning funds on the contract and sending the message to L1
     /// where tokens would be unlocked
     /// @dev do not rely on this function, it will be deprecated in the future
     /// @param _assetId The asset id of the withdrawn asset
     /// @param _assetData The data that is passed to the asset handler contract
-    function withdraw(bytes32 _assetId, bytes memory _assetData) public override returns (bytes32) {
-        return _withdrawSender(_assetId, _assetData, msg.sender, true);
-    }
-
-    function withdrawToken(address _l2NativeToken, bytes memory _assetData) public returns (bytes32) {
-        bytes32 recordedAssetId = INativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).assetId(_l2NativeToken);
-        uint256 recordedOriginChainId = INativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).originChainId(recordedAssetId);
-        if (recordedOriginChainId == L1_CHAIN_ID) {
-            revert AssetIdNotSupported(recordedAssetId);
-        }
-        bytes32 assetId = _ensureTokenRegisteredWithNTV(_l2NativeToken);
-        return _withdrawSender(assetId, _assetData, msg.sender, true);
+    function withdraw(bytes32 _assetId, bytes calldata _assetData) external returns (bytes32) {
+        _withdrawSender(_assetId, _assetData, msg.sender, true);
     }
 
     /*//////////////////////////////////////////////////////////////
@@ -313,16 +315,29 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     }
 
     function _withdrawLegacy(address _l1Receiver, address _l2Token, uint256 _amount, address _sender) internal {
-        bytes32 assetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, l1TokenAddress(_l2Token));
-        bytes memory data = abi.encode(_amount, _l1Receiver);
+        address l1Address = l1TokenAddress(_l2Token);
+        if (l1Address == address(0)) {
+            revert TokenNotLegacy();
+        }
+        bytes32 assetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, l1Address);
+        bytes memory data = DataEncoding.encodeBridgeBurnData(_amount, _l1Receiver, _l2Token);
         _withdrawSender(assetId, data, _sender, false);
     }
 
     /// @notice Legacy getL1TokenAddress.
     /// @param _l2Token The address of token on L2.
     /// @return The address of token on L1.
     function l1TokenAddress(address _l2Token) public view returns (address) {
-        return IBridgedStandardToken(_l2Token).l1Address();
+        bytes32 assetId = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).assetId(_l2Token);
+        if (assetId == bytes32(0)) {
+            return address(0);
+        }
+        uint256 originChainId = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).originChainId(assetId);
+        if (originChainId != L1_CHAIN_ID) {
+            return address(0);
+        }
+
+        return IBridgedStandardToken(_l2Token).originToken();
     }
 
     /// @notice Legacy function used for backward compatibility to return L2 wrapped token

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

@@ -86,15 +86,6 @@ interface IL1Nullifier {
 
     function nullifyChainBalanceByNTV(uint256 _chainId, address _token) external;
 
-    function finalizeWithdrawal(
-        uint256 _chainId,
-        uint256 _l2BatchNumber,
-        uint256 _l2MessageIndex,
-        uint16 _l2TxNumberInBatch,
-        bytes calldata _message,
-        bytes32[] calldata _merkleProof
-    ) external;
-
     /// @dev Withdraw funds from the initiated deposit, that failed when finalizing on L2.
     /// @param _chainId The ZK chain id to which deposit was initiated.
     /// @param _depositSender The address of the entity that initiated the deposit.
@@ -117,4 +108,24 @@ interface IL1Nullifier {
         uint16 _l2TxNumberInBatch,
         bytes32[] calldata _merkleProof
     ) external;
+
+    /// @notice Legacy function to finalize withdrawal via the same
+    /// interface as the old L1SharedBridge.
+    /// @dev Note, that we need to keep this interface, since the `L2AssetRouter`
+    /// will continue returning the previous address as the `l1SharedBridge`. The value
+    /// returned by it is used in the SDK for finalizing withdrawals.
+    /// @param _chainId The chain ID of the transaction to check
+    /// @param _l2BatchNumber The L2 batch number where the withdrawal was processed
+    /// @param _l2MessageIndex The position in the L2 logs Merkle tree of the l2Log that was sent with the message
+    /// @param _l2TxNumberInBatch The L2 transaction number in the batch, in which the log was sent
+    /// @param _message The L2 withdraw data, stored in an L2 -> L1 message
+    /// @param _merkleProof The Merkle proof of the inclusion L2 -> L1 message about withdrawal initialization
+    function finalizeWithdrawal(
+        uint256 _chainId,
+        uint256 _l2BatchNumber,
+        uint256 _l2MessageIndex,
+        uint16 _l2TxNumberInBatch,
+        bytes calldata _message,
+        bytes32[] calldata _merkleProof
+    ) external;
 }

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

@@ -7,4 +7,13 @@ pragma solidity 0.8.24;
 /// @custom:security-contact [email protected]
 interface IL1SharedBridgeLegacy {
     function l2BridgeAddress(uint256 _chainId) external view returns (address);
+
+    event LegacyDepositInitiated(
+        uint256 indexed chainId,
+        bytes32 indexed l2DepositTxHash,
+        address indexed from,
+        address to,
+        address l1Asset,
+        uint256 amount
+    );
 }

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

@@ -2,8 +2,6 @@
 
 pragma solidity ^0.8.20;
 
-import {UpgradeableBeacon} from "@openzeppelin/contracts-v4/proxy/beacon/UpgradeableBeacon.sol";
-
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 interface IL2SharedBridgeLegacy {
@@ -14,8 +12,6 @@ interface IL2SharedBridgeLegacy {
         uint256 amount
     );
 
-    function l2TokenBeacon() external returns (UpgradeableBeacon);
-
     function withdraw(address _l1Receiver, address _l2Token, uint256 _amount) external;
 
     function l1TokenAddress(address _l2Token) external view returns (address);

l1-contracts/contracts/bridge/ntv/IL1NativeTokenVault.sol

@@ -4,14 +4,13 @@ pragma solidity 0.8.24;
 
 import {IL1Nullifier} from "../interfaces/IL1Nullifier.sol";
 import {INativeTokenVault} from "./INativeTokenVault.sol";
-import {IL1AssetDeploymentTracker} from "../interfaces/IL1AssetDeploymentTracker.sol";
 
 /// @title L1 Native token vault contract interface
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
 /// @notice The NTV is an Asset Handler for the L1AssetRouter to handle native tokens
 // is IL1AssetHandler, IL1BaseTokenAssetHandler {
-interface IL1NativeTokenVault is INativeTokenVault, IL1AssetDeploymentTracker {
+interface IL1NativeTokenVault is INativeTokenVault {
     /// @notice The L1Nullifier contract
     function L1_NULLIFIER() external view returns (IL1Nullifier);
 

l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol

@@ -130,7 +130,7 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
         bytes32,
         address,
         address _assetHandlerAddressOnCounterpart
-    ) external view override onlyAssetRouter {
+    ) external view onlyAssetRouter {
         if (_assetHandlerAddressOnCounterpart != L2_NATIVE_TOKEN_VAULT_ADDR) {
             revert WrongCounterpart();
         }