Closed code423n4 closed 1 year ago
I (Jay) from the C4 staff created this issue as the warden (dicethedev) was having issues submitting it to the contest directly. I was able to work with the warden to get the issue submitted, so withdrawing this issue.
Lines of code
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
Impact
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.Proof of Concept
https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/BytecodeCompressor.sol#L35-L68.
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 linerequire(encodedData.length * 4 == _bytecode.length, "Encoded data length should be 4 times shorter than the original bytecode")
; assumes thatencodedData.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.