Piltover

Appchain Core

6.1 The update_state(...) can be blocked by a malicious actor

The update_state(...) function ensures that the layout_bridge_output passed as an argument is verified through the fact registry. The operator must first pass the Stark proof to the fact registry to prove the fact and then call update_state(...). It also ensures that the proof is verified with at least 50 security bits.

The issue lies in the verification process within the update_state(...) function. When all verifications for a given fact are retrieved, the code only checks the security bits of the verification at the 0th index of the returned array.

fn update_state(
    ref self: ContractState,
    snos_output: Span<felt252>,
    layout_bridge_output: Span<felt252>,
    onchain_data_hash: felt252,
    onchain_data_size: u256,
) {
    // ...
    let program_info = self.config.program_info.read();
    // ...
    let output_hash = poseidon_hash_span(layout_bridge_output);
    // ...
    let fact = poseidon_hash_span(
        array![program_info.bootloader_program_hash, output_hash].span(),
    );
    let verifications = IFactRegistryDispatcher {
        contract_address: self.config.get_facts_registry(),
    }
        .get_all_verifications_for_fact_hash(fact);

    if verifications.len() == 0 {
        core::panic_with_felt252(errors::NO_FACT_REGISTERED)
    };
    // @audit-issue It always checks the element at 0th index
    assert!(*verifications.at(0).security_bits > 50);
    // ...
}

This means that if a malicious actor registers a valid fact with fewer than 50 security bits before the legitimate operator registers the same fact with the required 50 or more security bits, the update_state(...) function will revert. This is because the assert!(*verifications.at(0).security_bits > 50) check will fail, as it will evaluate the low-security verification submitted by the attacker.

The impact of this vulnerability is that a malicious actor can effectively grief the operator by front-running the state update and registering a low-security verification for a valid fact, preventing the legitimate state transition. This could lead to a denial of service for state updates, potentially halting critical operations.

Given the asynchronous nature of the sequencer and the orchestrator (operator), where the sequencer continuously produces blocks and the orchestrator submits state updates periodically, this vulnerability presents a significant risk. Since the sequencer's RPC endpoint for block data is public, an attacker has a window of opportunity to fetch this data, compute a proof, and submit it with lower security bits before the legitimate operator. This action not only blocks the orchestrator's update_state(...) call but can also necessitate a chain reorganization. If the sequencer has produced blocks ahead of the blocked state update, those blocks cannot be finalized, leading to instability and potential rollbacks in the appchain.

Recommendation

Consider revisiting the fact verification mechanism. Instead of relying on a specific order or index of registered facts, ensure that the system verifies if any submitted proof for the fact meets the required security threshold. This could involve iterating through all registered proofs for a given fact or utilizing a dedicated function within the fact registry that directly checks for a minimum security level.

Resolution

This issue has been addressed in 82665b7e by using the utils library of integrity to check if any submitted proofs for the fact meets the security threshold.

6.2 Excessive snos_output size or message count can lead to update_state(...) failure and DoS

The Piltover system's state is updated on Starknet via the update_state(...) function, which accepts the Starknet OS output (snos_output) as calldata. This snos_output can include messages to be relayed from the Appchain to Starknet, as well as messages to be processed for the Appchain itself. The processing of these messages occurs within the update_state(...) transaction's execution flow, primarily handled by functions within the src/messaging/component.cairo file.

Two primary issues can arise from the snos_output and its constituent message content, potentially leading to a Denial of Service (DoS) by causing the update_state(...) transaction to revert on Starknet:

1. snos_output Calldata Size Exceeding Starknet Limits: The entire snos_output is passed as calldata to the update_state(...) function. Starknet imposes a transaction calldata limit (currently 5000 felts, as per Starknet documentation). Specific sequencer implementations like Madara might also enforce stricter limits on parts of this calldata (e.g., Madara's default configuration for message segments). If the total size of snos_output exceeds these calldata limits, the update_state(...) transaction will fail;
2. Message Processing Leading to Excessive Event Emissions: The snos_output can trigger the processing of messages in both directions (to Starknet and to the Appchain). Both respective processing functions emit events, contributing to a cumulative event count for the update_state(...) transaction;

The process_messages_to_starknet(...) function handles messages directed from the Appchain to Starknet:

// @audit Processes messages from snos_output to Starknet, emitting events.
fn process_messages_to_starknet(
    ref self: ComponentState<TContractState>, messages: Span<MessageToStarknet>,
) {
    let mut messages = messages;

    loop {
        match messages.pop_front() {
            Option::Some(m) => {
                let from = *m.from_address;
                let to = *m.to_address;
                let payload = *m.payload;

                let message_hash = hash::compute_message_hash_appc_to_sn(from, to, payload);

                // @audit-issue Event emission for messages to Starknet.
                // @audit This contributes to the per-transaction event limit.
                self.emit(MessageToStarknetReceived { message_hash, from, to, payload });
                // ...
            Option::None => { break; },
        };
    };
}

This function emits a MessageToStarknetReceived event for each message processed.

Similarly, messages intended for the Appchain (originating from Starknet and included in snos_output for state updates) are processed by process_messages_to_appchain(...), which also emits events:

// @audit Processes messages from snos_output to the Appchain, also emitting events.
fn process_messages_to_appchain(
    ref self: ComponentState<TContractState>, messages: Span<MessageToAppchain>,
) {
    let mut messages = messages;

    loop {
        match messages.pop_front() {
            Option::Some(m) => {
                let from = *m.from_address;
                let to = *m.to_address;
                let payload = *m.payload;
                let selector = *m.selector;
                let nonce = *m.nonce;

                let message_hash = hash::compute_message_hash_sn_to_appc(
                    from, to, selector, payload, nonce,
                );
                // ...
                // @audit-issue Event emission for messages to the Appchain.
                // @audit This also contributes to the total events per transaction.
                self.emit(MessageToAppchainSealed {
                    message_hash, from, to, selector, payload, nonce
                });
            },
            Option::None => { break; },
        };
    };
}

This function emits a MessageToAppchainSealed event for each such message processed.

Starknet imposes a global limit of 1000 events that a single transaction can emit (as per Starknet documentation). If the combined number of messages (to Starknet and to Appchain) processed within a single update_state(...) call leads to the total event count from both functions exceeding this Starknet-defined limit, the entire update_state(...) transaction will revert.

While the loops within these functions do not have explicit iteration caps, the effective limitations are imposed externally by Starknet's transaction-level constraints. If an Appchain generates an snos_output that is either too large (calldata) or triggers the processing of too many messages cumulatively (event count), the update_state(...) call will fail, preventing state finalization.

The Piltover framework currently assumes upstream components (e.g., Appchain's sequencer/blockifier) will constrain snos_output. However, without intrinsic safeguards, misconfigurations or malicious snos_output could trigger these DoS conditions.

It might lead to a situation, where the Appchain becomes unable to finalize its blocks and prove its state on Starknet. This occurs because the update_state(...) transaction consistently reverts due to exceeding Starknet's calldata size limits or cumulative event emission limits from processing messages in both directions.

Recommendation

Consider revisiting the message processing mechanisms and implementing limits on the maximum number of messages that can be processed during a single state update. A possible mitigation might include changes at different levels of the tech stack, such as at the sequencer or blockifier level, or directly in the contract by splitting the message processing into batches.

Resolution

As mentioned in the issue comment, the appropriate place to handle these limitations is at the sequencer level. Since Blockifier serves as the common execution layer for Starknet transactions, block limits must be configured to ensure that state updates are correctly processed by Piltover (which may differ from Starknet current limits).

Therefore, no changes will be made at the Cairo level to address this in the current state of Piltover. It is the responsibility of appchain sequencers to enforce constraints that prevent blocks from exceeding the limits that Piltover can handle for state updates.

6.3 Missing validation of constituent program hash in layout_bridge_output

The update_state(...) function processes state updates by verifying proofs of Starknet OS (SNOS) executions. This involves a bootloader_program running a layout_bridge_program. The output of this, layout_bridge_output, is structured for verification by an external fact registry (Herodotus).

The layout_bridge_output span is understood to have a specific structure where various elements represent Poseidon hashes of programs or data. The element at index 2 is confirmed to be the layout_bridge_program_hash and the element at index 4 is the snos_output_hash; both are correctly validated. However, the element at index 3 of layout_bridge_output, which is understood to represent a program hash integral to the generation of this layout_bridge_output (potentially the bootloader program that ran the layout bridge, or another relevant program), is not validated against any expected value from the contract's configuration.

Given that the element at index 3 of layout_bridge_output represents a program hash crucial to the output's generation, the lack of its validation against an expected value is a concern. While the fact hash submitted to the Herodotus registry is calculated using the program_info.bootloader_program_hash from contract storage, a critical validation is missing: ensuring that the specific program hash present at layout_bridge_output.at(3) (which played a role in creating layout_bridge_output) matches an expected, legitimate program hash stored in the configuration. Without this check, if layout_bridge_output was generated using an unexpected or malicious intermediary program (whose hash would be at index 3), the system might be misled. This could compromise the integrity of the state transition mechanism by allowing outputs from unintended program flows to be processed, even if other parts of the proof verification appear consistent.

fn update_state(
    ref self: ContractState,
    snos_output: Span<felt252>,
    layout_bridge_output: Span<felt252>, // ...
) {
    // ...
    let program_info = self.config.program_info.read();
    // @audit The layout bridge program hash is correctly checked at index 2.
    let layout_bridge_program_hash = layout_bridge_output.at(2);
    assert(
        program_info.layout_bridge_program_hash == *layout_bridge_program_hash,
        errors::LAYOUT_BRIDGE_INVALID_PROGRAM_HASH,
    );
    let snos_output_hash = poseidon_hash_span(snos_output);
    // @audit The snos output hash is correctly checked at index 4.
    let snos_output_hash_in_bridge_output = layout_bridge_output.at(4);
    assert(
        snos_output_hash == *snos_output_hash_in_bridge_output, errors::SNOS_INVALID_OUTPUT_HASH,
    );
    let output_hash = poseidon_hash_span(layout_bridge_output);
    // ...
    // @audit The fact is calculated using the bootloader program hash from storage.
    // @audit-issue However, there is no check on the program hash at `layout_bridge_output.at(3)`.
    let fact = poseidon_hash_span(
        array![program_info.bootloader_program_hash, output_hash].span(),
    );
    let verifications = IFactRegistryDispatcher {
        contract_address: self.config.get_facts_registry(),
    }
        .get_all_verifications_for_fact_hash(fact);
    if verifications.len() == 0 {
        core::panic_with_felt252(errors::NO_FACT_REGISTERED)
    }; // ...
}

Recommendation

It is understood that the element at index 3 of the layout_bridge_output span represents a program hash integral to the generation of this output. This program hash should be explicitly validated. Consider adding an assertion to the update_state(...) function to check the hash at layout_bridge_output.at(3) against a corresponding expected program hash value stored in the contract's configuration (e.g., within program_info or a dedicated configuration field if it's a different program hash than the main bootloader). This ensures that the layout_bridge_output is consistent with the expected program execution pathway and prevents processing outputs derived from unverified or unexpected programs.

Resolution

This has been addressed in ce5e895b.

In the current implementation, the program hash that should match this value is the bootloader program hash. Since the layout bridge is a verifier written in Cairo, the verification happens on a bootloaded Starknet OS program.

The bootloader program hash is currently not configurable by the operator, and it is defined by SHARP that is bootloading the jobs.

This opens an issue if such bootloader program is changed (which is considered very unlikely at the moment) without prior notice, which could block Piltover. However this can be solved by updating the program info.

6.4 State can be updated with KZG data availability without verification

The update_state(...) function is responsible for processing the Starknet OS (SNOS) output to verify and apply state transitions. The SNOS output format supports two modes for data availability: on-chain data and KZG (Kate-Zaverucha-Goldberg) data availability. If KZG DA is used, indicated by the use_kzg_da flag, the state diff is not included directly in the output. Instead, commitments to the state diff are provided, which must be verified against KZG proofs for the data blobs.

The deserialize_os_output(...) function correctly identifies when KZG DA is being used by checking the use_kzg_da flag. However, it only consumes the KZG-related data from the input stream without performing any verification.

pub fn deserialize_os_output(ref input_iter: SpanIter<felt252>) -> StarknetOsOutput {
    // ...
    let header = read_segment(ref input_iter, HEADER_SIZE);
    let use_kzg_da = header[USE_KZG_DA_OFFSET];
    // ...
    if use_kzg_da.is_non_zero() {
        let kzg_segment = read_segment(ref input_iter, 2);
        let n_blobs: usize = (*kzg_segment.at(KZG_N_BLOBS_OFFSET))
            .try_into()
            .expect('Invalid n_blobs');
        // @audit-issue If `use_kzg_da` is non-zero, the function reads the segment but no verification of the corresponding KZG proofs is performed.
        let _ = read_segment(ref input_iter, 2 * 2 * n_blobs);
    }
    // ...
}

The current implementation of the update_state(...) function in Piltover does not support handling the KZG DA case. If a SNOS output is submitted with the use_kzg_da flag set to 1, the function will proceed without verifying the corresponding state_diff, as the logic to handle KZG proofs is absent. This means that a key component of the state transition verification is missing when this data availability scheme is used.

Recommendation

Consider implementing a robust mechanism to handle the KZG data availability case. A recommended approach, similar to Starknet's official L1 contract, is to introduce a separate entry point, such as update_state_kzg_da(...), which accepts the necessary KZG proofs as arguments. This function should perform the required cryptographic verification of the proofs against the commitments present in the SNOS output.

Alternatively, if supporting KZG DA is not an immediate priority, consider adding a check to the deserialize_os_output(...) function to revert the transaction if use_kzg_da is non-zero, thereby explicitly disallowing state updates via KZG DA until the verification logic is fully implemented.

Resolution

Since the KZG DA processing requires a totally different handle of the output and KZG proofs, the support has been explicitely removed in a6e5511c where piltover will reject any Starknet OS (SNOS) output with the use_kzg_da flag set to true.

Since no work has been done yet to correctly implement a new entrypoint update_state_kzg_da to accept KZG proofs, the support will be added in a future update of piltover.

6.5 Subsequent calls to start_message_cancellation(...) will overwrite the previous cancellation time

Users of the Piltover system can request to cancel messages sent from Starknet to an Appchain while they are in a pending state. This is a two-step process. First, the user calls the start_message_cancellation(...) function, which records that a cancellation has been initiated for a specific message. This makes the message eligible for final cancellation after a predetermined delay (e.g., 5 days), storing the timestamp of this initiation. The second step involves calling cancel_message(...) after this delay has passed, which, if the message has not been sealed by then, marks it as cancelled.

However, the start_message_cancellation(...) function does not check if a cancellation process has already been started for the given message. If start_message_cancellation(...) is called again for the same message hash, it will overwrite the existing timestamp in sn_to_appc_cancellations with the new current block timestamp.

This means that each subsequent call to start_message_cancellation(...) for the same message effectively resets the waiting period required before cancel_message(...) can be successfully executed. While it is unlikely that a user would intentionally do this, an accidental or misinformed repeated call could prolong the time until the message can be cancelled. This presents a minor operational inconvenience, as it could delay the user's ability to finalize the cancellation.

fn start_message_cancellation(...) -> MessageHash {
    // ...
    // @audit-issue Subsequent calls will overwrite the previous cancellation start time.
    self.sn_to_appc_cancellations.write(message_hash, starknet::get_block_timestamp());
    // ...
    return message_hash;
}

Recommendation

Consider revisiting the logic of start_message_cancellation(...) to prevent the cancellation waiting period from being unintentionally reset by multiple calls for the same message.

Resolution

Addressed in 4ca0a3d0 where a new status Cancelling has been added. This new status is useful to clarify the exact status of the message, which also ensures that the start_message_cancellation can't be called twice on the same message.

6.6 The DataAvailabilityFact elements are not validated in the update_state(...) function

The update_state(...) function in the appchain contract is called by whitelisted operators to process state transitions for the Appchain. It takes the StarknetOS program output ( snos_output) as an input, which includes the complete state diff, meaning the primary data for state recreation is available in calldata on Starknet. The function also accepts onchain_data_hash and onchain_data_size as input parameters. These two parameters are used to construct a DataAvailabilityFact structure. This DataAvailabilityFact, in conjunction with the layout_bridge_output (another input to the function), is used to create the state_transition_fact. This state_transition_fact is then emitted in an event for every state update.

The problem in the update_state(...) function is that while other inputs like snos_output and layout_bridge_output undergo several validation checks, the onchain_data_hash parameter, which is a crucial component of the DataAvailabilityFact, is not validated. An operator can provide any arbitrary felt252 value for onchain_data_hash when calling this function.

This allows a whitelisted operator to supply a potentially misleading onchain_data_hash. Consequently, an incorrect state_transition_fact would be generated using this unverified hash and emitted as an event. Although the ability to call update_state(...) is restricted to a whitelisted group of operators, this lack of validation can lead to the emission of a state_transition_fact that inaccurately represents data availability details. This compromises the integrity of the information emitted in the state_transition_fact.

fn update_state(
    ref self: ContractState,
    snos_output: Span<felt252>,
    layout_bridge_output: Span<felt252>,
    // @audit The onchain_data_hash parameter is an input from the operator.
    onchain_data_hash: felt252,
    onchain_data_size: u256,
) {
    // ... 
    // @audit-issue The onchain_data_hash is used here directly as received from the input,
    // without any prior validation within this function.
    let data_availability_fact: DataAvailabilityFact = DataAvailabilityFact {
        onchain_data_hash, onchain_data_size,
    };

    // @audit The state_transition_fact is constructed using the potentially unvalidated
    // onchain_data_hash via the data_availability_fact.
    let state_transition_fact: u256 = encode_fact_with_onchain_data(
        layout_bridge_output, data_availability_fact,
    );
    // ... 
    // @audit The potentially incorrect state_transition_fact is emitted.
    self.emit(LogStateTransitionFact { state_transition_fact });
    // ...
}

Recommendation

Consider implementing validation for the onchain_data_hash parameter to ensure it accurately reflects the intended on-chain data source or commitment. Alternatively, if this hash is not deemed essential or its direct validation is complex within this context, evaluate its necessity in the DataAvailabilityFact and the emitted state_transition_fact.

Resolution

Since the usage of such values is not well defined in the Piltover context, to avoid any operator manipulation but not derive too much from the Starknet implementation in solidity, a proposal in ec5b1bcd enforces those values to be 0.

6.7 Missing input validation of the StarknetOS output fields

The update(...) function within the state component is invoked when a valid state update has been submitted by an operator through the update_state(...) function. Its role is to refresh the component's storage with the latest verified Appchain state information, including the block number, block hash, and state root. This stored information is subsequently queried by operators to determine the next state update required.

The current implementation of update(...) correctly asserts that the prev_block_number from the program_output aligns with the stored block_number, and that the initial_root matches the stored state_root. However, when compared to the Solidity implementation in the Starknet.sol L1 core contract, two input validation checks are absent before the state is updated:

1. A check to ensure that the new_block_number from the program_output is strictly greater than the currently stored block number;
2. A check to ensure that the prev_block_hash from the program_output is identical to the currently stored block hash;

fn update(ref self: ComponentState<TContractState>, program_output: StarknetOsOutput) {
    assert(
        self.block_number.read() == program_output.prev_block_number,
        errors::INVALID_BLOCK_NUMBER,
    );
    // @audit-issue Two checks are missing as compared to Starknet.sol implementation:
    // - new block number MUST be greater than previous block number
    // - the previous block hash from program output MUST be the same as the last stored
    //   hash
    self.block_number.write(program_output.new_block_number);
    self.block_hash.write(program_output.new_block_hash);
    
    assert(
        self.state_root.read() == program_output.initial_root, errors::INVALID_PREVIOUS_ROOT,
    );
    self.state_root.write(program_output.final_root);
}

The program_output that is passed to the update_state(...) function also includes the full_output and os_program_hash fields which are not validated anywhere in the code.

1. The full_output flag can be set to true or false by the orchestrator. If this flag is not consistently handled or validated, it might complicate the process for off-chain components attempting to accurately recreate the Appchain's state;
2. The os_program_hash field is expected to be zero if the StarknetOS (SNOS) is run directly, which is the anticipated scenario for the Appchain contract. If an aggregator were used, this field would contain the hash of the SNOS program;

While not strictly necessary, the missing checks could be added for consistency reasons with the reference implementation and to decrease the attack surface area.

Recommendation

Consider adding two additional validation steps to the update(...) function, similar to those found in the Starknet.sol reference implementation.

Additionally, consider introducing validation for the full_output flag and the os_program_hash field from the program_output.

Resolution

This issue has been addresses in 6121c750 where:

1. The previous block hash in the Starknet OS (SNOS) output needs to match the current piltover state;
2. The new block number must be greater than the current piltover state. One exception is for the genesis block, where the special value of felt252::MAX can be followed by block number 0 (or more in future SNOS implementations);

No assumptions are made for the block number to be prev_block_number + 1. It is currently how SNOS works (processing only one block in an execution), but this will change in the future and the proposed changes will still be compatible. 3. Complementary change has been done in a48e208d to ensure the full_output is not supported when set to true.

The os_program_hash is for now expected to be 0, which is now enforced in 2007539c. However, this will change in the future once applicative recursion will become available, and the changes will be done accordingly.

6.8 Unused code

The ContractChanges struct from snos_output.cairo file remains unused in the codebase. Since state diffs are currently not utilized, this struct can be safely removed from the codebase to keep the code clean and readable.

Recommendation

Consider removing the unused ContractChanges struct.

Resolution

Addressed in fd2c9554.