Merge branch 'grarco/allowlist-fix' (#2819)

* grarco/allowlist-fix:
  Changelog #2819
  Fixes and improves allowlist test
  Improves tx allowlist error. Tx allowlist check in mempool and `process_proposal`
  Moves tx allowlist check to allow replay protection optimizations
  Ensures fee payment even in case of unallowed wasm code
  Updates governance test to cover vp allowlist

.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))

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) => {

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;

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 {

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<D, H>(tx: &Tx, state: &WlState<D, H>) -> 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));
-            }
-        }
-    }
 }