diff --git a/crates/precompiles/src/validator_config_v2/dispatch.rs b/crates/precompiles/src/validator_config_v2/dispatch.rs index d514a18073..b0b0754292 100644 --- a/crates/precompiles/src/validator_config_v2/dispatch.rs +++ b/crates/precompiles/src/validator_config_v2/dispatch.rs @@ -196,8 +196,12 @@ mod tests { tempo_contracts::precompiles::VALIDATOR_CONFIG_V2_ADDRESS.as_slice(), ); msg_data.extend_from_slice(validator_addr.as_slice()); - msg_data.extend_from_slice(b"192.168.1.1:8000"); - msg_data.extend_from_slice(b"192.168.1.1"); + let ingress = b"192.168.1.1:8000"; + msg_data.push(ingress.len() as u8); + msg_data.extend_from_slice(ingress); + let egress = b"192.168.1.1"; + msg_data.push(egress.len() as u8); + msg_data.extend_from_slice(egress); let message = alloy::primitives::keccak256(&msg_data); // Sign with namespace diff --git a/crates/precompiles/src/validator_config_v2/mod.rs b/crates/precompiles/src/validator_config_v2/mod.rs index fcfd66c656..91919718e3 100644 --- a/crates/precompiles/src/validator_config_v2/mod.rs +++ b/crates/precompiles/src/validator_config_v2/mod.rs @@ -339,7 +339,7 @@ impl ValidatorConfigV2 { /// /// **FORMAT**: /// - Namespace: [`VALIDATOR_NS_ADD`] or [`VALIDATOR_NS_ROTATE`] - /// - Message: `keccak256(abi.encodePacked(chainId, contractAddr, validatorAddr, ingress, egress))` + /// - Message: `keccak256(chainId || contractAddr || validatorAddr || uint8(ingress.len) || ingress || uint8(egress.len) || egress))` fn verify_validator_signature( &self, namespace: &[u8], @@ -353,7 +353,9 @@ impl ValidatorConfigV2 { hasher.update(self.storage.chain_id().to_be_bytes()); hasher.update(VALIDATOR_CONFIG_V2_ADDRESS.as_slice()); hasher.update(validator_address.as_slice()); + hasher.update([ingress.len() as u8]); hasher.update(ingress.as_bytes()); + hasher.update([egress.len() as u8]); hasher.update(egress.as_bytes()); let message = hasher.finalize(); @@ -579,6 +581,14 @@ impl ValidatorConfigV2 { self.require_new_address(v1_val.validatorAddress)?; self.require_new_pubkey(v1_val.publicKey)?; + let deactivated_at_height = if v1_val.active { + PublicKey::decode(v1_val.publicKey.as_slice()) + .map_err(|_| ValidatorConfigV2Error::invalid_public_key())?; + 0 + } else { + block_height + }; + let egress = v1_val .outboundAddress .parse::() @@ -587,8 +597,6 @@ impl ValidatorConfigV2 { let ingress_hash = self.require_unique_ingress_ip(&v1_val.inboundAddress)?; - let deactivated_at_height = if v1_val.active { 0 } else { block_height }; - self.append_validator( v1_val.validatorAddress, v1_val.publicKey, @@ -657,7 +665,9 @@ mod tests { data.extend_from_slice(&1u64.to_be_bytes()); data.extend_from_slice(VALIDATOR_CONFIG_V2_ADDRESS.as_slice()); data.extend_from_slice(validator_address.as_slice()); + data.push(ingress.len() as u8); data.extend_from_slice(ingress.as_bytes()); + data.push(egress.len() as u8); data.extend_from_slice(egress.as_bytes()); let message = keccak256(&data); @@ -691,6 +701,11 @@ mod tests { } } + fn valid_pubkey(seed: u64) -> FixedBytes<32> { + let pk = PrivateKey::from_seed(seed); + FixedBytes::from_slice(&pk.public_key().encode()) + } + /// Helper to make a complete add call with generated keys fn make_valid_add_call( addr: Address, @@ -1353,7 +1368,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: v1_addr, - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1364,7 +1379,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: v2_addr, - publicKey: FixedBytes::<32>::from([0x22; 32]), + publicKey: valid_pubkey(2), active: false, inboundAddress: "192.168.1.2:8000".to_string(), outboundAddress: "192.168.1.2:9000".to_string(), @@ -1381,7 +1396,7 @@ mod tests { assert_eq!(v2.validator_count()?, 1); let migrated = v2.validator_by_index(0)?; assert_eq!(migrated.validatorAddress, v1_addr); - assert_eq!(migrated.publicKey, FixedBytes::<32>::from([0x11; 32])); + assert_eq!(migrated.publicKey, valid_pubkey(1)); assert_eq!(migrated.deactivatedAtHeight, 0); // Migrate second validator @@ -1427,7 +1442,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: v1_addr, - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1480,7 +1495,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: Address::random(), - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1491,7 +1506,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: Address::random(), - publicKey: FixedBytes::<32>::from([0x22; 32]), + publicKey: valid_pubkey(2), active: true, inboundAddress: "192.168.1.2:8000".to_string(), outboundAddress: "192.168.1.2:9000".to_string(), @@ -1527,7 +1542,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: Address::random(), - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1538,7 +1553,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: Address::random(), - publicKey: FixedBytes::<32>::from([0x22; 32]), + publicKey: valid_pubkey(2), active: true, inboundAddress: "192.168.1.2:8000".to_string(), outboundAddress: "192.168.1.2:9000".to_string(), @@ -1563,6 +1578,41 @@ mod tests { }) } + #[test] + fn test_migration_rejects_invalid_ed25519_pubkey() -> eyre::Result<()> { + let mut storage = HashMapStorageProvider::new(1); + let owner = Address::random(); + + StorageCtx::enter(&mut storage, || { + let mut v1 = v1(); + v1.initialize(owner)?; + + // [0x02; 32] is not a valid Ed25519 curve point but is non-zero so V1 accepts it. + let bad_key = [0x02u8; 32]; + v1.add_validator( + owner, + tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { + newValidatorAddress: Address::random(), + publicKey: FixedBytes::<32>::from(bad_key), + active: true, + inboundAddress: "192.168.1.1:8000".to_string(), + outboundAddress: "192.168.1.1:9000".to_string(), + }, + )?; + + let mut v2 = ValidatorConfigV2::new(); + v2.storage.set_block_number(100); + let result = + v2.migrate_validator(owner, IValidatorConfigV2::migrateValidatorCall { idx: 0 }); + assert_eq!( + result, + Err(ValidatorConfigV2Error::invalid_public_key().into()) + ); + + Ok(()) + }) + } + #[test] fn test_add_validator_reuses_deactivated_address() -> eyre::Result<()> { let mut storage = HashMapStorageProvider::new(1); @@ -1724,7 +1774,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: v1_addr, - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1768,7 +1818,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: v1_addr, - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1853,7 +1903,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: Address::random(), - publicKey: FixedBytes::<32>::from([0x11; 32]), + publicKey: valid_pubkey(1), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.1.1:9000".to_string(), @@ -1863,7 +1913,7 @@ mod tests { owner, tempo_contracts::precompiles::IValidatorConfig::addValidatorCall { newValidatorAddress: Address::random(), - publicKey: FixedBytes::<32>::from([0x22; 32]), + publicKey: valid_pubkey(2), active: true, inboundAddress: "192.168.1.1:8000".to_string(), outboundAddress: "192.168.2.1:9000".to_string(), diff --git a/tips/ref-impls/src/ValidatorConfigV2.sol b/tips/ref-impls/src/ValidatorConfigV2.sol index 96c238755f..6370348ded 100644 --- a/tips/ref-impls/src/ValidatorConfigV2.sol +++ b/tips/ref-impls/src/ValidatorConfigV2.sol @@ -84,7 +84,15 @@ contract ValidatorConfigV2 is IValidatorConfigV2 { _validateAddParams(validatorAddress, publicKey, ingress, egress); bytes32 message = keccak256( - abi.encodePacked(block.chainid, address(this), validatorAddress, ingress, egress) + abi.encodePacked( + block.chainid, + address(this), + validatorAddress, + uint8(bytes(ingress).length), + ingress, + uint8(bytes(egress).length), + egress + ) ); _verifyEd25519Signature( bytes("TEMPO_VALIDATOR_CONFIG_V2_ADD_VALIDATOR"), publicKey, message, signature @@ -153,7 +161,15 @@ contract ValidatorConfigV2 is IValidatorConfigV2 { _validateRotateParams(publicKey, ingress, egress); bytes32 message = keccak256( - abi.encodePacked(block.chainid, address(this), validatorAddress, ingress, egress) + abi.encodePacked( + block.chainid, + address(this), + validatorAddress, + uint8(bytes(ingress).length), + ingress, + uint8(bytes(egress).length), + egress + ) ); _verifyEd25519Signature( bytes("TEMPO_VALIDATOR_CONFIG_V2_ROTATE_VALIDATOR"), publicKey, message, signature diff --git a/tips/ref-impls/src/interfaces/IValidatorConfigV2.sol b/tips/ref-impls/src/interfaces/IValidatorConfigV2.sol index 3fd9ef70ba..503782feca 100644 --- a/tips/ref-impls/src/interfaces/IValidatorConfigV2.sol +++ b/tips/ref-impls/src/interfaces/IValidatorConfigV2.sol @@ -98,7 +98,7 @@ interface IValidatorConfigV2 { /// @notice Add a new validator (owner only) /// @dev The signature must be an Ed25519 signature over: - /// keccak256(abi.encodePacked("TEMPO", "_VALIDATOR_CONFIG_V2_ADD_VALIDATOR", chainId, contractAddress, validatorAddress, ingress, egress)) + /// keccak256(abi.encodePacked("TEMPO", "_VALIDATOR_CONFIG_V2_ADD_VALIDATOR", chainId, contractAddress, validatorAddress, uint8(bytes(ingress).length), ingress, uint8(bytes(egress).length), egress)) /// This proves the caller controls the private key corresponding to publicKey. /// Reverts if isInitialized() returns false. /// @param validatorAddress The address of the new validator @@ -133,7 +133,7 @@ interface IValidatorConfigV2 { /// - Egress must be parseable as /// - The signature must prove ownership of the new public key /// The signature must be an Ed25519 signature over: - /// keccak256(abi.encodePacked("TEMPO", "_VALIDATOR_CONFIG_V2_ROTATE_VALIDATOR", chainId, contractAddress, validatorAddress, ingress, egress)) + /// keccak256(abi.encodePacked("TEMPO", "_VALIDATOR_CONFIG_V2_ROTATE_VALIDATOR", chainId, contractAddress, validatorAddress, uint8(bytes(ingress).length), ingress, uint8(bytes(egress).length), egress)) /// @param validatorAddress The address of the validator to rotate /// @param publicKey The new validator's Ed25519 communication public key /// @param ingress The new validator's inbound address `:` for incoming connections diff --git a/tips/ref-impls/test/ValidatorConfigV2.t.sol b/tips/ref-impls/test/ValidatorConfigV2.t.sol index 736518e337..ead54b1bfa 100644 --- a/tips/ref-impls/test/ValidatorConfigV2.t.sol +++ b/tips/ref-impls/test/ValidatorConfigV2.t.sol @@ -52,7 +52,13 @@ contract ValidatorConfigV2Test is BaseTest { { bytes32 message = keccak256( abi.encodePacked( - uint64(block.chainid), address(validatorConfigV2), validatorAddress, ingress, egress + uint64(block.chainid), + address(validatorConfigV2), + validatorAddress, + uint8(bytes(ingress).length), + ingress, + uint8(bytes(egress).length), + egress ) ); // Forge's signEd25519 does simple concat(namespace, message), but the Rust @@ -76,7 +82,13 @@ contract ValidatorConfigV2Test is BaseTest { { bytes32 message = keccak256( abi.encodePacked( - uint64(block.chainid), address(validatorConfigV2), validatorAddress, ingress, egress + uint64(block.chainid), + address(validatorConfigV2), + validatorAddress, + uint8(bytes(ingress).length), + ingress, + uint8(bytes(egress).length), + egress ) ); bytes memory ns = bytes("TEMPO_VALIDATOR_CONFIG_V2_ROTATE_VALIDATOR"); diff --git a/tips/ref-impls/test/invariants/ValidatorConfigV2.t.sol b/tips/ref-impls/test/invariants/ValidatorConfigV2.t.sol index 347ac5496f..6872a252e6 100644 --- a/tips/ref-impls/test/invariants/ValidatorConfigV2.t.sol +++ b/tips/ref-impls/test/invariants/ValidatorConfigV2.t.sol @@ -117,7 +117,13 @@ contract ValidatorConfigV2InvariantTest is InvariantBaseTest { { bytes32 message = keccak256( abi.encodePacked( - uint64(block.chainid), address(validatorConfigV2), validatorAddress, ingress, egress + uint64(block.chainid), + address(validatorConfigV2), + validatorAddress, + uint8(bytes(ingress).length), + ingress, + uint8(bytes(egress).length), + egress ) ); bytes memory ns = bytes("TEMPO_VALIDATOR_CONFIG_V2_ADD_VALIDATOR"); @@ -138,7 +144,13 @@ contract ValidatorConfigV2InvariantTest is InvariantBaseTest { { bytes32 message = keccak256( abi.encodePacked( - uint64(block.chainid), address(validatorConfigV2), validatorAddress, ingress, egress + uint64(block.chainid), + address(validatorConfigV2), + validatorAddress, + uint8(bytes(ingress).length), + ingress, + uint8(bytes(egress).length), + egress ) ); bytes memory ns = bytes("TEMPO_VALIDATOR_CONFIG_V2_ROTATE_VALIDATOR"); diff --git a/tips/tip-1017.md b/tips/tip-1017.md index 4fe0c5b424..8ecb6feca0 100644 --- a/tips/tip-1017.md +++ b/tips/tip-1017.md @@ -106,7 +106,7 @@ interface IValidatorConfigV2 { /// @notice Thrown when trying to delete a validator that is already deleted error ValidatorAlreadyDeleted(); - /// @notice Thrown when public key is invalid (zero) + /// @notice Thrown when public key is invalid (zero or not a valid Ed25519 curve point) error InvalidPublicKey(); /// @notice Thrown when the Ed25519 signature verification fails @@ -183,7 +183,7 @@ interface IValidatorConfigV2 { /// @notice Add a new validator (owner only) /// @dev The signature must be an Ed25519 signature over: - /// keccak256(abi.encodePacked(bytes8(chainId), contractAddress, validatorAddress, ingress, egress)) + /// keccak256(bytes8(chainId) || contractAddress || validatorAddress || uint8(bytes(ingress).length) || ingress || uint8(bytes(egress).length) || egress)) /// using the namespace "TEMPO_VALIDATOR_CONFIG_V2_ADD_VALIDATOR". /// This proves the caller controls the private key corresponding to publicKey. /// Reverts if isInitialized() returns false. @@ -219,7 +219,7 @@ interface IValidatorConfigV2 { /// - Egress must be parseable as . /// - The signature must prove ownership of the new public key /// The signature must be an Ed25519 signature over: - /// keccak256(abi.encodePacked(bytes8(chainId), contractAddress, validatorAddress, ingress, egress)) + /// keccak256(bytes8(chainId) || contractAddress || validatorAddress || uint8(bytes(ingress).length) || ingress || uint8(bytes(egress).length) || egress)) /// using the namespace "TEMPO_VALIDATOR_CONFIG_V2_ROTATE_VALIDATOR". /// @param validatorAddress The address of the validator to rotate /// @param publicKey The new validator's Ed25519 communication public key @@ -328,13 +328,15 @@ signature is checked over the a full message containing: the length of the names **Message:** ``` -message = keccak256(abi.encodePacked( - bytes8(chainId), // uint64: Prevents cross-chain replay - contractAddress, // address: Prevents cross-contract replay - validatorAddress, // address: Binds to specific validator address - ingress, // string: Binds network configuration - egress // string: Binds network configuration -)) +message = keccak256( + bytes8(chainId) // uint64: Prevents cross-chain replay + || contractAddress // address: Prevents cross-contract replay + || validatorAddress // address: Binds to specific validator address + || uint8(ingress.length) // uint8: Length prefix to prevent ingress/egress boundary collision + || ingress // string: Binds network configuration + || uint8(egress.length) // uint8: Length prefix to prevent egress boundary collision + || egress // string: Binds network configuration +) ``` The Ed25519 signature is computed over the message using the namespace parameter @@ -592,7 +594,7 @@ precompile does NOT proxy reads to V1. - On first call (when `validatorsArray.length == 0`), copies `owner` from V1 if V2 owner is `address(0)` - Reads the validator from V1 at index `idx` - Creates a V2 validator entry with: - - `publicKey`: copied from V1 + - `publicKey`: copied from V1 (must be a valid Ed25519 curve point; reverts with `InvalidPublicKey` otherwise) - `validatorAddress`: copied from V1 - `ingress`: copied from V1 `inboundAddress` - `egress`: copied from V1 `outboundAddress` @@ -640,7 +642,7 @@ interface IValidatorConfigV1 { - **Validator address copied**: The V2 validator address is copied from V1 - **Address changeable post-migration**: Owner or validator can update validator addresses via `transferValidatorOwnership` - **No signatures required**: V1 validators are imported without Ed25519 signatures - (they were already validated in V1) + (the public key is validated as a decodable Ed25519 curve point during migration) - **All validators imported**: Both active and inactive V1 validators are imported; active ones have `addedAtHeight == block.height` and `deactivatedAtHeight == 0`, inactive ones have `addedAtHeight == deactivatedAtHeight == block.height`. @@ -720,7 +722,7 @@ The test suite must cover: 12. **PublicKeyAlreadyExists**: Cannot re-use same public key 13. **ValidatorNotFound**: Cannot query/delete non-existent validator 14. **ValidatorAlreadyDeleted**: Cannot delete twice -15. **InvalidPublicKey**: Rejects zero public key +15. **InvalidPublicKey**: Rejects zero public key and invalid Ed25519 curve points 16. **InvalidSignature**: Rejects wrong signature, wrong length, wrong signer 17. **IngressAlreadyExists**: Cannot use ingress IP already in use by active validator (even with different port) diff --git a/xtask/src/genesis_args.rs b/xtask/src/genesis_args.rs index 3a61da1069..e00b1a115b 100644 --- a/xtask/src/genesis_args.rs +++ b/xtask/src/genesis_args.rs @@ -1004,12 +1004,14 @@ fn initialize_validator_config_v2( let ingress = addr.to_string(); let egress = addr.ip().to_string(); - // message: keccak256(chainId || contractAddr || validatorAddr || ingress || egress) + // message: keccak256(chainId || contractAddr || validatorAddr || uint8(ingress.len) || ingress || uint8(egress.len) || egress) let mut hasher = Keccak256::new(); hasher.update(chain_id.to_be_bytes()); hasher.update(VALIDATOR_CONFIG_V2_ADDRESS.as_slice()); hasher.update(validator_address.as_slice()); + hasher.update([ingress.len() as u8]); hasher.update(ingress.as_bytes()); + hasher.update([egress.len() as u8]); hasher.update(egress.as_bytes()); let message = hasher.finalize();