Governor admin can set verifier address and verify params to a wrong or invalid address intentionally

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/Governance.sol#L45
https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/Executor.sol#L231
https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/Executor.sol#L248
https://github.com/code-423n4/2022-10-zksync/blob/456078b53a6d09636b84522ac8f3e8049e4e3af5/ethereum/contracts/zksync/facets/Executor.sol#L260

Vulnerability details

Impact

Governor admin has the power to set the verifier address to whatever he wants.

If the admin set the verifier address to wrong or invalid address intentionally,

the validator cannot properly process the transaction and prove the transaction is processed.

Proof of Concept

The governor can set the verifier address and verify params to whatever he wants by calling this function:

/// @notice Change the address of the verifier smart contract
/// @param _newVerifier Verifier smart contract address
function setVerifier(Verifier _newVerifier) external onlyGovernor {
	Verifier oldVerifier = s.verifier;
	if (oldVerifier != _newVerifier) {
		s.verifier = _newVerifier;
		emit NewVerifier(address(oldVerifier), address(_newVerifier));
	}
}

/// @notice Change the verifier parameters
/// @param _newVerifierParams New parameters for the verifier
function setVerifierParams(VerifierParams calldata _newVerifierParams) external onlyGovernor {
	VerifierParams memory oldVerifierParams = s.verifierParams;

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

For example, if the admin misbehave and set the verifier to address(0) or a random EOA account, the validator cannot properly prove the message by calling:

/// @notice Blocks commitment verification.
/// @notice Only verifies block commitments without any other processing
function proveBlocks(
	StoredBlockInfo calldata _prevBlock,
	StoredBlockInfo[] calldata _committedBlocks,
	ProofInput calldata _proof

note that the verifiy params is used in

// Save the variable from the storage to memory to save gas
VerifierParams memory verifierParams = s.verifierParams;

// Initialize the array, that will be used as public input to the ZKP
uint256[] memory proofPublicInput = new uint256[](committedBlocksLength);

// Check that the block passed by the validator is indeed the first unverified block
require(_hashStoredBlockInfo(_prevBlock) == s.storedBlockHashes[currentTotalBlocksVerified], "t1");

bytes32 prevBlockCommitment = _prevBlock.commitment;
for (uint256 i = 0; i < committedBlocksLength; i = i.uncheckedInc()) {
	require(
		_hashStoredBlockInfo(_committedBlocks[i]) ==
			s.storedBlockHashes[currentTotalBlocksVerified.uncheckedInc()],
		"o1"
	);

	bytes32 currentBlockCommitment = _committedBlocks[i].commitment;
	proofPublicInput[i] = _getBlockProofPublicInput(
		prevBlockCommitment,
		currentBlockCommitment,
		_proof,
		verifierParams
	);

	prevBlockCommitment = currentBlockCommitment;
	currentTotalBlocksVerified = currentTotalBlocksVerified.uncheckedInc();
}

and the verifier address is used in:

// #if DUMMY_VERIFIER == false
bool successVerifyProof = s.verifier.verify_serialized_proof(proofPublicInput, _proof.serializedProof);
require(successVerifyProof, "p"); // Proof verification fail

If this verification fails, Blocks commitment verification failed.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add more parameter when admin setting the verifier to make sure that the verifier can verify dummy proof.

Also, we recommend the project use multsig wallet to safe guard to the admin address.

[miladpiri]

This seems like self-attack, and does not show a clear vulnerability. So, invalid issue!

[c4-sponsor]

miladpiri marked the issue as sponsor disputed

[c4-judge]

GalloDaSballo marked the issue as duplicate of #144

[GalloDaSballo]

L

[c4-judge]

Duplicate of #239