From 502ee80e230548b3e72bcf9416e37c5419d3c213 Mon Sep 17 00:00:00 2001 From: Stanislav Bezkorovainyi Date: Mon, 9 Dec 2024 21:54:06 +0100 Subject: [PATCH] Support quick registration of NTV assets (#56) For better UX: - Allow NTV have bridgeBurnData with optional last tokenAddress. - If the `tokenAddress` is non zero, the NTV may attempt to register the token if asset handler is not yet present --- l1-contracts/contracts/bridge/L1Nullifier.sol | 9 +- .../bridge/asset-router/AssetRouterBase.sol | 20 +++- .../bridge/asset-router/IL2AssetRouter.sol | 2 - .../bridge/asset-router/L1AssetRouter.sol | 30 ++++-- .../bridge/asset-router/L2AssetRouter.sol | 30 ++---- .../bridge/ntv/INativeTokenVault.sol | 3 + .../bridge/ntv/L1NativeTokenVault.sol | 18 +++- .../bridge/ntv/L2NativeTokenVault.sol | 36 ++++++- .../contracts/bridge/ntv/NativeTokenVault.sol | 102 ++++++++++++++---- .../contracts/common/L1ContractErrors.sol | 3 + .../common/libraries/DataEncoding.sol | 39 ++++++- l1-contracts/deploy-scripts/Utils.sol | 30 +++--- .../upgrade/EcosystemUpgrade.s.sol | 16 ++- .../upgrade/FinalizeUpgrade.s.sol | 35 +++--- .../l1/integration/AssetRouterTest.t.sol | 2 +- .../L2Erc20TestAbstract.t.sol | 10 +- .../L1SharedBridge/L1SharedBridgeBase.t.sol | 4 +- .../L1SharedBridge/L1SharedBridgeFails.t.sol | 4 +- 18 files changed, 284 insertions(+), 109 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1Nullifier.sol b/l1-contracts/contracts/bridge/L1Nullifier.sol index defde97c7..a4bd5f489 100644 --- a/l1-contracts/contracts/bridge/L1Nullifier.sol +++ b/l1-contracts/contracts/bridge/L1Nullifier.sol @@ -646,8 +646,8 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable, assetId = DataEncoding.encodeNTVAssetId(block.chainid, _l1Token); } // For legacy deposits, the l2 receiver is not required to check tx data hash - // bytes memory transferData = abi.encode(_amount, _depositSender); - bytes memory assetData = abi.encode(_amount, address(0)); + // The token address does not have to be provided for this functionality either. + bytes memory assetData = DataEncoding.encodeBridgeBurnData(_amount, address(0), address(0)); _verifyAndClearFailedTransfer({ _checkedInLegacyBridge: false, @@ -696,7 +696,10 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable, uint16 _l2TxNumberInBatch, bytes32[] calldata _merkleProof ) external override onlyLegacyBridge { - bytes memory assetData = abi.encode(_amount, _depositSender); + // For legacy deposits, the l2 receiver is not required to check tx data hash + // The token address does not have to be provided for this functionality either. + 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); diff --git a/l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol b/l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol index c57c3a8f9..854c86b18 100644 --- a/l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol +++ b/l1-contracts/contracts/bridge/asset-router/AssetRouterBase.sol @@ -15,7 +15,8 @@ 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 /// @custom:security-contact security@matterlabs.dev @@ -133,6 +134,7 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable, /// @param _originalCaller The `msg.sender` address from the external call that initiated current one. /// @param _transferData The encoded data, which is used by the asset handler to determine L2 recipient and amount. Might include extra information. /// @param _passValue Boolean indicating whether to pass msg.value in the call. + /// @param _nativeTokenVault The address of the native token vault. /// @return bridgeMintCalldata The calldata used by remote asset handler to mint tokens for recipient. function _burn( uint256 _chainId, @@ -140,11 +142,23 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable, bytes32 _assetId, address _originalCaller, bytes memory _transferData, - bool _passValue + bool _passValue, + address _nativeTokenVault ) internal returns (bytes memory bridgeMintCalldata) { address l1AssetHandler = assetHandlerAddress[_assetId]; if (l1AssetHandler == address(0)) { - revert AssetHandlerDoesNotExist(_assetId); + // 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 + // more advanced error handling requires more extensive code and will be added in the future releases. + INativeTokenVault(_nativeTokenVault).tryRegisterTokenFromBurnData(_transferData, _assetId); + + // We do not do any additional transformations here (like setting `assetHandler` in the mapping), + // because we expect that all those happened inside `tryRegisterTokenFromBurnData` + + l1AssetHandler = _nativeTokenVault; } uint256 msgValue = _passValue ? msg.value : 0; diff --git a/l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol index 1839a4e77..8867c8856 100644 --- a/l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol +++ b/l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol @@ -36,6 +36,4 @@ interface IL2AssetRouter is IAssetRouterBase { /// a legacy asset. /// @param _assetId The assetId of the legacy token. function setLegacyTokenAssetHandler(bytes32 _assetId) external; - - function withdrawToken(address _l2NativeToken, bytes memory _assetData) external returns (bytes32); } diff --git a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol index f052cb28b..e8a8ded18 100644 --- a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol +++ b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol @@ -215,7 +215,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { _msgValue: 0, _assetId: _assetId, _originalCaller: _originalCaller, - _data: abi.encode(_amount, address(0)) + _data: DataEncoding.encodeBridgeBurnData(_amount, address(0), address(0)) }); // Note that we don't save the deposited amount, as this is for the base token, which gets sent to the refundRecipient if the tx fails @@ -267,17 +267,20 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { revert AssetIdNotSupported(assetId); } + address ntvCached = address(nativeTokenVault); + bytes memory bridgeMintCalldata = _burn({ _chainId: _chainId, _nextMsgValue: _value, _assetId: assetId, _originalCaller: _originalCaller, _transferData: transferData, - _passValue: true + _passValue: true, + _nativeTokenVault: ntvCached }); bytes32 txDataHash = DataEncoding.encodeTxDataHash({ - _nativeTokenVault: address(nativeTokenVault), + _nativeTokenVault: ntvCached, _encodingVersion: encodingVersion, _originalCaller: _originalCaller, _assetId: assetId, @@ -392,7 +395,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { } } - return (assetId, abi.encode(_depositAmount, _l2Receiver)); + return (assetId, DataEncoding.encodeBridgeBurnData(_depositAmount, _l2Receiver, _l1Token)); } /// @notice Ensures that token is registered with native token vault. @@ -526,7 +529,12 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { bytes32 _assetId; { - bytes memory bridgeMintCalldata; + // 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); // Legacy bridge is only expected to use native tokens for L1. @@ -536,16 +544,18 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { IERC20(_l1Token).forceApprove(address(nativeTokenVault), _amount); - bridgeMintCalldata = _burn({ + // Note, that starting from here `bridgeData` starts denoting bridgeMintData. + bridgeData = _burn({ _chainId: ERA_CHAIN_ID, _nextMsgValue: 0, _assetId: _assetId, _originalCaller: _originalCaller, - _transferData: abi.encode(_amount, _l2Receiver), - _passValue: false + _transferData: bridgeData, + _passValue: false, + _nativeTokenVault: address(nativeTokenVault) }); - bytes memory l2TxCalldata = getDepositCalldata(_originalCaller, _assetId, bridgeMintCalldata); + bytes memory l2TxCalldata = getDepositCalldata(_originalCaller, _assetId, bridgeData); // If the refund recipient is not specified, the refund will be sent to the sender of the transaction. // Otherwise, the refund will be sent to the specified address. @@ -567,7 +577,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { } { - bytes memory transferData = abi.encode(_amount, _l2Receiver); + bytes memory transferData = DataEncoding.encodeBridgeBurnData(_amount, _l2Receiver, _l1Token); // Save the deposited amount to claim funds on L1 if the deposit failed on L2 L1_NULLIFIER.bridgehubConfirmL2TransactionForwarded( ERA_CHAIN_ID, diff --git a/l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol index 382524147..0339829e6 100644 --- a/l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol +++ b/l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol @@ -7,9 +7,7 @@ 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"; @@ -152,18 +150,6 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter { return _withdrawSender(_assetId, _assetData, msg.sender, true); } - /// @dev IMPORTANT: this method will be deprecated in one of the future releases, so contracts - /// that rely on it must be upgradeable. - 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 != block.chainid && recordedOriginChainId != 0) { - revert AssetIdNotSupported(recordedAssetId); - } - bytes32 assetId = _ensureTokenRegisteredWithNTV(_l2NativeToken); - return _withdrawSender(assetId, _assetData, msg.sender, true); - } - /*////////////////////////////////////////////////////////////// Internal & Helpers //////////////////////////////////////////////////////////////*/ @@ -185,18 +171,19 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter { address _sender, bool _alwaysNewMessageFormat ) internal returns (bytes32 txHash) { - address assetHandler = assetHandlerAddress[_assetId]; - bytes memory _l1bridgeMintData = IAssetHandler(assetHandler).bridgeBurn({ + bytes memory l1bridgeMintData = _burn({ _chainId: L1_CHAIN_ID, - _msgValue: 0, + _nextMsgValue: 0, _assetId: _assetId, _originalCaller: _sender, - _data: _assetData + _transferData: _assetData, + _passValue: false, + _nativeTokenVault: L2_NATIVE_TOKEN_VAULT_ADDR }); bytes memory message; if (_alwaysNewMessageFormat || L2_LEGACY_SHARED_BRIDGE == address(0)) { - message = _getAssetRouterWithdrawMessage(_assetId, _l1bridgeMintData); + message = _getAssetRouterWithdrawMessage(_assetId, l1bridgeMintData); // slither-disable-next-line unused-return txHash = L2ContractHelper.sendMessageToL1(message); } else { @@ -206,7 +193,8 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter { if (l1Token == address(0)) { revert AssetIdNotSupported(_assetId); } - (uint256 amount, address l1Receiver) = abi.decode(_assetData, (uint256, address)); + // 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); } @@ -325,7 +313,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter { revert TokenNotLegacy(); } bytes32 assetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, l1Address); - bytes memory data = abi.encode(_amount, _l1Receiver); + bytes memory data = DataEncoding.encodeBridgeBurnData(_amount, _l1Receiver, _l2Token); _withdrawSender(assetId, data, _sender, false); } diff --git a/l1-contracts/contracts/bridge/ntv/INativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/INativeTokenVault.sol index 12718bd6f..39ca0e20f 100644 --- a/l1-contracts/contracts/bridge/ntv/INativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/ntv/INativeTokenVault.sol @@ -44,4 +44,7 @@ interface INativeTokenVault { /// @notice Used to get the expected bridged token address corresponding to its native counterpart function calculateCreate2TokenAddress(uint256 _originChainId, address _originToken) external view returns (address); + + /// @notice Tries to register a token from the provided `_burnData` and reverts if it is not possible. + function tryRegisterTokenFromBurnData(bytes calldata _burnData, bytes32 _expectedAssetId) external; } diff --git a/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol index 9066691a1..e93511b7b 100644 --- a/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol @@ -164,10 +164,10 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken address _originalCaller, // solhint-disable-next-line no-unused-vars bool _depositChecked, - bytes calldata _data + uint256 _depositAmount, + address _receiver, + address _nativeToken ) internal override returns (bytes memory _bridgeMintData) { - uint256 _depositAmount; - (_depositAmount, ) = abi.decode(_data, (uint256, address)); bool depositChecked = IL1AssetRouter(address(ASSET_ROUTER)).transferFundsToNTV( _assetId, _depositAmount, @@ -178,7 +178,9 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken _assetId: _assetId, _originalCaller: _originalCaller, _depositChecked: depositChecked, - _data: _data + _depositAmount: _depositAmount, + _receiver: _receiver, + _nativeToken: _nativeToken }); } @@ -193,7 +195,8 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken address _depositSender, bytes calldata _data ) external payable override requireZeroValue(msg.value) onlyAssetRouter whenNotPaused { - (uint256 _amount, ) = abi.decode(_data, (uint256, address)); + // slither-disable-next-line unused-return + (uint256 _amount, , ) = DataEncoding.decodeBridgeBurnData(_data); address l1Token = tokenAddress[_assetId]; if (_amount == 0) { revert NoFundsTransferred(); @@ -228,6 +231,11 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken INTERNAL & HELPER FUNCTIONS //////////////////////////////////////////////////////////////*/ + function _registerTokenIfBridgedLegacy(address) internal override returns (bytes32) { + // There are no legacy tokens present on L1. + return bytes32(0); + } + // get the computed address before the contract DeployWithCreate2 deployed using Bytecode of contract DeployWithCreate2 and salt specified by the sender function calculateCreate2TokenAddress( uint256 _originChainId, diff --git a/l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol index 4a822719a..af4708169 100644 --- a/l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol @@ -23,7 +23,7 @@ import {L2ContractHelper, IContractDeployer} from "../../common/libraries/L2Cont import {SystemContractsCaller} from "../../common/libraries/SystemContractsCaller.sol"; import {DataEncoding} from "../../common/libraries/DataEncoding.sol"; -import {NoLegacySharedBridge, TokenIsLegacy, TokenIsNotLegacy, EmptyAddress, EmptyBytes32, AddressMismatch, DeployFailed, AssetIdNotSupported} from "../../common/L1ContractErrors.sol"; +import {AssetIdAlreadyRegistered, NoLegacySharedBridge, TokenIsLegacy, TokenIsNotLegacy, EmptyAddress, EmptyBytes32, AddressMismatch, DeployFailed, AssetIdNotSupported} from "../../common/L1ContractErrors.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev @@ -84,8 +84,29 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault { } } + function _registerTokenIfBridgedLegacy(address _tokenAddress) internal override returns (bytes32) { + // In zkEVM immutables are stored in a storage of a system contract, + // so it makes sense to cache them for efficiency. + IL2SharedBridgeLegacy legacyBridge = L2_LEGACY_SHARED_BRIDGE; + if (address(legacyBridge) == address(0)) { + // No legacy bridge, the token must be native + return bytes32(0); + } + + address l1TokenAddress = legacyBridge.l1TokenAddress(_tokenAddress); + if (l1TokenAddress == address(0)) { + // The token is not legacy + return bytes32(0); + } + + return _registerLegacyTokenAssetId(_tokenAddress, l1TokenAddress); + } + /// @notice Sets the legacy token asset ID for the given L2 token address. function setLegacyTokenAssetId(address _l2TokenAddress) public { + if (assetId[_l2TokenAddress] != bytes32(0)) { + revert AssetIdAlreadyRegistered(); + } if (address(L2_LEGACY_SHARED_BRIDGE) == address(0)) { revert NoLegacySharedBridge(); } @@ -93,8 +114,15 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault { if (l1TokenAddress == address(0)) { revert TokenIsNotLegacy(); } - bytes32 newAssetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, l1TokenAddress); + _registerLegacyTokenAssetId(_l2TokenAddress, l1TokenAddress); + } + + function _registerLegacyTokenAssetId( + address _l2TokenAddress, + address _l1TokenAddress + ) internal returns (bytes32 newAssetId) { + newAssetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, _l1TokenAddress); IL2AssetRouter(L2_ASSET_ROUTER_ADDR).setLegacyTokenAssetHandler(newAssetId); tokenAddress[newAssetId] = _l2TokenAddress; assetId[_l2TokenAddress] = newAssetId; @@ -253,7 +281,7 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault { // on L2s we don't track the balance } - function _registerToken(address _nativeToken) internal override { + function _registerToken(address _nativeToken) internal override returns (bytes32) { if ( address(L2_LEGACY_SHARED_BRIDGE) != address(0) && L2_LEGACY_SHARED_BRIDGE.l1TokenAddress(_nativeToken) != address(0) @@ -261,7 +289,7 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault { // Legacy tokens should be registered via `setLegacyTokenAssetId`. revert TokenIsLegacy(); } - super._registerToken(_nativeToken); + return super._registerToken(_nativeToken); } /*////////////////////////////////////////////////////////////// diff --git a/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol index f760660f2..65b048e3d 100644 --- a/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol @@ -93,7 +93,7 @@ abstract contract NativeTokenVault is _registerToken(_nativeToken); } - function _registerToken(address _nativeToken) internal virtual { + function _registerToken(address _nativeToken) internal virtual returns (bytes32 newAssetId) { // We allow registering `WETH_TOKEN` inside `NativeTokenVault` only for L1 native token vault. // It is needed to allow withdrawing such assets. We restrict all WETH-related // operations to deposits from L1 only to be able to upgrade their logic more easily in the @@ -107,7 +107,7 @@ abstract contract NativeTokenVault is if (assetId[_nativeToken] != bytes32(0)) { revert AssetIdAlreadyRegistered(); } - _unsafeRegisterNativeToken(_nativeToken); + newAssetId = _unsafeRegisterNativeToken(_nativeToken); } /// @inheritdoc INativeTokenVault @@ -200,33 +200,98 @@ abstract contract NativeTokenVault is whenNotPaused returns (bytes memory _bridgeMintData) { + (uint256 amount, address receiver, address tokenAddress) = _decodeBurnAndCheckAssetId(_data, _assetId); if (originChainId[_assetId] != block.chainid) { - _bridgeMintData = _bridgeBurnBridgedToken(_chainId, _assetId, _originalCaller, _data); + _bridgeMintData = _bridgeBurnBridgedToken({ + _chainId: _chainId, + _assetId: _assetId, + _originalCaller: _originalCaller, + _amount: amount, + _receiver: receiver, + _tokenAddress: tokenAddress + }); } else { _bridgeMintData = _bridgeBurnNativeToken({ _chainId: _chainId, _assetId: _assetId, _originalCaller: _originalCaller, _depositChecked: false, - _data: _data + _depositAmount: amount, + _receiver: receiver, + _nativeToken: tokenAddress }); } } + function tryRegisterTokenFromBurnData(bytes calldata _data, bytes32 _expectedAssetId) external { + (uint256 amount, address receiver, address tokenAddress) = DataEncoding.decodeBridgeBurnData(_data); + + if (tokenAddress == address(0)) { + revert ZeroAddress(); + } + + bytes32 storedAssetId = assetId[tokenAddress]; + if (storedAssetId != bytes32(0)) { + revert AssetIdAlreadyRegistered(); + } + + // This token has not been registered within this NTV yet. Usually this means that the + // token is native to the chain and the user would prefer to get it registered as such. + // However, there are exceptions (e.g. bridged legacy ERC20 tokens on L2) when the + // assetId has not been stored yet. We will ask the implementor to double check that the token + // is not legacy. + + // We try to register it as legacy token. If it fails, we know + // it is a native one and so register it as a native token. + bytes32 newAssetId = _registerTokenIfBridgedLegacy(tokenAddress); + if (newAssetId == bytes32(0)) { + newAssetId = _registerToken(tokenAddress); + } + + if (newAssetId != _expectedAssetId) { + revert AssetIdMismatch(_expectedAssetId, newAssetId); + } + } + + function _decodeBurnAndCheckAssetId( + bytes calldata _data, + bytes32 _suppliedAssetId + ) internal returns (uint256 amount, address receiver, address parsedTokenAddress) { + (amount, receiver, parsedTokenAddress) = DataEncoding.decodeBridgeBurnData(_data); + + if (parsedTokenAddress == address(0)) { + // This means that the user wants the native token vault to resolve the + // address. In this case, it is assumed that the assetId is already registered. + parsedTokenAddress = tokenAddress[_suppliedAssetId]; + } + + // If it is still zero, it means that the token has not been registered. + if (parsedTokenAddress == address(0)) { + revert ZeroAddress(); + } + + bytes32 storedAssetId = assetId[parsedTokenAddress]; + if (_suppliedAssetId != storedAssetId) { + revert AssetIdMismatch(storedAssetId, _suppliedAssetId); + } + } + + function _registerTokenIfBridgedLegacy(address _token) internal virtual returns (bytes32); + function _bridgeBurnBridgedToken( uint256 _chainId, bytes32 _assetId, address _originalCaller, - bytes calldata _data + uint256 _amount, + address _receiver, + address _tokenAddress ) internal requireZeroValue(msg.value) returns (bytes memory _bridgeMintData) { - (uint256 _amount, address _receiver) = abi.decode(_data, (uint256, address)); if (_amount == 0) { // "Amount cannot be zero"); revert AmountMustBeGreaterThanZero(); } - address bridgedToken = tokenAddress[_assetId]; - IBridgedStandardToken(bridgedToken).bridgeBurn(_originalCaller, _amount); + IBridgedStandardToken(_tokenAddress).bridgeBurn(_originalCaller, _amount); _handleChainBalanceIncrease(_chainId, _assetId, _amount, false); emit BridgeBurn({ @@ -244,11 +309,11 @@ abstract contract NativeTokenVault is if (originChainId == 0) { revert ZeroAddress(); } - erc20Metadata = getERC20Getters(bridgedToken, originChainId); + erc20Metadata = getERC20Getters(_tokenAddress, originChainId); } address originToken; { - originToken = IBridgedStandardToken(bridgedToken).originToken(); + originToken = IBridgedStandardToken(_tokenAddress).originToken(); if (originToken == address(0)) { revert ZeroAddress(); } @@ -268,16 +333,17 @@ abstract contract NativeTokenVault is bytes32 _assetId, address _originalCaller, bool _depositChecked, - bytes calldata _data + uint256 _depositAmount, + address _receiver, + address _nativeToken ) internal virtual returns (bytes memory _bridgeMintData) { - (uint256 _depositAmount, address _receiver) = abi.decode(_data, (uint256, address)); - address nativeToken = tokenAddress[_assetId]; if (nativeToken == WETH_TOKEN) { // This ensures that WETH_TOKEN can never be bridged from chains it is native to. // It can only be withdrawn from the chain where it has already gotten. revert BurningNativeWETHNotSupported(); } + if (_assetId == BASE_TOKEN_ASSET_ID) { if (_depositAmount != msg.value) { revert ValueMismatch(_depositAmount, msg.value); @@ -291,7 +357,7 @@ abstract contract NativeTokenVault is } _handleChainBalanceIncrease(_chainId, _assetId, _depositAmount, true); if (!_depositChecked) { - uint256 expectedDepositAmount = _depositFunds(_originalCaller, IERC20(nativeToken), _depositAmount); // note if _originalCaller is this contract, this will return 0. This does not happen. + uint256 expectedDepositAmount = _depositFunds(_originalCaller, IERC20(_nativeToken), _depositAmount); // note if _originalCaller is this contract, this will return 0. This does not happen. // The token has non-standard transfer logic if (_depositAmount != expectedDepositAmount) { revert TokensWithFeesNotSupported(); @@ -305,12 +371,12 @@ abstract contract NativeTokenVault is bytes memory erc20Metadata; { - erc20Metadata = getERC20Getters(nativeToken, originChainId[_assetId]); + erc20Metadata = getERC20Getters(_nativeToken, originChainId[_assetId]); } _bridgeMintData = DataEncoding.encodeBridgeMintData({ _originalCaller: _originalCaller, _remoteReceiver: _receiver, - _originToken: nativeToken, + _originToken: _nativeToken, _amount: _depositAmount, _erc20Metadata: erc20Metadata }); @@ -351,8 +417,8 @@ abstract contract NativeTokenVault is /// @notice Registers a native token address for the vault. /// @dev It does not perform any checks for the correctnesss of the token contract. /// @param _nativeToken The address of the token to be registered. - function _unsafeRegisterNativeToken(address _nativeToken) internal { - bytes32 newAssetId = DataEncoding.encodeNTVAssetId(block.chainid, _nativeToken); + function _unsafeRegisterNativeToken(address _nativeToken) internal returns (bytes32 newAssetId) { + newAssetId = DataEncoding.encodeNTVAssetId(block.chainid, _nativeToken); tokenAddress[newAssetId] = _nativeToken; assetId[_nativeToken] = newAssetId; originChainId[newAssetId] = block.chainid; diff --git a/l1-contracts/contracts/common/L1ContractErrors.sol b/l1-contracts/contracts/common/L1ContractErrors.sol index 2cc93bc28..06aaf0fda 100644 --- a/l1-contracts/contracts/common/L1ContractErrors.sol +++ b/l1-contracts/contracts/common/L1ContractErrors.sol @@ -377,6 +377,9 @@ error NoLegacySharedBridge(); // 0x78d2ed02 error ChainAlreadyLive(); +// 0xde4c0b96 +error InvalidNTVBurnData(); + enum SharedBridgeKey { PostUpgradeFirstBatch, LegacyBridgeFirstBatch, diff --git a/l1-contracts/contracts/common/libraries/DataEncoding.sol b/l1-contracts/contracts/common/libraries/DataEncoding.sol index 46441fccc..4c623febb 100644 --- a/l1-contracts/contracts/common/libraries/DataEncoding.sol +++ b/l1-contracts/contracts/common/libraries/DataEncoding.sol @@ -5,7 +5,7 @@ pragma solidity 0.8.24; import {L2_NATIVE_TOKEN_VAULT_ADDR} from "../L2ContractAddresses.sol"; import {LEGACY_ENCODING_VERSION, NEW_ENCODING_VERSION} from "../../bridge/asset-router/IAssetRouterBase.sol"; import {INativeTokenVault} from "../../bridge/ntv/INativeTokenVault.sol"; -import {IncorrectTokenAddressFromNTV, UnsupportedEncodingVersion} from "../L1ContractErrors.sol"; +import {IncorrectTokenAddressFromNTV, UnsupportedEncodingVersion, InvalidNTVBurnData} from "../L1ContractErrors.sol"; /** * @author Matter Labs @@ -13,6 +13,41 @@ import {IncorrectTokenAddressFromNTV, UnsupportedEncodingVersion} from "../L1Con * @notice Helper library for transfer data encoding and decoding to reduce possibility of errors. */ library DataEncoding { + /// @notice Abi.encodes the data required for bridgeBurn for NativeTokenVault. + /// @param _amount The amount of token to be transferred. + /// @param _remoteReceiver The address which to receive tokens on remote chain. + /// @param _maybeTokenAddress The helper field that should be either equal to 0 (in this case + /// it is assumed that the token has been registered within NativeTokenVault already) or it + /// can be equal to the address of the token on the current chain. Providing non-zero address + /// allows it to be automatically registered in case it is not yet a part of NativeTokenVault. + /// @return The encoded bridgeBurn data + function encodeBridgeBurnData( + uint256 _amount, + address _remoteReceiver, + address _maybeTokenAddress + ) internal pure returns (bytes memory) { + return abi.encode(_amount, _remoteReceiver, _maybeTokenAddress); + } + + /// @notice Function decoding bridgeBurn data previously encoded with this library. + /// @param _data The encoded data for bridgeBurn + /// @return amount The amount of token to be transferred. + /// @return receiver The address which to receive tokens on remote chain. + /// @return maybeTokenAddress The helper field that should be either equal to 0 (in this case + /// it is assumed that the token has been registered within NativeTokenVault already) or it + /// can be equal to the address of the token on the current chain. Providing non-zero address + /// allows it to be automatically registered in case it is not yet a part of NativeTokenVault. + function decodeBridgeBurnData( + bytes memory _data + ) internal pure returns (uint256 amount, address receiver, address maybeTokenAddress) { + if (_data.length != 96) { + // For better error handling + revert InvalidNTVBurnData(); + } + + (amount, receiver, maybeTokenAddress) = abi.decode(_data, (uint256, address, address)); + } + /// @notice Abi.encodes the data required for bridgeMint on remote chain. /// @param _originalCaller The address which initiated the transfer. /// @param _remoteReceiver The address which to receive tokens on remote chain. @@ -116,7 +151,7 @@ library DataEncoding { revert IncorrectTokenAddressFromNTV(_assetId, tokenAddress); } - (uint256 depositAmount, ) = abi.decode(_transferData, (uint256, address)); + (uint256 depositAmount, , ) = decodeBridgeBurnData(_transferData); txDataHash = keccak256(abi.encode(_originalCaller, tokenAddress, depositAmount)); } else if (_encodingVersion == NEW_ENCODING_VERSION) { // Similarly to calldata, the txDataHash is collision-resistant. diff --git a/l1-contracts/deploy-scripts/Utils.sol b/l1-contracts/deploy-scripts/Utils.sol index 2fca6c2b9..bde1dd959 100644 --- a/l1-contracts/deploy-scripts/Utils.sol +++ b/l1-contracts/deploy-scripts/Utils.sol @@ -211,24 +211,26 @@ library Utils { * @dev Returns the bytecode of a given system contract. */ function readPrecompileBytecode(string memory filename) internal view returns (bytes memory) { + string memory path = string.concat( + "/../system-contracts/zkout/", + filename, + ".yul/contracts-preprocessed/precompiles/", + filename, + ".yul.json" + ); + // It is the only exceptional case if (keccak256(abi.encodePacked(filename)) == keccak256(abi.encodePacked("EventWriter"))) { - return - vm.readFileBinary( - // solhint-disable-next-line func-named-parameters - string.concat("../system-contracts/contracts-preprocessed/artifacts/", filename, ".yul.zbin") - ); + path = string.concat( + "/../system-contracts/zkout/", + filename, + ".yul/contracts-preprocessed/", + filename, + ".yul.json" + ); } - return - vm.readFileBinary( - // solhint-disable-next-line func-named-parameters - string.concat( - "../system-contracts/contracts-preprocessed/precompiles/artifacts/", - filename, - ".yul.zbin" - ) - ); + return readFoundryBytecode(path); } /** diff --git a/l1-contracts/deploy-scripts/upgrade/EcosystemUpgrade.s.sol b/l1-contracts/deploy-scripts/upgrade/EcosystemUpgrade.s.sol index 3f21fdff2..e0bc48627 100644 --- a/l1-contracts/deploy-scripts/upgrade/EcosystemUpgrade.s.sol +++ b/l1-contracts/deploy-scripts/upgrade/EcosystemUpgrade.s.sol @@ -352,7 +352,7 @@ contract EcosystemUpgrade is Script { maxFeePerGas: 0, maxPriorityFeePerGas: 0, paymaster: uint256(uint160(address(0))), - nonce: 25, + nonce: getProtocolUpgradeNonce(), value: 0, reserved: [uint256(0), uint256(0), uint256(0), uint256(0)], // Note, that the data is empty, it will be fully composed inside the `GatewayUpgrade` contract @@ -368,7 +368,11 @@ contract EcosystemUpgrade is Script { } function getNewProtocolVersion() public returns (uint256) { - return 0x1900000000; + return 0x1b00000000; + } + + function getProtocolUpgradeNonce() public returns (uint256) { + return (getNewProtocolVersion() >> 32); } function getOldProtocolDeadline() public returns (uint256) { @@ -378,7 +382,12 @@ contract EcosystemUpgrade is Script { } function getOldProtocolVersion() public returns (uint256) { - return 0x1800000002; + // Mainnet is the only network that has not been upgraded. + if (block.chainid == 1) { + return 0x1800000002; + } else { + return 0x1900000000; + } } function provideSetNewVersionUpgradeCall() public returns (Call[] memory calls) { @@ -1144,7 +1153,6 @@ contract EcosystemUpgrade is Script { abi.encode( config.tokens.tokenWethAddress, addresses.bridges.sharedBridgeProxy, - config.eraChainId, config.contracts.oldSharedBridgeProxyAddress ) ); diff --git a/l1-contracts/deploy-scripts/upgrade/FinalizeUpgrade.s.sol b/l1-contracts/deploy-scripts/upgrade/FinalizeUpgrade.s.sol index 00a730194..6d2eb7c49 100644 --- a/l1-contracts/deploy-scripts/upgrade/FinalizeUpgrade.s.sol +++ b/l1-contracts/deploy-scripts/upgrade/FinalizeUpgrade.s.sol @@ -41,12 +41,7 @@ contract FinalizeUpgrade is Script { if (bh.baseTokenAssetId(chains[i]) == bytes32(0)) { vm.broadcast(); - Bridgehub(bridgehub).setLegacyBaseTokenAssetId(chains[i]); - } - - if (bh.getZKChain(chains[i]) == address(0)) { - vm.broadcast(); - Bridgehub(bridgehub).setLegacyChainAddress(chains[i]); + Bridgehub(bridgehub).registerLegacyChain(chains[i]); } } } @@ -63,18 +58,26 @@ contract FinalizeUpgrade is Script { for (uint256 i = 0; i < tokens.length; i++) { uint256 tokenBalance; - if (tokens[i] != ETH_TOKEN_ADDRESS) { - uint256 balance = IERC20(tokens[i]).balanceOf(nullifier); - if (balance != 0) { + if (vault.assetId(tokens[i]) == bytes32(0)) { + if (tokens[i] != ETH_TOKEN_ADDRESS) { + uint256 balance = IERC20(tokens[i]).balanceOf(nullifier); + if (balance != 0) { + vm.broadcast(); + vault.transferFundsFromSharedBridge(tokens[i]); + } else { + vm.broadcast(); + vault.registerToken(tokens[i]); + } + } else { vm.broadcast(); - vault.transferFundsFromSharedBridge(tokens[i]); - } + vault.registerEthToken(); - vm.broadcast(); - vault.registerToken(tokens[i]); - } else { - vm.broadcast(); - vault.registerEthToken(); + uint256 balance = address(nullifier).balance; + if (balance != 0) { + vm.broadcast(); + vault.transferFundsFromSharedBridge(tokens[i]); + } + } } // TODO: we need to reduce complexity of this one diff --git a/l1-contracts/test/foundry/l1/integration/AssetRouterTest.t.sol b/l1-contracts/test/foundry/l1/integration/AssetRouterTest.t.sol index 881c38e88..512dd203a 100644 --- a/l1-contracts/test/foundry/l1/integration/AssetRouterTest.t.sol +++ b/l1-contracts/test/foundry/l1/integration/AssetRouterTest.t.sol @@ -169,7 +169,7 @@ contract AssetRouterTest is L1ContractDeployer, ZKChainDeployer, TokenDeployer, depositToL1(ETH_TOKEN_ADDRESS); bytes memory secondBridgeCalldata = bytes.concat( NEW_ENCODING_VERSION, - abi.encode(l2TokenAssetId, abi.encode(uint256(100), address(this))) + abi.encode(l2TokenAssetId, abi.encode(uint256(100), address(this), tokenL1Address)) ); IERC20(tokenL1Address).approve(address(l1NativeTokenVault), 100); bridgehub.requestL2TransactionTwoBridges{value: 250000000000100}( diff --git a/l1-contracts/test/foundry/l1/integration/l2-tests-in-l1-context/L2Erc20TestAbstract.t.sol b/l1-contracts/test/foundry/l1/integration/l2-tests-in-l1-context/L2Erc20TestAbstract.t.sol index 049f7304f..c7e4d5ba5 100644 --- a/l1-contracts/test/foundry/l1/integration/l2-tests-in-l1-context/L2Erc20TestAbstract.t.sol +++ b/l1-contracts/test/foundry/l1/integration/l2-tests-in-l1-context/L2Erc20TestAbstract.t.sol @@ -12,6 +12,7 @@ import {IL2NativeTokenVault} from "contracts/bridge/ntv/IL2NativeTokenVault.sol" import {UpgradeableBeacon} from "@openzeppelin/contracts-v4/proxy/beacon/UpgradeableBeacon.sol"; import {BeaconProxy} from "@openzeppelin/contracts-v4/proxy/beacon/BeaconProxy.sol"; +import {DataEncoding} from "contracts/common/libraries/DataEncoding.sol"; import {L2_ASSET_ROUTER_ADDR, L2_NATIVE_TOKEN_VAULT_ADDR, L2_BRIDGEHUB_ADDR, L2_TO_L1_MESSENGER_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; import {ETH_TOKEN_ADDRESS, SETTLEMENT_LAYER_RELAY_SENDER} from "contracts/common/Config.sol"; @@ -99,7 +100,7 @@ abstract contract L2Erc20TestAbstract is Test, SharedL2ContractDeployer { BridgedStandardERC20(l2TokenAddress).reinitializeToken(getters, "TestTokenNewName", "TTN", 20); } - function test_withdrawToken() external { + function test_withdrawTokenNoRegistration() public { TestnetERC20Token l2NativeToken = new TestnetERC20Token("token", "T", 18); l2NativeToken.mint(address(this), 100); @@ -112,6 +113,11 @@ abstract contract L2Erc20TestAbstract is Test, SharedL2ContractDeployer { abi.encode(bytes32(uint256(1))) ); - IL2AssetRouter(L2_ASSET_ROUTER_ADDR).withdrawToken(address(l2NativeToken), abi.encode(100, address(1))); + bytes32 assetId = DataEncoding.encodeNTVAssetId(block.chainid, address(l2NativeToken)); + + IL2AssetRouter(L2_ASSET_ROUTER_ADDR).withdraw( + assetId, + DataEncoding.encodeBridgeBurnData(100, address(1), address(l2NativeToken)) + ); } } diff --git a/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol b/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol index 9f4c45d67..bd32d0430 100644 --- a/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol +++ b/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol @@ -206,7 +206,7 @@ contract L1AssetRouterTestBase is L1AssetRouterTest { } function test_bridgeRecoverFailedTransfer_Eth() public { - bytes memory transferData = abi.encode(amount, alice); + bytes memory transferData = abi.encode(amount, alice, ETH_TOKEN_ADDRESS); bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); _setSharedBridgeDepositHappened(chainId, txHash, txDataHash); require(l1Nullifier.depositHappened(chainId, txHash) == txDataHash, "Deposit not set"); @@ -502,7 +502,7 @@ contract L1AssetRouterTestBase is L1AssetRouterTest { _encodingVersion: LEGACY_ENCODING_VERSION, _originalCaller: alice, _assetId: nativeTokenVault.BASE_TOKEN_ASSET_ID(), - _transferData: abi.encode(amount, bob) + _transferData: abi.encode(amount, bob, ETH_TOKEN_ADDRESS) }); assertEq(request.txDataHash, expectedTxHash); diff --git a/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol b/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol index eae01e406..5c892cccb 100644 --- a/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol +++ b/l1-contracts/test/foundry/l1/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol @@ -278,7 +278,7 @@ contract L1AssetRouterFailTest is L1AssetRouterTest { function test_bridgeRecoverFailedTransfer_Eth_claimFailedDepositFailed() public { vm.deal(address(nativeTokenVault), 0); - bytes memory transferData = abi.encode(amount, alice); + bytes memory transferData = abi.encode(amount, alice, ETH_TOKEN_ADDRESS); bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); _setSharedBridgeDepositHappened(chainId, txHash, txDataHash); require(l1Nullifier.depositHappened(chainId, txHash) == txDataHash, "Deposit not set"); @@ -357,7 +357,7 @@ contract L1AssetRouterFailTest is L1AssetRouterTest { vm.store(address(l1Nullifier), bytes32(isWithdrawalFinalizedStorageLocation - 5), bytes32(uint256(2))); uint256 l2BatchNumber = 0; - bytes memory transferData = abi.encode(amount, alice); + bytes memory transferData = abi.encode(amount, alice, ETH_TOKEN_ADDRESS); bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); _setSharedBridgeDepositHappened(eraChainId, txHash, txDataHash); require(l1Nullifier.depositHappened(eraChainId, txHash) == txDataHash, "Deposit not set");