Description

In the CombinedSkipCircuit's define function, a trusted block height and header hash are provided as public inputs (trusted_block and trusted_header_hash) as well as a target block height (target_block). The target block's header hash is then obtained by using the Tendermint X circuit, TendermintSkipCircuit:

let target_header_hash = builder.skip::<MAX_VALIDATOR_SET_SIZE, CHAIN_ID_SIZE_BYTES>(
    C::CHAIN_ID_BYTES,
    C::SKIP_MAX,
    trusted_block,
    trusted_header_hash,
    target_block,
);

For soundness of the CombinedSkipCircuit, the skip function should ensure that, assuming that trusted_block and trusted_header_hash are legitimate, the target_header_hash is the hash of a legitimate Tendermint block with block height target_block according to the Tendermint light client consensus rules.

However, the skip function is implemented as follows.

fn skip<const MAX_VALIDATOR_SET_SIZE: usize, const CHAIN_ID_SIZE_BYTES: usize>(
    &mut self,
    chain_id_bytes: &[u8],
    skip_max: usize,
    trusted_block: U64Variable,
    trusted_header_hash: Bytes32Variable,
    target_block: U64Variable,
) -> Bytes32Variable {
    let mut input_stream = VariableStream::new();
    input_stream.write(&trusted_block);
    input_stream.write(&trusted_header_hash);
    input_stream.write(&target_block);
    let output_stream = self.async_hint(
        input_stream,
        SkipOffchainInputs::<MAX_VALIDATOR_SET_SIZE> {},
    );
    let skip_variable = output_stream.read::<VerifySkipVariable<MAX_VALIDATOR_SET_SIZE>>(self);

    let target_header = skip_variable.target_header;

    self.verify_skip::<MAX_VALIDATOR_SET_SIZE, CHAIN_ID_SIZE_BYTES>(
        chain_id_bytes,
        skip_max,
        &skip_variable,
    );
    target_header
}

No constraints are introduced by this function itself, only by the following call.

self.verify_skip::<MAX_VALIDATOR_SET_SIZE, CHAIN_ID_SIZE_BYTES>(
    chain_id_bytes,
    skip_max,
    &skip_variable,
);

Note that before this call, while the arguments trusted_block, trusted_header_hash, and target_block are used to calculate skip_variable in this implementation of the prover, there are no constraints introduced in the circuit enforcing a relation between skip_variable and these arguments. An attacker can thus use an arbitrary chain of Tendermint blocks that pass the consensus checks — but using a fake initial block skip_variable.trusted_header of their choosing — and thereby have wide latitude in what to choose as skip_variable.target_header while fuilfilling the verify_skip constraints. For example, the attacker could use arbitrary validators they made up to create a chain of Tendermint blocks, with arbitrary data hashes of their choosing included in these blocks.

Impact

The target_header_hash used by CombinedSkipCircuit can be set to the hash of a Tendermint block for which the attacker could set the data hash and data hash of preceeding blocks in a valid chain to a value of their choosing. As target_header_hash is used as the root of trust for the prove_data_commitment function, this ultimately means that the attacker can produce a CombinedSkipCircuit proof for arbitrary data commitments.

Recommendations

In the skip function, add constraints that check that skip_variable.trusted_block, skip_variable.trusted_header, and skip_variable.target_block are equal to the parameters trusted_block, trusted_header_hash, and target_block that were passed to the function, respectively.

Remediation

This issue has been acknowledged by Succinct, and a fix was implemented in the following commit: .