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

Insufficient Message Length Validation in L1Nullifier's finalizeDeposit Handler

bauchibred

Summary

The L1Nullifier._parseL2WithdrawalMessage function has insufficient message length validation when handling messages with the newly added finalizeDeposit selector. The function only checks for a minimum length of 36 bytes but attempts to read at least 68 bytes of data, which could lead to buffer overflows or corrupted data reads.

Vulnerability Details

Take a look at Take a look at https://github.com/Cyfrin/2024-10-zksync/blob/cfc1251de29379a9548eeff1eea3c78267288356/era-contracts/l1-contracts/contracts/bridge/L1Nullifier.sol#L607-L618:

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);
}
// slither-disable-next-line unused-return
(, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset); // originChainId, not used for L2->L1 txs
(assetId, offset) = UnsafeBytes.readBytes32(_l2ToL1message, offset);
transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset);
}

The function attempts to read:

  1. 4 bytes - function selector (already read)

  2. 32 bytes - uint256 for originChainId

  3. 32 bytes - bytes32 for assetId

This means the function requires at least 68 bytes (4 + 32 + 32) to safely read all required data. However, the current check only validates that the message is at least 36 bytes long.

This is particularly concerning when compared to the rigorous length checks in other message handlers in the same function where the amount of data read is strictly validated to the amount of minimum data required, see

https://github.com/Cyfrin/2024-10-zksync/blob/cfc1251de29379a9548eeff1eea3c78267288356/era-contracts/l1-contracts/contracts/bridge/L1Nullifier.sol#L563-L606

if (bytes4(functionSignature) == IMailbox.finalizeEthWithdrawal.selector) {
// The data is expected to be at least 56 bytes long.
if (_l2ToL1message.length < 56) {
revert L2WithdrawalMessageWrongLength(_l2ToL1message.length);
}
// this message is a base token withdrawal
(l1Receiver, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset);
// slither-disable-next-line unused-return
(amount, ) = UnsafeBytes.readUint256(_l2ToL1message, offset);
assetId = BRIDGE_HUB.baseTokenAssetId(_chainId);
address baseToken = BRIDGE_HUB.baseToken(_chainId);
transferData = DataEncoding.encodeBridgeMintData({
_originalCaller: address(0),
_remoteReceiver: l1Receiver,
_originToken: baseToken,
_amount: amount,
_erc20Metadata: new bytes(0)
});
} else if (bytes4(functionSignature) == IL1ERC20Bridge.finalizeWithdrawal.selector) {
// this message is a token withdrawal
​
// Check that the message length is correct.
// It should be equal to the length of the function signature + address + address + uint256 = 4 + 20 + 20 + 32 =
// 76 (bytes).
if (_l2ToL1message.length != 76) {
revert L2WithdrawalMessageWrongLength(_l2ToL1message.length);
}
(l1Receiver, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset);
// We use the IL1ERC20Bridge for backward compatibility with old withdrawals.
address l1Token;
(l1Token, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset);
// slither-disable-next-line unused-return
(amount, ) = UnsafeBytes.readUint256(_l2ToL1message, offset);
​
l1NativeTokenVault.ensureTokenIsRegistered(l1Token);
assetId = DataEncoding.encodeNTVAssetId(block.chainid, l1Token);
transferData = DataEncoding.encodeBridgeMintData({
_originalCaller: address(0),
_remoteReceiver: l1Receiver,
_originToken: l1Token,
_amount: amount,
_erc20Metadata: new bytes(0)
});
}

Impact

We could then have buffer overflows or corrupted data reads, when reading assetId if message length is not in the right format, but big enough to pass the check.

Tools Used

Manual review

Recommendations

Update the length check to account for all required data:

else if (bytes4(functionSignature) == IAssetRouterBase.finalizeDeposit.selector) {
// The data must be at least 68 bytes: selector (4) + originChainId (32) + assetId (32)
if (_l2ToL1message.length < 68) {
revert WrongMsgLength(68, _l2ToL1message.length);
}
// ... rest of the code
}
Updates

Lead Judging Commences

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

Inadequate length check for IAssetRouterBase.finalizeDeposit.Selector in function _parseL2WithdrawalMessage

Appeal created

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

Inadequate length check for IAssetRouterBase.finalizeDeposit.Selector in function _parseL2WithdrawalMessage

Support

FAQs

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