Era
ZKsync
FoundryLayer 2
500,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Chain migration is not possible when MAX_NUMBER_OF_ZK_CHAINS is reached even if it is enforced to ignore that maximum value

crunter

Summary

When adding a new zkChain to the Bridge Hub, it is possible for the sender to force its inclusion even when the maximum allowed amount of chains has been reached, this by using a boolean flag to indicate it. However, even when the flag has been set to bypass the maximum value check, the process will revert if this value has been reached due to failure to check the flag a few steps later, turning the flag useless and making impossible to add more zkChains to the Bridge Hub.

Vulnerability Details

When adding a new zkChain in the Bridge Hub, the _registerNewZKChain internal function is called to update the zkChainMap mapping. This function receives the chain information and the _checkMaxNumberOfZKChains boolean to determine if the MAX_NUMBER_OF_ZK_CHAINS value will be checked. This boolean is used to bypass the maximum allowed amount of registered chains if desired.

BridgeHub.sol
​
function _registerNewZKChain(uint256 _chainId, address _zkChain, bool _checkMaxNumberOfZKChains) internal {
// slither-disable-next-line unused-return
zkChainMap.set(_chainId, _zkChain);
​
// @audit - if _checkMaxNumberOfZKChains is set to false, the MAX_NUMBER_OF_ZK_CHAINS check will be bypassed
@> if (_checkMaxNumberOfZKChains && zkChainMap.length() > MAX_NUMBER_OF_ZK_CHAINS) {
revert ZKChainLimitReached();
}
}

According to the NATSPEC:

Providing _checkMaxNumberOfZKChains = false may be preferable in cases where we want to guarantee that a chain can be added. These include:
1. Migration of a chain from the mapping in the old CTM
2. Migration of a chain to a new settlement layer

Nevertheless, for the second case, if the MAX_NUMBER_OF_ZK_CHAINS value has been reached the process will fail even if the _checkMaxNumberOfZKChains value is set to false as it is not checked in the MessageRoot::_addNewChain function.

During chain migration, the Bridgehub::bridgeMint function is called. If the contract of the zkChain is not yet deployed, it will be created and registered in the system, with _checkMaxNumberOfZKChains set as false. Then the MessageRoot::_addNewChain function is called and the MAX_NUMBER_OF_ZK_CHAINS value is checked again, without taking into account the _checkMaxNumberOfZKChains value. Even if the variable is set to false, the process will revert when the maximum value has been reached.

Bridgehub.sol
​
function bridgeMint(
uint256, // originChainId
bytes32 _assetId,
bytes calldata _bridgehubMintData
) external payable override onlyAssetRouter whenMigrationsNotPaused {
BridgehubMintCTMAssetData memory bridgehubData = abi.decode(_bridgehubMintData, (BridgehubMintCTMAssetData));
​
... snip
​
// @audit - if the zkChain contract is not yet deployed, create it and register it
address zkChain = getZKChain(bridgehubData.chainId);
bool contractAlreadyDeployed = zkChain != address(0);
@> if (!contractAlreadyDeployed) {
zkChain = IChainTypeManager(ctm).forwardedBridgeMint(bridgehubData.chainId, bridgehubData.ctmData);
if (zkChain == address(0)) {
revert ChainIdNotRegistered(bridgehubData.chainId);
}
​
// We want to allow any chain to be migrated,
​
// @audit - _checkMaxNumberOfZKChains is set as false to bypass the maximum zkchains allowed check
@> _registerNewZKChain(bridgehubData.chainId, zkChain, false);
​
// @audit - here the maximum zkchains allowed check is performed again, without any chance to bypass it
@> messageRoot.addNewChain(bridgehubData.chainId);
}
​
... snip
}
MessageRoot.sol
​
function _addNewChain(uint256 _chainId) internal {
uint256 cachedChainCount = chainCount;
​
// @audit if MAX_NUMBER_OF_ZK_CHAINS has been reached, then revert
@> if (cachedChainCount >= MAX_NUMBER_OF_ZK_CHAINS) {
@> revert TooManyChains(cachedChainCount, MAX_NUMBER_OF_ZK_CHAINS);
}
​
++chainCount;
chainIndex[_chainId] = cachedChainCount;
chainIndexToId[cachedChainCount] = _chainId;
​
// slither-disable-next-line unused-return
bytes32 initialHash = chainTree[_chainId].setup(CHAIN_TREE_EMPTY_ENTRY_HASH);
​
// slither-disable-next-line unused-return
sharedTree.pushNewLeaf(MessageHashing.chainIdLeafHash(initialHash, _chainId));
​
emit AddedChain(_chainId, cachedChainCount);
}
​

Impact

Impact: High

It is not possible to add more chains even when it is intended to be allowed.

Likelihood: Low

Tools Used

Manual Review

Recommendations

It is recommended allow to bypass the maximum value checking in the MessageRoot::_addNewChain function.

MessageRoot.sol
​
- function _addNewChain(uint256 _chainId) internal {
+ function _addNewChain(uint256 _chainId, bool _checkMaxNumberOfZKChains) internal {
uint256 cachedChainCount = chainCount;
​
- if (cachedChainCount >= MAX_NUMBER_OF_ZK_CHAINS) {
+ if (_checkMaxNumberOfZKChains && cachedChainCount >= MAX_NUMBER_OF_ZK_CHAINS) {
revert TooManyChains(cachedChainCount, MAX_NUMBER_OF_ZK_CHAINS);
}
​
++chainCount;
chainIndex[_chainId] = cachedChainCount;
chainIndexToId[cachedChainCount] = _chainId;
​
// slither-disable-next-line unused-return
bytes32 initialHash = chainTree[_chainId].setup(CHAIN_TREE_EMPTY_ENTRY_HASH);
​
// slither-disable-next-line unused-return
sharedTree.pushNewLeaf(MessageHashing.chainIdLeafHash(initialHash, _chainId));
​
emit AddedChain(_chainId, cachedChainCount);
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Migration blocked due to chain limit in `MessageRoot` contract

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.