Description

In circuits/src/keccak/mod.rs, the function AssignedKeccakInputs::to_instance_values is implemented as follows:

/// Flattens `self`, disregarding the length if `self` is fixed, and keeping it otherwise.
pub fn to_instance_values(&self) -> Vec<AssignedValue<F>> {
    let flattening_function = match self.input_type() {
        KeccakInputType::Fixed => AssignedKeccakInput::flatten,
        KeccakInputType::Variable => {
            AssignedKeccakInput::flatten_with_len_and_commitment
        }
    };
    self.0.iter().flat_map(flattening_function).collect()
}

The instance should contain the commitment hash and commitment-point limbs also in the fixed case. However, AssignedKeccakInput::flatten is implemented as follows:

/// Appends the application public inputs to the vk hash.
pub fn flatten(&self) -> Vec<AssignedValue<F>> {
    let mut result = vec![self.app_vk_hash];
    result.extend_from_slice(&self.app_public_inputs);
    result
}

The behavior of flatten is not as one might expect from the name, as it ignores the commitment hash and commitment-point limbs.

Impact

The function AssignedKeccakInputs::to_instance_values will return an incorrect result in the case of fixed inputs.

The practical impact is unclear. The function is ultimately only called by KeccakCircuit::synthesize, and KeccakCircuit::new panics on fixed inputs:

// ...
if is_fixed {
    Self::fixed_input(ctx, &range, &mut keccak, &assigned_input);
    panic!("Keccak fixed length no longer supported");
} else {
// ...

Recommendations

The implementation for AssignedKeccakInputs::to_instance_values should be updated in the fixed case, with one possibility being disabling support for that case (i.e., panicking in that case).

The current implementation of AssignedKeccakInput::flatten is reasonable as used by other callers, specifically in KeccakCircuit::var_input, as it is used to extract the input to the proof ID keccak calculated from circuit ID and public inputs. Perhaps it makes sense to rename flatten for that purpose though, to reduce confusion. For example, extract_proof_id_input or similar might be more descriptive.

Remediation

This issue has been acknowledged by Nebra, and a fix was implemented in commit 3af8a51e↗.