Unprovable batches will be generated upon doing a system upgrade via shadow proposals

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/governance/Governance.sol#L142-L145

Vulnerability details

Impact

If a system upgrade is ever done via a shadow proposal, unprovable batches will start to be submitted by the operator as the public input used in the proof generation by the EraVM and the ones used by the Verifier would differ.

Proof of Concept

This is gonna take a while, as we have to go deep in how the node works, but I will try to make it as simple as possible. The contract responsible of updating zkSync Era (both on-chain and off-chain components) is the Governance contract, via two types of proposals:

• Transparent $\Rightarrow$ both the hash of the operation and the operation itself are publicly visible on-chain
• Shadow $\Rightarrow$ only the hash of the operation is public

which emits the following events, respectively

    /// @notice Emitted when transparent operation is scheduled.
    event TransparentOperationScheduled(bytes32 indexed _id, uint256 delay, Operation _operation);

    /// @notice Emitted when shadow operation is scheduled.
    event ShadowOperationScheduled(bytes32 indexed _id, uint256 delay);

Now, when we do a system upgrade, for example, update the bytecode of the bootloader, the node must know we changed such a value and update itself accordingly, as it is used as a public input to both the verifier AND the circuits (see here) otherwise

1. If the verifier has the wrong values, there is a completeness bug as it is rejecting VALID proofs
2. If the node has the wrong values, the proofs submitted to the verifier would be always rejected as invalid (this is a design decision to prevent a malicious operator from executing transactions with malicious bytecode)

Our situation is the number 2, as the node "thinks" he is using the correct values and will start sending unprovable batches to the verifier. Let's go step by step.

The node listens to events emitted by core contracts by calling the eth_getLogs RPC method, which returns all the events emitted by an address in a specified block range (see here). The code responsible for that is in governance_upgrades.rs, deep inside the node. In a nutshell, a thread will be running on the background listening to events and updating the database as soon as a certain event appears. If we go to the main function

#[async_trait::async_trait]
impl EventProcessor for GovernanceUpgradesEventProcessor {
    async fn process_events(
        &mut self,
        storage: &mut Connection<'_, Core>,
        client: &dyn EthClient,
        events: Vec<Log>,
    ) -> Result<(), Error> { ... }
}

we see that it first filters all the events by their signature (the first topic)

for event in events
            .into_iter()
            .filter(|event| event.topics[0] == self.upgrade_proposal_signature)
        { ... }

and then updates the database if the target of the operation is the diamond proxy address AND it is an upgrade transaction

            for call in governance_operation
                .calls
                .into_iter()
                .filter(|call| call.target == self.diamond_proxy_address)
            {
                // We might not get an upgrade operation here, but something else instead
                // (e.g. `acceptGovernor` call), so if parsing doesn't work, just skip the call.
                let Ok(upgrade) = ProtocolUpgrade::try_from(call) else {
                    tracing::warn!(
                        "Failed to parse governance operation call as protocol upgrade, skipping"
                    );
                    continue;
                };

                ... // @audit update the database
            }

However, if we go to the signature declaration at the top of the file

impl GovernanceUpgradesEventProcessor {
    pub fn new(
        diamond_proxy_address: Address,
        last_seen_version_id: ProtocolVersionId,
        governance_contract: &Contract,
    ) -> Self {
        Self {
            diamond_proxy_address,
            last_seen_version_id,
            upgrade_proposal_signature: governance_contract
                .event("TransparentOperationScheduled")
                .expect("TransparentOperationScheduled event is missing in abi")
                .signature(),
        }
    }
}

it is ONLY listening to the TransparentOperationScheduled events being emitted by Governance, nothing about the shadow ones. That means if a successful ShadowOperationScheduled operation is executed and any of l2BootloaderBytecodeHash, l2DefaultAccountBytecodeHash or zkPorterIsAvailable is updated, the node will ignore such an upgrade and keep using the old ones, even though they are NOT used anymore by the Verifier as public inputs, which makes the node submit unprovable batches again and again.

Recommended Mitigation Steps

This is a tricky matter, but given the fact that there is a security council that can instant upgrade the system, so if a proposal is meant to fix a critical bug in production there would be no time for an attacker to bin-diff the changes and exploit the system, I would remove the shadow proposals logic, as it is redundant with a security council in place. That is, remove

Governance, function scheduleShadow

    /// @notice Propose "shadow" upgrade, upgrade data is not publishing on-chain.
    /// @notice The owner will be able to execute the proposal either:
    /// - With a `delay` timelock on its own.
    /// - With security council instantly.
    /// @dev Only the current owner can propose an upgrade.
    /// @param _id The operation hash (see `hashOperation` function)
    /// @param _delay The delay time (in seconds) after which the proposed upgrade may be executed by the owner.
    function scheduleShadow(bytes32 _id, uint256 _delay) external onlyOwner {
        _schedule(_id, _delay);
        emit ShadowOperationScheduled(_id, _delay);
    }

completely.

Assessed type

Other

[saxenism]

This is Out of Scope issue. The issue with server not supporting shadow upgrades is a rare and fixable event.

[c4-sponsor]

saxenism (sponsor) disputed

[alex-ppg]

The Warden specifies that a shadow proposal involving a system upgrade will not be properly detected by zkSync Era nodes and thus will cause unprovable batches to be generated.

I consider this to be an out-of-scope issue as it effectively describes an upgrade being improperly performed by the zkSync Era team and being submitted as a shadow proposal instead of a transparent one. Additionally, a shadow proposal representing an upgrade is inherently incompatible with the node's behavior as the Warden specifies so there is no ""real"" solution to this incorrect upgrade scenario.

I appreciate the effort and in-depth technical assessment of the exhibit, however, I cannot consider a shadow proposal system upgrade as being an acceptable operational scenario for the zkSync Era blockchain. This would have been better suited as part of a QA / Analysis report.

[c4-judge]

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

[nethoxa]

Hey, @alex-ppg, thanks for taking the time in reviewing this. I would like to point out some things:

• A system upgrade can be from adding new features, to fixing a critical bug in secret to avoid a malicious attacker from seeing it on-chain, and exploiting it (which is the main purpose of shadow proposals as stated in the docs). It's not that the upgrade is "improperly performed by the zkSync Era team", there is a clear and valid operational flow in the upgrade model, so the scenario I used holds. It's NOT a misbehavior of the zkSync Era team, it IS a flaw in the upgrade model, which is in scope for this contest as the root issue is whithin the Governance.sol file (that's why I submitted this issue in first place)
• Such a flaw has never been stated publicly in neither the channel nor in the contest README, which is the source of trust in what is known and what is not
• This issue affects the entire hyperchain ecosystem, as other hyperchains will be listening too to the Governance contract for upgrades, so it's not only zkSync Era the one that would crash, but all hyperchains would start submitting unprovable batches one after another and crash in sequence (which is pretty disastrous thinking in economic damages)
• Moreover, there is another different issue within the upgrade model, although this one is clearly out of scope and that's why I did not send it in this contest. For completeness and the project's knowledge, even a transparent one will crash the node as it listens to TransparentOperationScheduled event, which is emitted when a proposal is SCHEDULED, not EXECUTED, so the variables mentioned above would not be updated during the delay of the proposal. It's the same as I explained in my submission, but clockwise, that is, the node has the to-be-used pk but the diamond storage has the old ones, that is, the verifier would be rejecting VALID batches.

I strongly believe this issue is within the scope of the contest, as the root issue is within the Governance.sol file, the system upgrade through shadow proposal falls under the upgrade model stated in the docs and because of the REAL threat to the hyperchain ecosystem this issue poses.

[alex-ppg]

Hey @nethoxa, thanks for your due diligence and contribution to the PJQA process! My initial assumption is indeed incorrect, shadow proposals are designed to be utilized for sensitive contract upgrades. This does not affect the out-of-scope relevancy of the submission, however.

Atomically, the shadow proposal has no impact from a smart contract perspective and is in fact a described and desirable trait per the documentation of the system. The main impact is to off-chain nodes whose code is not equipped to handle shadow proposals. As such, the vulnerability's root cause is in the nodes themselves rather than the code. When the shadow proposal is executed, its details are publicly available and thus the nodes can react at that point rather than at the submission stage.

I consider this a valid vulnerability and a valid bug report albeit for the scope of the nodes rather than the zkSync Era contest that concerns itself with smart contracts. A bug bounty exists for the relevant code, and I implore you to seek a bug bounty via that or the zkSync Era team directly instead of via this contest.

I appreciate your effort and thank you for correcting my incorrect assumption, but the C4 contest is not meant to cover this vulnerability.