diff --git a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol index bc97e9bfa..9f478a598 100644 --- a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol +++ b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol @@ -507,9 +507,8 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { } bytes32 _assetId; - bytes memory bridgeMintCalldata; - { + bytes memory bridgeMintCalldata; // Inner call to encode data to decrease local var numbers _assetId = _ensureTokenRegisteredWithNTV(_l1Token); IERC20(_l1Token).forceApprove(address(nativeTokenVault), _amount); @@ -522,9 +521,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { _transferData: abi.encode(_amount, _l2Receiver), _passValue: false }); - } - { bytes memory l2TxCalldata = getDepositCalldata(_originalCaller, _assetId, bridgeMintCalldata); // If the refund recipient is not specified, the refund will be sent to the sender of the transaction. @@ -546,12 +543,21 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { txHash = BRIDGE_HUB.requestL2TransactionDirect{value: msg.value}(request); } - // Save the deposited amount to claim funds on L1 if the deposit failed on L2 - L1_NULLIFIER.bridgehubConfirmL2TransactionForwarded( - ERA_CHAIN_ID, - DataEncoding.encodeLegacyTxDataHash(_originalCaller, _l1Token, _amount), - txHash - ); + { + bytes memory transferData = abi.encode(_amount, _l2Receiver); + // Save the deposited amount to claim funds on L1 if the deposit failed on L2 + L1_NULLIFIER.bridgehubConfirmL2TransactionForwarded( + ERA_CHAIN_ID, + DataEncoding.encodeTxDataHash({ + _encodingVersion: LEGACY_ENCODING_VERSION, + _originalCaller: _originalCaller, + _assetId: _assetId, + _nativeTokenVault: address(nativeTokenVault), + _transferData: transferData + }), + txHash + ); + } emit LegacyDepositInitiated({ chainId: ERA_CHAIN_ID, diff --git a/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol index 805e1e041..12427d9ed 100644 --- a/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol @@ -21,7 +21,7 @@ import {DataEncoding} from "../../common/libraries/DataEncoding.sol"; import {BridgedStandardERC20} from "../BridgedStandardERC20.sol"; import {BridgeHelper} from "../BridgeHelper.sol"; -import {EmptyDeposit, Unauthorized, TokensWithFeesNotSupported, TokenNotSupported, NonEmptyMsgValue, ValueMismatch, AddressMismatch, AssetIdMismatch, AmountMustBeGreaterThanZero, ZeroAddress, DeployingBridgedTokenForNativeToken} from "../../common/L1ContractErrors.sol"; +import {AssetIdAlreadyRegistered, EmptyDeposit, Unauthorized, TokensWithFeesNotSupported, TokenNotSupported, NonEmptyMsgValue, ValueMismatch, AddressMismatch, AssetIdMismatch, AmountMustBeGreaterThanZero, ZeroAddress, DeployingBridgedTokenForNativeToken} from "../../common/L1ContractErrors.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev @@ -92,6 +92,9 @@ abstract contract NativeTokenVault is INativeTokenVault, IAssetHandler, Ownable2 revert TokenNotSupported(WETH_TOKEN); } require(_nativeToken.code.length > 0, "NTV: empty token"); + if (assetId[_nativeToken] != bytes32(0)) { + revert AssetIdAlreadyRegistered(); + } _unsafeRegisterNativeToken(_nativeToken); } diff --git a/l1-contracts/contracts/common/L1ContractErrors.sol b/l1-contracts/contracts/common/L1ContractErrors.sol index a31ac3f14..c4e6d6669 100644 --- a/l1-contracts/contracts/common/L1ContractErrors.sol +++ b/l1-contracts/contracts/common/L1ContractErrors.sol @@ -416,6 +416,8 @@ error NotBridgehub(address addr); error InvalidAddress(address expected, address actual); // 0xfa5cd00f error NotAllowed(address addr); +// 0x1929b7de +error IncorrectTokenAddressFromNTV(bytes32 assetId, address tokenAddress); enum SharedBridgeKey { PostUpgradeFirstBatch, diff --git a/l1-contracts/contracts/common/libraries/DataEncoding.sol b/l1-contracts/contracts/common/libraries/DataEncoding.sol index e586bd051..9a7616023 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 {UnsupportedEncodingVersion} from "../L1ContractErrors.sol"; +import {IncorrectTokenAddressFromNTV, UnsupportedEncodingVersion} from "../L1ContractErrors.sol"; /** * @author Matter Labs @@ -107,8 +107,17 @@ library DataEncoding { ) internal view returns (bytes32 txDataHash) { if (_encodingVersion == LEGACY_ENCODING_VERSION) { address tokenAddress = INativeTokenVault(_nativeTokenVault).tokenAddress(_assetId); + + // This is a double check to ensure that the used token for the legacy encoding is correct. + // This revert should never be emitted and in real life and should only serve as a guard in + // case of inconsistent state of Native Token Vault. + bytes32 expectedAssetId = encodeNTVAssetId(block.chainid, tokenAddress); + if (_assetId != expectedAssetId) { + revert IncorrectTokenAddressFromNTV(_assetId, tokenAddress); + } + (uint256 depositAmount, ) = abi.decode(_transferData, (uint256, address)); - txDataHash = encodeLegacyTxDataHash(_originalCaller, tokenAddress, depositAmount); + txDataHash = keccak256(abi.encode(_originalCaller, tokenAddress, depositAmount)); } else if (_encodingVersion == NEW_ENCODING_VERSION) { // Similarly to calldata, the txDataHash is collision-resistant. // In the legacy data hash, the first encoded variable was the address, which is padded with zeros during `abi.encode`. @@ -120,20 +129,6 @@ library DataEncoding { } } - /// @notice Encodes the legacy transaction data hash. - /// @dev the encoding strats with 0t - /// @param _originalCaller The address of the entity that initiated the deposit. - /// @param _l1Token The address of the L1 token. - /// @param _amount The amount of the L1 token. - /// @return txDataHash The resulting encoded transaction data hash. - function encodeLegacyTxDataHash( - address _originalCaller, - address _l1Token, - uint256 _amount - ) internal pure returns (bytes32) { - return keccak256(abi.encode(_originalCaller, _l1Token, _amount)); - } - /// @notice Decodes the token data by combining chain id, asset deployment tracker and asset data. function decodeTokenData( bytes calldata _tokenData 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 dd3b8c145..f74c190fd 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 @@ -107,7 +107,7 @@ contract L1AssetRouterTestBase is L1AssetRouterTest { vm.mockCall( address(nativeTokenVault), abi.encodeWithSelector(IL1BaseTokenAssetHandler.tokenAddress.selector, tokenAssetId), - abi.encode(address(0)) + abi.encode(address(token)) ); vm.prank(bridgehubAddress); sharedBridge.bridgehubDeposit(chainId, alice, 0, abi.encode(address(token), amount, bob)); diff --git a/l1-contracts/test/foundry/l1/unit/concrete/Bridges/NativeTokenVault/L1NativeTokenVault.sol b/l1-contracts/test/foundry/l1/unit/concrete/Bridges/NativeTokenVault/L1NativeTokenVault.sol new file mode 100644 index 000000000..3b9e9c6f1 --- /dev/null +++ b/l1-contracts/test/foundry/l1/unit/concrete/Bridges/NativeTokenVault/L1NativeTokenVault.sol @@ -0,0 +1,47 @@ +import {StdStorage, stdStorage} from "forge-std/Test.sol"; +import {Test} from "forge-std/Test.sol"; +import {Vm} from "forge-std/Vm.sol"; + +import {L1NativeTokenVault} from "contracts/bridge/ntv/L1NativeTokenVault.sol"; +import {L1AssetRouter} from "contracts/bridge/asset-router/L1AssetRouter.sol"; +import {IL1Nullifier} from "contracts/bridge/interfaces/IL1Nullifier.sol"; +import {AssetIdAlreadyRegistered} from "contracts/common/L1ContractErrors.sol"; + +contract SomeToken { + constructor() {} + + function name() external { + // Just some function so that the bytecode is not empty, + // the actional functionality is not used. + } +} + +contract L1NativeTokenVaultTest is Test { + address assetRouter; + + L1NativeTokenVault ntv; + SomeToken token; + + function setUp() public { + assetRouter = makeAddr("assetRouter"); + + ntv = new L1NativeTokenVault(makeAddr("wethToken"), assetRouter, uint256(0), IL1Nullifier(address(0))); + + token = new SomeToken(); + } + + function test_revertWhenRegisteringSameAddressTwice() external { + vm.mockCall( + assetRouter, + abi.encodeCall( + L1AssetRouter.setAssetHandlerAddressThisChain, + (bytes32(uint256(uint160(address(token)))), address(ntv)) + ), + hex"" + ); + ntv.registerToken(address(token)); + + vm.expectRevert(AssetIdAlreadyRegistered.selector); + ntv.registerToken(address(token)); + } +}