diff --git a/.changelog/unreleased/bug-fixes/4036-validate-validator-metadata.md b/.changelog/unreleased/bug-fixes/4036-validate-validator-metadata.md new file mode 100644 index 00000000000..f6afd72cb99 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/4036-validate-validator-metadata.md @@ -0,0 +1,2 @@ +- Validate validator metadata from on-chain validator creation and metadata + changes. ([\#4036](https://github.com/anoma/namada/pull/4036)) \ No newline at end of file diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 770f7886b86..36e0d4bbc2a 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -637,11 +637,10 @@ pub async fn submit_become_validator( .is_applied_and_valid(wrapper_hash.as_ref(), &cmt) .is_none() { - display_line!( - namada.io(), + return Err(error::Error::Tx(error::TxSubmitError::Other( "Transaction failed. No key or addresses have been saved." - ); - safe_exit(1) + .to_string(), + ))); } // add validator address and keys to the wallet diff --git a/crates/apps_lib/src/config/genesis/transactions.rs b/crates/apps_lib/src/config/genesis/transactions.rs index a359554067e..2873488b1dd 100644 --- a/crates/apps_lib/src/config/genesis/transactions.rs +++ b/crates/apps_lib/src/config/genesis/transactions.rs @@ -20,7 +20,6 @@ use namada_sdk::collections::HashSet; use namada_sdk::dec::Dec; use namada_sdk::key::common::PublicKey; use namada_sdk::key::{common, ed25519, RefTo, SerializeWithBorsh, SigScheme}; -use namada_sdk::proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN; use namada_sdk::proof_of_stake::types::ValidatorMetaData; use namada_sdk::signing::{sign_tx, SigningTxData}; use namada_sdk::string_encoding::StringEncoded; @@ -1385,60 +1384,14 @@ pub fn validate_validator_account( // Check that the validator metadata is not too large let metadata = &signed_tx.data.metadata; - if metadata.email.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + let errors = metadata.validate(); + if !errors.is_empty() { panic!( - "The email metadata of the validator with address {} is too long, \ - must be within {MAX_VALIDATOR_METADATA_LEN} characters", + "Metadata of the validator with address {} are invalid: \ + {errors:#?}", signed_tx.data.address ); } - if let Some(description) = metadata.description.as_ref() { - if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN { - panic!( - "The description metadata of the validator with address {} is \ - too long, must be within {MAX_VALIDATOR_METADATA_LEN} \ - characters", - signed_tx.data.address - ); - } - } - if let Some(website) = metadata.website.as_ref() { - if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN { - panic!( - "The website metadata of the validator with address {} is too \ - long, must be within {MAX_VALIDATOR_METADATA_LEN} characters", - signed_tx.data.address - ); - } - } - if let Some(discord_handle) = metadata.discord_handle.as_ref() { - if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN { - panic!( - "The discord handle metadata of the validator with address {} \ - is too long, must be within {MAX_VALIDATOR_METADATA_LEN} \ - characters", - signed_tx.data.address - ); - } - } - if let Some(avatar) = metadata.avatar.as_ref() { - if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN { - panic!( - "The avatar metadata of the validator with address {} is too \ - long, must be within {MAX_VALIDATOR_METADATA_LEN} characters", - signed_tx.data.address - ); - } - } - if let Some(name) = metadata.name.as_ref() { - if name.len() as u64 > MAX_VALIDATOR_METADATA_LEN { - panic!( - "The name metadata of the validator with address {} is too \ - long, must be within {MAX_VALIDATOR_METADATA_LEN} characters", - signed_tx.data.address - ); - } - } // Check signature let mut is_valid = { diff --git a/crates/proof_of_stake/src/error.rs b/crates/proof_of_stake/src/error.rs index 165cec9b7ab..8eb160aa4db 100644 --- a/crates/proof_of_stake/src/error.rs +++ b/crates/proof_of_stake/src/error.rs @@ -6,6 +6,7 @@ use namada_core::chain::Epoch; use namada_core::dec::Dec; use thiserror::Error; +use crate::parameters::MAX_VALIDATOR_METADATA_LEN; use crate::types::ValidatorState; use crate::{rewards, Error}; @@ -165,6 +166,16 @@ pub enum ConsensusKeyChangeError { MustBeEd25519, } +#[allow(missing_docs)] +#[derive(Error, Debug)] +pub enum ValidatorMetaDataError { + #[error( + "The {0} metadata is too long, must be within \ + {MAX_VALIDATOR_METADATA_LEN} characters" + )] + FieldTooLong(&'static str), +} + impl From for Error { fn from(err: BecomeValidatorError) -> Self { Self::new(err) diff --git a/crates/proof_of_stake/src/storage.rs b/crates/proof_of_stake/src/storage.rs index a3d0a61cea7..0fe961d28e1 100644 --- a/crates/proof_of_stake/src/storage.rs +++ b/crates/proof_of_stake/src/storage.rs @@ -963,6 +963,35 @@ where Ok(()) } +/// Read validator's metadata. +pub fn read_validator_metadata( + storage: &S, + validator: &Address, +) -> Result> +where + S: StorageRead, +{ + let email = read_validator_email(storage, validator)?; + let description = read_validator_description(storage, validator)?; + let website = read_validator_website(storage, validator)?; + let discord_handle = read_validator_discord_handle(storage, validator)?; + let avatar = read_validator_avatar(storage, validator)?; + let name = read_validator_name(storage, validator)?; + + // Email is the only required field for a validator in storage + match email { + Some(email) => Ok(Some(ValidatorMetaData { + email, + description, + website, + discord_handle, + avatar, + name, + })), + None => Ok(None), + } +} + /// Get the last epoch in which rewards were claimed from storage, if any pub fn get_last_reward_claim_epoch( storage: &S, diff --git a/crates/proof_of_stake/src/types/mod.rs b/crates/proof_of_stake/src/types/mod.rs index 988d1a73968..e0809eeb677 100644 --- a/crates/proof_of_stake/src/types/mod.rs +++ b/crates/proof_of_stake/src/types/mod.rs @@ -21,8 +21,8 @@ pub use rev_order::ReverseOrdTokenAmount; use serde::{Deserialize, Serialize}; use crate::lazy_map::NestedMap; -use crate::parameters::PosParams; -use crate::{Epoch, KeySeg, LazyMap, LazySet, LazyVec}; +use crate::parameters::{PosParams, MAX_VALIDATOR_METADATA_LEN}; +use crate::{Epoch, KeySeg, LazyMap, LazySet, LazyVec, ValidatorMetaDataError}; /// Stored positions of validators in validator sets pub type ValidatorSetPositions = crate::epoched::NestedEpoched< @@ -419,6 +419,46 @@ pub struct ValidatorMetaData { pub name: Option, } +impl ValidatorMetaData { + /// Validator validator metadata. Returns an empty vec only if all fields + /// are valid. + pub fn validate(&self) -> Vec { + let mut errors = vec![]; + if self.email.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + errors.push(ValidatorMetaDataError::FieldTooLong("email")); + } + if let Some(description) = self.description.as_ref() { + if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + errors + .push(ValidatorMetaDataError::FieldTooLong("description")); + } + } + if let Some(website) = self.website.as_ref() { + if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + errors.push(ValidatorMetaDataError::FieldTooLong("website")); + } + } + if let Some(discord_handle) = self.discord_handle.as_ref() { + if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + errors.push(ValidatorMetaDataError::FieldTooLong( + "discord handle", + )); + } + } + if let Some(avatar) = self.avatar.as_ref() { + if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + errors.push(ValidatorMetaDataError::FieldTooLong("avatar")); + } + } + if let Some(name) = self.name.as_ref() { + if name.len() as u64 > MAX_VALIDATOR_METADATA_LEN { + errors.push(ValidatorMetaDataError::FieldTooLong("name")); + } + } + errors + } +} + #[cfg(any(test, feature = "testing"))] impl Default for ValidatorMetaData { fn default() -> Self { diff --git a/crates/proof_of_stake/src/vp.rs b/crates/proof_of_stake/src/vp.rs index dcb2d989f1c..60a0b6e4371 100644 --- a/crates/proof_of_stake/src/vp.rs +++ b/crates/proof_of_stake/src/vp.rs @@ -14,7 +14,7 @@ use namada_tx::BatchedTxRef; use namada_vp_env::{Error, Result, VpEnv}; use thiserror::Error; -use crate::storage::read_owned_pos_params; +use crate::storage::{read_owned_pos_params, read_validator_metadata}; use crate::storage_key::is_params_key; use crate::types::BondId; use crate::{storage_key, token}; @@ -299,6 +299,23 @@ where } } + // Validate new and changed validator metadata + for validator in became_validator.iter().chain(&changed_metadata) { + let metadata = read_validator_metadata(&ctx.post(), validator)?; + let Some(metadata) = metadata else { + return Err(Error::new_alloc(format!( + "Missing validator {validator} metadata" + ))); + }; + let errors = metadata.validate(); + if !errors.is_empty() { + return Err(Error::new_alloc(format!( + "Metadata of the validator with address {validator} are \ + invalid: {errors:#?}", + ))); + } + } + for key in keys_changed { if is_params_key(key) { return Err(Error::new_const( diff --git a/crates/sdk/src/queries/vp/pos.rs b/crates/sdk/src/queries/vp/pos.rs index 0076419c73d..5fcf4e06fd8 100644 --- a/crates/sdk/src/queries/vp/pos.rs +++ b/crates/sdk/src/queries/vp/pos.rs @@ -22,13 +22,10 @@ use namada_proof_of_stake::storage::{ read_below_capacity_validator_set_addresses_with_stake, read_consensus_validator_set_addresses, read_consensus_validator_set_addresses_with_stake, read_pos_params, - read_total_active_stake, read_total_stake, read_validator_avatar, - read_validator_description, read_validator_discord_handle, - read_validator_email, read_validator_last_slash_epoch, - read_validator_max_commission_rate_change, read_validator_name, - read_validator_stake, read_validator_website, unbond_handle, - validator_commission_rate_handle, validator_incoming_redelegations_handle, - validator_slashes_handle, + read_total_active_stake, read_total_stake, read_validator_last_slash_epoch, + read_validator_max_commission_rate_change, read_validator_metadata, + read_validator_stake, unbond_handle, validator_commission_rate_handle, + validator_incoming_redelegations_handle, validator_slashes_handle, }; pub use namada_proof_of_stake::types::ValidatorStateInfo; use namada_proof_of_stake::types::{ @@ -325,25 +322,7 @@ where D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, { - let email = read_validator_email(ctx.state, &validator)?; - let description = read_validator_description(ctx.state, &validator)?; - let website = read_validator_website(ctx.state, &validator)?; - let discord_handle = read_validator_discord_handle(ctx.state, &validator)?; - let avatar = read_validator_avatar(ctx.state, &validator)?; - let name = read_validator_name(ctx.state, &validator)?; - - // Email is the only required field for a validator in storage - match email { - Some(email) => Ok(Some(ValidatorMetaData { - email, - description, - website, - discord_handle, - avatar, - name, - })), - _ => Ok(None), - } + read_validator_metadata(ctx.state, &validator) } /// Get the validator state diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 0a5a39cc01d..d609aaddd78 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -26,6 +26,7 @@ use namada_sdk::account::AccountPublicKeysMap; use namada_sdk::collections::HashMap; use namada_sdk::error::TxSubmitError; use namada_sdk::migrations; +use namada_sdk::proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN; use namada_sdk::queries::RPC; use namada_sdk::token::{self, DenominatedAmount}; use namada_sdk::tx::{self, Tx, TX_TRANSFER_WASM, VP_USER_WASM}; @@ -2565,6 +2566,95 @@ fn wrap_tx_by_elsewho() -> Result<()> { Ok(()) } +/// Test for PoS validator metadata validation. +/// +/// 1. Run the ledger node. +/// 2. Submit a valid metadata change tx. +/// 3. Check that the metadata has changed. +/// 4. Submit an invalid metadata change tx. +/// 5. Check that the metadata has not changed. +/// 6. Submit a tx to become validator with invalid metadata. +#[test] +fn pos_validator_metadata_validation() -> Result<()> { + // 1. Run the ledger node. + let (node, _services) = setup::setup()?; + + // 2. Submit a valid metadata change tx. + let valid_desc: String = "0123456789".repeat(50); + assert_eq!(valid_desc.len() as u64, MAX_VALIDATOR_METADATA_LEN); + let tx_args = apply_use_device(vec![ + "change-metadata", + "--validator", + "validator-0-validator", + "--description", + &valid_desc, + ]); + let captured = CapturedOutput::of(|| run(&node, Bin::Client, tx_args)); + println!("{:?}", captured.result); + assert_matches!(captured.result, Ok(_)); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // 3. Check that the metadata has changed. + let query_args = apply_use_device(vec![ + "validator-metadata", + "--validator", + "validator-0-validator", + ]); + let captured = + CapturedOutput::of(|| run(&node, Bin::Client, query_args.clone())); + println!("{:?}", captured.result); + assert!(captured.contains(&valid_desc)); + + // 4. Submit an invalid metadata change tx. + let invalid_desc: String = format!("N{valid_desc}"); + assert!(invalid_desc.len() as u64 > MAX_VALIDATOR_METADATA_LEN); + let tx_args = apply_use_device(vec![ + "change-metadata", + "--validator", + "validator-0-validator", + "--description", + &invalid_desc, + "--force", + ]); + let captured = CapturedOutput::of(|| run(&node, Bin::Client, tx_args)); + println!("{:?}", captured.result); + assert_matches!(captured.result, Ok(_)); + assert!(captured.contains(TX_REJECTED)); + + // 5. Check that the metadata has not changed. + let captured = CapturedOutput::of(|| run(&node, Bin::Client, query_args)); + println!("{:?}", captured.result); + assert!(captured.contains(&valid_desc)); + + // 6. Submit a tx to become validator with invalid metadata. + let new_validator = "new-validator"; + let tx_args = apply_use_device(vec![ + "init-validator", + "--alias", + new_validator, + "--name", + new_validator, + "--account-keys", + "bertha-key", + "--commission-rate", + "0.05", + "--max-commission-rate-change", + "0.01", + "--email", + "null@null.net", + "--signing-keys", + "bertha-key", + "--description", + &invalid_desc, + "--unsafe-dont-encrypt", + ]); + let captured = CapturedOutput::of(|| run(&node, Bin::Client, tx_args)); + assert_matches!(captured.result, Err(_)); + assert!(captured.contains(TX_REJECTED)); + + Ok(()) +} + fn make_migration_json() -> (Hash, tempfile::NamedTempFile) { let file = tempfile::Builder::new().tempfile().expect("Test failed"); let updates = [migrations::DbUpdateType::Add {