bug(consensus): non-canonical keys in ValidatorRegistry trigger unhandled node halt during block transition

Summary

A structural discrepancy between ValidatorRegistry.sol validation and abi_decode_validator_set allows non-canonical Ed25519 public keys to enter active registry state and subsequently halt the consensus node during block transition. Additionally, aggregate active voting power is not validated against u64 overflow in the registry or during ValidatorSet construction, deferring overflow checks to an unhandled panic in total_voting_power.

Technical Details

In contracts/src/validator-manager/ValidatorRegistry.sol#L98-L99, validator registration only asserts raw byte length:

require(publicKey.length == ED25519_PUBLIC_KEY_LENGTH, InvalidPublicKeyFormat());

The contract does not verify that the 32 bytes represent a valid decompressed Curve25519 point. When keccak256(publicKey) is unique, arbitrary 32-byte sequences are accepted and activated. In Curve25519 point decoding, approximately half of candidate 32-byte representations are invalid or non-canonical field elements, as evidenced by MALFORMED_PK in crates/eth-engine/src/abi_utils.rs#L249.

The registry contract relies on _countActiveValidatorsWithPositiveVotingPower in removeValidator (#L147-L149) and updateValidatorVotingPower (#L182-L184) to guard against reducing the active validator set below 1. Because the registry treats a malformed entry as fully active, other valid active validators can be removed or decremented to zero voting power while the malformed entry remains.

When the consensus layer fetches the active set via get_signing_validator_set, abi_decode_validator_set in crates/eth-engine/src/abi_utils.rs#L85-L96 invokes try_decode_validator. Any entry failing PublicKey::from_bytes is logged and skipped. If the remaining active registrations are all malformed, lines 98-103 bail:

if validators.is_empty() {
    eyre::bail!(
        "No active validators with valid public keys — validator set would be empty \
         ({skipped} active validator(s) skipped due to malformed public keys)"
    );
}

In crates/malachite-app/src/handlers/decided.rs#L102-L105, prepare_next_height propagates this error directly via ?:

let next_height_info =
    prepare_next_height(decided_height, block, new_sync_state, engine).await?;

Because decided::handle terminates before setting state.decision, the error bubbles up through handle_consensus in crates/malachite-app/src/app.rs#L297 and terminates the main consensus select loop at app.rs#L177-L178, aborting the node process entirely.

A related arithmetic boundary condition exists with aggregate voting power. ValidatorRegistry.sol does not track the aggregate voting power of active validators. Neither abi_decode_validator_set nor ValidatorSet::new validates cumulative voting power during construction. When cumulative voting power exceeds u64::MAX, the overflow check in crates/types/src/validator_set.rs#L105-L110 triggers .expect("total voting power overflow"), panicking within active consensus execution.

Remediation

Remediation requires validating cumulative voting power during decoding in abi_decode_validator_set and inside ValidatorSet::new to reject overflowing sets cleanly prior to consensus runtime. Additionally, ValidatorRegistry.sol should track active aggregate voting power against type(uint64).max, while crates/malachite-app/src/handlers/decided.rs should prevent decoder errors in prepare_next_height from abruptly crashing the main application loop.

A few details that may help make the impact and regression test precise:

All four registry mutations involved here (registerValidator, activateValidator, removeValidator, and updateValidatorVotingPower) are onlyOwner. The trigger therefore requires the registry owner/governance path to accept and activate the malformed key, then reduce/remove the valid entries. Please state whether owner compromise or an erroneous authorized update is in scope; it materially affects reachability and severity.
A minimal key regression case is: activate one valid key and one 32-byte key rejected by the node's PublicKey::from_bytes; then zero or remove the valid key. The registry's positive-power count can still pass because it counts the malformed entry. Assert the resulting active set fails decoding as described. Keep this fail-closed: catching the error and silently continuing with a stale set on only some nodes could create inconsistent validator views.
For the arithmetic case, two active powers of u64::MAX and 1 are individually valid uint64 values but overflow the aggregate. ValidatorSet::new currently sorts and checks non-emptiness; it does not validate the total. A regression should exercise total_voting_power() through the consensus path and confirm the malformed set is rejected as a structured error before the panic.
It would also strengthen impact evidence to record the observed lifecycle (process exit vs. consensus/app halt) from an integration test, rather than infer the exact operational outcome from the propagated error alone.

On mutation reachability, ValidatorRegistry is owned by PermissionedValidatorManager, which delegates registerValidator to ValidatorRegisterer accounts and activateValidator, removeValidator, and updateValidatorVotingPower to individual Controller accounts. This does not require root contract owner key compromise. An authorized operator provisioned with a registerer role providing a 32-byte non-canonical Ed25519 representation (such as a non-square field element during point decompression, which ValidatorManagement.s.sol explicitly notes cannot be verified on-chain in EVM), followed by routine operational rotation via a controller removing or zeroing a valid validator, satisfies _countActiveValidatorsWithPositiveVotingPower > 1 on-chain because the contract treats the malformed registration as active power.

The minimal regression flow matches that exact sequence: activate one valid key and one 32-byte payload failing PublicKey::from_bytes, then zero the valid validator's voting power. On-chain validation passes since the active count remains 1, leaving only the non-canonical key in getActiveValidatorSet(). On block transition, abi_decode_validator_set skips the malformed key, leaving the filtered set empty and bailing with "No active validators with valid public keys". Fail-closed semantics here are necessary: falling back to a stale validator set across nodes would split the consensus state machine and violate BFT safety invariants if nodes observe divergent registry states across block transitions.

For the arithmetic case with individual powers of u64::MAX and 1, constructing ValidatorSet via ValidatorSet::try_new catches aggregate overflow during construction and returns a structured ValidatorSetError::VotingPowerOverflow rather than triggering a thread panic inside total_voting_power() during the active consensus round. Threading this through abi_decode_validator_set surfaces the failure directly as an eyre::Result at the block transition boundary, matching the fail-closed error handling of empty validator sets.

On the runtime lifecycle, the propagated error bubbles from prepare_next_height through decided::handle into handle_consensus in crates/malachite-app/src/app.rs. In go(), handle_consensus(...).await.wrap_err(...) returns Err, exiting the consensus event loop. In App::start(), the app_req_task is aborted, a redb savepoint is persisted, and the error is returned to handles.app.await in App::run(), which returns directly to rt.block_on(app.run()) in main.rs and terminates the node process with a non-zero exit status.

Hi, I'd like to take this one per CONTRIBUTING.md.

I've traced the full crash path described here and agree with the analysis:

Malformed-key halt: abi_decode_validator_set (crates/eth-engine/src/abi_utils.rs) silently skips entries failing PublicKey::from_bytes, then bails when the set is empty; prepare_next_height (crates/malachite-app/src/handlers/decided.rs) propagates via ? into handle_consensus (crates/malachite-app/src/app.rs), killing the main select loop.
u64 overflow: ValidatorSet::new (crates/types/src/validator_set.rs) uses .expect("total voting power overflow") — a panic inside consensus runtime.
Root cause: ValidatorRegistry.sol only checks publicKey.length == 32 and never validates Curve25519 decompressibility, and tracks no aggregate voting power, so malformed entries can satisfy the "active set >= 1" guards while being undecodable by the CL.

Proposed remediation (split into reviewable commits):

CL hardening (highest priority, no consensus-state change): in abi_decode_validator_set, validate cumulative voting power against u64::MAX before construction and fail cleanly; in decided::handle, catch prepare_next_height decoder errors and transition to a recoverable error state instead of aborting the process loop.
Registry defense-in-depth: track active aggregate voting power in ValidatorRegistry.sol capped at type(uint64).max, and reject non-decompressible Ed25519 points at registration time (via a validation helper or precompile).
Regression tests: malformed-key fixture that previously halted the node; u64-overflow voting-power fixture.

Could you please assign this issue to me? Happy to start with the CL-hardening portion if you'd like it split across PRs.

Navigation Menu

Repository navigation

bug(consensus): non-canonical keys in ValidatorRegistry trigger unhandled node halt during block transition #489

Summary

Technical Details

Remediation

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

bug(consensus): non-canonical keys in ValidatorRegistry trigger unhandled node halt during block transition #489

Description

Summary

Technical Details

Remediation

Activity

osr21 commented on Oct 10, 2026

Yudis-bit commented on Oct 11, 2026

feafel commented on Oct 11, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions