feat: ZKChain and Gateway Upgrade Audit (#1052)

Co-authored-by: kelemeno <[email protected]> Co-authored-by: kelemeno <[email protected]> Co-authored-by: Vlad Bochok <[email protected]> Co-authored-by: Raid Ateir <[email protected]>
matter-labs · Dec 12, 2024 · f72d206 · f72d206
1 parent 8208402
commit f72d206
Show file tree

Hide file tree

Showing 52 changed files with 366 additions and 281 deletions.
diff --git a/da-contracts/contracts/DAContractsErrors.sol b/da-contracts/contracts/DAContractsErrors.sol
@@ -3,8 +3,6 @@ pragma solidity ^0.8.21;
 
 // 0x53dee67b
 error PubdataCommitmentsEmpty();
-// 0x7734c31a
-error PubdataCommitmentsTooBig();
 // 0x53e6d04d
 error InvalidPubdataCommitmentsSize();
 // 0xafd53e2f

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/contracts/bridge/BridgeHelper.sol b/l1-contracts/contracts/bridge/BridgeHelper.sol
@@ -11,7 +11,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
@@ -77,17 +77,20 @@ contract BridgedStandardERC20 is ERC20PermitUpgradeable, IBridgedStandardToken,
     /// @param _originToken Address of the origin token that can be deposited to mint this bridged token
     /// @param _data The additional data that the L1 bridge provide for initialization.
     /// In this case, it is packed `name`/`symbol`/`decimals` of the L1 token.
-    function bridgeInitialize(address _originToken, bytes calldata _data) external initializer returns (uint256) {
+    function bridgeInitialize(address _originToken, bytes calldata _data) external initializer {
         if (_originToken == address(0)) {
             revert ZeroAddress();
         }
         originToken = _originToken;
 
         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
-        (uint256 chainId, bytes memory nameBytes, bytes memory symbolBytes, bytes memory decimalsBytes) = DataEncoding
-            .decodeTokenData(_data);
+        // slither-disable-next-line unused-return
+        (, nameBytes, symbolBytes, decimalsBytes) = DataEncoding.decodeTokenData(_data);
 
         ERC20Getters memory getters;
         string memory decodedName;
@@ -129,7 +132,6 @@ contract BridgedStandardERC20 is ERC20PermitUpgradeable, IBridgedStandardToken,
 
         availableGetters = getters;
         emit BridgeInitialize(_originToken, decodedName, decodedSymbol, decimals_);
-        return chainId;
     }
 
     /// @notice A method to be called by the governor to update the token's metadata.

diff --git a/l1-contracts/contracts/bridge/L1ERC20Bridge.sol b/l1-contracts/contracts/bridge/L1ERC20Bridge.sol
@@ -188,7 +188,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
         if (_l1Token == ETH_TOKEN_ADDRESS) {
             revert ETHDepositNotSupported();
         }
-        uint256 amount = _depositFundsToAssetRouter(msg.sender, IERC20(_l1Token), _amount);
+        uint256 amount = _approveFundsToAssetRouter(msg.sender, IERC20(_l1Token), _amount);
         if (amount != _amount) {
             // The token has non-standard transfer logic
             revert TokensWithFeesNotSupported();
@@ -203,6 +203,9 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
             _l2TxGasPerPubdataByte: _l2TxGasPerPubdataByte,
             _refundRecipient: _refundRecipient
         });
+        // clearing approval
+        // slither-disable-next-line unused-return
+        IERC20(_l1Token).approve(address(L1_ASSET_ROUTER), 0);
         depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;
         emit DepositInitiated({
             l2DepositTxHash: l2TxHash,
@@ -219,10 +222,12 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
 
     /// @dev Transfers tokens from the depositor address to the native token vault address.
     /// @return The difference between the contract balance before and after the transferring of funds.
-    function _depositFundsToAssetRouter(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
-        uint256 balanceBefore = _token.balanceOf(address(L1_ASSET_ROUTER));
-        _token.safeTransferFrom(_from, address(L1_ASSET_ROUTER), _amount);
-        uint256 balanceAfter = _token.balanceOf(address(L1_ASSET_ROUTER));
+    function _approveFundsToAssetRouter(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
+        uint256 balanceBefore = _token.balanceOf(address(this));
+        _token.safeTransferFrom(_from, address(this), _amount);
+        // slither-disable-next-line unused-return
+        _token.approve(address(L1_ASSET_ROUTER), _amount);
+        uint256 balanceAfter = _token.balanceOf(address(this));
 
         return balanceAfter - balanceBefore;
     }

diff --git a/l1-contracts/contracts/bridge/L1Nullifier.sol b/l1-contracts/contracts/bridge/L1Nullifier.sol
@@ -30,8 +30,7 @@ import {DataEncoding} from "../common/libraries/DataEncoding.sol";
 
 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 {Unauthorized, SharedBridgeKey, DepositExists, AddressAlreadySet, InvalidProof, DepositDoesNotExist, SharedBridgeValueNotSet, WithdrawalAlreadyFinalized, L2WithdrawalMessageWrongLength, InvalidSelector, ZeroAddress} from "../common/L1ContractErrors.sol";
 
 /// @author Matter Labs
 /// @custom:security-contact [email protected]
@@ -122,14 +121,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)) {
@@ -138,14 +129,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 {
@@ -204,8 +187,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 {
-        require(msg.sender == address(l1NativeTokenVault), "L1N: not NTV");
+    function nullifyChainBalanceByNTV(uint256 _chainId, address _token) external onlyL1NTV {
         __DEPRECATED_chainBalance[_chainId][_token] = 0;
     }
 
@@ -267,7 +249,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.
@@ -408,10 +390,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);
@@ -568,8 +549,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)
             });
@@ -592,8 +573,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/L2WrappedBaseToken.sol b/l1-contracts/contracts/bridge/L2WrappedBaseToken.sol
@@ -84,7 +84,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();
     }
 

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
@@ -14,11 +14,11 @@ interface IL2AssetRouter {
 
     function withdraw(bytes32 _assetId, bytes calldata _transferData) external;
 
-    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;
 
-    /// @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
@@ -110,7 +110,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 {
@@ -141,9 +141,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);
     }
@@ -157,7 +155,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.
@@ -377,10 +374,17 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
 
         // Do the transfer if allowance to Shared bridge is bigger than amount
         // And if there is not enough allowance for the NTV
-        if (
+        bool weCanTransfer = false;
+        if (l1Token.allowance(address(legacyBridge), address(this)) >= _amount) {
+            _originalCaller = address(legacyBridge);
+            weCanTransfer = true;
+        } else if (
             l1Token.allowance(_originalCaller, address(this)) >= _amount &&
             l1Token.allowance(_originalCaller, address(nativeTokenVault)) < _amount
         ) {
+            weCanTransfer = true;
+        }
+        if (weCanTransfer) {
             // slither-disable-next-line arbitrary-send-erc20
             l1Token.safeTransferFrom(_originalCaller, address(nativeTokenVault), _amount);
             return true;
@@ -515,7 +519,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
         // Save the deposited amount to claim funds on L1 if the deposit failed on L2
         L1_NULLIFIER.bridgehubConfirmL2TransactionForwarded(
             ERA_CHAIN_ID,
-            keccak256(abi.encode(_originalCaller, _l1Token, _amount)),
+            DataEncoding.encodeLegacyTxDataHash(_originalCaller, _l1Token, _amount),
             txHash
         );