From 59d11f7ad7cb79d22268dc624c42b946af8dc0ff Mon Sep 17 00:00:00 2001 From: Stanislav Bezkorovainyi Date: Mon, 2 Dec 2024 16:26:33 +0100 Subject: [PATCH] Fix audittens m02 (#21) --- l1-contracts/contracts/bridge/L1ERC20Bridge.sol | 9 ++++----- .../contracts/bridge/asset-router/L1AssetRouter.sol | 9 ++++++++- .../contracts/bridge/interfaces/IAssetHandler.sol | 2 -- l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol | 2 -- l1-contracts/contracts/common/L1ContractErrors.sol | 4 ++++ .../state-transition/chain-deps/facets/Getters.sol | 1 - 6 files changed, 16 insertions(+), 11 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1ERC20Bridge.sol b/l1-contracts/contracts/bridge/L1ERC20Bridge.sol index 1ff15b810..bb3f6dd56 100644 --- a/l1-contracts/contracts/bridge/L1ERC20Bridge.sol +++ b/l1-contracts/contracts/bridge/L1ERC20Bridge.sol @@ -13,7 +13,7 @@ import {IL1AssetRouter} from "./asset-router/IL1AssetRouter.sol"; import {L2ContractHelper} from "../common/libraries/L2ContractHelper.sol"; import {ReentrancyGuard} from "../common/ReentrancyGuard.sol"; -import {EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported, ApprovalFailed} from "../common/L1ContractErrors.sol"; +import {AssetRouterAllowanceNotZero, EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported} from "../common/L1ContractErrors.sol"; import {ETH_TOKEN_ADDRESS} from "../common/Config.sol"; /// @author Matter Labs @@ -203,10 +203,9 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard { _l2TxGasPerPubdataByte: _l2TxGasPerPubdataByte, _refundRecipient: _refundRecipient }); - // clearing approval - bool success = IERC20(_l1Token).approve(address(L1_ASSET_ROUTER), 0); - if (!success) { - revert ApprovalFailed(); + // Ensuring that all the funds that were locked into this bridge were spent by the asset router / native token vault. + if (IERC20(_l1Token).allowance(address(this), address(L1_ASSET_ROUTER)) != 0) { + revert AssetRouterAllowanceNotZero(); } depositAmount[msg.sender][_l1Token][l2TxHash] = _amount; emit DepositInitiated({ diff --git a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol index 76ad029c1..4ae2b952d 100644 --- a/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol +++ b/l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol @@ -23,7 +23,7 @@ import {ReentrancyGuard} from "../../common/ReentrancyGuard.sol"; 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 {NonEmptyMsgValue, UnsupportedEncodingVersion, AssetIdNotSupported, AssetHandlerDoesNotExist, Unauthorized, ZeroAddress, TokenNotSupported, AddressAlreadyUsed} from "../../common/L1ContractErrors.sol"; +import {LegacyBridgeUsesNonNativeToken, NonEmptyMsgValue, UnsupportedEncodingVersion, AssetIdNotSupported, AssetHandlerDoesNotExist, Unauthorized, ZeroAddress, TokenNotSupported, AddressAlreadyUsed} from "../../common/L1ContractErrors.sol"; import {L2_ASSET_ROUTER_ADDR} from "../../common/L2ContractAddresses.sol"; import {IBridgehub, L2TransactionRequestTwoBridgesInner, L2TransactionRequestDirect} from "../../bridgehub/IBridgehub.sol"; @@ -529,6 +529,13 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard { bytes memory bridgeMintCalldata; // Inner call to encode data to decrease local var numbers _assetId = _ensureTokenRegisteredWithNTV(_l1Token); + // Legacy bridge is only expected to use native tokens for L1. + if (_assetId != DataEncoding.encodeNTVAssetId(block.chainid, _l1Token)) { + revert LegacyBridgeUsesNonNativeToken(); + } + + IERC20(_l1Token).forceApprove(address(nativeTokenVault), _amount); + bridgeMintCalldata = _burn({ _chainId: ERA_CHAIN_ID, _nextMsgValue: 0, diff --git a/l1-contracts/contracts/bridge/interfaces/IAssetHandler.sol b/l1-contracts/contracts/bridge/interfaces/IAssetHandler.sol index 720039da0..57f58eb59 100644 --- a/l1-contracts/contracts/bridge/interfaces/IAssetHandler.sol +++ b/l1-contracts/contracts/bridge/interfaces/IAssetHandler.sol @@ -2,8 +2,6 @@ pragma solidity 0.8.24; -import {NonEmptyMsgValue} from "../../common/L1ContractErrors.sol"; - /// @title Asset Handler contract interface /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev diff --git a/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol index cacf39811..383b980ef 100644 --- a/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol @@ -24,8 +24,6 @@ import {ETH_TOKEN_ADDRESS} from "../../common/Config.sol"; import {L2_NATIVE_TOKEN_VAULT_ADDR} from "../../common/L2ContractAddresses.sol"; import {DataEncoding} from "../../common/libraries/DataEncoding.sol"; -import {AssetHandlerModifiers} from "../interfaces/AssetHandlerModifiers.sol"; - import {Unauthorized, ZeroAddress, NoFundsTransferred, InsufficientChainBalance, WithdrawFailed, OriginChainIdNotFound} from "../../common/L1ContractErrors.sol"; /// @author Matter Labs diff --git a/l1-contracts/contracts/common/L1ContractErrors.sol b/l1-contracts/contracts/common/L1ContractErrors.sol index f7a829877..0316f192b 100644 --- a/l1-contracts/contracts/common/L1ContractErrors.sol +++ b/l1-contracts/contracts/common/L1ContractErrors.sol @@ -426,6 +426,10 @@ error InvalidProofLengthForFinalNode(); error TokenIsNotLegacy(); // 0xa51fa558 error TokenIsLegacy(); +// 0x29963361 +error LegacyBridgeUsesNonNativeToken(); +// 0x11832de8 +error AssetRouterAllowanceNotZero(); // 0xb20b58ce error NoLegacySharedBridge(); diff --git a/l1-contracts/contracts/state-transition/chain-deps/facets/Getters.sol b/l1-contracts/contracts/state-transition/chain-deps/facets/Getters.sol index f71d1e735..1e8ab1e54 100644 --- a/l1-contracts/contracts/state-transition/chain-deps/facets/Getters.sol +++ b/l1-contracts/contracts/state-transition/chain-deps/facets/Getters.sol @@ -14,7 +14,6 @@ import {IBridgehub} from "../../../bridgehub/IBridgehub.sol"; import {UncheckedMath} from "../../../common/libraries/UncheckedMath.sol"; import {IGetters} from "../../chain-interfaces/IGetters.sol"; import {ILegacyGetters} from "../../chain-interfaces/ILegacyGetters.sol"; -import {InvalidSelector} from "../../../common/L1ContractErrors.sol"; import {SemVer} from "../../../common/libraries/SemVer.sol"; // While formally the following import is not used, it is needed to inherit documentation from it