Incorrect `_setVerifierParams()` implementation

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol#L147-L157

Vulnerability details

Impact

To properly upgrade VerifierParams, none of the parameters: recursionNodeLevelVkHash, recursionLeafLevelVkHash, recursionCircuitsSetVksHash can be empty (bytes32(0)). However, the current implementation of _setVerifierParams() does not enforce this requirement. If at least one of these parameters is non-zero - _setVerifierParams() will upgrade VerifierParams.

This basically means, that it's possible to upgrade VerifierParams even when some of the parameters are bytes32(0). This should not be possible. The VerifierParams should be upgraded only when all provided parameters are non-empty.

This behavior is even confirmed in the previous version of the zkSync code:

File: previous contest

    function _setVerifierParams(VerifierParams calldata _newVerifierParams) private {
        if (
            _newVerifierParams.recursionNodeLevelVkHash == bytes32(0) ||
            _newVerifierParams.recursionLeafLevelVkHash == bytes32(0) ||
            _newVerifierParams.recursionCircuitsSetVksHash == bytes32(0)
        ) {
            return;
        }

During the previous contest, function _setVerifierParams() was using || operator, while the current implementation of _setVerifierParams() is using && operator.

Proof of Concept

File: BaseZkSyncUpgrade.sol

        if (
            _newVerifierParams.recursionNodeLevelVkHash == bytes32(0) &&
            _newVerifierParams.recursionLeafLevelVkHash == bytes32(0) &&
            _newVerifierParams.recursionCircuitsSetVksHash == bytes32(0)
        ) {
            return;
        }

        VerifierParams memory oldVerifierParams = s.verifierParams;
        s.verifierParams = _newVerifierParams;
        emit NewVerifierParams(oldVerifierParams, _newVerifierParams);

As demonstrated above, function uses AND operator (&&) instead of OR (||). This basically means, that if at least one parameter is not bytes32(0) - then above condition won't be fulfilled and function will continue its execution and set VerifierParams.

E.g., let's consider a scenario, where:

_newVerifierParams.recursionNodeLevelVkHash == bytes32(0)
_newVerifierParams.recursionLeafLevelVkHash == bytes32(0)
_newVerifierParams.recursionCircuitsSetVksHash == bytes32(11)

Even though above params are not correct (recursionNodeLevelVkHash and recursionLeafLevelVkHash are bytes32(0)), function won't return at lines 147-153, because recursionCircuitsSetVksHash is not bytes32(0) and it will update VerifierParams.

This leads to the conclusion, that _setVerifierParams() will upgrade VerifierParams, even when some of them are empty.

Tools Used

Manual code review

Recommended Mitigation Steps

Use OR instead of AND. It should not be possible to upgrade VerifierParams when any of the parameter is bytes32(0):

The code should be changed to:

        if (
            _newVerifierParams.recursionNodeLevelVkHash == bytes32(0) ||
            _newVerifierParams.recursionLeafLevelVkHash == bytes32(0) ||
            _newVerifierParams.recursionCircuitsSetVksHash == bytes32(0)
        ) {
            return;
        }

Assessed type

Invalid Validation

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

This is considered invalid because if one can change verifier params they can do anything with the system so zero value isn’t a concern (and practically it should be never zero)

[alex-ppg]

The Warden specifies that the verifier parameter adjustment permits zero values for some of the elements of the _newVerifierParams structure which is conditional on an administrator's mistake and thus a finding better suited as part of a QA / Analysis report.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope