Merge branch 'brent/limit-metadata-size' (#2845)

* origin/brent/limit-metadata-size:
  wasm/vp_user: only read metadata if it's changed
  fix bug for deleting metadata
  genesis metadata validation
  changelog: add #2845
  refactor
  check metadata length in client
  check metadata length in VP

# Conflicts:
#	crates/sdk/src/tx.rs

.changelog/unreleased/improvements/2845-limit-metadata-size.md

@@ -0,0 +1,2 @@
+- Limit the character length of the validator metadata strings.
+  ([\#2845](https://github.com/anoma/namada/pull/2845))

crates/apps/src/lib/config/genesis/transactions.rs

@@ -24,6 +24,7 @@ use namada::core::token;
 use namada::core::token::{DenominatedAmount, NATIVE_MAX_DECIMAL_PLACES};
 use namada::ledger::pos::common::PublicKey;
 use namada::ledger::pos::types::ValidatorMetaData;
+use namada::proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN;
 use namada::tx::data::{pos, Fee, TxType};
 use namada::tx::{
     verify_standalone_sig, Code, Commitment, Data, Section, SignatureIndex, Tx,
@@ -1362,6 +1363,54 @@ 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 {
+        panic!(
+            "The email metadata of the validator with address {} is too long, \
+             must be within {MAX_VALIDATOR_METADATA_LEN} characters",
+            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
+            );
+        }
+    }
+
     // Check signature
     let mut is_valid = {
         let maybe_threshold = {

crates/proof_of_stake/src/parameters.rs

@@ -135,6 +135,9 @@ pub enum ValidationError {
     UnbondingLenTooShort(u64, u64),
 }
 
+/// The maximum string length of any validator metadata
+pub const MAX_VALIDATOR_METADATA_LEN: u64 = 500;
+
 /// The number of fundamental units per whole token of the native staking token
 pub const TOKENS_PER_NAM: u64 = 1_000_000;
 

crates/sdk/src/error.rs

@@ -309,6 +309,9 @@ pub enum TxSubmitError {
     /// An empty string was provided as a new email
     #[error("An empty string cannot be provided as a new email")]
     InvalidEmail,
+    /// The metadata string is too long
+    #[error("The provided metadata string is too long")]
+    MetadataTooLong,
     /// The consensus key is not Ed25519
     #[error("The consensus key must be an ed25519 key")]
     ConsensusKeyNotEd25519,

crates/sdk/src/tx.rs

@@ -51,7 +51,9 @@ use namada_governance::storage::proposal::{
 };
 use namada_governance::storage::vote::ProposalVote;
 use namada_ibc::storage::{channel_key, ibc_token};
-use namada_proof_of_stake::parameters::PosParams;
+use namada_proof_of_stake::parameters::{
+    PosParams, MAX_VALIDATOR_METADATA_LEN,
+};
 use namada_proof_of_stake::types::{CommissionPair, ValidatorState};
 use namada_token::storage_key::balance_key;
 use namada_token::DenominatedAmount;
@@ -698,6 +700,68 @@ pub async fn build_validator_metadata_change(
             );
             return Err(Error::from(TxSubmitError::InvalidEmail));
         }
+        // Check that the email is within MAX_VALIDATOR_METADATA_LEN characters
+        if email.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+            edisplay_line!(
+                context.io(),
+                "Email provided is too long, must be within \
+                 {MAX_VALIDATOR_METADATA_LEN} characters"
+            );
+            if !tx_args.force {
+                return Err(Error::from(TxSubmitError::MetadataTooLong));
+            }
+        }
+    }
+
+    // Check that any new metadata provided is within MAX_VALIDATOR_METADATA_LEN
+    // characters
+    if let Some(description) = description.as_ref() {
+        if description.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+            edisplay_line!(
+                context.io(),
+                "Description provided is too long, must be within \
+                 {MAX_VALIDATOR_METADATA_LEN} characters"
+            );
+            if !tx_args.force {
+                return Err(Error::from(TxSubmitError::MetadataTooLong));
+            }
+        }
+    }
+    if let Some(website) = website.as_ref() {
+        if website.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+            edisplay_line!(
+                context.io(),
+                "Website provided is too long, must be within \
+                 {MAX_VALIDATOR_METADATA_LEN} characters"
+            );
+            if !tx_args.force {
+                return Err(Error::from(TxSubmitError::MetadataTooLong));
+            }
+        }
+    }
+    if let Some(discord_handle) = discord_handle.as_ref() {
+        if discord_handle.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+            edisplay_line!(
+                context.io(),
+                "Discord handle provided is too long, must be within \
+                 {MAX_VALIDATOR_METADATA_LEN} characters"
+            );
+            if !tx_args.force {
+                return Err(Error::from(TxSubmitError::MetadataTooLong));
+            }
+        }
+    }
+    if let Some(avatar) = avatar.as_ref() {
+        if avatar.len() as u64 > MAX_VALIDATOR_METADATA_LEN {
+            edisplay_line!(
+                context.io(),
+                "Avatar provided is too long, must be within \
+                 {MAX_VALIDATOR_METADATA_LEN} characters"
+            );
+            if !tx_args.force {
+                return Err(Error::from(TxSubmitError::MetadataTooLong));
+            }
+        }
     }
 
     // If there's a new commission rate, it must be valid

wasm/wasm_source/src/vp_user.rs

@@ -15,6 +15,7 @@ use core::ops::Deref;
 
 use namada_vp_prelude::*;
 use once_cell::unsync::Lazy;
+use proof_of_stake::parameters::MAX_VALIDATOR_METADATA_LEN;
 use proof_of_stake::storage::{read_pos_params, validator_state_handle};
 use proof_of_stake::storage_key::{
     is_below_capacity_validator_set_key, is_bond_epoched_meta_key, is_bond_key,
@@ -188,11 +189,19 @@ fn validate_pos_changes(
     // Metadata changes must be signed by the validator whose
     // metadata is manipulated
     let is_valid_metadata_change = || {
-        let metadata = is_validator_metadata_key(key);
-        match metadata {
-            Some(address) => address == owner && **valid_sig,
+        let is_valid = match is_validator_metadata_key(key) {
+            Some(address) => {
+                let metadata = ctx.post().read::<String>(key)?;
+                let valid_len = if let Some(metadata) = metadata {
+                    (metadata.len() as u64) <= MAX_VALIDATOR_METADATA_LEN
+                } else {
+                    true
+                };
+                valid_len && address == owner && **valid_sig
+            }
             None => false,
-        }
+        };
+        VpResult::Ok(is_valid)
     };
 
     // Changes in validator state
@@ -324,7 +333,7 @@ fn validate_pos_changes(
         || is_valid_reward_claim()
         || is_valid_redelegation()
         || is_valid_commission_rate_change()
-        || is_valid_metadata_change()
+        || is_valid_metadata_change()?
         || is_valid_become_validator()
         || **valid_sig)
 }