Bytecode Validation Issues with Known Bytecode Hashes

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/KnownCodesStorage.sol#L47

Vulnerability details

Impact

The issue with the bytecode validation process may allow bytecode hashes that are no longer compatible with zk-circuit format or protocol changes to be deployed, potentially leading to deployment of incompatible and erroneous/unprovable code.

Proof of Concept

During the execution of a transaction, there is a function called markFactoryDeps that serves to label the hash of a bytecode as "known." This function, internally calls the function _markBytecodeAsPublished to both validate and mark the bytecode as "known". https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/KnownCodesStorage.sol#L28 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/KnownCodesStorage.sol#L47

During the bytecode validation process, the system checks whether the format of the hash is compatible with the format supported by zk-circuit. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/KnownCodesStorage.sol#L75

Suppose a hash of a contract (let's call it Hash(X)) has been already marked as "known". An issue arises if later some changes occur, such as:

• modifications to the zk-circuit leading to different compatible bytecode hash format
• or the discontinuation of support of certain opcodes in zkSync Era resulting in only the bytecode hash with higher version being acceptable

In this scenario, if another user is intended deploy the same contract with hash Hash(X) again, despite its incompatibility with the latest zk-circuit or protocol changes, the bytecode validation process will be skipped during the marking process because it has already been marked as "known" (getMarker(Hash(X)) = 1). However, the bytecode hash H(X) is no longer compatible with the system and should not be permitted for deployment. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/KnownCodesStorage.sol#L48

The primary reason for this issue is that the bytecode validation process is only conducted when the marker is zero (i.e., when the bytecode hash is not already marked as "known"). However, it is crucial that the bytecode validation process is carried out regardless of the marker status of the bytecode hash.

Tools Used

Recommended Mitigation Steps

The function should be restructured to ensure that the bytecode validation process is invoked prior to checking the marker.

function _markBytecodeAsPublished(bytes32 _bytecodeHash, bool _shouldSendToL1) internal {

        _validateBytecode(_bytecodeHash);

        if (getMarker(_bytecodeHash) == 0) {

            if (_shouldSendToL1) {
                L1_MESSENGER_CONTRACT.requestBytecodeL1Publication(_bytecodeHash);
            }

            // Save as known, to not resend the log to L1
            assembly {
                sstore(_bytecodeHash, 1)
            }

            emit MarkedAsKnown(_bytecodeHash, _shouldSendToL1);
        }
    }

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as low quality report

[miladpiri]

The warden basically says “you perform validation of bytecode hash here": https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/KnownCodesStorage.sol#L75

However, if the zk circuits have a breaking change (i.e. old bytecodes become invalid), it is not clear what would you do.

It is Invalid because it assumes that we need to make breaking changes in the future.

[c4-sponsor]

miladpiri (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[c4-judge]

GalloDaSballo removed the grade

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

While I mostly agree with the Sponsor, this is the other side of risks of upgrades, if some opcode performs in a faulty way for some time then some bytecode data could have to be kept while it no longer being compatible with circuits

This is a theoretical edge case, so QA seems more appropriate (as another approach would be to revert back or to re-write some history through some patch not in scope)