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
-
Integer overflow: In the
publishCompressedBytecode
function, the checkdictionary.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. Ifdictionary.length
exceeds2 ** 16
, the multiplicationdictionary.length * 8
will overflow, and the check will pass even if the dictionary is too large. This can be fixed by changing the check todictionary.length <= 2 ** 13
. -
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 exceedsdictionary.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 thatindexOfEncodedChunk
is within the bounds of the dictionary array. -
Reentrancy: The
publishCompressedBytecode
function makes two external calls: one to thesendToL1
function of an unknown contract and another to themarkBytecodeAsPublished
function of a storage contract. If either of these functions can call back into theBytecodeCompressor
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. -
Unauthenticated access: The
publishCompressedBytecode
function can be called by anyone, which means that anyone can publish compressed bytecode on behalf of someone else. If theKNOWN_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.
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
);
}
VSCode, HardHat
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.