Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: kl/l06-gw-audit-2 misleading docu #908

Open
wants to merge 1 commit into
base: gw-audit-2-fixes
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridge/BridgeHelper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {DataEncoding} from "../common/libraries/DataEncoding.sol";
/**
* @author Matter Labs
* @custom:security-contact [email protected]
* @notice Helper library for working with L2 contracts on L1.
* @notice Helper library for working with native tokens on both L1 and L2.
*/
library BridgeHelper {
/// @dev Receives and parses (name, symbol, decimals) from the token contract
Expand Down
3 changes: 1 addition & 2 deletions l1-contracts/contracts/bridge/L1Nullifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
emit BridgehubDepositFinalized(_chainId, _txDataHash, _txHash);
}

/// @dev Calls the internal `_encodeTxDataHash`. Used as a wrapped for try / catch case.
/// @dev Calls the library `encodeTxDataHash`. Used as a wrapped for try / catch case.
/// @dev Encodes the transaction data hash using either the latest encoding standard or the legacy standard.
/// @param _encodingVersion EncodingVersion.
/// @param _originalCaller The address of the entity that initiated the deposit.
Expand Down Expand Up @@ -411,7 +411,6 @@ contract L1Nullifier is IL1Nullifier, ReentrancyGuard, Ownable2StepUpgradeable,
// Handling special case for withdrawal from ZKsync Era initiated before Shared Bridge.
(bytes32 assetId, bytes memory transferData) = _verifyWithdrawal(_finalizeWithdrawalParams);

// Handling special case for withdrawal from zkSync Era initiated before Shared Bridge.
if (_isPreSharedBridgeEraEthWithdrawal(chainId, l2BatchNumber)) {
// Checks that the withdrawal wasn't finalized already.
bool alreadyFinalized = IGetters(ERA_DIAMOND_PROXY).isEthWithdrawalFinalized(l2BatchNumber, l2MessageIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
}

/// @notice Used to set the asset handler address for a given asset ID on a remote ZK chain
/// @dev No access control on the caller, as msg.sender is encoded in the assetId.
/// @param _chainId The ZK chain ID.
/// @param _originalCaller The `msg.sender` address from the external call that initiated current one.
/// @param _assetId The encoding of asset ID.
Expand Down
16 changes: 15 additions & 1 deletion l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
}

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

/// @notice Initiates a withdrawal by burning funds on the contract and sending the message to L1
Expand All @@ -137,6 +137,16 @@
_withdrawSender(_assetId, _assetData, msg.sender, true);
}

/*//////////////////////////////////////////////////////////////
Internal & Helpers
//////////////////////////////////////////////////////////////*/

/// @inheritdoc AssetRouterBase
function _ensureTokenRegisteredWithNTV(address _token) internal override returns (bytes32 assetId) {
IL2NativeTokenVault nativeTokenVault = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR);
nativeTokenVault.ensureTokenIsRegistered(_token);
}

/// @notice Initiates a withdrawal by burning funds on the contract and sending the message to L1
/// where tokens would be unlocked
/// @param _assetId The asset id of the withdrawn asset
Expand Down Expand Up @@ -165,7 +175,7 @@
L2ContractHelper.sendMessageToL1(message);
} else {
address l1Token = IL2NativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).tokenAddress(_assetId);
require(l1Token != address(0), "Unsupported asset Id by NTV");

Check failure on line 178 in l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 178 in l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 178 in l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 178 in l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 178 in l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements

Check failure on line 178 in l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol

View workflow job for this annotation

GitHub Actions / lint

GC: Use Custom Errors instead of require statements
(uint256 amount, address l1Receiver) = abi.decode(_assetData, (uint256, address));
message = _getSharedBridgeWithdrawMessage(l1Receiver, l1Token, amount);
IL2SharedBridgeLegacy(L2_LEGACY_SHARED_BRIDGE).sendMessageToL1(message);
Expand Down Expand Up @@ -195,6 +205,10 @@
return abi.encodePacked(IL1ERC20Bridge.finalizeWithdrawal.selector, _l1Receiver, _l1Token, _amount);
}

/*//////////////////////////////////////////////////////////////
LEGACY FUNCTIONS
//////////////////////////////////////////////////////////////*/

/// @notice Legacy finalizeDeposit.
/// @dev Finalizes the deposit and mint funds.
/// @param _l1Sender The address of token sender on L1.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ interface IAssetHandler {
/// @param _data the actual data specified for the function
function bridgeMint(uint256 _chainId, bytes32 _assetId, bytes calldata _data) external payable;

/// @notice Burns bridged tokens and returns the calldata for L2 -> L1 message.
/// @notice Burns bridged tokens and returns the calldata for L2 <-> L1 message.
/// @dev In case of native token vault _data is the tuple of _depositAmount and _l2Receiver.
/// @param _chainId the chainId that the message will be sent to
/// @param _msgValue the msg.value of the L2 transaction. For now it is always 0.
Expand Down
6 changes: 3 additions & 3 deletions l1-contracts/contracts/bridge/ntv/NativeTokenVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ abstract contract NativeTokenVault is INativeTokenVault, IAssetHandler, Ownable2
/// @dev For more details see https://docs.openzeppelin.com/contracts/3.x/api/proxy#UpgradeableBeacon.
IBeacon public bridgedTokenBeacon;

/// @dev A mapping assetId => tokenAddress
mapping(bytes32 assetId => uint256 chainId) public originChainId;
/// @dev A mapping assetId => originChainId
mapping(bytes32 assetId => uint256 originChainId) public originChainId;

/// @dev A mapping assetId => tokenAddress
mapping(bytes32 assetId => address tokenAddress) public tokenAddress;
Expand Down Expand Up @@ -160,7 +160,7 @@ abstract contract NativeTokenVault is INativeTokenVault, IAssetHandler, Ownable2
//////////////////////////////////////////////////////////////*/

/// @inheritdoc IAssetHandler
/// @notice Allows bridgehub to acquire mintValue for L1->L2 transactions.
/// @notice Allows bridgehub to acquire mintValue for L1->L2 and L2->L1 transactions.
/// @dev In case of native token vault _data is the tuple of _depositAmount and _receiver.
function bridgeBurn(
uint256 _chainId,
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridgehub/Bridgehub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {MigrationPaused, AssetIdAlreadyRegistered, ChainAlreadyLive, ChainNotLeg

/// @author Matter Labs
/// @custom:security-contact [email protected]
/// @dev The Bridgehub contract serves as the primary entry point for L1<->L2 communication,
/// @dev The Bridgehub contract serves as the primary entry point for L1->L2 communication,
/// facilitating interactions between end user and bridges.
/// It also manages state transition managers, base tokens, and chain registrations.
/// Bridgehub is also an IL1AssetHandler for the chains themselves, which is used to migrate the chains
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ contract CTMDeploymentTracker is ICTMDeploymentTracker, ReentrancyGuard, Ownable
/// @dev Bridgehub smart contract that is used to operate with L2 via asynchronous L2 <-> L1 communication.
IBridgehub public immutable override BRIDGE_HUB;

/// @dev Bridgehub smart contract that is used to operate with L2 via asynchronous L2 <-> L1 communication.
/// @dev L1AssetRouter smart contract that is used to bridge assets (including chains) between L1 and L2.
IAssetRouterBase public immutable override L1_ASSET_ROUTER;

/// @dev The encoding version of the data.
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridgehub/MessageRoot.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ bytes32 constant CHAIN_TREE_EMPTY_ENTRY_HASH = bytes32(
0x46700b4d40ac5c35af2c22dda2787a91eb567b06c924a8fb8ae9a05b20c08c21
);

// Chain tree consists of batch commitments as their leaves. We use hash of "new bytes(96)" as the hash of an empty leaf.
// The single shared tree consists of the roots of chain trees as its. We use hash of "new bytes(96)" as the hash of an empty leaf.
bytes32 constant SHARED_ROOT_TREE_EMPTY_HASH = bytes32(
0x46700b4d40ac5c35af2c22dda2787a91eb567b06c924a8fb8ae9a05b20c08c21
);
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/common/L2ContractAddresses.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ address constant L2_COMPLEX_UPGRADER_ADDR = address(0x800f);
/// @dev The address used to execute the genesis upgrade
address constant L2_GENESIS_UPGRADE_ADDR = address(0x10001);

/// @dev The address of the L2 bridge hub system contract, used to start L2<>L2 transactions
/// @dev The address of the L2 bridge hub system contract, used to start L1->L2 transactions
address constant L2_BRIDGEHUB_ADDR = address(0x10002);

/// @dev the address of the l2 asset router.
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/common/libraries/DataEncoding.sol
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ library DataEncoding {
return keccak256(abi.encode(_chainId, L2_NATIVE_TOKEN_VAULT_ADDR, _assetData));
}

/// @notice Encodes the asset data by combining chain id, NTV as asset deployment tracker and asset data.
/// @notice Encodes the asset data by combining chain id, NTV as asset deployment tracker and token address.
/// @param _chainId The id of the chain token is native to.
/// @param _tokenAddress The address of token that has to be encoded (asset data is the address itself).
/// @return The encoded asset data.
Expand Down
4 changes: 1 addition & 3 deletions l1-contracts/contracts/common/libraries/FullMerkle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,9 @@ library FullMerkle {
}

/**
* @dev Initialize a {Bytes32PushTree} using {Merkle.efficientHash} to hash internal nodes.
* @dev Initialize a {FullTree} using {Merkle.efficientHash} to hash internal nodes.
* The capacity of the tree (i.e. number of leaves) is set to `2**levels`.
*
* Calling this function on MerkleTree that was already setup and used will reset it to a blank state.
*
* IMPORTANT: The zero value should be carefully chosen since it will be stored in the tree representing
* empty leaves. It should be a value that is not expected to be part of the tree.
* @param zero The zero value to be used in the tree.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,6 @@ contract ChainTypeManager is IChainTypeManager, ReentrancyGuard, Ownable2StepUpg
return getZKChain(_chainId);
}

// check not registered
Diamond.DiamondCutData memory diamondCut = abi.decode(_diamondCut, (Diamond.DiamondCutData));

{
Expand Down Expand Up @@ -442,7 +441,6 @@ contract ChainTypeManager is IChainTypeManager, ReentrancyGuard, Ownable2StepUpg
function registerSettlementLayer(uint256 _newSettlementLayerChainId, bool _isWhitelisted) external onlyOwner {
require(_newSettlementLayerChainId != 0, "Bad chain id");

// Currently, we require that the sync layer is deployed by the same CTM.
require(getZKChain(_newSettlementLayerChainId) != address(0), "CTM: sync layer not registered");

IBridgehub(BRIDGE_HUB).registerSettlementLayer(_newSettlementLayerChainId, _isWhitelisted);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,6 @@ contract AdminFacet is ZKChainBase, IAdmin {
}

/// @inheritdoc IAdmin
/// @dev Note that this function does not check that the caller is the chain admin.
function forwardedBridgeRecoverFailedTransfer(
uint256 /* _chainId */,
bytes32 /* _assetInfo */,
Expand Down
Loading