Summary

Users can create proposals where any attempt that tries to cancel them reaches the gas / transaction limit on the L1, therefore completely circumventing the GovernorGuardianVeto module / safeguard.

Vulnerability Details

The guardians have to use the function cancelL2GovernorProposal within the L1 contract Guardians (https://github.com/Cyfrin/2024-10-zksync/blob/b0c5565b6078a93cc3358a5d5012258caa701385/zk-governance/l1-contracts/src/Guardians.sol#L93) to cancel a proposal. This requests an L2 transaction, which ultimately calls cancel on GovernorGuardianVeto (https://github.com/Cyfrin/2024-10-zksync/blob/b0c5565b6078a93cc3358a5d5012258caa701385/zk-governance/l2-contracts/src/extensions/GovernorGuardianVeto.sol#L32).
It is important to note that the guardians have to pass in the whole description string to the function cancelL2GovernorProposal. The hashing only happens within the function and the hash is then submitted to the L2. It is also important to note that every blockchain has (implicit) limits for the length of calldata. There is an upper limit for the size of transactions (128kB for geth: https://github.com/ethereum/go-ethereum/blob/74ef47462f2a9daea04e81687c4d0b4598826ca2/core/txpool/legacypool/legacypool.go#L56) and a block gas limit (30 million for Ethereum). Because processing the calldata costs gas (especially hashing it), one of these automatically restricts the largest possible string that can be passed in such that the transaction is still included. Note that these limits are typically higher on an L2, the block gas limit is for instance 2**32 on ZkSync Era. But even if there were exactly the same, the following attack would still work, which is discussed below.

A malicious user can exploit the previously mentioned facts and submit proposals that are uncancellable. To do so, they submit a proposal with an extremely long description. The function propose (https://github.com/Cyfrin/2024-10-zksync/blob/cfc1251de29379a9548eeff1eea3c78267288356/zk-governance/l2-contracts/lib/openzeppelin-contracts/contracts/governance/Governor.sol#L273) does not restrict this length in any way. The length of the description is chosen such that the transaction is directly at its limit (either the transaction size limit or the block gas limit, whatever is smaller). Then, when the governors try to cancel the proposal, they need to submit the exact same string to get the same resulting proposal ID. However, this transaction would be above the limit and could not be executed. As previously argued, the limits (especially block gas limit) will be much lower on Ethereum. But even if they are the same, the transaction could not be executed: This happens because both the gas usage and the additional calldata size is larger for cancelL2GovernorProposal than for propose. cancelL2GovernorProposal performs more work (signature verification, requesting an L2 verification, etc...). Most importantly, it hashes the description twice (once in hashL2Proposal and once in the function itself vs. only once for propose. The hashing gas cost is linear in the size of the data, so very expensive for such huge strings.
But also the additional calldata is longer. propose only has the arrays targets, values, and calldatas. cancelL2GovernorProposal includes these as well, but additionally _txRequest, _signers, and _signatures.

Impact

The purpose of GovernorGuardianVeto is for the guardians to be able to veto any proposals, for instance when they are malicious. As shown previously, this mechanism can be circumvented and it is possible to create proposals that cannot be cancelled by the guardians. The L2 governors ZkGovOpsGovernor and ZkTokenGovernor use this module, which both can be used to create proposals that steal funds (e.g. by minting with a ZkTokenGovernor proposal) or break the system (with malicious governance proposals). This therefore seems to match funds being "nearly at risk".

Recommendations

Change cancelL2GovernorProposal such that it accepts the description hash instead of the description string.