Merge pull request #1112 from anoma/james/ethbridge/e2e-wrapped-erc20…

…-transfers

Ensure wrapped ERC20 transfers are correctly authorized and add end-to-end tests for them

ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs

@@ -73,7 +73,13 @@ where
     } in transfers
     {
         let mut changed = if asset != &wrapped_native_erc20 {
-            mint_wrapped_erc20s(storage, asset, receiver, amount)?
+            let changed =
+                mint_wrapped_erc20s(storage, asset, receiver, amount)?;
+            tracing::info!(
+                "Minted wrapped ERC20s - (receiver - {receiver}, amount - \
+                 {amount})",
+            );
+            changed
         } else {
             redeem_native_token(storage, receiver, amount)?
         };

shared/src/ledger/native_vp/ethereum_bridge/authorize.rs

@@ -1,37 +1,56 @@
 //! Functionality to do with checking whether a transaction is authorized by the
 //! "owner" of some key under this account
-use eyre::Result;
-use namada_core::types::address::Address;
+use std::collections::BTreeSet;
 
-use crate::ledger::native_vp::StorageReader;
+use namada_core::types::address::Address;
 
+/// For wrapped ERC20 transfers, checks that `verifiers` contains the `sender`'s
+/// address - we delegate to the sender's VP to authorize the transfer (for
+/// regular Namada accounts, this will be `vp_implicit` or `vp_user`).
 pub(super) fn is_authorized(
-    _reader: impl StorageReader,
-    _tx_data: &[u8],
-    _owner: &Address,
-) -> Result<bool> {
-    tracing::warn!(
-        "authorize::is_authorized is not implemented, so all transfers are \
-         authorized"
-    );
-    Ok(true)
+    verifiers: &BTreeSet<Address>,
+    sender: &Address,
+    receiver: &Address,
+) -> bool {
+    verifiers.contains(sender) && verifiers.contains(receiver)
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::ledger::native_vp;
     use crate::types::address;
 
     #[test]
-    fn test_is_authorized_established_address() -> Result<()> {
-        let reader = native_vp::testing::FakeStorageReader::default();
-        let tx_data = vec![];
-        let owner = address::testing::established_address_1();
+    fn test_is_authorized_passes() {
+        let sender = address::testing::established_address_1();
+        let receiver = address::testing::established_address_2();
+        let verifiers = BTreeSet::from([sender.clone(), receiver.clone()]);
 
-        let authorized = is_authorized(reader, &tx_data, &owner)?;
+        let authorized = is_authorized(&verifiers, &sender, &receiver);
 
         assert!(authorized);
-        Ok(())
+    }
+
+    #[test]
+    fn test_is_authorized_fails() {
+        let sender = address::testing::established_address_1();
+        let receiver = address::testing::established_address_2();
+        let verifiers = BTreeSet::default();
+
+        let authorized = is_authorized(&verifiers, &sender, &receiver);
+
+        assert!(!authorized);
+
+        let verifiers = BTreeSet::from([sender.clone()]);
+
+        let authorized = is_authorized(&verifiers, &sender, &receiver);
+
+        assert!(!authorized);
+
+        let verifiers = BTreeSet::from([receiver.clone()]);
+
+        let authorized = is_authorized(&verifiers, &sender, &receiver);
+
+        assert!(!authorized);
     }
 }

shared/src/ledger/native_vp/ethereum_bridge/bridge_pool_vp.rs

@@ -182,8 +182,12 @@ where
 }
 
 /// Check if a delta matches the delta given by a transfer
-fn check_delta(delta: &(Address, Amount), transfer: &PendingTransfer) -> bool {
-    delta.0 == transfer.transfer.sender && delta.1 == transfer.transfer.amount
+fn check_delta(
+    sender: &Address,
+    amount: &Amount,
+    transfer: &PendingTransfer,
+) -> bool {
+    *sender == transfer.transfer.sender && *amount == transfer.transfer.amount
 }
 
 /// Helper struct for handling the different escrow
@@ -319,7 +323,8 @@ where
                 (&escrow_key).try_into().expect("This should not fail"),
                 (&owner_key).try_into().expect("This should not fail"),
             ) {
-                Ok(Some(delta)) if check_delta(&delta, &transfer) => {}
+                Ok(Some((sender, _, amount)))
+                    if check_delta(&sender, &amount, &transfer) => {}
                 other => {
                     tracing::debug!(
                         "The assets of the transfer were not properly \

shared/src/ledger/native_vp/ethereum_bridge/vp.rs

@@ -142,13 +142,28 @@ where
             Some(CheckType::Escrow) => return self.check_escrow(verifiers),
             None => return Ok(false),
         };
-        let (sender, _) = match check_balance_changes(&self.ctx, key_a, key_b)?
-        {
-            Some(sender) => sender,
-            None => return Ok(false),
-        };
-        let authed = authorize::is_authorized(&self.ctx, tx_data, &sender)?;
-        Ok(authed)
+        let (sender, receiver, _) =
+            match check_balance_changes(&self.ctx, key_a, key_b)? {
+                Some(sender) => sender,
+                None => return Ok(false),
+            };
+        if authorize::is_authorized(verifiers, &sender, &receiver) {
+            tracing::info!(
+                ?verifiers,
+                ?sender,
+                ?receiver,
+                "Ethereum Bridge VP authorized transfer"
+            );
+            Ok(true)
+        } else {
+            tracing::info!(
+                ?verifiers,
+                ?sender,
+                ?receiver,
+                "Ethereum Bridge VP rejected unauthorized transfer"
+            );
+            Ok(false)
+        }
     }
 }
 
@@ -230,14 +245,18 @@ fn determine_check_type(
 /// Checks that the balances at both `key_a` and `key_b` have changed by some
 /// amount, and that the changes balance each other out. If the balance changes
 /// are invalid, the reason is logged and a `None` is returned. Otherwise,
-/// return the `Address` of the owner of the balance which is decreasing,
-/// and by how much it decreased, which should be authorizing the balance
-/// change.
+/// return:
+/// - the `Address` of the sender i.e. the owner of the balance which is
+///   decreasing
+/// - the `Address` of the receiver i.e. the owner of the balance which is
+///   increasing
+/// - the `Amount` of the transfer i.e. by how much the sender's balance
+///   decreased, or equivalently by how much the receiver's balance increased
 pub(super) fn check_balance_changes(
     reader: impl StorageReader,
     key_a: wrapped_erc20s::Key,
     key_b: wrapped_erc20s::Key,
-) -> Result<Option<(Address, Amount)>> {
+) -> Result<Option<(Address, Address, Amount)>> {
     let (balance_a, balance_b) =
         match (key_a.suffix.clone(), key_b.suffix.clone()) {
             (
@@ -342,9 +361,13 @@ pub(super) fn check_balance_changes(
     }
 
     if balance_a_delta < 0 {
-        if let wrapped_erc20s::KeyType::Balance { owner } = key_a.suffix {
+        if let wrapped_erc20s::KeyType::Balance { owner: sender } = key_a.suffix
+        {
+            let wrapped_erc20s::KeyType::Balance { owner: receiver } =
+                key_b.suffix else { unreachable!() };
             Ok(Some((
-                owner,
+                sender,
+                receiver,
                 Amount::from(
                     u64::try_from(balance_b_delta)
                         .expect("This should not fail"),
@@ -355,9 +378,13 @@ pub(super) fn check_balance_changes(
         }
     } else {
         assert!(balance_b_delta < 0);
-        if let wrapped_erc20s::KeyType::Balance { owner } = key_b.suffix {
+        if let wrapped_erc20s::KeyType::Balance { owner: sender } = key_b.suffix
+        {
+            let wrapped_erc20s::KeyType::Balance { owner: receiver } =
+                key_a.suffix else { unreachable!() };
             Ok(Some((
-                owner,
+                sender,
+                receiver,
                 Amount::from(
                     u64::try_from(balance_a_delta)
                         .expect("This should not fail"),