More checks for certain PoS transactions in validator VP

.changelog/unreleased/improvements/2169-pos-sigs-for-vp.md

@@ -0,0 +1,3 @@
+- Improve the validator VP to ensure that only the validator themself
+  can execute transactions that manipulate its own validator data
+  ([\#2169](https://github.com/anoma/namada/pull/2169))

.github/workflows/scripts/e2e.json

@@ -23,8 +23,9 @@
     "e2e::ledger_tests::deactivate_and_reactivate_validator": 67,
     "e2e::ledger_tests::change_validator_metadata": 31,
     "e2e::ledger_tests::pos_rewards": 44,
+    "e2e::ledger_tests::test_invalid_validator_txs": 73,
     "e2e::wallet_tests::wallet_address_cmds": 1,
     "e2e::wallet_tests::wallet_encrypted_key_cmds": 1,
     "e2e::wallet_tests::wallet_encrypted_key_cmds_env_var": 1,
     "e2e::wallet_tests::wallet_unencrypted_key_cmds": 1
-}
+}

proof_of_stake/src/storage.rs

@@ -191,19 +191,28 @@ pub fn validator_commission_rate_key(validator: &Address) -> Key {
         .expect("Cannot obtain a storage key")
 }
 
-/// Is storage key for validator's commissionr ate?
-pub fn is_validator_commission_rate_key(key: &Key) -> Option<&Address> {
+/// Is storage key for validator's commission rate?
+pub fn is_validator_commission_rate_key(
+    key: &Key,
+) -> Option<(&Address, Epoch)> {
     match &key.segments[..] {
         [
             DbKeySeg::AddressSeg(addr),
             DbKeySeg::StringSeg(prefix),
             DbKeySeg::AddressSeg(validator),
             DbKeySeg::StringSeg(key),
+            DbKeySeg::StringSeg(lazy_map),
+            DbKeySeg::StringSeg(data),
+            DbKeySeg::StringSeg(epoch),
         ] if addr == &ADDRESS
             && prefix == VALIDATOR_STORAGE_PREFIX
-            && key == VALIDATOR_COMMISSION_RATE_STORAGE_KEY =>
+            && key == VALIDATOR_COMMISSION_RATE_STORAGE_KEY
+            && lazy_map == LAZY_MAP_SUB_KEY
+            && data == lazy_map::DATA_SUBKEY =>
         {
-            Some(validator)
+            let epoch = Epoch::parse(epoch.clone())
+                .expect("Should be able to parse the epoch");
+            Some((validator, epoch))
         }
         _ => None,
     }
@@ -236,6 +245,30 @@ pub fn is_validator_max_commission_rate_change_key(
     }
 }
 
+/// Is storage key for some piece of validator metadata?
+pub fn is_validator_metadata_key(key: &Key) -> Option<&Address> {
+    match &key.segments[..] {
+        [
+            DbKeySeg::AddressSeg(addr),
+            DbKeySeg::StringSeg(prefix),
+            DbKeySeg::AddressSeg(validator),
+            DbKeySeg::StringSeg(metadata),
+        ] if addr == &ADDRESS
+            && prefix == VALIDATOR_STORAGE_PREFIX
+            && matches!(
+                metadata.as_str(),
+                VALIDATOR_EMAIL_KEY
+                    | VALIDATOR_DESCRIPTION_KEY
+                    | VALIDATOR_WEBSITE_KEY
+                    | VALIDATOR_DISCORD_KEY
+            ) =>
+        {
+            Some(validator)
+        }
+        _ => None,
+    }
+}
+
 /// Storage key for validator's rewards products.
 pub fn validator_rewards_product_key(validator: &Address) -> Key {
     validator_prefix(validator)

tests/src/e2e/ledger_tests.rs

@@ -3457,3 +3457,211 @@ fn change_validator_metadata() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_invalid_validator_txs() -> Result<()> {
+    let pipeline_len = 2;
+    let unbonding_len = 4;
+    let test = setup::network(
+        |mut genesis, base_dir: &_| {
+            genesis.parameters.pos_params.pipeline_len = pipeline_len;
+            genesis.parameters.pos_params.unbonding_len = unbonding_len;
+            // genesis.parameters.parameters.min_num_of_blocks = 6;
+            // genesis.parameters.parameters.max_expected_time_per_block = 1;
+            // genesis.parameters.parameters.epochs_per_year = 31_536_000;
+            let mut genesis = setup::set_validators(
+                2,
+                genesis,
+                base_dir,
+                default_port_offset,
+            );
+            let bonds = genesis.transactions.bond.unwrap();
+            genesis.transactions.bond = Some(
+                bonds
+                    .into_iter()
+                    .filter(|bond| {
+                        (&bond.data.validator).as_ref() != "validator-1"
+                    })
+                    .collect(),
+            );
+            genesis
+        },
+        None,
+    )?;
+
+    set_ethereum_bridge_mode(
+        &test,
+        &test.net.chain_id,
+        &Who::Validator(0),
+        ethereum_bridge::ledger::Mode::Off,
+        None,
+    );
+
+    // 1. Run the ledger node
+    let _bg_validator_0 =
+        start_namada_ledger_node_wait_wasm(&test, Some(0), Some(40))?
+            .background();
+
+    let _bg_validator_1 =
+        start_namada_ledger_node_wait_wasm(&test, Some(1), Some(40))?
+            .background();
+
+    let validator_0_rpc = get_actor_rpc(&test, &Who::Validator(0));
+    let validator_1_rpc = get_actor_rpc(&test, &Who::Validator(1));
+
+    // put money in the validator-1 account from its balance account so that it
+    // can deactivate and reactivate
+    let tx_args = vec![
+        "transfer",
+        "--source",
+        "validator-1-balance-key",
+        "--target",
+        "validator-1-validator-key",
+        "--amount",
+        "100.0",
+        "--token",
+        "NAM",
+        "--node",
+        &validator_1_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(1), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is valid.")?;
+    client.assert_success();
+
+    // put money in the validator-0 account from its balance account so that it
+    // can deactivate and reactivate
+    let tx_args = vec![
+        "transfer",
+        "--source",
+        "validator-0-balance-key",
+        "--target",
+        "validator-0-validator-key",
+        "--amount",
+        "100.0",
+        "--token",
+        "NAM",
+        "--node",
+        &validator_0_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is valid.")?;
+    client.assert_success();
+
+    // Try to change validator-1 commission rate as validator-0
+    let tx_args = vec![
+        "change-commission-rate",
+        "--validator",
+        "validator-1",
+        "--commission-rate",
+        "0.06",
+        "--signing-keys",
+        "validator-0-validator-key",
+        "--node",
+        &validator_0_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is invalid.")?;
+    client.assert_success();
+
+    // Try to deactivate validator-1 as validator-0
+    let tx_args = vec![
+        "deactivate-validator",
+        "--validator",
+        "validator-1",
+        "--signing-keys",
+        "validator-0-validator-key",
+        "--node",
+        &validator_0_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is invalid.")?;
+    client.assert_success();
+
+    // Try to change the validator-1 website as validator-0
+    let tx_args = vec![
+        "change-metadata",
+        "--validator",
+        "validator-1",
+        "--website",
+        "[email protected]",
+        "--signing-keys",
+        "validator-0-validator-key",
+        "--node",
+        &validator_0_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is invalid.")?;
+    client.assert_success();
+
+    // Deactivate validator-1
+    let tx_args = vec![
+        "deactivate-validator",
+        "--validator",
+        "validator-1",
+        "--signing-keys",
+        "validator-1-validator-key",
+        "--node",
+        &validator_1_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(1), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is valid.")?;
+    client.assert_success();
+
+    let deactivate_epoch = get_epoch(&test, &validator_1_rpc)?;
+    let start = Instant::now();
+    let loop_timeout = Duration::new(120, 0);
+    loop {
+        if Instant::now().duration_since(start) > loop_timeout {
+            panic!(
+                "Timed out waiting for epoch: {}",
+                deactivate_epoch + pipeline_len
+            );
+        }
+        let epoch = epoch_sleep(&test, &validator_1_rpc, 40)?;
+        if epoch >= deactivate_epoch + pipeline_len {
+            break;
+        }
+    }
+
+    // Check the state of validator-1
+    let tx_args = vec![
+        "validator-state",
+        "--validator",
+        "validator-1",
+        "--node",
+        &validator_1_rpc,
+    ];
+    let mut client = run!(test, Bin::Client, tx_args, Some(40))?;
+    client.exp_regex(r"Validator [a-z0-9]+ is inactive")?;
+    client.assert_success();
+
+    // Try to reactivate validator-1 as validator-0
+    let tx_args = vec![
+        "reactivate-validator",
+        "--validator",
+        "validator-1",
+        "--signing-keys",
+        "validator-0-validator-key",
+        "--node",
+        &validator_0_rpc,
+    ];
+    let mut client =
+        run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction applied with result:")?;
+    client.exp_string("Transaction is invalid.")?;
+    client.assert_success();
+
+    Ok(())
+}

vp_prelude/src/lib.rs

@@ -14,7 +14,6 @@ use std::convert::TryFrom;
 use std::marker::PhantomData;
 
 pub use borsh::{BorshDeserialize, BorshSerialize};
-pub use borsh_ext;
 use borsh_ext::BorshSerializeExt;
 pub use namada_core::ledger::governance::storage as gov_storage;
 pub use namada_core::ledger::parameters;
@@ -34,10 +33,10 @@ use namada_core::types::storage::{
 };
 pub use namada_core::types::*;
 pub use namada_macros::validity_predicate;
-pub use namada_proof_of_stake::storage as proof_of_stake;
 use namada_vm_env::vp::*;
 use namada_vm_env::{read_from_buffer, read_key_val_bytes_from_buffer};
 pub use sha2::{Digest, Sha256, Sha384, Sha512};
+pub use {borsh_ext, namada_proof_of_stake as proof_of_stake};
 
 pub fn sha256(bytes: &[u8]) -> Hash {
     let digest = Sha256::digest(bytes);

wasm/wasm_source/src/vp_implicit.rs

@@ -32,7 +32,7 @@ impl<'a> From<&'a storage::Key> for KeyType<'a> {
             Self::Pk(address)
         } else if let Some([_, owner]) = token::is_any_token_balance_key(key) {
             Self::Token { owner }
-        } else if proof_of_stake::is_pos_key(key) {
+        } else if proof_of_stake::storage::is_pos_key(key) {
             Self::PoS
         } else if gov_storage::keys::is_vote_key(key) {
             let voter_address = gov_storage::keys::get_voter_address(key);
@@ -132,10 +132,10 @@ fn validate_tx(
             }
             KeyType::PoS => {
                 // Allow the account to be used in PoS
-                let bond_id = proof_of_stake::is_bond_key(key)
+                let bond_id = proof_of_stake::storage::is_bond_key(key)
                     .map(|(bond_id, _)| bond_id)
                     .or_else(|| {
-                        proof_of_stake::is_unbond_key(key)
+                        proof_of_stake::storage::is_unbond_key(key)
                             .map(|(bond_id, _, _)| bond_id)
                     });
                 let valid = match bond_id {

wasm/wasm_source/src/vp_user.rs

@@ -27,7 +27,7 @@ impl<'a> From<&'a storage::Key> for KeyType<'a> {
     fn from(key: &'a storage::Key) -> KeyType<'a> {
         if let Some([_, owner]) = token::is_any_token_balance_key(key) {
             Self::Token { owner }
-        } else if proof_of_stake::is_pos_key(key) {
+        } else if proof_of_stake::storage::is_pos_key(key) {
             Self::PoS
         } else if gov_storage::keys::is_vote_key(key) {
             let voter_address = gov_storage::keys::get_voter_address(key);
@@ -107,10 +107,10 @@ fn validate_tx(
             }
             KeyType::PoS => {
                 // Allow the account to be used in PoS
-                let bond_id = proof_of_stake::is_bond_key(key)
+                let bond_id = proof_of_stake::storage::is_bond_key(key)
                     .map(|(bond_id, _)| bond_id)
                     .or_else(|| {
-                        proof_of_stake::is_unbond_key(key)
+                        proof_of_stake::storage::is_unbond_key(key)
                             .map(|(bond_id, _, _)| bond_id)
                     });
                 let valid = match bond_id {