Murisi/draft2

.changelog/unreleased/bug-fixes/1607-fix-tx-malleability.md

@@ -0,0 +1,2 @@
+- Fixed bug that allowed transactions to be modified without invalidating
+  transaction hash ([\#1607](https://github.com/anoma/namada/pull/1607))

.changelog/unreleased/bug-fixes/1611-move-init-proposal-content.md

@@ -0,0 +1,3 @@
+- Move the content and code of init proposal transactions
+  into separare section to reduce tx size for hardware wallets
+  ([\#1611](https://github.com/anoma/namada/pull/1611))

apps/src/bin/namada-client/cli.rs

@@ -148,6 +148,17 @@ pub async fn main() -> Result<()> {
                     tx::submit_withdraw::<HttpClient>(&client, ctx, args)
                         .await?;
                 }
+                Sub::TxCommissionRateChange(TxCommissionRateChange(args)) => {
+                    wait_until_node_is_synched(&args.tx.ledger_address).await;
+                    let client =
+                        HttpClient::new(args.tx.ledger_address.clone())
+                            .unwrap();
+                    let args = args.to_sdk(&mut ctx);
+                    tx::submit_validator_commission_change::<HttpClient>(
+                        &client, ctx, args,
+                    )
+                    .await?;
+                }
                 // Ledger queries
                 Sub::QueryEpoch(QueryEpoch(args)) => {
                     wait_until_node_is_synched(&args.ledger_address).await;

apps/src/lib/cli.rs

@@ -164,6 +164,7 @@ pub mod cmds {
                 .subcommand(Bond::def().display_order(2))
                 .subcommand(Unbond::def().display_order(2))
                 .subcommand(Withdraw::def().display_order(2))
+                .subcommand(TxCommissionRateChange::def().display_order(2))
                 // Queries
                 .subcommand(QueryEpoch::def().display_order(3))
                 .subcommand(QueryTransfers::def().display_order(3))
@@ -198,6 +199,8 @@ pub mod cmds {
                 Self::parse_with_ctx(matches, TxInitProposal);
             let tx_vote_proposal =
                 Self::parse_with_ctx(matches, TxVoteProposal);
+            let tx_commission_rate_change =
+                Self::parse_with_ctx(matches, TxCommissionRateChange);
             let bond = Self::parse_with_ctx(matches, Bond);
             let unbond = Self::parse_with_ctx(matches, Unbond);
             let withdraw = Self::parse_with_ctx(matches, Withdraw);
@@ -232,6 +235,7 @@ pub mod cmds {
                 .or(tx_init_proposal)
                 .or(tx_vote_proposal)
                 .or(tx_init_validator)
+                .or(tx_commission_rate_change)
                 .or(bond)
                 .or(unbond)
                 .or(withdraw)
@@ -294,6 +298,7 @@ pub mod cmds {
         TxUpdateVp(TxUpdateVp),
         TxInitAccount(TxInitAccount),
         TxInitValidator(TxInitValidator),
+        TxCommissionRateChange(TxCommissionRateChange),
         TxInitProposal(TxInitProposal),
         TxVoteProposal(TxVoteProposal),
         TxRevealPk(TxRevealPk),
@@ -1534,6 +1539,32 @@ pub mod cmds {
         }
     }
 
+    #[derive(Clone, Debug)]
+    pub struct TxCommissionRateChange(
+        pub args::CommissionRateChange<args::CliTypes>,
+    );
+
+    impl SubCmd for TxCommissionRateChange {
+        const CMD: &'static str = "change-commission-rate";
+
+        fn parse(matches: &ArgMatches) -> Option<Self>
+        where
+            Self: Sized,
+        {
+            matches.subcommand_matches(Self::CMD).map(|matches| {
+                TxCommissionRateChange(args::CommissionRateChange::parse(
+                    matches,
+                ))
+            })
+        }
+
+        fn def() -> App {
+            App::new(Self::CMD)
+                .about("Change commission raate.")
+                .add_args::<args::CommissionRateChange<args::CliTypes>>()
+        }
+    }
+
     #[derive(Clone, Debug)]
     pub struct TxVoteProposal(pub args::VoteProposal<args::CliTypes>);
 
@@ -3101,11 +3132,11 @@ pub mod args {
         }
     }
 
-    impl CliToSdk<TxCommissionRateChange<SdkTypes>>
-        for TxCommissionRateChange<CliTypes>
+    impl CliToSdk<CommissionRateChange<SdkTypes>>
+        for CommissionRateChange<CliTypes>
     {
-        fn to_sdk(self, ctx: &mut Context) -> TxCommissionRateChange<SdkTypes> {
-            TxCommissionRateChange::<SdkTypes> {
+        fn to_sdk(self, ctx: &mut Context) -> CommissionRateChange<SdkTypes> {
+            CommissionRateChange::<SdkTypes> {
                 tx: self.tx.to_sdk(ctx),
                 validator: ctx.get(&self.validator),
                 rate: self.rate,
@@ -3114,7 +3145,7 @@ pub mod args {
         }
     }
 
-    impl Args for TxCommissionRateChange<CliTypes> {
+    impl Args for CommissionRateChange<CliTypes> {
         fn parse(matches: &ArgMatches) -> Self {
             let tx = Tx::parse(matches);
             let validator = VALIDATOR.parse(matches);

apps/src/lib/client/tx.rs

@@ -23,15 +23,11 @@ use namada::types::dec::Dec;
 use namada::types::governance::{
     OfflineProposal, OfflineVote, Proposal, ProposalVote, VoteType,
 };
-use namada::types::hash::Hash;
 use namada::types::key::*;
 use namada::types::storage::{Epoch, Key};
 use namada::types::token;
-use namada::types::transaction::governance::{
-    InitProposalData, ProposalType, VoteProposalData,
-};
+use namada::types::transaction::governance::{ProposalType, VoteProposalData};
 use namada::types::transaction::{InitValidator, TxType};
-use sha2::{Digest as Sha2Digest, Sha256};
 
 use super::rpc;
 use crate::cli::context::WalletAddress;
@@ -213,16 +209,14 @@ pub async fn submit_init_validator<
     let extra = tx.add_section(Section::ExtraData(Code::from_hash(
         validator_vp_code_hash,
     )));
-    let extra_hash =
-        Hash(extra.hash(&mut Sha256::new()).finalize_reset().into());
     let data = InitValidator {
         account_key,
         consensus_key: consensus_key.ref_to(),
         protocol_key,
         dkg_key,
         commission_rate,
         max_commission_rate_change,
-        validator_vp_code_hash: extra_hash,
+        validator_vp_code_hash: extra.get_hash(),
     };
     let data = data.try_to_vec().expect("Encoding tx data shouldn't fail");
     tx.header.chain_id = tx_args.chain_id.clone().unwrap();
@@ -566,13 +560,14 @@ pub async fn submit_init_proposal<C: namada::ledger::queries::Client + Sync>(
         Ok(())
     } else {
         let signer = ctx.get(&signer);
-        let tx_data: Result<InitProposalData, _> = proposal.clone().try_into();
-        let init_proposal_data = if let Ok(data) = tx_data {
-            data
-        } else {
-            eprintln!("Invalid data for init proposal transaction.");
-            safe_exit(1)
-        };
+        let tx_data = proposal.clone().try_into();
+        let (mut init_proposal_data, init_proposal_content, init_proposal_code) =
+            if let Ok(data) = tx_data {
+                data
+            } else {
+                eprintln!("Invalid data for init proposal transaction.");
+                safe_exit(1)
+            };
 
         let balance =
             rpc::get_token_balance(client, &ctx.native_token, &proposal.author)
@@ -592,22 +587,38 @@ pub async fn submit_init_proposal<C: namada::ledger::queries::Client + Sync>(
             safe_exit(1);
         }
 
-        if init_proposal_data.content.len()
+        if init_proposal_content.len()
             > governance_parameters.max_proposal_content_size as usize
         {
             eprintln!("Proposal content size too big.",);
             safe_exit(1);
         }
 
         let mut tx = Tx::new(TxType::Raw);
-        let data = init_proposal_data
-            .try_to_vec()
-            .expect("Encoding proposal data shouldn't fail");
         let tx_code_hash = query_wasm_code_hash(client, args::TX_INIT_PROPOSAL)
             .await
             .unwrap();
         tx.header.chain_id = ctx.config.ledger.chain_id.clone();
         tx.header.expiration = args.tx.expiration;
+        // Put the content of this proposal into an extra section
+        {
+            let content_sec = tx.add_section(Section::ExtraData(Code::new(
+                init_proposal_content,
+            )));
+            let content_sec_hash = content_sec.get_hash();
+            init_proposal_data.content = content_sec_hash;
+        }
+        // Put any proposal code into an extra section
+        if let Some(init_proposal_code) = init_proposal_code {
+            let code_sec = tx
+                .add_section(Section::ExtraData(Code::new(init_proposal_code)));
+            let code_sec_hash = code_sec.get_hash();
+            init_proposal_data.r#type =
+                ProposalType::Default(Some(code_sec_hash));
+        }
+        let data = init_proposal_data
+            .try_to_vec()
+            .expect("Encoding proposal data shouldn't fail");
         tx.set_data(Data::new(data));
         tx.set_code(Code::from_hash(tx_code_hash));
 
@@ -1036,7 +1047,7 @@ pub async fn submit_validator_commission_change<
 >(
     client: &C,
     mut ctx: Context,
-    mut args: args::TxCommissionRateChange,
+    mut args: args::CommissionRateChange,
 ) -> Result<(), tx::Error> {
     args.tx.chain_id = args
         .tx

apps/src/lib/node/ledger/shell/finalize_block.rs

@@ -159,7 +159,7 @@ where
                 continue;
             }
 
-            let tx = if let Ok(()) = tx.validate_header() {
+            let tx = if tx.validate_tx().is_ok() {
                 tx
             } else {
                 tracing::error!(
@@ -311,7 +311,7 @@ where
                         DecryptedTx::Decrypted { has_valid_pow: _ } => {
                             if let Some(code_sec) = tx
                                 .get_section(tx.code_sechash())
-                                .and_then(Section::code_sec)
+                                .and_then(|x| Section::code_sec(x.as_ref()))
                             {
                                 stats.increment_tx_type(
                                     code_sec.code.hash().to_string(),
@@ -927,6 +927,7 @@ mod test_finalize_block {
     use namada::proto::{Code, Data, Section, Signature};
     use namada::types::dec::POS_DECIMAL_PRECISION;
     use namada::types::governance::ProposalVote;
+    use namada::types::hash::Hash;
     use namada::types::key::tm_consensus_key_raw_hash;
     use namada::types::storage::Epoch;
     use namada::types::time::DurationSecs;
@@ -991,11 +992,11 @@ mod test_finalize_block {
             wrapper.set_code(Code::new(
                 format!("transaction data: {}", i).as_bytes().to_owned(),
             ));
+            wrapper.encrypt(&Default::default());
             wrapper.add_section(Section::Signature(Signature::new(
-                &wrapper.header_hash(),
+                vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
                 &keypair,
             )));
-            wrapper.encrypt(&Default::default());
             if i > 1 {
                 processed_txs.push(ProcessedTx {
                     tx: wrapper.to_bytes(),
@@ -1229,11 +1230,14 @@ mod test_finalize_block {
                     .as_bytes()
                     .to_owned(),
             ));
+            wrapper_tx.encrypt(&Default::default());
             wrapper_tx.add_section(Section::Signature(Signature::new(
-                &wrapper_tx.header_hash(),
+                vec![
+                    wrapper_tx.header_hash(),
+                    wrapper_tx.sections[0].get_hash(),
+                ],
                 &keypair,
             )));
-            wrapper_tx.encrypt(&Default::default());
             valid_txs.push(wrapper_tx.clone());
             processed_txs.push(ProcessedTx {
                 tx: wrapper_tx.to_bytes(),
@@ -1316,7 +1320,7 @@ mod test_finalize_block {
 
             let proposal = InitProposalData {
                 id: Some(proposal_id),
-                content: vec![],
+                content: Hash::default(),
                 author: validator.clone(),
                 voting_start_epoch: Epoch::default(),
                 voting_end_epoch: Epoch::default().next(),
@@ -1327,6 +1331,8 @@ mod test_finalize_block {
             storage_api::governance::init_proposal(
                 &mut shell.wl_storage,
                 proposal,
+                vec![],
+                None,
             )
             .unwrap();
 

apps/src/lib/node/ledger/shell/mod.rs

@@ -793,8 +793,8 @@ where
         }
 
         // Tx signature check
-        let tx_type = match tx.validate_header() {
-            Ok(()) => tx.header(),
+        let tx_type = match tx.validate_tx() {
+            Ok(_) => tx.header(),
             Err(msg) => {
                 response.code = ErrorCodes::InvalidSig.into();
                 response.log = msg.to_string();
@@ -1378,11 +1378,14 @@ mod test_mempool_validate {
         invalid_wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
         invalid_wrapper
             .set_data(Data::new("transaction data".as_bytes().to_owned()));
+        invalid_wrapper.encrypt(&Default::default());
         invalid_wrapper.add_section(Section::Signature(Signature::new(
-            &invalid_wrapper.header_hash(),
+            vec![
+                invalid_wrapper.header_hash(),
+                invalid_wrapper.sections[0].get_hash(),
+            ],
             &keypair,
         )));
-        invalid_wrapper.encrypt(&Default::default());
 
         // we mount a malleability attack to try and remove the fee
         let mut new_wrapper =
@@ -1443,11 +1446,11 @@ mod test_mempool_validate {
         wrapper.header.chain_id = shell.chain_id.clone();
         wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
         wrapper.set_data(Data::new("transaction data".as_bytes().to_owned()));
+        wrapper.encrypt(&Default::default());
         wrapper.add_section(Section::Signature(Signature::new(
-            &wrapper.header_hash(),
+            vec![wrapper.header_hash(), wrapper.sections[0].get_hash()],
             &keypair,
         )));
-        wrapper.encrypt(&Default::default());
 
         // Write wrapper hash to storage
         let wrapper_hash = wrapper.header_hash();
@@ -1539,7 +1542,11 @@ mod test_mempool_validate {
         tx.set_code(Code::new("wasm_code".as_bytes().to_owned()));
         tx.set_data(Data::new("transaction data".as_bytes().to_owned()));
         tx.add_section(Section::Signature(Signature::new(
-            &tx.header_hash(),
+            vec![
+                tx.header_hash(),
+                tx.sections[0].get_hash(),
+                tx.sections[1].get_hash(),
+            ],
             &keypair,
         )));
 
@@ -1570,7 +1577,11 @@ mod test_mempool_validate {
         tx.set_code(Code::new("wasm_code".as_bytes().to_owned()));
         tx.set_data(Data::new("transaction data".as_bytes().to_owned()));
         tx.add_section(Section::Signature(Signature::new(
-            &tx.header_hash(),
+            vec![
+                tx.header_hash(),
+                tx.sections[0].get_hash(),
+                tx.sections[1].get_hash(),
+            ],
             &keypair,
         )));