fix(validator-config-v2): ed25519 decode check on migration, digest preimage collision fix#2858
fix(validator-config-v2): ed25519 decode check on migration, digest preimage collision fix#2858
Conversation
howydev
requested review from
0xKitsune,
0xrusowsky,
SuperFluffy,
Zygimantass,
dankrad,
danrobinson,
fgimenez,
klkvr,
legion2002 and
mattsse
as code owners
howydev
force-pushed
the
howy/sigp-audit-fixes
branch
from
2ba7db6 to
4c8566a
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 20796/21859 lines (95.14%) File details
contractsCoverage: 209/383 lines (54.57%) File details
Total: 21005/22242 lines (94.44%) |
🐺 Ralph Security Review
Findings
❌ Consolidation failed · Thread 📜 24 events🔍 |
Amp-Thread-ID: https://ampcode.com/threads/T-019c963d-1942-7219-a89a-b0e727c3dadb Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c963d-1942-7219-a89a-b0e727c3dadb Co-authored-by: Amp <amp@ampcode.com>
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
🐺 Ralph Review
The signature digest hardening (adding uint8(ingress.len) / uint8(egress.len) length prefixes) is well-implemented and consistent across Rust, Solidity, and spec. However, the strict migration validation introduces upgrade-liveness risks, and a pre-existing ZELLIC-24 regression remains in the V2 precompile.
🚨 [SECURITY] ZELLIC-24 Regression: Validators Can Block Deactivation via Address Front-Running
File: crates/precompiles/src/validator_config_v2/mod.rs:411 (not in this diff, but in the modified file)
V2's deactivateValidator and rotateValidator use validatorAddress as the lookup key. Since validators can change their address via transferValidatorOwnership, a malicious validator can front-run owner deactivation by transferring to a new address, causing the owner's tx to revert with ValidatorNotFound. This is a regression of ZELLIC-24, which V1 fixed with index-based targeting (change_validator_status_by_index).
Recommended Fix: Reintroduce index-based targeting for owner administrative operations.
File: crates/precompiles/src/validator_config_v2/mod.rs:~592 (not in this diff)
Same root cause class as the inline finding below: V1 allows duplicate ingress IPs (same IP, different port) but migrate_validator calls require_unique_ingress_ip which strips the port and rejects duplicates. A malicious V1 validator can set their inboundAddress to share another validator's IP, permanently blocking migration.
Recommended Fix: During migration, quarantine validators with duplicate IPs in a deactivated state rather than reverting.
| self.require_new_address(v1_val.validatorAddress)?; | ||
| self.require_new_pubkey(v1_val.publicKey)?; | ||
|
|
||
| PublicKey::decode(v1_val.publicKey.as_slice()) |
There was a problem hiding this comment.
🚨 [SECURITY] V2 Migration Permanently Bricked by Invalid V1 Public Key
This PublicKey::decode check will revert if any V1 validator has an invalid Ed25519 public key. Since V1 never validated keys (only checked non-zero), invalid keys can exist in V1 state. Because migrate_validator enforces strict sequential indexing (call.idx == current_count) and initialize_if_migrated requires all V1 validators to be migrated, a single invalid key permanently blocks the entire V2 upgrade. The owner cannot fix the key in V1 (only the validator's ECDSA address can call update_validator) and cannot skip the index.
The test test_migration_rejects_invalid_ed25519_pubkey verifies the revert but does not cover the catastrophic downstream effect: migration is permanently stuck and V2 can never initialize.
Recommended Fix: Instead of reverting, gracefully handle invalid keys during migration — zero out the public key and force the validator into a deactivated state, allowing the migration index to advance.
The test needs a non-zero key (so V1 accepts it) that is also not a valid Ed25519 curve point (so V2 rejects it during migration). The previous simplification to [0u8;32] was rejected by V1's zero-check, and [0xFF;32] is actually a valid Ed25519 point. Restore dynamic search starting from 1. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c96b6-f605-736c-859a-4fae7636a1e2
howydev
force-pushed
the
howy/sigp-audit-fixes
branch
from
197edd4 to
e1507e0
Compare
Amp-Thread-ID: https://ampcode.com/threads/T-019c9723-aad6-7169-8dcb-627b96589df3 Co-authored-by: Amp <amp@ampcode.com>
5 participants
Summary
Two fixes to ValidatorConfigV2 (TIP-1017):
1. Ed25519 decode check during migration
migrate_validatornow validates that V1 public keys are decodable Ed25519 curve points, reverting withInvalidPublicKeyif not. V1 did not enforce this, so invalid keys could have been stored.2. Digest preimage collision fix
The signature digest for
addValidatorandrotateValidatorusedabi.encodePacked(..., ingress, egress)which is ambiguous — e.g.("192.168.0.1:800", "12.3.4.5")and("192.168.0.1:8001", "2.3.4.5")produce the same byte sequence. Fixed by adding auint8length prefix before ingress to disambiguate the boundary.Files changed
tips/tip-1017.mdValidatorConfigV2.sol,IValidatorConfigV2.solValidatorConfigV2.t.sol,invariants/ValidatorConfigV2.t.solcrates/precompiles/src/validator_config_v2/mod.rs,dispatch.rsxtask/src/genesis_args.rs