Merge branch 'kl/custom-asset-bridging' of ssh://github.com/matter-la…

…bs/era-contracts into kl/more-system-contracts

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

@@ -145,3 +145,73 @@ jobs:
 
       - name: Compare
         run: diff tools/data/Verifier.sol l1-contracts/contracts/state-transition/Verifier.sol
+
+  coverage:
+    defaults:
+      run:
+        working-directory: l1-contracts
+    needs: [build, lint]
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout the repository
+        uses: actions/checkout@v4
+        with:
+          submodules: recursive
+
+      - name: Use Foundry
+        uses: foundry-rs/foundry-toolchain@v1
+
+      - name: Use Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: 18.18.0
+          cache: yarn
+
+      - name: Install dependencies
+        run: yarn
+
+      - name: Restore artifacts cache
+        uses: actions/cache/restore@v3
+        with:
+          fail-on-cache-miss: true
+          key: artifacts-l1-${{ github.sha }}
+          path: |
+            l1-contracts/artifacts
+            l1-contracts/cache
+            l1-contracts/typechain
+
+      - name: Run coverage
+        run: FOUNDRY_PROFILE=default yarn test:foundry && FOUNDRY_PROFILE=default yarn coverage:foundry --report summary --report lcov
+
+      # 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/*' --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
+      # not met. The below step can post a comment (when it's `github-token` is specified) but it's
+      # not as useful, and this action cannot fail CI based on a minimum coverage threshold, which
+      # is why we use both in this way.
+      - name: Post coverage report
+        if: github.event_name == 'pull_request' # This action fails when ran outside of a pull request.
+        uses: romeovs/[email protected]
+        with:
+          delete-old-comments: true
+          lcov-file: ./l1-contracts/lcov.info
+          github-token: ${{ secrets.GITHUB_TOKEN }} # Adds a coverage summary comment to the PR.
+
+      - name: Verify minimum coverage
+        uses: zgosalvez/github-actions-report-lcov@v2
+        with:
+          coverage-files: ./l1-contracts/lcov.info
+          working-directory: l1-contracts
+          minimum-coverage: 85 # Set coverage threshold.

.gitignore

@@ -26,3 +26,4 @@ l1-contracts/broadcast/*
 l1-contracts/script-config/*
 l1-contracts/script-out/*
 !l1-contracts/script-out/.gitkeep
+*.timestamp

contracts-review-prep.md

@@ -46,34 +46,13 @@ We need to either:
   for Markdown's syntax, type some text into the left window and
   watch the results in the right.
 
-### redundant call \_getAssetProperties
-
-> (, \_assetId) = \_getAssetProperties(\_assetId); // Handles the non-legacy case
-
-Most likely a redundant call:
-
-- for the new assets not needed
-- for legacy tokens the handleLegacyData already provided the correct data.
-
-This one could be used in theory if the data has new format, but still uses the address instead of normal id. However, not sure we should support such cases, so it will be removed and tests updated.
-
 ### not allowing legacy withdrawals
 
 > require(!\_isEraLegacyEthWithdrawal(\_chainId, \_l2BatchNumber), "ShB: legacy eth withdrawal");
 
 No method to finalize an old withdrawal.
 We will manually finalize all legacy withdrawals before the upgrade, i.e. withdrawals that happened before the previous Bridgehub upgrade.
 
-### empty branch when matching function signatures
-
-> (assetId, offset) = UnsafeBytes.readBytes32(\_l2ToL1message, offset);
-
-        transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset);
-    } else if (bytes4(functionSignature) == this.finalizeWithdrawal.selector) {
-        //todo
-
-Currently, the support of old selector with ERC20 is completed, but the new format with chainId in the `finalizeWithdraw` selector hasn't been implemented in the `_parseL2WithdrawalMessage` function.
-
 ### Custom Errors not implemented
 
 > require(expectedDepositAmount == \_depositAmount, "3T"); // The token has non-standard transfer logic
@@ -82,5 +61,5 @@ Custom errors will be introduced for all contracts.
 
 ## Migration plan
 
-- Bulkheads will need o be migrated
-- Tokens will have to be transferred
+- Bulkheads will need to be migrated (methods added)
+- Tokens will have to be transferred (methods added)

l1-contracts/contracts/bridge/L1NativeTokenVault.sol

@@ -50,6 +50,10 @@ contract L1NativeTokenVault is
     /// NOTE: this function may be removed in the future, don't rely on it!
     mapping(uint256 chainId => mapping(address l1Token => uint256 balance)) public chainBalance;
 
+    /// @dev Maps token balances for each chain to verify that chain balance was migrated from shared bridge.
+    /// This serves only for migration purposes.
+    mapping(uint256 chainId => mapping(address l1Token => bool migrated)) public chainBalanceMigrated;
+
     /// @dev A mapping assetId => tokenAddress
     mapping(bytes32 assetId => address tokenAddress) public tokenAddress;
 
@@ -65,6 +69,12 @@ contract L1NativeTokenVault is
         _;
     }
 
+    /// @notice Checks that the message sender is the shared bridge itself.
+    modifier onlySelf() {
+        require(msg.sender == address(this), "NTV only");
+        _;
+    }
+
     /// @dev Contract is expected to be used as proxy implementation.
     /// @dev Initialize the implementation to prevent Parity hack.
     constructor(
@@ -86,6 +96,64 @@ contract L1NativeTokenVault is
         _transferOwnership(_owner);
     }
 
+    /// @dev Accepts ether only from the Shared Bridge.
+    receive() external payable {
+        require(address(L1_SHARED_BRIDGE) == msg.sender, "NTV: ETH only accepted from Shared Bridge");
+    }
+
+    /// @dev Transfer tokens from shared bridge as part of migration process.
+    /// @param _token The address of token to be transferred (address(1) for ether and contract address for ERC20).
+    function transferFundsFromSharedBridge(address _token) external onlySelf {
+        if (_token == ETH_TOKEN_ADDRESS) {
+            uint256 balanceBefore = address(this).balance;
+            L1_SHARED_BRIDGE.transferEthToNTV();
+            uint256 balanceAfter = address(this).balance;
+            require(balanceAfter > balanceBefore, "NTV: 0 eth transferred");
+        } else {
+            uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
+            uint256 sharedBridgeChainBalance = IERC20(_token).balanceOf(address(L1_SHARED_BRIDGE));
+            require(sharedBridgeChainBalance > 0, "NTV: 0 amount to transfer");
+            L1_SHARED_BRIDGE.transferTokenToNTV(_token);
+            uint256 balanceAfter = IERC20(_token).balanceOf(address(this));
+            require(balanceAfter - balanceBefore >= sharedBridgeChainBalance, "NTV: wrong amount transferred");
+        }
+    }
+
+    /// @dev Set chain token balance as part of migration process.
+    /// @param _token The address of token to be transferred (address(1) for ether and contract address for ERC20).
+    /// @param _targetChainId The chain ID of the corresponding hyperchain.
+    function transferBalancesFromSharedBridge(address _token, uint256 _targetChainId) external onlySelf {
+        require(
+            chainBalanceMigrated[_targetChainId][_token] == false,
+            "NTV: chain balance for the token already migrated"
+        );
+        uint256 sharedBridgeChainBalance = L1_SHARED_BRIDGE.chainBalance(_targetChainId, _token);
+        chainBalance[_targetChainId][_token] = chainBalance[_targetChainId][_token] + sharedBridgeChainBalance;
+        chainBalanceMigrated[_targetChainId][_token] = true;
+    }
+
+    /// @dev Transfer tokens from shared bridge as part of migration process.
+    /// @dev Unlike `transferFundsFromSharedBridge` is provides a concrete limit on the gas used for the transfer and even if it will fail, it will not revert the whole transaction.
+    function safeTransferFundsFromSharedBridge(address _token, uint256 _gasPerToken) external {
+        try this.transferFundsFromSharedBridge{gas: _gasPerToken}(_token) {} catch {
+            // A reasonable amount of gas will be provided to transfer the token.
+            // If the transfer fails, we don't want to revert the whole transaction.
+        }
+    }
+
+    /// @dev Set chain token balance as part of migration process.
+    /// @dev Unlike `transferBalancesFromSharedBridge` is provides a concrete limit on the gas used for the transfer and even if it will fail, it will not revert the whole transaction.
+    function safeTransferBalancesFromSharedBridge(
+        address _token,
+        uint256 _targetChainId,
+        uint256 _gasPerToken
+    ) external {
+        try this.transferBalancesFromSharedBridge{gas: _gasPerToken}(_token, _targetChainId) {} catch {
+            // A reasonable amount of gas will be provided to transfer the token.
+            // If the transfer fails, we don't want to revert the whole transaction.
+        }
+    }
+
     /// @dev We want to be able to bridge native tokens automatically, this means registering them on the fly
     /// @notice Allows the bridge to register a token address for the vault.
     /// @notice No access control is ok, since the bridging of tokens should be permissionless. This requires permissionless registration.
@@ -196,7 +264,7 @@ contract L1NativeTokenVault is
             assembly {
                 callSuccess := call(gas(), _l1Receiver, amount, 0, 0, 0, 0)
             }
-            require(callSuccess, "NTV: withdraw failed");
+            require(callSuccess, "NTV: withdrawal failed, no funds or cannot transfer to receiver");
         } else {
             // Withdraw funds
             IERC20(l1Token).safeTransfer(_l1Receiver, amount);
@@ -222,7 +290,7 @@ contract L1NativeTokenVault is
             assembly {
                 callSuccess := call(gas(), _depositSender, _amount, 0, 0, 0, 0)
             }
-            require(callSuccess, "NTV: claimFailedDeposit failed");
+            require(callSuccess, "NTV: claimFailedDeposit failed, no funds or cannot transfer to receiver");
         } else {
             IERC20(l1Token).safeTransfer(_depositSender, _amount);
             // Note we don't allow weth deposits anymore, but there might be legacy weth deposits.

l1-contracts/contracts/bridge/L1SharedBridge.sol

@@ -89,6 +89,11 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
     // slither-disable-next-line uninitialized-state
     mapping(uint256 chainId => bool enabled) public hyperbridgingEnabled;
 
+    /// @dev Maps token balances for each chain to prevent unauthorized spending across hyperchains.
+    /// This serves as a security measure until hyperbridging is implemented.
+    /// NOTE: this function may be removed in the future, don't rely on it!
+    mapping(uint256 chainId => mapping(address l1Token => uint256 balance)) public chainBalance;
+
     /// @dev A mapping assetId => assetHandlerAddress
     /// @dev Tracks the address of Asset Handler contracts, where bridged funds are locked for each asset
     /// @dev P.S. this liquidity was locked directly in SharedBridge before
@@ -118,7 +123,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
     modifier onlyBridgehubOrEra(uint256 _chainId) {
         require(
             msg.sender == address(BRIDGE_HUB) || (_chainId == ERA_CHAIN_ID && msg.sender == ERA_DIAMOND_PROXY),
-            "L1SharedBridge: msg.value not equal to amount bridgehub or era chain"
+            "L1SharedBridge: msg.sender not equal to bridgehub or era chain"
         );
         _;
     }
@@ -165,6 +170,24 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
         }
     }
 
+    /// @dev transfer token to shared bridge as part of upgrade
+    function transferEthToNTV() external {
+        address ntvAddress = address(nativeTokenVault);
+        require(msg.sender == ntvAddress, "ShB: not NTV");
+        uint256 amount = address(this).balance;
+        bool callSuccess;
+        // Low-level assembly call, to avoid any memory copying (save gas)
+        assembly {
+            callSuccess := call(gas(), ntvAddress, amount, 0, 0, 0, 0)
+        }
+    }
+
+    /// @dev transfer token to shared bridge as part of upgrade
+    function transferTokenToNTV(address _token) external {
+        require(msg.sender == address(nativeTokenVault), "ShB: not NTV");
+        IERC20(_token).safeTransfer(address(nativeTokenVault), IERC20(_token).balanceOf(address(this)));
+    }
+
     /// @dev Sets the L1ERC20Bridge contract address. Should be called only once.
     function setL1Erc20Bridge(address _legacyBridge) external onlyOwner {
         require(address(legacyBridge) == address(0), "ShB: legacy bridge already set");
@@ -174,8 +197,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
 
     /// @dev Sets the L1ERC20Bridge contract address. Should be called only once.
     function setNativeTokenVault(IL1NativeTokenVault _nativeTokenVault) external onlyOwner {
-        require(address(nativeTokenVault) == address(0), "ShB: legacy bridge already set");
-        require(address(_nativeTokenVault) != address(0), "ShB: legacy bridge 0");
+        require(address(nativeTokenVault) == address(0), "ShB: native token vault already set");
+        require(address(_nativeTokenVault) != address(0), "ShB: native token vault 0");
         nativeTokenVault = _nativeTokenVault;
     }
 
@@ -189,7 +212,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
         address sender = msg.sender == address(nativeTokenVault) ? L2_NATIVE_TOKEN_VAULT_ADDRESS : msg.sender;
         bytes32 assetId = keccak256(abi.encode(uint256(block.chainid), sender, _additionalData));
         assetHandlerAddress[assetId] = _assetHandlerAddress;
-        assetDeploymentTracker[assetId] = sender;
+        assetDeploymentTracker[assetId] = msg.sender;
         emit AssetHandlerRegisteredInitial(assetId, _assetHandlerAddress, _additionalData, sender);
     }
 
@@ -226,6 +249,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
 
     /// @notice Allows bridgehub to acquire mintValue for L1->L2 transactions.
     /// @dev If the corresponding L2 transaction fails, refunds are issued to a refund recipient on L2.
+    /// @param _chainId The chain ID of the hyperchain to which deposit.
+    /// @param _assetId The deposited asset ID.
+    /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one.
+    /// @param _amount The total amount of tokens to be bridged.
     function bridgehubDepositBaseToken(
         uint256 _chainId,
         bytes32 _assetId,
@@ -258,7 +285,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
             : _assetId;
         l1AssetHandler = assetHandlerAddress[_assetId];
         // Check if no asset handler is set
-        if (l1AssetHandler == address(0) && uint256(_assetId) <= type(uint160).max) {
+        if (l1AssetHandler == address(0)) {
+            require(uint256(_assetId) <= type(uint160).max, "ShB: only address can be registered");
             l1AssetHandler = address(nativeTokenVault);
             nativeTokenVault.registerToken(address(uint160(uint256(_assetId))));
         }
@@ -310,6 +338,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
     }
 
     /// @notice Initiates a deposit transaction within Bridgehub, used by `requestL2TransactionTwoBridges`.
+    /// @param _chainId The chain ID of the hyperchain to which deposit.
+    /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one.
+    /// @param _l2Value The L2 `msg.value` from the L1 -> L2 deposit transaction.
+    /// @param _data The calldata for the second bridge deposit.
     function bridgehubDeposit(
         uint256 _chainId,
         address _prevMsgSender,
@@ -339,7 +371,6 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
 
         require(BRIDGE_HUB.baseTokenAssetId(_chainId) != _assetId, "ShB: baseToken deposit not supported");
 
-        (, _assetId) = _getAssetProperties(_assetId); // Handles the non-legacy case
         bytes memory bridgeMintCalldata = _burn({
             _chainId: _chainId,
             _l2Value: _l2Value,
@@ -413,6 +444,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
 
     /// @notice Confirms the acceptance of a transaction by the Mailbox, as part of the L2 transaction process within Bridgehub.
     /// This function is utilized by `requestL2TransactionTwoBridges` to validate the execution of a transaction.
+    /// @param _chainId The chain ID of the hyperchain to which confirm the deposit.
+    /// @param _txDataHash The keccak256 hash of abi.encode(msgSender, l1Token, amount)
+    /// @param _txHash The hash of the L1->L2 transaction to confirm the deposit.
     function bridgehubConfirmL2Transaction(
         uint256 _chainId,
         bytes32 _txDataHash,
@@ -663,12 +697,23 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
             transferData = abi.encode(amount, l1Receiver);
         } else if (bytes4(functionSignature) == IL1ERC20Bridge.finalizeWithdrawal.selector) {
             // We use the IL1ERC20Bridge for backward compatibility with old withdrawals.
-
+            address l1Token;
             // this message is a token withdrawal
-            (assetId, offset) = UnsafeBytes.readBytes32(_l2ToL1message, offset);
-            transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset);
+
+            // Check that the message length is correct.
+            // It should be equal to the length of the function signature + address + address + uint256 = 4 + 20 + 20 + 32 =
+            // 76 (bytes).
+            require(_l2ToL1message.length == 76, "ShB wrong msg len 2");
+            (l1Receiver, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset);
+            (l1Token, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset);
+            (amount, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset);
+
+            assetId = keccak256(abi.encode(block.chainid, NATIVE_TOKEN_VAULT_VIRTUAL_ADDRESS, l1Token));
+            transferData = abi.encode(amount, l1Receiver);
         } else if (bytes4(functionSignature) == this.finalizeWithdrawal.selector) {
             //todo
+            (assetId, offset) = UnsafeBytes.readBytes32(_l2ToL1message, offset);
+            transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset);
         } else {
             revert("ShB Incorrect message function selector");
         }
@@ -721,6 +766,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade
     /// 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
     /// newly-deployed token does not support any custom logic, i.e. rebase tokens' functionality is not supported.
+    /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one.
     /// @param _l2Receiver The account address that should receive funds on L2
     /// @param _l1Token The L1 token address which is deposited
     /// @param _amount The total amount of tokens to be bridged