Findings report

myFindings.md

Finding

Risk rating * Medium Risk

Title * Summarize your findings for the bug or vulnerability. (This will be the issue title.)

Here are some potential security vulnerabilities that I have identified in this particular contract (BytecodeCompressor.sol)

Links to affected code * Provide GitHub links, including line numbers, to all instances of this bug throughout the repo. (How do I link to line numbers on GitHub?)

https://github.com/code-423n4/2023-03-zksync/blob/main/contracts/BytecodeCompressor.sol
https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L43 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L62-L63 https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L50

Vulnerability details * Link to all referenced sections of code in GitHub. You can use markdown including markdown math notation in this field

Impact

1. Integer overflow: In the publishCompressedBytecode function, the check dictionary.length <= 2 ** 16 * 8 is intended to ensure that the dictionary does not become too large, but it is not sufficient to prevent an integer overflow. If dictionary.length exceeds 2 ** 16, the multiplication dictionary.length * 8 will overflow, and the check will pass even if the dictionary is too large. This can be fixed by changing the check to dictionary.length <= 2 ** 13.

2. Out-of-bounds access: In the publishCompressedBytecode function, the index indexOfEncodedChunk is calculated as uint256(encodedData.readUint16(encodedDataPointer)) * 8, which is then used to read a 64-bit value from the dictionary array. If indexOfEncodedChunk exceeds dictionary.length, an out-of-bounds access will occur and may result in unexpected behavior or a crash. This can be fixed by adding a check to ensure that indexOfEncodedChunk is within the bounds of the dictionary array.

3. Reentrancy: The publishCompressedBytecode function makes two external calls: one to the sendToL1 function of an unknown contract and another to the markBytecodeAsPublished function of a storage contract. If either of these functions can call back into the BytecodeCompressor contract, it could result in a reentrancy vulnerability. This can be mitigated by using the "checks-effects-interactions" pattern and ensuring that any external calls are made at the end of the function after all state changes have been made.

4. Unauthenticated access: The publishCompressedBytecode function can be called by anyone, which means that anyone can publish compressed bytecode on behalf of someone else. If the KNOWN_CODE_STORAGE_CONTRACT is used to verify the authenticity of bytecode, this could lead to unauthorized code being marked as published. This can be fixed by adding an authentication mechanism to ensure that only authorized users can publish compressed bytecode.

Proof of Concept

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L35-L68.

function publishCompressedBytecode(
        bytes calldata _bytecode,
        bytes calldata _rawCompressedData
    ) external payable returns (bytes32 bytecodeHash) {
        unchecked {
            (bytes calldata dictionary, bytes calldata encodedData) = _decodeRawBytecode(_rawCompressedData);

            require(dictionary.length % 8 == 0, "Dictionary length should be a multiple of 8");
            require(dictionary.length <= 2 ** 16 * 8, "Dictionary is too big");
            require(
                encodedData.length * 4 == _bytecode.length,
                "Encoded data length should be 4 times shorter than the original bytecode"
            );

            for (uint256 encodedDataPointer = 0; encodedDataPointer < encodedData.length; encodedDataPointer += 2) {
                uint256 indexOfEncodedChunk = uint256(encodedData.readUint16(encodedDataPointer)) * 8;
                require(indexOfEncodedChunk < dictionary.length, "Encoded chunk index is out of bounds");

                uint64 encodedChunk = dictionary.readUint64(indexOfEncodedChunk);
                uint64 realChunk = _bytecode.readUint64(encodedDataPointer * 4);

                require(encodedChunk == realChunk, "Encoded chunk does not match the original bytecode");
            }
        }

        bytecodeHash = Utils.hashL2Bytecode(_bytecode);

        bytes32 rawCompressedDataHash = L1_MESSENGER_CONTRACT.sendToL1(_rawCompressedData);
        KNOWN_CODE_STORAGE_CONTRACT.markBytecodeAsPublished(
            bytecodeHash,
            rawCompressedDataHash,
            _rawCompressedData.length
        );
    }

Tools Used

VSCode, HardHat

Recommended Mitigation Steps

Test for dictionary size limit: You can test the contract's behavior when the dictionary becomes overcrowded by trying to add more than 2^16 + 1 elements to the dictionary.

Test for out-of-bounds error: You can test the contract's behavior by passing an encoded chunk index that is out of bounds.

Test for permission control: You can test the contract's behavior by attempting to call the publishCompressedBytecode function from an address that does not have permission to do so.

Also, There are no overflow checks in the code. For example, in the publishCompressedBytecode function, the line require(encodedData.length * 4 == _bytecode.length, "Encoded data length should be 4 times shorter than the original bytecode"); assumes that encodedData.length * 4 will not overflow. It's always a good idea to perform overflow checks, especially in Solidity. It's recommended to use safe math libraries like OpenZeppelin's SafeMath to avoid potential issues.