[Lines of code](https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L225-L257) [Vulnerability details](https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L225-L257) function finalizeWithdrawal( uint256 _l2BlockNumber, uint256 _l2MessageIndex, uint16 _l2TxNumberInBlock, bytes calldata _message, bytes32[] calldata _merkleProof ) external nonReentrant senderCanCallFunction(allowList) { require(!isWithdrawalFinalized[_l2BlockNumber][_l2MessageIndex], "pw"); L2Message memory l2ToL1Message = L2Message({ txNumberInBlock: _l2TxNumberInBlock, sender: l2Bridge, data: _message }); (address l1Receiver, address l1Token, uint256 amount) = _parseL2WithdrawalMessage(l2ToL1Message.data); // Preventing the stack too deep error { bool success = zkSyncMailbox.proveL2MessageInclusion( _l2BlockNumber, _l2MessageIndex, l2ToL1Message, _merkleProof ); require(success, "nq"); } isWithdrawalFinalized[_l2BlockNumber][_l2MessageIndex] = true; // Withdraw funds IERC20(l1Token).safeTransfer(l1Receiver, amount); emit WithdrawalFinalized(l1Receiver, l1Token, amount); } If some conditions match, an attacker can transfer all tokens to an arbitrary address This is a bit lengthy, so I'll break it up as follows You can see each source code on Proof of Concept ### 1: Possibility of conditions to occur For this attack vector to be valid, the value of acutualRootHash would be bytes(0) as the default value. As a result, the value of s.l2LogsRootHashes may be bytes(0)for the following reasons * There is no checking _storedBlock.l2LogsTreeRoot is not bytes(0) before setting the s.l2LogsRootHashes[currentBlockNumber] by [_executeOneBlock()](https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/Executor.sol#L190-L196) * The possibility abuse of admin privileges by the validator to execute executeBlocks() ### 2: How to pass proveL2MessageInclusion If the above case is met, the value actualRootHash would be bytes(0) bytes32 actualRootHash = s.l2LogsRootHashes[_blockNumber]; The value of calculatedRootHash is calculated by this function. The calculateRoot allows empty proofs therefore users can get bytes(0) as a returned value of this function This issue is similar to this situation [code-423n4/2022-05-opensea-seaport-findings#201]() Finally this checking can pass return actualRootHash == calculatedRootHash; ### 3: How to set arbitrary amount by parseL2WithdrawalMessage The UnsafeBytes library gets the value from bytes. And this check requires _l2ToL1message.length should be 76 bytes values. The bytes values are read in the following order **functionSignature → l1Receiver → l1TokenAddress → amount** As a result, if you set the following format, you can transfer tokens freely /* 0x + 11a2ccc1 // 4bytes : function sig = keccak256("finalizeWithdrawal(uint256,uint256,uint16,bytes,bytes32[])") + 237C8Aea434dE4784d23d145069c6D0Bef629A19 // 20bytes Received Address (my received address) + 2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599 // 20bytes L1Token Address(WBTC) + 00000000000000000000000000000000000000000000000000001319718A5000 // 32bytes Amount(2100e4*10*6) -> 0x11a2ccc1237C8Aea434dE4784d23d145069c6D0Bef629A192260FAC5E5542a773Aa44fBCfeDf7C193bc2C59900000000000000000000000000000000000000000000000000001319718A5000 */ Thus, all tokens can be transferred freely once certain conditions are met This is a critical issue and can be prevented by adding certain checks ## Impact It has possible to lose all funds of L1ERC20Bridge.sol contract due to insufficient checking ## Proof of Concept ### 1: Possibility of conditions to occur bytes32 actualRootHash = s.l2LogsRootHashes[_blockNumber]; The value of s.l2LogsRootHashes is set as follows. function _executeOneBlock(StoredBlockInfo memory _storedBlock, uint256 _executedBlockIdx) internal { /* ~~~ */ // Save root hash of L2 -> L1 logs tree s.l2LogsRootHashes[currentBlockNumber] = _storedBlock.l2LogsTreeRoot; } And this function is executed by executeBlocks function. function executeBlocks(StoredBlockInfo[] calldata _blocksData) external nonReentrant onlyValidator { uint256 nBlocks = _blocksData.length; for (uint256 i = 0; i < nBlocks; i = i.uncheckedInc()) { _executeOneBlock(_blocksData[i], i); emit BlockExecution(_blocksData[i].blockNumber, _blocksData[i].blockHash, _blocksData[i].commitment); } s.totalBlocksExecuted = s.totalBlocksExecuted + nBlocks; require(s.totalBlocksExecuted <= s.totalBlocksVerified, "n"); // Can't execute blocks more then committed and proven currently. } ### 2: How to pass proveL2MessageInclusion function proveL2MessageInclusion( uint256 _blockNumber, uint256 _index, L2Message calldata _message, bytes32[] calldata _proof ) external view returns (bool) { return _proveL2LogInclusion(_blockNumber, _index, _L2MessageToLog(_message), _proof); } function _proveL2LogInclusion( uint256 _blockNumber, uint256 _index, L2Log memory _log, bytes32[] calldata _proof ) internal view returns (bool) { require(_blockNumber <= s.totalBlocksExecuted, "xx"); bytes32 hashedLog = keccak256( abi.encodePacked(_log.l2ShardId, _log.isService, _log.txNumberInBlock, _log.sender, _log.key, _log.value) ); // Check that hashed log is not the default one, // otherwise it means that the value is out of range of sent L2 -> L1 logs require(hashedLog != L2_L1_LOGS_TREE_DEFAULT_LEAF_HASH, "tw"); // Check that the proof length is exactly the same as tree height, to prevent // any shorter/longer paths attack on the Merkle path validation require(_proof.length == L2_TO_L1_LOG_MERKLE_TREE_HEIGHT, "rz"); bytes32 calculatedRootHash = Merkle.calculateRoot(_proof, _index, hashedLog); bytes32 actualRootHash = s.l2LogsRootHashes[_blockNumber]; return actualRootHash == calculatedRootHash; } function calculateRoot( bytes32[] calldata _path, uint256 _index, bytes32 _itemHash ) internal pure returns (bytes32) { uint256 pathLength = _path.length; require(pathLength > 0, "xc"); require(pathLength < 256, "bt"); require(_index < 2**pathLength, "pz"); bytes32 currentHash = _itemHash; for (uint256 i = 0; i < pathLength; i = i.uncheckedInc()) { if (_index % 2 == 0) { currentHash = keccak256(abi.encode(currentHash, _path[i])); } else { currentHash = keccak256(abi.encode(_path[i], currentHash)); } _index /= 2; } return currentHash; } ### 3: How to set arbitrary amount by parseL2WithdrawalMessage [https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L260^L279]() function _parseL2WithdrawalMessage(bytes memory _l2ToL1message) internal pure returns ( address l1Receiver, address l1Token, uint256 amount ) { // 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). require(_l2ToL1message.length == 76, "kk"); (uint32 functionSignature, uint256 offset) = UnsafeBytes.readUint32(_l2ToL1message, 0); require(bytes4(functionSignature) == this.finalizeWithdrawal.selector, "nt"); (l1Receiver, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset); (l1Token, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset); (amount, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset); } library UnsafeBytes { function readUint32(bytes memory _bytes, uint256 _start) internal pure returns (uint32 result, uint256 offset) { assembly { offset := add(_start, 4) result := mload(add(_bytes, offset)) } } function readAddress(bytes memory _bytes, uint256 _start) internal pure returns (address result, uint256 offset) { assembly { offset := add(_start, 20) result := mload(add(_bytes, offset)) } } function readUint256(bytes memory _bytes, uint256 _start) internal pure returns (uint256 result, uint256 offset) { assembly { offset := add(_start, 32) result := mload(add(_bytes, offset)) } } function readBytes32(bytes memory _bytes, uint256 _start) internal pure returns (bytes32 result, uint256 offset) { assembly { offset := add(_start, 32) result := mload(add(_bytes, offset)) } } } ## Tools Used Manual ## Recommended Mitigation Steps You should add checking as follows Don’t allow setting s.l2LogsRootHashes to bytes32(0) function _executeOneBlock(StoredBlockInfo memory _storedBlock, uint256 _executedBlockIdx) internal { /* ~~~ */ // Add require(_storedBlock.l2LogsTreeRoot != bytes32(0)); s.l2LogsRootHashes[currentBlockNumber] = _storedBlock.l2LogsTreeRoot; } Don’t allow the empty _path function calculateRoot( bytes32[] calldata _path, uint256 _index, bytes32 _itemHash ) internal pure returns (bytes32) { uint256 pathLength = _path.length; require(pathLength > 0, "xc"); require(pathLength < 256, "bt"); require(_index < 2**pathLength, "pz"); bytes32 currentHash = _itemHash; for (uint256 i = 0; i < pathLength; i = i.uncheckedInc()) { if (_index % 2 == 0) { currentHash = keccak256(abi.encode(currentHash, _path[i])); } else { currentHash = keccak256(abi.encode(_path[i], currentHash)); } _index /= 2; } // Add require(currentHash != bytes32(0)) return currentHash; } --- The text was updated successfully, but these errors were encountered: All reactions