Merge pull request #72 from matter-labs/sb-merge-grc-2-sls

Merge grc 2 into sync-layer-stable

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

@@ -175,7 +175,7 @@ jobs:
             da-contracts/out
             l1-contracts/cache-forge
             l1-contracts/out
-            l1-contracts/zkout
+            # TODO: cached `zkout` and the one for tests produce different hashes and so it causes the tests to fail
             l2-contracts/cache-forge
             l2-contracts/zkout
             system-contracts/zkout

da-contracts/contracts/CalldataDA.sol

@@ -70,7 +70,7 @@ abstract contract CalldataDA {
 
         // Now, we need to double check that the provided input was indeed returned by the L2 DA validator.
         if (keccak256(_operatorDAInput[:ptr]) != _l2DAValidatorOutputHash) {
-            revert InvalidL2DAOutputHash();
+            revert InvalidL2DAOutputHash(_l2DAValidatorOutputHash);
         }
 
         // The rest of the output was provided specifically by the operator

da-contracts/contracts/DAContractsErrors.sol

@@ -25,8 +25,8 @@ error InvalidNumberOfBlobs(uint256 blobsProvided, uint256 maxBlobsSupported);
 // 0xcd384e46
 error InvalidBlobsHashes(uint256 operatorDAInputLength, uint256 blobsProvided);
 
-// 0xe9e79528
-error InvalidL2DAOutputHash();
+// 0xd2531c15
+error InvalidL2DAOutputHash(bytes32 l2DAValidatorOutputHash);
 
 // 0x3db6e664
 error OneBlobWithCalldata();

da-contracts/contracts/IL1Messenger.sol

@@ -7,5 +7,5 @@ pragma solidity 0.8.24;
  * @notice The interface of the L1 Messenger contract, responsible for sending messages to L1.
  */
 interface IL1Messenger {
-    function sendToL1(bytes memory _message) external returns (bytes32);
+    function sendToL1(bytes calldata _message) external returns (bytes32);
 }

da-contracts/contracts/RollupL1DAValidator.sol

@@ -149,9 +149,7 @@ contract RollupL1DAValidator is IL1DAValidator, CalldataDA {
         // we iterate over the `_operatorDAInput`, while advancing the pointer by `BLOB_DA_INPUT_SIZE` each time
         for (uint256 i = 0; i < _blobsProvided; ++i) {
             bytes calldata commitmentData = _operatorDAInput[:PUBDATA_COMMITMENT_SIZE];
-            bytes32 prepublishedCommitment = bytes32(
-                _operatorDAInput[PUBDATA_COMMITMENT_SIZE:PUBDATA_COMMITMENT_SIZE + 32]
-            );
+            bytes32 prepublishedCommitment = bytes32(_operatorDAInput[PUBDATA_COMMITMENT_SIZE:BLOB_DA_INPUT_SIZE]);
 
             if (prepublishedCommitment != bytes32(0)) {
                 // We double check that this commitment has indeed been published.

da-contracts/contracts/da-layers/avail/DummyAvailBridge.sol

@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: MIT
 
+pragma solidity 0.8.24;
+
 import {IAvailBridge} from "./IAvailBridge.sol";
 import {IVectorx} from "./IVectorx.sol";
 import {DummyVectorX} from "./DummyVectorX.sol";
@@ -15,7 +17,7 @@ contract DummyAvailBridge is IAvailBridge {
         return vectorxContract;
     }
 
-    function verifyBlobLeaf(MerkleProofInput calldata input) external view returns (bool) {
+    function verifyBlobLeaf(MerkleProofInput calldata) external view returns (bool) {
         return true;
     }
 }

da-contracts/contracts/da-layers/avail/DummyVectorX.sol

@@ -1,9 +1,11 @@
 // SPDX-License-Identifier: MIT
 
+pragma solidity 0.8.24;
+
 import {IVectorx} from "./IVectorx.sol";
 
 contract DummyVectorX is IVectorx {
-    function rangeStartBlocks(bytes32 rangeHash) external view returns (uint32 startBlock) {
+    function rangeStartBlocks(bytes32) external view returns (uint32 startBlock) {
         return 1;
     }
 }

da-contracts/contracts/da-layers/avail/IAvailBridge.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.24;
 import {IVectorx} from "./IVectorx.sol";
 
 interface IAvailBridge {
+    // solhint-disable-next-line gas-struct-packing
     struct Message {
         // single-byte prefix representing the message type
         bytes1 messageType;

l1-contracts/contracts/bridge/L1Nullifier.sol

@@ -28,7 +28,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 {LegacyBridgeNotSet, Unauthorized, SharedBridgeKey, DepositExists, AddressAlreadySet, InvalidProof, DepositDoesNotExist, SharedBridgeValueNotSet, WithdrawalAlreadyFinalized, L2WithdrawalMessageWrongLength, InvalidSelector, SharedBridgeValueNotSet, ZeroAddress} from "../common/L1ContractErrors.sol";
 import {WrongL2Sender, NativeTokenVaultAlreadySet, EthTransferFailed, WrongMsgLength} from "./L1BridgeContractErrors.sol";
 
 /// @author Matter Labs
@@ -246,7 +246,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
     /// @param _l1AssetRouter The address of the asset router.
     function setL1AssetRouter(address _l1AssetRouter) external onlyOwner {
         if (address(l1AssetRouter) != address(0)) {
-            revert AddressAlreadySet(address(_l1AssetRouter));
+            revert AddressAlreadySet(address(l1AssetRouter));
         }
         if (_l1AssetRouter == address(0)) {
             revert ZeroAddress();
@@ -679,7 +679,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
     /// method in `L1ERC20Bridge`.
     ///
     /// @param _depositSender The address of the deposit initiator.
-    /// @param _l1Asset The address of the deposited L1 ERC20 token.
+    /// @param _l1Token The address of the deposited L1 ERC20 token.
     /// @param _amount The amount of the deposit that failed.
     /// @param _l2TxHash The L2 transaction hash of the failed deposit finalization.
     /// @param _l2BatchNumber The L2 batch number where the deposit finalization was processed.
@@ -688,7 +688,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
     /// @param _merkleProof The Merkle proof of the processing L1 -> L2 transaction with deposit finalization.
     function claimFailedDepositLegacyErc20Bridge(
         address _depositSender,
-        address _l1Asset,
+        address _l1Token,
         uint256 _amount,
         bytes32 _l2TxHash,
         uint256 _l2BatchNumber,
@@ -701,7 +701,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
         bytes memory assetData = DataEncoding.encodeBridgeBurnData(_amount, address(0), address(0));
 
         /// the legacy bridge can only be used with L1 native tokens.
-        bytes32 assetId = DataEncoding.encodeNTVAssetId(block.chainid, _l1Asset);
+        bytes32 assetId = DataEncoding.encodeNTVAssetId(block.chainid, _l1Token);
 
         _verifyAndClearFailedTransfer({
             _checkedInLegacyBridge: true,
@@ -754,11 +754,15 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
         /// @dev We use a deprecated field to support L2->L1 legacy withdrawals, which were started
         /// by the legacy bridge.
         address legacyL2Bridge = __DEPRECATED_l2BridgeAddress[_chainId];
+        if (legacyL2Bridge == address(0)) {
+            revert LegacyBridgeNotSet();
+        }
+
         FinalizeL1DepositParams memory finalizeWithdrawalParams = FinalizeL1DepositParams({
             chainId: _chainId,
             l2BatchNumber: _l2BatchNumber,
             l2MessageIndex: _l2MessageIndex,
-            l2Sender: legacyL2Bridge == address(0) ? L2_ASSET_ROUTER_ADDR : legacyL2Bridge,
+            l2Sender: legacyL2Bridge,
             l2TxNumberInBatch: _l2TxNumberInBatch,
             message: _message,
             merkleProof: _merkleProof

l1-contracts/contracts/bridge/L2WrappedBaseTokenStore.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.8.0;
 
 import {Ownable2Step} from "@openzeppelin/contracts-v4/access/Ownable2Step.sol";
 
-import {ZeroAddress, Unauthorized} from "../common/L1ContractErrors.sol";
+import {ZeroAddress, Unauthorized, WrappedBaseTokenAlreadyRegistered} from "../common/L1ContractErrors.sol";
 
 /// @title L2WrappedBaseTokenStore
 /// @author Matter Labs
@@ -69,6 +69,9 @@ contract L2WrappedBaseTokenStore is Ownable2Step {
         if (_l2WBaseToken == address(0)) {
             revert ZeroAddress();
         }
+        if (l2WBaseTokenAddress[_chainId] != address(0)) {
+            revert WrappedBaseTokenAlreadyRegistered();
+        }
         _setWBaseTokenAddress(_chainId, _l2WBaseToken);
     }
 

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

@@ -15,7 +15,7 @@ import {DataEncoding} from "../../common/libraries/DataEncoding.sol";
 import {L2_NATIVE_TOKEN_VAULT_ADDR} from "../../common/L2ContractAddresses.sol";
 
 import {IBridgehub} from "../../bridgehub/IBridgehub.sol";
-import {Unauthorized, AssetHandlerDoesNotExist} from "../../common/L1ContractErrors.sol";
+import {Unauthorized} from "../../common/L1ContractErrors.sol";
 import {INativeTokenVault} from "../ntv/INativeTokenVault.sol";
 
 /// @author Matter Labs
@@ -51,7 +51,7 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable,
      * variables without shifting down storage in the inheritance chain.
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
-    uint256[47] private __gap;
+    uint256[48] private __gap;
 
     /// @notice Checks that the message sender is the bridgehub.
     modifier onlyBridgehub() {
@@ -147,8 +147,8 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable,
     ) internal returns (bytes memory bridgeMintCalldata) {
         address l1AssetHandler = assetHandlerAddress[_assetId];
         if (l1AssetHandler == address(0)) {
-            // As a UX feature, whenever we asset handler is not present, we always try to register asset within native token vault.
-            // The Native Token Vault is trusted to correctly return whether an asset belongs to it.
+            // As a UX feature, whenever an asset handler is not present, we always try to register asset within native token vault.
+            // The Native Token Vault is trusted to revert in an asset does not belong to it.
             //
             // Note, that it may "pollute" error handling a bit: instead of getting error for asset handler not being
             // present, the user will get whatever error the native token vault will return, however, providing

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

@@ -45,7 +45,7 @@ interface IAssetRouterBase {
         address assetDeploymentTracker
     );
 
-    event AssetHandlerRegistered(bytes32 indexed assetId, address indexed _assetAddress);
+    event AssetHandlerRegistered(bytes32 indexed assetId, address indexed _assetHandlerAddress);
 
     event DepositFinalizedAssetRouter(uint256 indexed chainId, bytes32 indexed assetId, bytes assetData);
 

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

@@ -33,7 +33,7 @@ interface IL1AssetRouter is IAssetRouterBase, IL1SharedBridgeLegacy {
         bytes32 indexed l2DepositTxHash,
         address indexed from,
         address to,
-        address l1Asset,
+        address l1Token,
         uint256 amount
     );
 

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

@@ -30,7 +30,7 @@ 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;
+    function setAssetHandlerAddress(uint256 _originChainId, bytes32 _assetId, address _assetHandlerAddress) external;
 
     /// @notice Function that allows native token vault to register itself as the asset handler for
     /// a legacy asset.

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

@@ -22,7 +22,7 @@ import {DataEncoding} from "../../common/libraries/DataEncoding.sol";
 import {AddressAliasHelper} from "../../vendor/AddressAliasHelper.sol";
 import {TWO_BRIDGES_MAGIC_VALUE, ETH_TOKEN_ADDRESS} from "../../common/Config.sol";
 import {NativeTokenVaultAlreadySet} from "../L1BridgeContractErrors.sol";
-import {LegacyBridgeUsesNonNativeToken, NonEmptyMsgValue, UnsupportedEncodingVersion, AssetIdNotSupported, AssetHandlerDoesNotExist, Unauthorized, ZeroAddress, TokenNotSupported, AddressAlreadyUsed} from "../../common/L1ContractErrors.sol";
+import {LegacyBridgeUsesNonNativeToken, NonEmptyMsgValue, UnsupportedEncodingVersion, AssetIdNotSupported, AssetHandlerDoesNotExist, Unauthorized, ZeroAddress, TokenNotSupported, AddressAlreadyUsed, TokensWithFeesNotSupported} from "../../common/L1ContractErrors.sol";
 import {L2_ASSET_ROUTER_ADDR} from "../../common/L2ContractAddresses.sol";
 
 import {IBridgehub, L2TransactionRequestTwoBridgesInner, L2TransactionRequestDirect} from "../../bridgehub/IBridgehub.sol";
@@ -436,8 +436,14 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
             weCanTransfer = true;
         }
         if (weCanTransfer) {
+            uint256 balanceBefore = l1Token.balanceOf(address(nativeTokenVault));
             // slither-disable-next-line arbitrary-send-erc20
             l1Token.safeTransferFrom(_originalCaller, address(nativeTokenVault), _amount);
+            uint256 balanceAfter = l1Token.balanceOf(address(nativeTokenVault));
+
+            if (balanceAfter - balanceBefore != _amount) {
+                revert TokensWithFeesNotSupported();
+            }
             return true;
         }
         return false;
@@ -529,7 +535,11 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
 
         bytes32 _assetId;
         {
-            // todo: add comment and explain why this is needed
+            // Note, that to keep the code simple, while avoiding "stack too deep" error,
+            // this `bridgeData` variable is reused in two places with different meanings:
+            // - Firstly, it denotes the bridgeBurn data to be used for the NativeTokenVault
+            // - Secondly, after the call to `_burn` function, it denotes the `bridgeMint` data that
+            // will be sent to the L2 counterpart of the L1NTV.
             bytes memory bridgeData = DataEncoding.encodeBridgeBurnData(_amount, _l2Receiver, _l1Token);
             // Inner call to encode data to decrease local var numbers
             _assetId = _ensureTokenRegisteredWithNTV(_l1Token);
@@ -538,8 +548,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
                 revert LegacyBridgeUsesNonNativeToken();
             }
 
-            IERC20(_l1Token).forceApprove(address(nativeTokenVault), _amount);
-
+            // Note, that starting from here `bridgeData` starts denoting bridgeMintData.
             bridgeData = _burn({
                 _chainId: ERA_CHAIN_ID,
                 _nextMsgValue: 0,
@@ -592,7 +601,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
             l2DepositTxHash: txHash,
             from: _originalCaller,
             to: _l2Receiver,
-            l1Asset: _l1Token,
+            l1Token: _l1Token,
             amount: _amount
         });
     }

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

@@ -7,14 +7,13 @@ 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";
 import {IL1ERC20Bridge} from "../interfaces/IL1ERC20Bridge.sol";
 
 import {IBridgehub} from "../../bridgehub/IBridgehub.sol";
 import {AddressAliasHelper} from "../../vendor/AddressAliasHelper.sol";
+import {ReentrancyGuard} from "../../common/ReentrancyGuard.sol";
 
 import {L2_NATIVE_TOKEN_VAULT_ADDR, L2_BRIDGEHUB_ADDR} from "../../common/L2ContractAddresses.sol";
 import {L2ContractHelper} from "../../common/libraries/L2ContractHelper.sol";
@@ -25,7 +24,7 @@ import {TokenNotLegacy, EmptyAddress, InvalidCaller, AmountMustBeGreaterThanZero
 /// @custom:security-contact [email protected]
 /// @notice The "default" bridge implementation for the ERC20 tokens. Note, that it does not
 /// support any custom token logic, i.e. rebase tokens' functionality is not supported.
-contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
+contract L2AssetRouter is AssetRouterBase, IL2AssetRouter, ReentrancyGuard {
     /// @dev The address of the L2 legacy shared bridge.
     address public immutable L2_LEGACY_SHARED_BRIDGE;
 
@@ -49,8 +48,8 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     }
 
     /// @notice Checks that the message sender is the L1 Asset Router.
-    modifier onlyAssetRouterCounterpartOrSelf(uint256 _originChainId) {
-        if (_originChainId == L1_CHAIN_ID) {
+    modifier onlyAssetRouterCounterpartOrSelf(uint256 _chainId) {
+        if (_chainId == L1_CHAIN_ID) {
             // Only the L1 Asset Router counterpart can initiate and finalize the deposit.
             if ((AddressAliasHelper.undoL1ToL2Alias(msg.sender) != L1_ASSET_ROUTER) && (msg.sender != address(this))) {
                 revert InvalidCaller(msg.sender);
@@ -86,7 +85,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         address _legacySharedBridge,
         bytes32 _baseTokenAssetId,
         address _aliasedOwner
-    ) AssetRouterBase(_l1ChainId, _eraChainId, IBridgehub(L2_BRIDGEHUB_ADDR)) {
+    ) AssetRouterBase(_l1ChainId, _eraChainId, IBridgehub(L2_BRIDGEHUB_ADDR)) reentrancyGuardInitializer {
         L2_LEGACY_SHARED_BRIDGE = _legacySharedBridge;
         if (_l1AssetRouter == address(0)) {
             revert EmptyAddress();
@@ -133,7 +132,13 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         uint256,
         bytes32 _assetId,
         bytes calldata _transferData
-    ) public payable override(AssetRouterBase, IAssetRouterBase) onlyAssetRouterCounterpartOrSelf(L1_CHAIN_ID) {
+    )
+        public
+        payable
+        override(AssetRouterBase, IAssetRouterBase)
+        onlyAssetRouterCounterpartOrSelf(L1_CHAIN_ID)
+        nonReentrant
+    {
         if (_assetId == BASE_TOKEN_ASSET_ID) {
             revert AssetIdNotSupported(BASE_TOKEN_ASSET_ID);
         }
@@ -148,7 +153,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     /// that rely on it must be upgradeable.
     /// @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) {
+    function withdraw(bytes32 _assetId, bytes memory _assetData) public override nonReentrant returns (bytes32) {
         return _withdrawSender(_assetId, _assetData, msg.sender, true);
     }
 
@@ -195,6 +200,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
             if (l1Token == address(0)) {
                 revert AssetIdNotSupported(_assetId);
             }
+            // slither-disable-next-line unused-return
             (uint256 amount, address l1Receiver, ) = DataEncoding.decodeBridgeBurnData(_assetData);
             message = _getSharedBridgeWithdrawMessage(l1Receiver, l1Token, amount);
             txHash = IL2SharedBridgeLegacy(L2_LEGACY_SHARED_BRIDGE).sendMessageToL1(message);
@@ -286,7 +292,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
     /// @param _l1Receiver The account address that should receive funds on L1
     /// @param _l2Token The L2 token address which is withdrawn
     /// @param _amount The total amount of tokens to be withdrawn
-    function withdraw(address _l1Receiver, address _l2Token, uint256 _amount) external {
+    function withdraw(address _l1Receiver, address _l2Token, uint256 _amount) external nonReentrant {
         if (_amount == 0) {
             revert AmountMustBeGreaterThanZero();
         }
@@ -304,7 +310,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
         address _l2Token,
         uint256 _amount,
         address _sender
-    ) external onlyLegacyBridge {
+    ) external onlyLegacyBridge nonReentrant {
         _withdrawLegacy(_l1Receiver, _l2Token, _amount, _sender);
     }
 
@@ -331,7 +337,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
             return address(0);
         }
 
-        return IBridgedStandardToken(_l2Token).l1Address();
+        return IBridgedStandardToken(_l2Token).originToken();
     }
 
     /// @notice Legacy function used for backward compatibility to return L2 wrapped token