From cab0561a66c907f67f990135dbfd7976965a9f46 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Wed, 13 Sep 2023 15:49:45 +0100 Subject: [PATCH 1/3] Validate txs moving 0 value to the Bridge pool --- .../ethereum_bridge/bridge_pool_vp.rs | 204 +++++++++++++++++- 1 file changed, 197 insertions(+), 7 deletions(-) diff --git a/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs b/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs index 89b1b33253..22fee8b2f0 100644 --- a/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs +++ b/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs @@ -412,17 +412,59 @@ struct EscrowDelta<'a, KIND> { _kind: PhantomData<*const KIND>, } -impl EscrowDelta<'_, KIND> { - fn validate_changed_keys(&self, changed_keys: &BTreeSet) -> bool { +impl EscrowDelta<'_, TokenCheck> { + fn validate_changed_keys( + &self, + changed_keys: &BTreeSet, + pending: &PendingTransfer, + ) -> bool { + let EscrowDelta { + token, + payer_account, + escrow_account, + .. + } = self; + + let owner_key = balance_key(token, payer_account); + let escrow_key = balance_key(token, escrow_account); + + let has_owner_key = changed_keys.contains(&owner_key); + let has_escrow_key = changed_keys.contains(&escrow_key); + + let amount_is_zero = pending.transfer.amount.is_zero(); + + hints::likely(!amount_is_zero && has_owner_key && has_escrow_key) + || hints::unlikely( + amount_is_zero && !has_owner_key && !has_escrow_key, + ) + } +} + +impl EscrowDelta<'_, GasCheck> { + fn validate_changed_keys( + &self, + changed_keys: &BTreeSet, + pending: &PendingTransfer, + ) -> bool { let EscrowDelta { token, payer_account, escrow_account, .. } = self; + let owner_key = balance_key(token, payer_account); let escrow_key = balance_key(token, escrow_account); - changed_keys.contains(&owner_key) && changed_keys.contains(&escrow_key) + + let has_owner_key = changed_keys.contains(&owner_key); + let has_escrow_key = changed_keys.contains(&escrow_key); + + let amount_is_zero = pending.gas_fee.amount.is_zero(); + + hints::likely(!amount_is_zero && has_owner_key && has_escrow_key) + || hints::unlikely( + amount_is_zero && !has_owner_key && !has_escrow_key, + ) } } @@ -437,9 +479,15 @@ struct EscrowCheck<'a> { impl EscrowCheck<'_> { #[inline] - fn validate_changed_keys(&self, changed_keys: &BTreeSet) -> bool { - self.gas_check.validate_changed_keys(changed_keys) - && self.token_check.validate_changed_keys(changed_keys) + fn validate_changed_keys( + &self, + changed_keys: &BTreeSet, + pending: &PendingTransfer, + ) -> bool { + self.gas_check.validate_changed_keys(changed_keys, pending) + && self + .token_check + .validate_changed_keys(changed_keys, pending) } } @@ -541,7 +589,7 @@ where let wnam_address = read_native_erc20_address(&self.ctx.pre())?; let escrow_checks = self.determine_escrow_checks(&wnam_address, &transfer)?; - if !escrow_checks.validate_changed_keys(keys_changed) { + if !escrow_checks.validate_changed_keys(keys_changed, &transfer) { tracing::debug!( ?transfer, "Missing storage modifications in the Bridge pool" @@ -1849,4 +1897,146 @@ mod test_bridge_pool_vp { Expect::True, ); } + + /// Test that the Bridge pool native VP validates transfers that + /// do not contain gas fees and no associated changed keys. + #[test] + fn test_no_gas_fees_with_no_changed_keys() { + let nam_addr = nam(); + let pending = PendingTransfer { + transfer: TransferToEthereum { + kind: TransferToEthereumKind::Erc20, + asset: wnam(), + sender: bertha_address(), + recipient: EthAddress([1; 20]), + amount: Amount::from(1u64), + }, + gas_fee: GasFee { + token: nam(), + // NOTE: testing 0 amount + amount: Amount::zero(), + payer: bertha_address(), + }, + }; + let delta = EscrowDelta { + token: Cow::Borrowed(&nam_addr), + payer_account: &bertha_address(), + escrow_account: &BRIDGE_ADDRESS, + expected_debit: Amount::zero(), + expected_credit: Amount::zero(), + // NOTE: testing gas fees + _kind: PhantomData::<*const GasCheck>, + }; + // NOTE: testing no changed keys + let empty_keys = BTreeSet::new(); + + assert!(delta.validate_changed_keys(&empty_keys, &pending)); + } + + /// Test that the Bridge pool native VP rejects transfers that + /// do not contain gas fees and has associated changed keys. + #[test] + fn test_no_gas_fees_with_changed_keys() { + let nam_addr = nam(); + let pending = PendingTransfer { + transfer: TransferToEthereum { + kind: TransferToEthereumKind::Erc20, + asset: wnam(), + sender: bertha_address(), + recipient: EthAddress([1; 20]), + amount: Amount::from(1u64), + }, + gas_fee: GasFee { + token: nam(), + // NOTE: testing 0 amount + amount: Amount::zero(), + payer: bertha_address(), + }, + }; + let delta = EscrowDelta { + token: Cow::Borrowed(&nam_addr), + payer_account: &bertha_address(), + escrow_account: &BRIDGE_ADDRESS, + expected_debit: Amount::zero(), + expected_credit: Amount::zero(), + // NOTE: testing gas fees + _kind: PhantomData::<*const GasCheck>, + }; + let owner_key = balance_key(&nam_addr, &bertha_address()); + // NOTE: testing changed keys + let some_changed_keys = BTreeSet::from([owner_key]); + + assert!(!delta.validate_changed_keys(&some_changed_keys, &pending)); + } + + /// Test that the Bridge pool native VP validates transfers + /// moving no value and with no associated changed keys. + #[test] + fn test_no_amount_with_no_changed_keys() { + let nam_addr = nam(); + let pending = PendingTransfer { + transfer: TransferToEthereum { + kind: TransferToEthereumKind::Erc20, + asset: wnam(), + sender: bertha_address(), + recipient: EthAddress([1; 20]), + // NOTE: testing 0 amount + amount: Amount::zero(), + }, + gas_fee: GasFee { + token: nam(), + amount: Amount::from(1u64), + payer: bertha_address(), + }, + }; + let delta = EscrowDelta { + token: Cow::Borrowed(&nam_addr), + payer_account: &bertha_address(), + escrow_account: &BRIDGE_ADDRESS, + expected_debit: Amount::zero(), + expected_credit: Amount::zero(), + // NOTE: testing token transfers + _kind: PhantomData::<*const TokenCheck>, + }; + // NOTE: testing no changed keys + let empty_keys = BTreeSet::new(); + + assert!(delta.validate_changed_keys(&empty_keys, &pending)); + } + + /// Test that the Bridge pool native VP rejects transfers + /// moving no value and with associated changed keys. + #[test] + fn test_no_amount_with_changed_keys() { + let nam_addr = nam(); + let pending = PendingTransfer { + transfer: TransferToEthereum { + kind: TransferToEthereumKind::Erc20, + asset: wnam(), + sender: bertha_address(), + recipient: EthAddress([1; 20]), + // NOTE: testing 0 amount + amount: Amount::zero(), + }, + gas_fee: GasFee { + token: nam(), + amount: Amount::from(1u64), + payer: bertha_address(), + }, + }; + let delta = EscrowDelta { + token: Cow::Borrowed(&nam_addr), + payer_account: &bertha_address(), + escrow_account: &BRIDGE_ADDRESS, + expected_debit: Amount::zero(), + expected_credit: Amount::zero(), + // NOTE: testing token transfers + _kind: PhantomData::<*const TokenCheck>, + }; + let owner_key = balance_key(&nam_addr, &bertha_address()); + // NOTE: testing changed keys + let some_changed_keys = BTreeSet::from([owner_key]); + + assert!(!delta.validate_changed_keys(&some_changed_keys, &pending)); + } } From ce407a837e0af1a1a9007a8cf7921aff623d00ec Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Thu, 14 Sep 2023 13:18:12 +0100 Subject: [PATCH 2/3] Refactor validate_changed_keys in the Bridge pool VP --- .../ethereum_bridge/bridge_pool_vp.rs | 155 ++++++------------ 1 file changed, 54 insertions(+), 101 deletions(-) diff --git a/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs b/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs index 22fee8b2f0..9853a1daca 100644 --- a/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs +++ b/shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs @@ -387,6 +387,7 @@ where escrow_account: &BRIDGE_POOL_ADDRESS, expected_debit: expected_gas_debit, expected_credit: expected_gas_credit, + transferred_amount: &transfer.gas_fee.amount, _kind: PhantomData, }, token_check: EscrowDelta { @@ -395,6 +396,7 @@ where escrow_account: token_check_escrow_acc, expected_debit: expected_token_debit, expected_credit: expected_token_credit, + transferred_amount: &transfer.transfer.amount, _kind: PhantomData, }, }) @@ -409,15 +411,31 @@ struct EscrowDelta<'a, KIND> { escrow_account: &'a Address, expected_debit: Amount, expected_credit: Amount, + transferred_amount: &'a Amount, _kind: PhantomData<*const KIND>, } -impl EscrowDelta<'_, TokenCheck> { - fn validate_changed_keys( - &self, - changed_keys: &BTreeSet, - pending: &PendingTransfer, - ) -> bool { +impl EscrowDelta<'_, KIND> { + /// Validate an [`EscrowDelta`]. + /// + /// # Conditions for validation + /// + /// If the transferred amount in the [`EscrowDelta`] is nil, + /// then no keys could have been changed. If the transferred + /// amount is greater than zero, then the appropriate escrow + /// keys must have been written to by some wasm tx. + #[inline] + fn validate(&self, changed_keys: &BTreeSet) -> bool { + if hints::unlikely(self.transferred_amount_is_nil()) { + self.check_escrow_keys_unchanged(changed_keys) + } else { + self.check_escrow_keys_changed(changed_keys) + } + } + + /// Check if all required escrow keys in `changed_keys` were modified. + #[inline] + fn check_escrow_keys_changed(&self, changed_keys: &BTreeSet) -> bool { let EscrowDelta { token, payer_account, @@ -428,23 +446,14 @@ impl EscrowDelta<'_, TokenCheck> { let owner_key = balance_key(token, payer_account); let escrow_key = balance_key(token, escrow_account); - let has_owner_key = changed_keys.contains(&owner_key); - let has_escrow_key = changed_keys.contains(&escrow_key); - - let amount_is_zero = pending.transfer.amount.is_zero(); - - hints::likely(!amount_is_zero && has_owner_key && has_escrow_key) - || hints::unlikely( - amount_is_zero && !has_owner_key && !has_escrow_key, - ) + changed_keys.contains(&owner_key) && changed_keys.contains(&escrow_key) } -} -impl EscrowDelta<'_, GasCheck> { - fn validate_changed_keys( + /// Check if no escrow keys in `changed_keys` were modified. + #[inline] + fn check_escrow_keys_unchanged( &self, changed_keys: &BTreeSet, - pending: &PendingTransfer, ) -> bool { let EscrowDelta { token, @@ -456,15 +465,17 @@ impl EscrowDelta<'_, GasCheck> { let owner_key = balance_key(token, payer_account); let escrow_key = balance_key(token, escrow_account); - let has_owner_key = changed_keys.contains(&owner_key); - let has_escrow_key = changed_keys.contains(&escrow_key); - - let amount_is_zero = pending.gas_fee.amount.is_zero(); + !changed_keys.contains(&owner_key) + && !changed_keys.contains(&escrow_key) + } - hints::likely(!amount_is_zero && has_owner_key && has_escrow_key) - || hints::unlikely( - amount_is_zero && !has_owner_key && !has_escrow_key, - ) + /// Check if the amount transferred to escrow is nil. + #[inline] + fn transferred_amount_is_nil(&self) -> bool { + let EscrowDelta { + transferred_amount, .. + } = self; + transferred_amount.is_zero() } } @@ -479,15 +490,9 @@ struct EscrowCheck<'a> { impl EscrowCheck<'_> { #[inline] - fn validate_changed_keys( - &self, - changed_keys: &BTreeSet, - pending: &PendingTransfer, - ) -> bool { - self.gas_check.validate_changed_keys(changed_keys, pending) - && self - .token_check - .validate_changed_keys(changed_keys, pending) + fn validate(&self, changed_keys: &BTreeSet) -> bool { + self.gas_check.validate(changed_keys) + && self.token_check.validate(changed_keys) } } @@ -589,7 +594,7 @@ where let wnam_address = read_native_erc20_address(&self.ctx.pre())?; let escrow_checks = self.determine_escrow_checks(&wnam_address, &transfer)?; - if !escrow_checks.validate_changed_keys(keys_changed, &transfer) { + if !escrow_checks.validate(keys_changed) { tracing::debug!( ?transfer, "Missing storage modifications in the Bridge pool" @@ -1903,34 +1908,21 @@ mod test_bridge_pool_vp { #[test] fn test_no_gas_fees_with_no_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - amount: Amount::from(1u64), - }, - gas_fee: GasFee { - token: nam(), - // NOTE: testing 0 amount - amount: Amount::zero(), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing gas fees _kind: PhantomData::<*const GasCheck>, }; // NOTE: testing no changed keys let empty_keys = BTreeSet::new(); - assert!(delta.validate_changed_keys(&empty_keys, &pending)); + assert!(delta.validate(&empty_keys)); } /// Test that the Bridge pool native VP rejects transfers that @@ -1938,27 +1930,14 @@ mod test_bridge_pool_vp { #[test] fn test_no_gas_fees_with_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - amount: Amount::from(1u64), - }, - gas_fee: GasFee { - token: nam(), - // NOTE: testing 0 amount - amount: Amount::zero(), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing gas fees _kind: PhantomData::<*const GasCheck>, }; @@ -1966,7 +1945,7 @@ mod test_bridge_pool_vp { // NOTE: testing changed keys let some_changed_keys = BTreeSet::from([owner_key]); - assert!(!delta.validate_changed_keys(&some_changed_keys, &pending)); + assert!(!delta.validate(&some_changed_keys)); } /// Test that the Bridge pool native VP validates transfers @@ -1974,34 +1953,21 @@ mod test_bridge_pool_vp { #[test] fn test_no_amount_with_no_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - // NOTE: testing 0 amount - amount: Amount::zero(), - }, - gas_fee: GasFee { - token: nam(), - amount: Amount::from(1u64), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing token transfers _kind: PhantomData::<*const TokenCheck>, }; // NOTE: testing no changed keys let empty_keys = BTreeSet::new(); - assert!(delta.validate_changed_keys(&empty_keys, &pending)); + assert!(delta.validate(&empty_keys)); } /// Test that the Bridge pool native VP rejects transfers @@ -2009,27 +1975,14 @@ mod test_bridge_pool_vp { #[test] fn test_no_amount_with_changed_keys() { let nam_addr = nam(); - let pending = PendingTransfer { - transfer: TransferToEthereum { - kind: TransferToEthereumKind::Erc20, - asset: wnam(), - sender: bertha_address(), - recipient: EthAddress([1; 20]), - // NOTE: testing 0 amount - amount: Amount::zero(), - }, - gas_fee: GasFee { - token: nam(), - amount: Amount::from(1u64), - payer: bertha_address(), - }, - }; let delta = EscrowDelta { token: Cow::Borrowed(&nam_addr), payer_account: &bertha_address(), escrow_account: &BRIDGE_ADDRESS, expected_debit: Amount::zero(), expected_credit: Amount::zero(), + // NOTE: testing 0 amount + transferred_amount: &Amount::zero(), // NOTE: testing token transfers _kind: PhantomData::<*const TokenCheck>, }; @@ -2037,6 +1990,6 @@ mod test_bridge_pool_vp { // NOTE: testing changed keys let some_changed_keys = BTreeSet::from([owner_key]); - assert!(!delta.validate_changed_keys(&some_changed_keys, &pending)); + assert!(!delta.validate(&some_changed_keys)); } } From a524dfaa593cdad513a5f5859d4975ac9abc6f3e Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Wed, 13 Sep 2023 15:53:58 +0100 Subject: [PATCH 3/3] Changelog for #1892 --- .../unreleased/improvements/1892-bridge-pool-zero-fees.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/1892-bridge-pool-zero-fees.md diff --git a/.changelog/unreleased/improvements/1892-bridge-pool-zero-fees.md b/.changelog/unreleased/improvements/1892-bridge-pool-zero-fees.md new file mode 100644 index 0000000000..83ff3dd0f8 --- /dev/null +++ b/.changelog/unreleased/improvements/1892-bridge-pool-zero-fees.md @@ -0,0 +1,2 @@ +- Allow Bridge pool transfers to pay zero gas fees + ([\#1892](https://github.com/anoma/namada/pull/1892)) \ No newline at end of file