Skip to content

Commit

Permalink
feat: ZKChain and Gateway Upgrade Audit (#1052) (#1147)
Browse files Browse the repository at this point in the history
Signed-off-by: Danil <[email protected]>
Co-authored-by: Bence Haromi <[email protected]>
Co-authored-by: kelemeno <[email protected]>
Co-authored-by: kelemeno <[email protected]>
Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
Co-authored-by: Raid Ateir <[email protected]>
Co-authored-by: Grzegorz Prusak <[email protected]>
Co-authored-by: Moshe Shababo <[email protected]>
Co-authored-by: Akosh Farkash <[email protected]>
Co-authored-by: Bruno França <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: Roman Brodetski <[email protected]>
Co-authored-by: vladbochok <[email protected]>
Co-authored-by: Danil <[email protected]>
Co-authored-by: Neo <[email protected]>
Co-authored-by: tommysr <[email protected]>
Co-authored-by: Rahul Saxena <[email protected]>
Co-authored-by: Artem Makhortov <[email protected]>
Co-authored-by: Zach Kolodny <[email protected]>
Co-authored-by: perekopskiy <[email protected]>
Co-authored-by: perekopskiy <[email protected]>
Co-authored-by: Ivan Schasny <[email protected]>
Co-authored-by: mm <[email protected]>
Co-authored-by: Dima Zhornyk <[email protected]>
Co-authored-by: Yberjon <[email protected]>
  • Loading branch information
1 parent 45cf28c commit 79215dc
Show file tree
Hide file tree
Showing 23 changed files with 180 additions and 103 deletions.
26 changes: 11 additions & 15 deletions .github/workflows/l1-contracts-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ jobs:
- name: Build da-contracts artifacts
run: yarn da build:foundry

- name: Build l1 artifacts
run: yarn l1 build

- name: Create cache
uses: actions/cache/save@v3
with:
Expand Down Expand Up @@ -287,18 +284,17 @@ jobs:
- name: Run coverage
run: FOUNDRY_PROFILE=default yarn test:foundry && FOUNDRY_PROFILE=default yarn coverage:foundry --report summary --report lcov

# TODO: for some reason filtering directories stopped working.
# # 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/forge-std/*' '../lib/murky/*' 'lib/*' '../lib/*' 'lib/' 'deploy-scripts/*' --output-file lcov.info --rc lcov_branch_coverage=1
# 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/bridge/asset-router/L1AssetRouter.sol' 'contracts/dev-contracts/*' '../lib/forge-std/*' '../lib/murky/*' 'lib/*' '../lib/*' 'lib/' --ignore-errors unused --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
Expand Down
13 changes: 3 additions & 10 deletions l1-contracts/contracts/bridge/L1ERC20Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported} from "../common/L1ContractErrors.sol";
import {ETH_TOKEN_ADDRESS} from "../common/Config.sol";

/// @author Matter Labs
Expand Down Expand Up @@ -203,11 +203,7 @@ 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();
}

depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;
emit DepositInitiated({
l2DepositTxHash: l2TxHash,
Expand All @@ -227,10 +223,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
function _approveFundsToAssetRouter(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
uint256 balanceBefore = _token.balanceOf(address(this));
_token.safeTransferFrom(_from, address(this), _amount);
bool success = _token.approve(address(L1_ASSET_ROUTER), _amount);
if (!success) {
revert ApprovalFailed();
}
_token.forceApprove(address(L1_ASSET_ROUTER), _amount);
uint256 balanceAfter = _token.balanceOf(address(this));

return balanceAfter - balanceBefore;
Expand Down
14 changes: 7 additions & 7 deletions l1-contracts/contracts/bridge/L1Nullifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -573,8 +573,8 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
address baseToken = BRIDGE_HUB.baseToken(_chainId);
transferData = DataEncoding.encodeBridgeMintData({
_originalCaller: address(0),
_l2Receiver: l1Receiver,
_l1Token: baseToken,
_remoteReceiver: l1Receiver,
_originToken: baseToken,
_amount: amount,
_erc20Metadata: new bytes(0)
});
Expand All @@ -598,15 +598,15 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
assetId = DataEncoding.encodeNTVAssetId(block.chainid, l1Token);
transferData = DataEncoding.encodeBridgeMintData({
_originalCaller: address(0),
_l2Receiver: l1Receiver,
_l1Token: l1Token,
_remoteReceiver: l1Receiver,
_originToken: l1Token,
_amount: amount,
_erc20Metadata: new bytes(0)
});
} else if (bytes4(functionSignature) == IAssetRouterBase.finalizeDeposit.selector) {
// The data is expected to be at least 36 bytes long to contain assetId.
if (_l2ToL1message.length < 36) {
revert WrongMsgLength(36, _l2ToL1message.length);
// The data is expected to be at least 68 bytes long to contain assetId.
if (_l2ToL1message.length < 68) {
revert WrongMsgLength(68, _l2ToL1message.length);
}
// slither-disable-next-line unused-return
(, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset); // originChainId, not used for L2->L1 txs
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridge/L2WrappedBaseToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ contract L2WrappedBaseToken is ERC20PermitUpgradeable, IL2WrappedBaseToken, IBri
/// Always reverts instead of minting anything!
/// Note: Use `deposit`/`depositTo` methods instead.
// solhint-disable-next-line no-unused-vars
function bridgeMint(address _to, uint256 _amount) external override onlyBridge {
function bridgeMint(address _to, uint256 _amount) external view override onlyBridge {
revert BridgeMintNotImplemented();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ abstract contract AssetRouterBase is IAssetRouterBase, Ownable2StepUpgradeable,
Internal Functions
//////////////////////////////////////////////////////////////*/

function _setAssetHandler(bytes32 _assetId, address _assetHandlerAddress) internal {
assetHandlerAddress[_assetId] = _assetHandlerAddress;
emit AssetHandlerRegistered(_assetId, _assetHandlerAddress);
}

/// @dev send the burn message to the asset
/// @notice Forwards the burn request for specific asset to respective asset handler.
/// @param _chainId The chain ID of the ZK chain to which to deposit.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ interface IAssetRouterBase {
bytes bridgeMintCalldata
);

event BridgehubWithdrawalInitiated(
uint256 chainId,
address indexed sender,
bytes32 indexed assetId,
bytes32 assetDataHash // Todo: What's the point of emitting hash?
);

event AssetHandlerRegisteredInitial(
bytes32 indexed assetId,
address indexed assetHandlerAddress,
Expand Down
9 changes: 0 additions & 9 deletions l1-contracts/contracts/bridge/asset-router/IL1AssetRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@ interface IL1AssetRouter is IAssetRouterBase, IL1SharedBridgeLegacy {
bytes32 indexed additionalData
);

event LegacyDepositInitiated(
uint256 indexed chainId,
bytes32 indexed l2DepositTxHash,
address indexed from,
address to,
address l1Asset,
uint256 amount
);

/// @notice Initiates a deposit by locking funds on the contract and sending the request
/// 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
Expand Down
5 changes: 5 additions & 0 deletions l1-contracts/contracts/bridge/asset-router/IL2AssetRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,9 @@ 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;

/// @notice Function that allows native token vault to register itself as the asset handler for
/// a legacy asset.
/// @param _assetId The assetId of the legacy token.
function setLegacyTokenAssetHandler(bytes32 _assetId) external;
}
2 changes: 2 additions & 0 deletions l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

pragma solidity 0.8.24;

// solhint-disable reason-string, gas-custom-errors

import {IERC20} from "@openzeppelin/contracts-v4/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts-v4/token/ERC20/utils/SafeERC20.sol";

Expand Down
51 changes: 33 additions & 18 deletions l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ 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";
Expand All @@ -19,7 +18,7 @@ import {AddressAliasHelper} from "../../vendor/AddressAliasHelper.sol";
import {L2_NATIVE_TOKEN_VAULT_ADDR, L2_BRIDGEHUB_ADDR} from "../../common/L2ContractAddresses.sol";
import {L2ContractHelper} from "../../common/libraries/L2ContractHelper.sol";
import {DataEncoding} from "../../common/libraries/DataEncoding.sol";
import {EmptyAddress, InvalidCaller, AmountMustBeGreaterThanZero, AssetIdNotSupported} from "../../common/L1ContractErrors.sol";
import {TokenNotLegacy, EmptyAddress, InvalidCaller, AmountMustBeGreaterThanZero, AssetIdNotSupported} from "../../common/L1ContractErrors.sol";

/// @author Matter Labs
/// @custom:security-contact [email protected]
Expand Down Expand Up @@ -69,6 +68,13 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
_;
}

modifier onlyNTV() {
if (msg.sender != L2_NATIVE_TOKEN_VAULT_ADDR) {
revert InvalidCaller(msg.sender);
}
_;
}

/// @dev Disable the initialization to prevent Parity hack.
/// @dev this contract is deployed in the L2GenesisUpgrade, and is meant as direct deployment without a proxy.
/// @param _l1AssetRouter The address of the L1 Bridge contract.
Expand Down Expand Up @@ -109,6 +115,12 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
_setAssetHandlerAddressThisChain(L2_NATIVE_TOKEN_VAULT_ADDR, _assetRegistrationData, _assetHandlerAddress);
}

function setLegacyTokenAssetHandler(bytes32 _assetId) external override onlyNTV {
// Note, that it is an asset handler, but not asset deployment tracker,
// which is located on L1.
_setAssetHandler(_assetId, L2_NATIVE_TOKEN_VAULT_ADDR);
}

/*//////////////////////////////////////////////////////////////
Receive transaction Functions
//////////////////////////////////////////////////////////////*/
Expand All @@ -131,26 +143,16 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
}

/*//////////////////////////////////////////////////////////////
LEGACY FUNCTIONS
INITIATTE DEPOSIT Functions
//////////////////////////////////////////////////////////////*/

/// @notice Initiates a withdrawal by burning funds on the contract and sending the message to L1
/// where tokens would be unlocked
/// @dev do not rely on this function, it will be deprecated in the future
/// @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) {
return _withdrawSender(_assetId, _assetData, msg.sender, true);
}

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 == L1_CHAIN_ID) {
revert AssetIdNotSupported(recordedAssetId);
}
bytes32 assetId = _ensureTokenRegisteredWithNTV(_l2NativeToken);
return _withdrawSender(assetId, _assetData, msg.sender, true);
function withdraw(bytes32 _assetId, bytes calldata _assetData) external returns (bytes32) {
_withdrawSender(_assetId, _assetData, msg.sender, true);
}

/*//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -313,16 +315,29 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
}

function _withdrawLegacy(address _l1Receiver, address _l2Token, uint256 _amount, address _sender) internal {
bytes32 assetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, l1TokenAddress(_l2Token));
bytes memory data = abi.encode(_amount, _l1Receiver);
address l1Address = l1TokenAddress(_l2Token);
if (l1Address == address(0)) {
revert TokenNotLegacy();
}
bytes32 assetId = DataEncoding.encodeNTVAssetId(L1_CHAIN_ID, l1Address);
bytes memory data = DataEncoding.encodeBridgeBurnData(_amount, _l1Receiver, _l2Token);
_withdrawSender(assetId, data, _sender, false);
}

/// @notice Legacy getL1TokenAddress.
/// @param _l2Token The address of token on L2.
/// @return The address of token on L1.
function l1TokenAddress(address _l2Token) public view returns (address) {
return IBridgedStandardToken(_l2Token).l1Address();
bytes32 assetId = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).assetId(_l2Token);
if (assetId == bytes32(0)) {
return address(0);
}
uint256 originChainId = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).originChainId(assetId);
if (originChainId != L1_CHAIN_ID) {
return address(0);
}

return IBridgedStandardToken(_l2Token).originToken();
}

/// @notice Legacy function used for backward compatibility to return L2 wrapped token
Expand Down
29 changes: 20 additions & 9 deletions l1-contracts/contracts/bridge/interfaces/IL1Nullifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,6 @@ interface IL1Nullifier {

function nullifyChainBalanceByNTV(uint256 _chainId, address _token) external;

function finalizeWithdrawal(
uint256 _chainId,
uint256 _l2BatchNumber,
uint256 _l2MessageIndex,
uint16 _l2TxNumberInBatch,
bytes calldata _message,
bytes32[] calldata _merkleProof
) external;

/// @dev Withdraw funds from the initiated deposit, that failed when finalizing on L2.
/// @param _chainId The ZK chain id to which deposit was initiated.
/// @param _depositSender The address of the entity that initiated the deposit.
Expand All @@ -117,4 +108,24 @@ interface IL1Nullifier {
uint16 _l2TxNumberInBatch,
bytes32[] calldata _merkleProof
) external;

/// @notice Legacy function to finalize withdrawal via the same
/// interface as the old L1SharedBridge.
/// @dev Note, that we need to keep this interface, since the `L2AssetRouter`
/// will continue returning the previous address as the `l1SharedBridge`. The value
/// returned by it is used in the SDK for finalizing withdrawals.
/// @param _chainId The chain ID of the transaction to check
/// @param _l2BatchNumber The L2 batch number where the withdrawal was processed
/// @param _l2MessageIndex The position in the L2 logs Merkle tree of the l2Log that was sent with the message
/// @param _l2TxNumberInBatch The L2 transaction number in the batch, in which the log was sent
/// @param _message The L2 withdraw data, stored in an L2 -> L1 message
/// @param _merkleProof The Merkle proof of the inclusion L2 -> L1 message about withdrawal initialization
function finalizeWithdrawal(
uint256 _chainId,
uint256 _l2BatchNumber,
uint256 _l2MessageIndex,
uint16 _l2TxNumberInBatch,
bytes calldata _message,
bytes32[] calldata _merkleProof
) external;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,13 @@ pragma solidity 0.8.24;
/// @custom:security-contact [email protected]
interface IL1SharedBridgeLegacy {
function l2BridgeAddress(uint256 _chainId) external view returns (address);

event LegacyDepositInitiated(
uint256 indexed chainId,
bytes32 indexed l2DepositTxHash,
address indexed from,
address to,
address l1Asset,
uint256 amount
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

pragma solidity ^0.8.20;

import {UpgradeableBeacon} from "@openzeppelin/contracts-v4/proxy/beacon/UpgradeableBeacon.sol";

/// @author Matter Labs
/// @custom:security-contact [email protected]
interface IL2SharedBridgeLegacy {
Expand All @@ -14,8 +12,6 @@ interface IL2SharedBridgeLegacy {
uint256 amount
);

function l2TokenBeacon() external returns (UpgradeableBeacon);

function withdraw(address _l1Receiver, address _l2Token, uint256 _amount) external;

function l1TokenAddress(address _l2Token) external view returns (address);
Expand Down
3 changes: 1 addition & 2 deletions l1-contracts/contracts/bridge/ntv/IL1NativeTokenVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ pragma solidity 0.8.24;

import {IL1Nullifier} from "../interfaces/IL1Nullifier.sol";
import {INativeTokenVault} from "./INativeTokenVault.sol";
import {IL1AssetDeploymentTracker} from "../interfaces/IL1AssetDeploymentTracker.sol";

/// @title L1 Native token vault contract interface
/// @author Matter Labs
/// @custom:security-contact [email protected]
/// @notice The NTV is an Asset Handler for the L1AssetRouter to handle native tokens
// is IL1AssetHandler, IL1BaseTokenAssetHandler {
interface IL1NativeTokenVault is INativeTokenVault, IL1AssetDeploymentTracker {
interface IL1NativeTokenVault is INativeTokenVault {
/// @notice The L1Nullifier contract
function L1_NULLIFIER() external view returns (IL1Nullifier);

Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
bytes32,
address,
address _assetHandlerAddressOnCounterpart
) external view override onlyAssetRouter {
) external view onlyAssetRouter {
if (_assetHandlerAddressOnCounterpart != L2_NATIVE_TOKEN_VAULT_ADDR) {
revert WrongCounterpart();
}
Expand Down
Loading

0 comments on commit 79215dc

Please sign in to comment.