diff --git a/.changelog/unreleased/bug-fixes/2819-allowlist-fix.md b/.changelog/unreleased/bug-fixes/2819-allowlist-fix.md new file mode 100644 index 0000000000..81bfabeb83 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2819-allowlist-fix.md @@ -0,0 +1,2 @@ +- Adjusts the tx allowlist check to not prevent fee payment. + ([\#2819](https://github.com/anoma/namada/pull/2819)) \ No newline at end of file diff --git a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs index 63c644a359..91ff5aa6fb 100644 --- a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -306,24 +306,21 @@ where None }; let tx_gas_meter = RefCell::new(tx_gas_meter); - let tx_result = protocol::check_tx_allowed(&tx, &self.state) - .and_then(|()| { - protocol::dispatch_tx( - tx, - processed_tx.tx.as_ref(), - TxIndex( - tx_index - .try_into() - .expect("transaction index out of bounds"), - ), - &tx_gas_meter, - &mut self.state, - &mut self.vp_wasm_cache, - &mut self.tx_wasm_cache, - wrapper_args.as_mut(), - ) - }) - .map_err(Error::TxApply); + let tx_result = protocol::dispatch_tx( + tx, + processed_tx.tx.as_ref(), + TxIndex( + tx_index + .try_into() + .expect("transaction index out of bounds"), + ), + &tx_gas_meter, + &mut self.state, + &mut self.vp_wasm_cache, + &mut self.tx_wasm_cache, + wrapper_args.as_mut(), + ) + .map_err(Error::TxApply); let tx_gas_meter = tx_gas_meter.into_inner(); match tx_result { Ok(result) => { diff --git a/crates/apps/src/lib/node/ledger/shell/mod.rs b/crates/apps/src/lib/node/ledger/shell/mod.rs index 67587b45d0..a456366d93 100644 --- a/crates/apps/src/lib/node/ledger/shell/mod.rs +++ b/crates/apps/src/lib/node/ledger/shell/mod.rs @@ -10,6 +10,7 @@ mod finalize_block; mod governance; mod init_chain; pub use init_chain::InitChainValidation; +use namada::vm::wasm::run::check_tx_allowed; use namada_sdk::state::StateRead; use namada_sdk::tx::data::GasLimit; pub mod prepare_proposal; @@ -1049,11 +1050,22 @@ where } }, TxType::Wrapper(wrapper) => { + // Tx allowlist + if let Err(err) = check_tx_allowed(&tx, &self.state) { + response.code = ResultCode::TxNotAllowlisted.into(); + response.log = format!( + "{INVALID_MSG}: Wrapper transaction code didn't pass \ + the allowlist checks {}", + err + ); + return response; + } + // Tx gas limit let mut gas_meter = TxGasMeter::new(wrapper.gas_limit); if gas_meter.add_wrapper_gas(tx_bytes).is_err() { response.code = ResultCode::TxGasLimit.into(); - response.log = "{INVALID_MSG}: Wrapper transactions \ + response.log = "{INVALID_MSG}: Wrapper transaction \ exceeds its gas limit" .to_string(); return response; diff --git a/crates/apps/src/lib/node/ledger/shell/process_proposal.rs b/crates/apps/src/lib/node/ledger/shell/process_proposal.rs index 37bbea1198..5b25856ee8 100644 --- a/crates/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/crates/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -90,9 +90,8 @@ where ); // Erroneous transactions were detected when processing - // the leader's proposal. We allow txs that do not - // deserialize properly, that have invalid signatures - // and that have invalid wasm code to reach FinalizeBlock. + // the leader's proposal. We allow txs that are invalid at runtime + // (wasm) to reach FinalizeBlock. let invalid_txs = tx_results.iter().any(|res| { let error = ResultCode::from_u32(res.code).expect( "All error codes returned from process_single_tx are valid", @@ -404,13 +403,7 @@ where } } TxType::Wrapper(wrapper) => { - // Account for gas and space. This is done even if the - // transaction is later deemed invalid, to - // incentivize the proposer to include only - // valid transaction and avoid wasting block - // resources (ABCI only) - - // Account for the tx's resources even in case of an error. + // Account for the tx's resources let allocated_gas = metadata.user_gas.try_dump(u64::from(wrapper.gas_limit)); let mut tx_gas_meter = TxGasMeter::new(wrapper.gas_limit); @@ -424,6 +417,17 @@ where }; } + // Tx allowlist + if let Err(err) = check_tx_allowed(&tx, &self.state) { + return TxResult { + code: ResultCode::TxNotAllowlisted.into(), + info: format!( + "Tx code didn't pass the allowlist check: {}", + err + ), + }; + } + // ChainId check if tx_chain_id != self.chain_id { return TxResult { diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index adaed2fafd..12ee722cef 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -96,8 +96,6 @@ pub enum Error { MaspNativeVpError(native_vp::masp::Error), #[error("Access to an internal address {0:?} is forbidden")] AccessForbidden(InternalAddress), - #[error("Tx is not allowed in allowlist parameter.")] - DisallowedTx, } /// Shell parameters for running wasm transactions. @@ -174,6 +172,7 @@ where CA: 'static + WasmCacheAccess + Sync, { match tx.header().tx_type { + // Raw trasaction type is allowed only for governance proposals TxType::Raw => apply_wasm_tx( tx, &tx_index, @@ -629,29 +628,6 @@ where }) } -/// Returns [`Error::DisallowedTx`] when the given tx is inner (decrypted) tx -/// and its code `Hash` is not included in the `tx_allowlist` parameter. -pub fn check_tx_allowed(tx: &Tx, state: &WlState) -> Result<()> -where - D: 'static + DB + for<'iter> DBIter<'iter> + Sync, - H: 'static + StorageHasher + Sync, -{ - if let TxType::Wrapper(_) = tx.header().tx_type { - if let Some(code_sec) = tx - .get_section(tx.code_sechash()) - .and_then(|x| Section::code_sec(&x)) - { - if crate::parameters::is_tx_allowed(state, &code_sec.code.hash()) - .map_err(Error::StorageError)? - { - return Ok(()); - } - } - return Err(Error::DisallowedTx); - } - Ok(()) -} - /// Apply a derived transaction to storage based on some protocol transaction. /// The logic here must be completely deterministic and will be executed by all /// full nodes every time a protocol transaction is included in a block. Storage @@ -1039,13 +1015,10 @@ mod tests { use borsh::BorshDeserialize; use eyre::Result; - use namada_core::address::testing::nam; use namada_core::ethereum_events::testing::DAI_ERC20_ETH_ADDRESS; use namada_core::ethereum_events::{EthereumEvent, TransferToNamada}; use namada_core::keccak::keccak_hash; - use namada_core::key::RefTo; - use namada_core::storage::{BlockHeight, Epoch}; - use namada_core::token::DenominatedAmount; + use namada_core::storage::BlockHeight; use namada_core::voting_power::FractionalVotingPower; use namada_core::{address, key}; use namada_ethereum_bridge::protocol::transactions::votes::{ @@ -1055,7 +1028,6 @@ mod tests { use namada_ethereum_bridge::storage::proof::EthereumProof; use namada_ethereum_bridge::storage::{vote_tallies, vp}; use namada_ethereum_bridge::test_utils; - use namada_tx::data::Fee; use namada_tx::{SignableEthMessage, Signed}; use namada_vote_ext::bridge_pool_roots::BridgePoolRootVext; use namada_vote_ext::ethereum_events::EthereumEventsVext; @@ -1193,55 +1165,4 @@ mod tests { Ok(()) } - - #[test] - fn test_apply_wasm_tx_allowlist() { - let (mut state, _validators) = test_utils::setup_default_storage(); - let keypair = key::testing::keypair_1(); - let wrapper_tx = WrapperTx::new( - Fee { - amount_per_gas_unit: DenominatedAmount::native( - Amount::from_uint(10, 0).expect("Test failed"), - ), - token: nam(), - }, - keypair.ref_to(), - Epoch(0), - Default::default(), - None, - ); - let mut tx = Tx::from_type(TxType::Wrapper(Box::new(wrapper_tx))); - // pseudo-random code hash - let code = vec![1_u8, 2, 3]; - let tx_hash = Hash::sha256(&code); - tx.set_code(namada_tx::Code::new(code, None)); - - // Check that using a disallowed tx leads to an error - { - let allowlist = vec![format!("{}-bad", tx_hash)]; - crate::parameters::update_tx_allowlist_parameter( - &mut state, allowlist, - ) - .unwrap(); - state.commit_tx(); - - let result = check_tx_allowed(&tx, &state); - assert_matches!(result.unwrap_err(), Error::DisallowedTx); - } - - // Check that using an allowed tx doesn't lead to `Error::DisallowedTx` - { - let allowlist = vec![tx_hash.to_string()]; - crate::parameters::update_tx_allowlist_parameter( - &mut state, allowlist, - ) - .unwrap(); - state.commit_tx(); - - let result = check_tx_allowed(&tx, &state); - if let Err(result) = result { - assert!(!matches!(result, Error::DisallowedTx)); - } - } - } } diff --git a/crates/namada/src/vm/wasm/run.rs b/crates/namada/src/vm/wasm/run.rs index 817e5c6432..7e3ff8be15 100644 --- a/crates/namada/src/vm/wasm/run.rs +++ b/crates/namada/src/vm/wasm/run.rs @@ -9,8 +9,8 @@ use borsh::BorshDeserialize; use namada_core::validity_predicate::VpSentinel; use namada_gas::{GasMetering, TxGasMeter, WASM_MEMORY_PAGE_GAS}; use namada_state::write_log::StorageModification; -use namada_state::{DBIter, State, StateRead, StorageHasher, DB}; -use namada_tx::data::TxSentinel; +use namada_state::{DBIter, State, StateRead, StorageHasher, StorageRead, DB}; +use namada_tx::data::{TxSentinel, TxType}; use namada_tx::{Commitment, Section, Tx}; use parity_wasm::elements; use thiserror::Error; @@ -84,11 +84,37 @@ pub enum Error { ConversionError(String), #[error("Invalid transaction signature")] InvalidTxSignature, + #[error("Storage error: {0}")] + StorageError(String), + #[error("Tx is not allowed in allowlist parameter")] + DisallowedTx, } /// Result for functions that may fail pub type Result = std::result::Result; +/// Returns [`Error::DisallowedTx`] when the given tx is a user tx and its code +/// `Hash` is not included in the `tx_allowlist` parameter. +pub fn check_tx_allowed(tx: &Tx, storage: &S) -> Result<()> +where + S: StorageRead, +{ + if let TxType::Wrapper(_) = tx.header().tx_type { + if let Some(code_sec) = tx + .get_section(tx.code_sechash()) + .and_then(|x| Section::code_sec(&x)) + { + if crate::parameters::is_tx_allowed(storage, &code_sec.code.hash()) + .map_err(|e| Error::StorageError(e.to_string()))? + { + return Ok(()); + } + } + return Err(Error::DisallowedTx); + } + Ok(()) +} + /// Execute a transaction code. Returns the set verifiers addresses requested by /// the transaction. #[allow(clippy::too_many_arguments)] @@ -101,7 +127,7 @@ pub fn tx( tx_wasm_cache: &mut TxCache, ) -> Result> where - S: StateRead + State, + S: StateRead + State + StorageRead, CA: 'static + WasmCacheAccess, { let tx_code = tx @@ -109,6 +135,11 @@ where .and_then(|x| Section::code_sec(x.as_ref())) .ok_or(Error::MissingSection(tx.code_sechash().to_string()))?; + // Check if the tx code is allowed (to be done after the check on the code + // section commitment to let the replay protection mechanism run some + // optimizations) + check_tx_allowed(tx, state)?; + // If the transaction code has a tag, ensure that the tag hash equals the // transaction code's hash. if let Some(tag) = &tx_code.tag { @@ -624,7 +655,8 @@ mod tests { use itertools::Either; use namada_state::StorageWrite; use namada_test_utils::TestWasms; - use namada_tx::data::TxType; + use namada_token::DenominatedAmount; + use namada_tx::data::{Fee, TxType}; use namada_tx::{Code, Data}; use test_log::test; use wasmer_vm::TrapCode; @@ -1283,6 +1315,73 @@ mod tests { assert!(!passed); } + #[test] + fn test_apply_wasm_tx_allowlist() { + let mut state = TestState::default(); + + let tx_read_key = TestWasms::TxReadStorageKey.read_bytes(); + // store the wasm code + let read_code_hash = Hash::sha256(&tx_read_key); + let code_len = (tx_read_key.len() as u64).serialize_to_vec(); + let key = Key::wasm_code(&read_code_hash); + let len_key = Key::wasm_code_len(&read_code_hash); + state.write_bytes(&key, tx_read_key).unwrap(); + state.write_bytes(&len_key, code_len).unwrap(); + + let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); + let mut wrapper_tx = Tx::from_type(TxType::Wrapper(Box::new( + namada_tx::data::WrapperTx::new( + Fee { + amount_per_gas_unit: DenominatedAmount::native(1.into()), + token: state.in_mem().native_token.clone(), + }, + namada_core::key::testing::common_sk_from_simple_seed(0) + .to_public(), + namada_state::Epoch(0), + 0.into(), + None, + ), + ))); + tx.add_code_from_hash(read_code_hash, None); + wrapper_tx.add_code_from_hash(read_code_hash, None); + tx.add_serialized_data(vec![]); + wrapper_tx.add_serialized_data(vec![]); + + // Check that using a disallowed wrapper tx leads to an error, but a raw + // tx is ok even if not allowlisted + { + let allowlist = vec![format!("{}-bad", read_code_hash)]; + crate::parameters::update_tx_allowlist_parameter( + &mut state, allowlist, + ) + .unwrap(); + state.commit_tx(); + + let result = check_tx_allowed(&wrapper_tx, &state); + assert_matches!(result.unwrap_err(), Error::DisallowedTx); + let result = check_tx_allowed(&tx, &state); + if let Err(result) = result { + assert!(!matches!(result, Error::DisallowedTx)); + } + } + + // Check that using an allowed wrapper tx doesn't lead to + // `Error::DisallowedTx` + { + let allowlist = vec![read_code_hash.to_string()]; + crate::parameters::update_tx_allowlist_parameter( + &mut state, allowlist, + ) + .unwrap(); + state.commit_tx(); + + let result = check_tx_allowed(&wrapper_tx, &state); + if let Err(result) = result { + assert!(!matches!(result, Error::DisallowedTx)); + } + } + } + fn execute_tx_with_code(tx_code: Vec) -> Result> { let tx_data = vec![]; let tx_index = TxIndex::default(); diff --git a/crates/tests/src/e2e/ledger_tests.rs b/crates/tests/src/e2e/ledger_tests.rs index e5f25e60c3..7a012f4d88 100644 --- a/crates/tests/src/e2e/ledger_tests.rs +++ b/crates/tests/src/e2e/ledger_tests.rs @@ -1884,20 +1884,24 @@ fn ledger_many_txs_in_a_block() -> Result<()> { Ok(()) } -/// In this test we: -/// 1. Run the ledger node -/// 2. Submit a valid proposal -/// 3. Query the proposal -/// 4. Query token balance (submitted funds) -/// 5. Query governance address balance -/// 6. Submit an invalid proposal -/// 7. Check invalid proposal was not accepted -/// 8. Query token balance (funds shall not be submitted) -/// 9. Send a yay vote from a validator -/// 10. Send a yay vote from a normal user -/// 11. Query the proposal and check the result -/// 12. Wait proposal grace and check proposal author funds -/// 13. Check governance address funds are 0 +// In this test we: +// 1. Run the ledger node +// 2. Submit a valid proposal +// 3. Query the proposal +// 4. Query token balance (submitted funds) +// 5. Query governance address balance +// 6. Submit an invalid proposal +// 7. Check invalid proposal was not accepted +// 8. Query token balance (funds shall not be submitted) +// 9. Send a yay vote from a validator +// 10. Send a yay vote from a normal user +// 11. Query the proposal and check the result +// 12. Wait proposal grace and check proposal author funds +// 13. Check governance address funds are 0 +// 14. Query the new parameters +// 15. Try to initialize a new account which should fail +// 16. Submit a tx that triggers an already existing account which should +// succeed #[test] fn proposal_submission() -> Result<()> { let test = setup::network( @@ -2196,7 +2200,7 @@ fn proposal_submission() -> Result<()> { client.exp_string("nam: 0")?; client.assert_success(); - // // 14. Query parameters + // 14. Query parameters let query_protocol_parameters = vec!["query-protocol-parameters", "--node", &validator_one_rpc]; @@ -2205,6 +2209,49 @@ fn proposal_submission() -> Result<()> { client.exp_regex(".*Min. proposal grace epochs: 9.*")?; client.assert_success(); + // 15. Try to initialize a new account with the no more allowlisted vp + let init_account = vec![ + "init-account", + "--public-keys", + // Value obtained from `namada::core::key::ed25519::tests::gen_keypair` + "tpknam1qpqfzxu3gt05jx2mvg82f4anf90psqerkwqhjey4zlqv0qfgwuvkzt5jhkp", + "--threshold", + "1", + "--code-path", + VP_USER_WASM, + "--alias", + "Test-Account", + "--signing-keys", + BERTHA_KEY, + "--node", + &validator_one_rpc, + ]; + let mut client = run!(test, Bin::Client, init_account, Some(30))?; + client.exp_regex("VP code is not allowed in allowlist parameter")?; + client.assert_success(); + + // 16. Submit a tx touching a previous account with the no more allowlisted + // vp and verify that the transaction succeeds, i.e. the non allowlisted + // vp can still run + let transfer = [ + "transfer", + "--source", + BERTHA, + "--target", + ALBERT, + "--token", + NAM, + "--amount", + "10", + "--signing-keys", + BERTHA_KEY, + "--node", + &validator_one_rpc, + ]; + let mut client = run!(test, Bin::Client, transfer, Some(40))?; + client.exp_string(TX_APPLIED_SUCCESS)?; + client.assert_success(); + Ok(()) } diff --git a/crates/tx/src/data/mod.rs b/crates/tx/src/data/mod.rs index 471535b747..bc9cb95dc9 100644 --- a/crates/tx/src/data/mod.rs +++ b/crates/tx/src/data/mod.rs @@ -63,30 +63,24 @@ pub enum ResultCode { InvalidTx = 2, /// Invalid signature InvalidSig = 3, - /// Tx is in invalid order - InvalidOrder = 4, - /// Tx wasn't expected - ExtraTxs = 5, - /// Undecryptable - Undecryptable = 6, /// The block is full - AllocationError = 7, + AllocationError = 4, /// Replayed tx - ReplayTx = 8, + ReplayTx = 5, /// Invalid chain ID - InvalidChainId = 9, + InvalidChainId = 6, /// Expired tx - ExpiredTx = 10, + ExpiredTx = 7, /// Exceeded gas limit - TxGasLimit = 11, + TxGasLimit = 8, /// Error in paying tx fee - FeeError = 12, + FeeError = 9, /// Invalid vote extension - InvalidVoteExtension = 13, + InvalidVoteExtension = 10, /// Tx is too large - TooLarge = 14, - /// Decrypted tx is expired - ExpiredDecryptedTx = 15, + TooLarge = 11, + /// Tx code is not allowlisted + TxNotAllowlisted = 12, // ========================================================================= // WARN: These codes shouldn't be changed between version! } @@ -99,11 +93,10 @@ impl ResultCode { // NOTE: pattern match on all `ResultCode` variants, in order // to catch potential bugs when adding new codes match self { - Ok | WasmRuntimeError | ExpiredDecryptedTx => true, - InvalidTx | InvalidSig | InvalidOrder | ExtraTxs - | Undecryptable | AllocationError | ReplayTx | InvalidChainId - | ExpiredTx | TxGasLimit | FeeError | InvalidVoteExtension - | TooLarge => false, + Ok | WasmRuntimeError => true, + InvalidTx | InvalidSig | AllocationError | ReplayTx + | InvalidChainId | ExpiredTx | TxGasLimit | FeeError + | InvalidVoteExtension | TooLarge | TxNotAllowlisted => false, } } diff --git a/wasm_for_tests/tx_invalid_data.wasm b/wasm_for_tests/tx_invalid_data.wasm index 7158c51405..9ae23573fc 100755 Binary files a/wasm_for_tests/tx_invalid_data.wasm and b/wasm_for_tests/tx_invalid_data.wasm differ diff --git a/wasm_for_tests/wasm_source/src/lib.rs b/wasm_for_tests/wasm_source/src/lib.rs index 7d65e3d3d7..86ac630133 100644 --- a/wasm_for_tests/wasm_source/src/lib.rs +++ b/wasm_for_tests/wasm_source/src/lib.rs @@ -49,7 +49,7 @@ pub mod main { ctx.write(&target_key, 9_u64)?; // parameters - let target_key = parameters_storage::get_tx_allowlist_storage_key(); + let target_key = parameters_storage::get_vp_allowlist_storage_key(); ctx.write(&target_key, vec!["hash"])?; Ok(()) } @@ -230,10 +230,11 @@ pub mod main { #[transaction(gas = 1000)] fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { let signed = tx_data; - let _data = signed.data().ok_or_err_msg("Missing data").map_err(|err| { - ctx.set_commitment_sentinel(); - err - })?; + let _data = + signed.data().ok_or_err_msg("Missing data").map_err(|err| { + ctx.set_commitment_sentinel(); + err + })?; Ok(()) } }