zkSync Era - adeolu's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L398

Vulnerability details

Impact

Here's a simplified explanation of the issue, for easier understanding:

• the executor contract can commit and revert batches.

• When you revert a batch, which includes a system contract upgrade, the contract only deletes the batch number but not the transaction hash associated with the upgrade.

• When you later try to commit new batches that are not meant to have a system contract upgrade, the contract's logic goes wrong.

• It wrongly treats the new batch as if it should be committed with a system contract upgrade because it sees that there's still a transaction hash for the upgrade in the system.

• because you already reverted a batch with upgrade, you dont expect further execution with that upgradeHash, most especially if reason for reverting is because the upgrade transaction hash may cause failure or unintended behaviours.

• This can result in a successful commit and possible execution of the said commit with unintended system contract upgrades by validators or even cause reverts because the system contract upgrade transaction hash doesn't match the expected logValue.

Flow of actions

tries to upgrade system contract ---> commits batch with system upgrade ---> notices fault ---> reverts batch with system contract upgrade ---> decides to no longer do upgrade for a while ---> tries to commit new batches without upgrades but cannot because system treats new batches commit as if it should have systems contract upgrade because previous systems contract upgrade hash from faulty upgrade is still present.

Here is a detailed explanation of the impact:

if we revert a committed batch or batches that has a system contracts upgrade committed, on the next commit, if the new batches to be committed are meant to have no system contracts upgrade, the commitBatch() logic will always work wrongly and try to commit the new batches as a batch with system contracts upgrade via _commitBatchesWithSystemContractsUpgrade() instead of committing it as a batch with no system contracts upgrade. This happens because in revertBatches, only s.l2SystemContractsUpgradeBatchNumber is deleted and s.l2SystemContractsUpgradeTxHash is not.

    function revertBatches(uint256 _newLastBatch) external nonReentrant onlyValidator {
        require(s.totalBatchesCommitted > _newLastBatch, "v1"); // The last committed batch is less than new last batch
        require(_newLastBatch >= s.totalBatchesExecuted, "v2"); // Already executed batches cannot be reverted
        //require (_newLastBatch < s.totalBatchesVerified, "v3");
        if (_newLastBatch < s.totalBatchesVerified) {
            s.totalBatchesVerified = _newLastBatch;
        }
        s.totalBatchesCommitted = _newLastBatch;

        // Reset the batch number of the executed system contracts upgrade transaction if the batch
        // where the system contracts upgrade was committed is among the reverted batches.
        if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
            delete s.l2SystemContractsUpgradeBatchNumber;
        }

        emit BlocksRevert(s.totalBatchesCommitted, s.totalBatchesVerified, s.totalBatchesExecuted);
    }

This will cause the commit to update s.l2SystemContractsUpgradeBatchNumber wrongly (because the batch is not meant to be an upgrade batch) in _commitBatchesWithSystemContractsUpgrade() or cause reverts because the expectedUpgradeTxHash will not be eq to the logValue as seen here

Note that s.l2SystemContractsUpgradeTxHash is only set via s.l2SystemContractsUpgradeTxHash() in BaseZKSyncUpgrade.sol and deleted/reset to bytes(0) upon sucessfull execution of the systems contracts upgrade batch via execute() in executor.sol. This means once s.l2SystemContractsUpgradeTxHash is set it is impossible to revert the value if you indeed want to revert a committed upgrade batch and prevent a system contracts upgrade from executing. There is no possible way to set s.l2SystemContractsUpgradeTxHash to bytes(0) again as if _l2ProtocolUpgradeTx.txType == 0, no logic is performed in _setL2SystemContractUpgrade() and s.l2SystemContractsUpgradeTxHash remains as previously set in storage.

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol#L166C1-L170C1

        // If the type is 0, it is considered as noop and so will not be required to be executed.
        if (_l2ProtocolUpgradeTx.txType == 0) {
            return bytes32(0);
        }

This issue prevents a reversal of not yet executed system contract upgrades because the s.l2SystemContractsUpgradeTxHash is not reverted too. In the case where the previously set l2SystemContractsUpgradeTxHash is to be stopped from executing with a batch due to whatever reason, it is not possible to do so.

Proof of Concept

revertBatches() is meant to be used to undo committed batches that have not been executed yet. It is possible that unexecuted batches may contain system contracts upgrades, in that case, revertBatches( handle that by deleting the s.l2SystemContractsUpgradeBatchNumber but not deleting the s.l2SystemContractsUpgradeTxHash. https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L409C1-L412C1

        // Reset the batch number of the executed system contracts upgrade transaction if the batch
        // where the system contracts upgrade was committed is among the reverted batches.
        if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
            delete s.l2SystemContractsUpgradeBatchNumber;
        }

upon successful reverting of committed batches with a system contracts upgrade, we should be able to commit new batches. Lets assume a new batch (with no intention to have it as a system contracts upgrade batch) is supplied to the commitBatches() fcn for committing. In the commitBatches() logic depending on the value of s.l2SystemContractsUpgradeTxHash and s.l2SystemContractsUpgradeBatchNumber we either do the commit with system contracts upgrade via _commitBatchesWithSystemContractsUpgrade() or without via _commitBatchesWithoutSystemContractsUpgrade().

In this case of trying to commit a batch with no system upgrade immediately after reverting a batch that contained a system contracts upgrade, the logic will force the new batch with no system contract upgrade to be committed with the _commitBatchesWithSystemContractsUpgrade() function instead of _commitBatchesWithoutSystemContractsUpgrade() function (because it is not supposed to be a system contracts upgrade batch). This happens because after the revert of a batch with system contracts upgrade via revertBatches(), s.l2SystemContractsUpgradeBatchNumber is deleted but the s.l2SystemContractsUpgradeTxHash (systemContractsUpgradeTxHash) is not deleted. commitBatches() has this conditional statement

if (systemContractsUpgradeTxHash == bytes32(0) || s.l2SystemContractsUpgradeBatchNumber != 0)

if the above statement is true it will commit batches without contracts upgrade via _commitBatchesWithoutSystemContractsUpgrade() and if false it will commit batches via _commitBatchesWithSystemContractsUpgrade. Since only s.l2SystemContractsUpgradeBatchNumber is deleted, s.l2SystemContractsUpgradeBatchNumber = 0 and systemContractsUpgradeTxHash != bytes(0).

This means if (systemContractsUpgradeTxHash == bytes32(0) || s.l2SystemContractsUpgradeBatchNumber != 0) will return false and _commitBatchesWithSystemContractsUpgrade will be used to commit the new batches which originally are not meant to have system contract upgrades.

    function commitBatches(StoredBatchInfo memory _lastCommittedBatchData, CommitBatchInfo[] calldata _newBatchesData)
        external
        override
        nonReentrant
        onlyValidator
    {
        // Check that we commit batches after last committed batch
        require(s.storedBatchHashes[s.totalBatchesCommitted] == _hashStoredBatchInfo(_lastCommittedBatchData), "i"); // incorrect previous batch data
        require(_newBatchesData.length > 0, "No batches to commit");

        bytes32 systemContractsUpgradeTxHash = s.l2SystemContractsUpgradeTxHash;
        // Upgrades are rarely done so we optimize a case with no active system contracts upgrade.
        if (systemContractsUpgradeTxHash == bytes32(0) || s.l2SystemContractsUpgradeBatchNumber != 0) {
            _commitBatchesWithoutSystemContractsUpgrade(_lastCommittedBatchData, _newBatchesData);
        } else {
            _commitBatchesWithSystemContractsUpgrade(
                _lastCommittedBatchData,
                _newBatchesData,
                systemContractsUpgradeTxHash
            );
        }

        s.totalBatchesCommitted = s.totalBatchesCommitted + _newBatchesData.length;
    }

Now that the new batches that weren't intended to be committed as a batch with system contract upgrades processed through _commitBatchesWithSystemContractsUpgrade(). two things can happen:

• s.l2SystemContractsUpgradeBatchNumber will be set to the new batch batchNumber as seen here. the state updates wrongly and the commit is sucessfull.

• _commitBatchesWithSystemContractsUpgrade() takes the systemContractsUpgradeTxHash and passes it into _commitOneBatch() as the expected hash and uses it in a require statement as seen here in __processL2Logs(). If this require statement is reached, it will fail because the expected hash (systemContractsUpgradeTxHash) is not equal to the logValue which is derived from the new batch _newBatch.systemLogs as seen here, if not reached (in a case where by one of these if statements are true before the require statement that compares the expected hash (systemContractsUpgradeTxHash) to the logValue is reached) the _commitBatchesWithSystemContractsUpgrade() function logic will continue and the commit will be sucessful but s.l2SystemContractsUpgradeBatchNumber will be updated wrongly because the batch commit was not meant to handle any system contract upgrades.

    /// @dev Commits new batches with a system contracts upgrade transaction.
    /// @param _lastCommittedBatchData The data of the last committed batch.
    /// @param _newBatchesData An array of batch data that needs to be committed.
    /// @param _systemContractUpgradeTxHash The transaction hash of the system contract upgrade.
    function _commitBatchesWithSystemContractsUpgrade(
        StoredBatchInfo memory _lastCommittedBatchData,
        CommitBatchInfo[] calldata _newBatchesData,
        bytes32 _systemContractUpgradeTxHash
    ) internal {
        // The system contract upgrade is designed to be executed atomically with the new bootloader, a default account,
        // ZKP verifier, and other system parameters. Hence, we ensure that the upgrade transaction is
        // carried out within the first batch committed after the upgrade.

        // While the logic of the contract ensures that the s.l2SystemContractsUpgradeBatchNumber is 0 when this function is called,
        // this check is added just in case. Since it is a hot read, it does not encure noticable gas cost.
        require(s.l2SystemContractsUpgradeBatchNumber == 0, "ik");

        // Save the batch number where the upgrade transaction was executed.
        s.l2SystemContractsUpgradeBatchNumber = _newBatchesData[0].batchNumber;

        for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
            // The upgrade transaction must only be included in the first batch.
            bytes32 expectedUpgradeTxHash = i == 0 ? _systemContractUpgradeTxHash : bytes32(0);
            _lastCommittedBatchData = _commitOneBatch(
                _lastCommittedBatchData,
                _newBatchesData[i],
                expectedUpgradeTxHash
            );

            s.storedBatchHashes[_lastCommittedBatchData.batchNumber] = _hashStoredBatchInfo(_lastCommittedBatchData);
            emit BlockCommit(
                _lastCommittedBatchData.batchNumber,
                _lastCommittedBatchData.batchHash,
                _lastCommittedBatchData.commitment
            );
        }
    }

If sucessfully committed, it can be proven and executed by the validators and a faulty/not intended/ expected to be reverted l2SystemContractsUpgradeTxHash will be used in the upgrade.

Tools Used

manual review

Recommended Mitigation Steps

when reverting batches, delete the s.l2SystemContractsUpgradeTxHash too along with the s.l2SystemContractsUpgradeBatchNumber if the batch contains a systems contract upgrade.

    function revertBatches(uint256 _newLastBatch) external nonReentrant onlyValidator {
        require(s.totalBatchesCommitted > _newLastBatch, "v1"); // The last committed batch is less than new last batch
        require(_newLastBatch >= s.totalBatchesExecuted, "v2"); // Already executed batches cannot be reverted

        if (_newLastBatch < s.totalBatchesVerified) {
            s.totalBatchesVerified = _newLastBatch;
        }
        s.totalBatchesCommitted = _newLastBatch;

        // Reset the batch number of the executed system contracts upgrade transaction if the batch
        // where the system contracts upgrade was committed is among the reverted batches.
        if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
            delete s.l2SystemContractsUpgradeBatchNumber;
            delete s.l2SystemContractsUpgradeTxHash;
        }

        emit BlocksRevert(s.totalBatchesCommitted, s.totalBatchesVerified, s.totalBatchesExecuted);
    }

Assessed type

Error