zkSync Era - evmboi32's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/ComplexUpgrader.sol#L21-L31 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/upgrades/DefaultUpgrade.sol#L25-L46 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol#L214-L230 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L398-L414 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L189-L197

Vulnerability details

Impact

In case of providing a faulty implementation (buggy) for an upgrade, the upgrade must be carried out and cannot be changed even if we revert the batches.

Proof of Concept

Let's imagine a scenario where the zkSync team wants to carry out an upgrade for some of the critical functionalities in the system.

This is done by the following steps:

• FORCE_DEPLOYER calls the upgrade function on the ComplexUpgrader

function upgrade(address _delegateTo, bytes calldata _calldata) external payable {
    require(msg.sender == FORCE_DEPLOYER, "Can only be called by FORCE_DEPLOYER");

    require(_delegateTo.code.length > 0, "Delegatee is an EOA");
    (bool success, bytes memory returnData) = _delegateTo.delegatecall(_calldata);
    assembly {
        if iszero(success) {
            revert(add(returnData, 0x20), mload(returnData))
        }
    }
}

• This will delegate call to upgrade in DefaultUpgrade.sol

function upgrade(ProposedUpgrade calldata _proposedUpgrade) public override returns (bytes32) {
    super.upgrade(_proposedUpgrade);

    _setNewProtocolVersion(_proposedUpgrade.newProtocolVersion);
    _upgradeL1Contract(_proposedUpgrade.l1ContractsUpgradeCalldata);
    _upgradeVerifier(_proposedUpgrade.verifier, _proposedUpgrade.verifierParams);
    _setBaseSystemContracts(_proposedUpgrade.bootloaderHash, _proposedUpgrade.defaultAccountHash);

    bytes32 txHash;
    txHash = _setL2SystemContractUpgrade(
        _proposedUpgrade.l2ProtocolUpgradeTx,
        _proposedUpgrade.factoryDeps,
        _proposedUpgrade.newProtocolVersion
    );

    _setAllowList(IAllowList(_proposedUpgrade.newAllowList));
    _postUpgrade(_proposedUpgrade.postUpgradeCalldata);

    emit UpgradeComplete(_proposedUpgrade.newProtocolVersion, txHash, _proposedUpgrade);

    return Diamond.DIAMOND_INIT_SUCCESS_RETURN_VALUE;
}

• Which sets the s.l2SystemContractsUpgradeTxHash

function _setL2SystemContractUpgrade(
    IMailbox.L2CanonicalTransaction calldata _l2ProtocolUpgradeTx,
    bytes[] calldata _factoryDeps,
    uint256 _newProtocolVersion
) internal returns (bytes32) {
    ...
    bytes memory encodedTransaction = abi.encode(_l2ProtocolUpgradeTx);
    ...

    bytes32 l2ProtocolUpgradeTxHash = keccak256(encodedTransaction);
    s.l2SystemContractsUpgradeTxHash = l2ProtocolUpgradeTxHash;
    return l2ProtocolUpgradeTxHash;
}

• Now in the new committed batch this upgrade tx will be included and eventually proven and executed.
• Let's say that before the batch that includes the upgrade is executed the team realizes that the upgraded system would have a critical vulnerability
• So they decide to revert the batches to before the upgrade tx was commited but this only deletes the s.l2SystemContractsUpgradeBatchNumber which means that the faulty implementation still has to be carried out in the next batch. This gives an attacker time to exploit the whole system in case of such an error.

function revertBatches(uint256 _newLastBatch) external nonReentrant onlyValidator {
    ....
    if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
        delete s.l2SystemContractsUpgradeBatchNumber;
    }
    ...
}

• Even if the team wants to upgrade the s.l2SystemContractsUpgradeTxHash it won't succeed because if trying to invoke the upgradeprocess again to set the new tx hash will fail.
• The current implementation of DefaultUpgrade tries to set the new protocol version which requires that the s.l2SystemContractsUpgradeBatchNumber == bytes32(0) so the process will fail and there is 100% certainty that the faulty implementation will be carried out even if the batch is reverted.

function _setNewProtocolVersion(uint256 _newProtocolVersion) internal {
    uint256 previousProtocolVersion = s.protocolVersion;
    require(
        _newProtocolVersion > previousProtocolVersion,
        "New protocol version is not greater than the current one"
    );

    // If the previous upgrade had an L2 system upgrade transaction, we require that it is finalized.
    require(s.l2SystemContractsUpgradeTxHash == bytes32(0), "Previous upgrade has not been finalized");
    require(
        s.l2SystemContractsUpgradeBatchNumber == 0,
        "The batch number of the previous upgrade has not been cleaned"
    );

    s.protocolVersion = _newProtocolVersion;
    emit NewProtocolVersion(previousProtocolVersion, _newProtocolVersion);
}

Tools Used

VS Code

Recommended Mitigation Steps

It would make more sense that in case of reverting an upgrade tx the s.l2SystemContractsUpgradeTxHash should be provided again to prevent such an error to increase the robustness of the system. In cases like this, the team has time to prevent the upgrade from happening, fix the error, and make an upgrade tx again rather than leaving the vulnerable upgrade in the wild.

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;

    if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
        delete s.l2SystemContractsUpgradeBatchNumber;
+       delete s.l2SystemContractsUpgradeTxHash;
    }

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

Assessed type

Upgradable

Make sure there is enough ETH to cover the fees when executing governance proposals

function _execute(Call[] calldata _calls) internal {
+    uint256 totalValue;
+    for (uint256 i = 0; i < _calls.length; ++i) {
+        totalValue += calls[i].value;
+    }
    
+    require(address(this).balance >= totalValue);

    for (uint256 i = 0; i < _calls.length; ++i) {
        (bool success, bytes memory returnData) = _calls[i].target.call{value: _calls[i].value}(_calls[i].data);
        if (!success) {
            // Propage an error if the call fails.
            assembly {
                revert(add(returnData, 0x20), mload(returnData))
            }
        }
    }
}

There is no check that the Getters facet cannot be frozen

In the provided documentation https://github.com/code-423n4/2023-10-zksync/blob/main/docs/Smart%20contract%20Section/L1%20smart%20contracts.md#gettersfacet it states that the Getters facet must never be frozen. But when doing a diamondCut there is no check if the facet we are updating the selectors for is the Getters facet. When the first selector is added to a new facet the facet itself is pushed into the facet array and _isFacetFreezable is set. Make sure to check if the facet in question is the Getters facet and require that the _isFacetFreezable is set to false.

function diamondCut(DiamondCutData memory _diamondCut) internal {
    FacetCut[] memory facetCuts = _diamondCut.facetCuts;
    address initAddress = _diamondCut.initAddress;
    bytes memory initCalldata = _diamondCut.initCalldata;
    uint256 facetCutsLength = facetCuts.length;
    for (uint256 i = 0; i < facetCutsLength; i = i.uncheckedInc()) {
        Action action = facetCuts[i].action;
        address facet = facetCuts[i].facet;
        bool isFacetFreezable = facetCuts[i].isFreezable;
        bytes4[] memory selectors = facetCuts[i].selectors;

        require(selectors.length > 0, "B"); // no functions for diamond cut

        if (action == Action.Add) {
            _addFunctions(facet, selectors, isFacetFreezable);
        } else if (action == Action.Replace) {
            _replaceFunctions(facet, selectors, isFacetFreezable);
        } else if (action == Action.Remove) {
            _removeFunctions(facet, selectors);
        } else {
            revert("C"); // undefined diamond cut action
        }
    }

    _initializeDiamondCut(initAddress, initCalldata);
    emit DiamondCut(facetCuts, initAddress, initCalldata);
}

Wrong comment in L1Messenger#publishPubdataAndClearState

Inside the header, it says that there are 2 bytes for the total len of compressed as opposed to the documentation https://github.com/code-423n4/2023-10-zksync/blob/main/docs/Smart%20contract%20Section/Handling%20pubdata%20in%20Boojum/State%20diff%20compression%20v1%20spec.md where it says that the total_logs_len is 3 bytes.

function publishPubdataAndClearState(
        bytes calldata _totalL2ToL1PubdataAndStateDiffs
    ) external onlyCallFromBootloader {
        ...
        /// Check State Diffs
        /// encoding is as follows:
        /// header (1 byte version, 2 bytes total len of compressed, 1 byte enumeration index size, 2 bytes number of initial writes)
        /// body (N bytes of initial writes [32 byte derived key || compressed value], M bytes repeated writes [enumeration index || compressed value])
        /// encoded state diffs: [20bytes address][32bytes key][32bytes derived key][8bytes enum index][32bytes initial value][32bytes final value]
        ...
}

We also use 3 bytes in the code

uint24 compressedStateDiffSize = uint24(bytes3(_totalL2ToL1PubdataAndStateDiffs[calldataPtr:calldataPtr + 3]));

```audi

# Unused variable in ```SystemContext#publishTimestampDataToL1```
```solidity
function publishTimestampDataToL1() external onlyCallFromBootloader {
    (uint128 currentBatchNumber, uint128 currentBatchTimestamp) = getBatchNumberAndTimestamp();
    (, uint128 currentL2BlockTimestamp) = getL2BlockNumberAndTimestamp();

    // The structure of the "setNewBatch" implies that currentBatchNumber > 0, but we still double check it
    require(currentBatchNumber > 0, "The current batch number must be greater than 0");
    bytes32 prevBatchHash = batchHash[currentBatchNumber - 1];

    // In order to spend less pubdata, the packed version is published
    uint256 packedTimestamps = (uint256(currentBatchTimestamp) << 128) | currentL2BlockTimestamp;

    SystemContractHelper.toL1(
        false,
        bytes32(uint256(SystemLogKey.PACKED_BATCH_AND_L2_BLOCK_TIMESTAMP_KEY)),
        bytes32(packedTimestamps)
    );
}

The variable bytes32 prevBatchHash = batchHash[currentBatchNumber - 1]; is unused.

Wrong check in the L1Messenger#publishPubdataAndClearState

The number of logs is compared to itself which is always true. Instead, it should be compared to L2_TO_L1_LOGS_MERKLE_TREE_LEAVES.

function publishPubdataAndClearState(
    bytes calldata _totalL2ToL1PubdataAndStateDiffs
) external onlyCallFromBootloader {
    uint256 calldataPtr = 0;

    /// Check logs
    uint32 numberOfL2ToL1Logs = uint32(bytes4(_totalL2ToL1PubdataAndStateDiffs[calldataPtr:calldataPtr + 4]));
    require(numberOfL2ToL1Logs <= numberOfL2ToL1Logs, "Too many L2->L1 logs");
    calldataPtr += 4;
    
    ...
}