Murisi/masp ibc replay protection using txdata on 0.39.0 #3409

murisi · 2024-06-12T19:16:04Z

Describe your changes

An attempt to address issue #3300. The approach taken is to include the IBC receiver in the pre-image of shielded Transaction outputs. More specifically, the following changes have been made:

The target of outgoing shielded actions is the hash of the IBC receiver (plus a 1 byte tag indicating an IBC recipient)
- For non-IBC receivers, we just prepend a 1 byte tag to the target Address indicating a non-IBC recipient
Modified IBC transactions to use the raw integer representation of amounts rather than the human readable decimal representation. This involved:
- Modifying the client to be able to derive the denominations of IBC tokens. This is done by essentially looking up the base token corresponding to an IBC token and finding the denomination of that
- Making the protocol return no denomination for IBC token instead of a zero
- This was necessary because MASP uses raw amounts, so we need to make sure that MASP-IBC computations utilize the same units
Modifying the MASP validity predicate to check MASP transaction inputs and outputs against IBC packets. This involved:
- Deriving the token Address that corresponds to an transfer PacketData and checking that the transparent inputs and outputs use that
- Checking that the transparent outputs of a MASP Transaction go to the receiver stated in PacketData in the case of an outgoing shielded action
- Checking that the transparent inputs of a MASP Transaction are the IBC internal address in the case of a refund or an incoming shielded action

This PR was derived from merging Namada 0.39.0 into #3406 .

Closes #3300

Indicate on which release or other PRs this topic is based on

Namada v0.39.0
Patched Hermes 1.8.2: heliaxdev/hermes#34

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

…ncodings are consistent.

codecov · 2024-06-12T19:48:27Z

Codecov Report

Attention: Patch coverage is 1.71149% with 402 lines in your changes missing coverage. Please review.

Project coverage is 53.77%. Comparing base (879a326) to head (7ff94bd).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/ledger/native_vp/masp.rs	0.00%	285 Missing ⚠️
crates/core/src/masp.rs	0.00%	41 Missing ⚠️
crates/sdk/src/rpc.rs	0.00%	35 Missing ⚠️
crates/sdk/src/masp.rs	0.00%	10 Missing ⚠️
crates/vp_env/src/lib.rs	0.00%	10 Missing ⚠️
crates/ibc/src/storage.rs	0.00%	8 Missing ⚠️
crates/core/src/token.rs	0.00%	5 Missing ⚠️
crates/node/src/bench_utils.rs	0.00%	3 Missing ⚠️
crates/shielded_token/src/conversion.rs	0.00%	2 Missing ⚠️
crates/ibc/src/actions.rs	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3409      +/-   ##
==========================================
- Coverage   53.92%   53.77%   -0.16%     
==========================================
  Files         317      317              
  Lines      107575   107807     +232     
==========================================
- Hits        58011    57970      -41     
- Misses      49564    49837     +273

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ta-on-0.39.0

cwgoes

The concept makes sense; I'm wondering if we can clean up the ChangedBalances calculation somewhat - it's very hard to follow.

crates/core/src/masp.rs

cwgoes · 2024-06-13T11:13:21Z

crates/namada/src/ledger/native_vp/masp.rs

@@ -279,66 +295,343 @@ where
        Ok(())
    }

+    /// Look up the IBC denomination from a IbcToken.


Should this function be defined in the ibc crate instead, and imported here? Seems like we might want to use it in multiple places (and I wonder if it is in fact already defined somewhere...)

You're right, this function is already implemented in the client (where the queries are sent to an RPC node). I'll investigate whether the protocol version here (which reads storage directly) can somehow reuse this logic.

I've discussed turning this function into an RPC endpoint with @tzemanovic . Doing so seems suboptimal because this function is a essentially a heuristic for guessing IBC denominations from IbcToken hashes based on the results the protocol has cached so far. Also it seems to me that somehow using generics to share this logic would be overkill for this limited issue. Do you think there's a way to eliminate the use of this function in the MASP VP @yito88 ?

crates/namada/src/ledger/native_vp/masp.rs

cwgoes · 2024-06-13T11:19:20Z

crates/namada/src/ledger/native_vp/masp.rs

+    }
+
+    // Apply the balance change to the changed balances structure
+    fn apply_balance_change(


Could all these functions simply be exported by the ibc crate? Also, could they be combined into one which takes all the state changes and returns the ChangedBalances for this transaction which the MASP VP needs?

Moving the ChangedBalances functions into the namada_ibc crate is problematic because those functions depend on the namada_vp_env crate (which itself depends on the namada_ibc crate). The latter dependency is due to those functions doing pre and post balance reads as well as IBC trace reads and packet commitment reads from storage. I suspect I'd have to restructure these functions in order to reduce their dependence on validity predicate functions first. Should I attempt restructuring those functions in this PR?

(discussed offline - let's do this separately)

crates/namada/src/ledger/native_vp/masp.rs

yito88 · 2024-06-13T11:51:29Z

crates/namada/src/ledger/native_vp/masp.rs

+                let receiver = msg.message.packet_data.receiver.to_string();
+                let addr = TAddrData::Ibc(receiver.clone());
+                acc.decoder.insert(ibc_taddr(receiver), addr);
+                self.apply_send_packet(&mut acc, msg, keys_changed)?;


We could generalize apply_send_packet for Transfer and NftTransfer

Agreed. I was just a bit reluctant to do this now because the NFT E2E tests are not a part of the tests crate in the Namada repo and require some additional setup which I've not done yet. Do you think you could get this done more quickly/efficiently than me and open up a separate PR to generalize Transfer to NftTransfer? If not, then I can start working on this...

I hope this could be helpful: #3422

Brilliant, thanks! Did you manage to run the NFT E2E tests on this? Just want to clarify #3409 (comment) , then I'll merge this in.

To be honest, I didn't try the NFT E2E test. I will test it after testing fungible token transfers in this PR.
Also, I'll release the updated Hermes for this PR to test in CI.

Ah, I see thanks. I've partially applied #3422 where it pertains to determining a token Address from a packet denom. However I just wanted to clarify whether or not logic to receive NFT messages is missing before applying the rest of the PR. See https://github.com/anoma/namada/pull/3422/files#r1645836855 . Thanks.

@yito88 For lack of time could we include this NFT support in the next release after the one today? If so, did you get a chance to test the fungible token transfers in this PR? Were there any issues that need urgent addressing?

cwgoes · 2024-06-17T07:44:56Z

crates/namada/src/ledger/native_vp/masp.rs

+            Ok(Address::Internal(InternalAddress::IbcToken(hash))) => {
+                hash.to_string()
+            }
+            _ => return Ok(token.as_ref().to_string()),


Are we sure that the token in this case is really the denom that we're looking for?

See #3409 (comment) .

cwgoes · 2024-06-17T07:45:42Z

crates/namada/src/ledger/native_vp/masp.rs

+            _ => return Ok(token.as_ref().to_string()),
+        };
+
+        if let Some(owner) = owner {


Why do we need to know the owner to query the IBC denomination at all?

I need to investigate this. For the purposes of validation, this function almost exactly mirrors the computation done during the construction of IBC transactions in the client. Compare this to

namada/crates/sdk/src/rpc.rs

Line 1406 in 08e5664

/// Look up the IBC denomination from a IbcToken.

, which is invoked at

namada/crates/sdk/src/tx.rs

Line 2583 in 08e5664

rpc::query_ibc_denom(context, &args.token.to_string(), Some(&source))

. cc @yito88

@yito88 can you clarify here? I'm quite confused about this

query_ibc_denom is for looking for readable IBC denom with IbcToken hash.
Using an owner was to avoid looking all IBC denoms up when the balance query didn't require --token. (Now the query requires both the token and the owner. The RPC function is left for now.)
I think we don't need the query in MASP VP because the IBC denom should be in the packet and the token address can be calculated from it.

For context, the token address for a receive packet is computed here in the MASP VP: https://github.com/anoma/namada/blob/4de6577c73e223504b0fcb226257ef7b85e248a7/crates/namada/src/ledger/native_vp/masp.rs#L483C25-L483C55 . This computation was designed to produce the same shielded token as the one produced by Hermes, which can be found at

namada/crates/sdk/src/tx.rs

Line 3505 in 5cc23b4

// Need to check the prefix

I think we don't need the query in MASP VP because the IBC denom should be in the packet and the token address can be calculated from it.

Thanks for the clarification @yito88 . Would this same reasoning apply to how Hermes derives the token from the IBC denom (also using query_ibc_denom and received_ibc_token)? Or is the context of the Hermes computation somehow different?

query_ibc_denom looks stored IBC denoms up. received_ibc_token returns the IBC denom to be minted/unescrowed. Hermes doesn't derive the token. I think you meant gen_ibc_shielding_transfer. It uses received_ibc_token because Hermes will attach the shielding transfer for the receiving token.

cwgoes · 2024-06-17T07:46:41Z

crates/namada/src/ledger/native_vp/masp.rs

+                &message.timeout_height_on_b,
+                &message.timeout_timestamp_on_b,
+            );
+        // Try to find a key change with the same port, channel, and commitment


Is there a reason we can't just construct the path directly and read from it (instead of iterating)? What data are we missing?

We're missing the sequence number. Also, Yuji suggested a different approach:

packet sequence can be obtained from the state with read_pre and the key next_sequence_*_key because the transaction has only one IBC message.

crates/namada/src/ledger/native_vp/masp.rs

crates/sdk/src/masp.rs

crates/namada/src/ledger/native_vp/masp.rs

yito88

I think we could make it a bit simpler because we can assume that the validated transaction has only one IBC transfer.

crates/namada/src/ledger/native_vp/masp.rs

crates/vp_env/src/lib.rs

crates/namada/src/ledger/native_vp/masp.rs

grarco · 2024-06-17T17:19:40Z

crates/namada/src/ledger/native_vp/masp.rs

+            .or_insert(ValueSum::zero());
+        // Enable funds to be received by the receiver in the
+        // IBC packet
+        *post_entry =


I guess the idea here is to mock a balance change to an address that we are not aware of on this chain (based on the ibc msg) so that then we can verify the matching between this ibc msg packet and masp transparent output with validate_transparent_output, correct?

Also, are we validating the optional ShieldingTransfer that the receiving chain gets? I see that this message is built in build_ibc_transfer and we also link the hash of the unshielding masp section of the source chain. The method execute called by the ibc native vp returns this data but it seems like we never use it. This is probably a question for @yito88

I guess the idea here is to mock a balance change to an address that we are not aware of on this chain (based on the ibc msg) so that then we can verify the matching between this ibc msg packet and masp transparent output with validate_transparent_output, correct?

Yes, exactly.

The method execute called by the ibc native vp returns this data but it seems like we never use it.

Right, IBC actions never use ShieldingTransfer.

@yito88 is this ok? Do we have a way to verify that the relayer is indeed forwarding the correct ShieldingTransfer?

Ah, the IBC VP has to validate the ShieldingTransfer for the given IBC message.
I'll open another PR.

…ery_denom.

…ts no longer used.

…f the success acknowledgement.

crates/core/src/masp.rs

cwgoes · 2024-06-20T12:19:15Z

crates/namada/src/ledger/native_vp/masp.rs

+            _ => return Ok(token.as_ref().to_string()),
+        };
+
+        if let Some(owner) = owner {


@yito88 can you clarify here? I'm quite confused about this

cwgoes · 2024-06-20T12:19:47Z

crates/namada/src/ledger/native_vp/masp.rs

+                &message.timeout_height_on_b,
+                &message.timeout_timestamp_on_b,
+            );
+        // Try to find a key change with the same port, channel, and commitment


cwgoes · 2024-06-20T12:20:10Z

crates/namada/src/ledger/native_vp/masp.rs

+    }
+
+    // Apply the balance change to the changed balances structure
+    fn apply_balance_change(


(discussed offline - let's do this separately)

crates/namada/src/ledger/native_vp/masp.rs

…0' (#3409)

…0' (#3409) * origin/murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0: Added changelog entry. Update balances in the MASP using a non-mutating style. Now charge gas in IBC denom query. Start using IBC ports to determine message formats. Added more comments and improved function naming. Simplify checking packet acknowledgement by assuming the uniqueness of the success acknowledgement. Removed is_any_shielded_action_balance_key and related code since thats no longer used. Now map denominations to tokens using ibc_token instead of reverse_query_denom. Subdivided some functions involved in processing IBC packets. Reduced the dependence on IBC events. Ensure that native tokens can always be decoded. Handle the is_sender_chain_source case in the MASP VP. Centralized the construction of TransparentAddresses to ensure that encodings are consistent. Split up the IBC validation logic in MASP. Now check that the IBC events are valid with respect to storage changes. Created a separate type that is either an Address or IBC receiver. Modified the MASP VP to check the IBC receivers. Now hash the TransferTarget into Transaction transparent outputs.

…n-0.39.0' (#3409)

murisi added 9 commits May 28, 2024 12:29

Now hash the TransferTarget into Transaction transparent outputs.

e02fe43

Modified the MASP VP to check the IBC receivers.

6cbe813

Created a separate type that is either an Address or IBC receiver.

20d113b

Now check that the IBC events are valid with respect to storage changes.

2b49dc0

Split up the IBC validation logic in MASP.

abbf3b2

Centralized the construction of TransparentAddresses to ensure that e…

b55f5bc

…ncodings are consistent.

Handle the is_sender_chain_source case in the MASP VP.

5547f09

Ensure that native tokens can always be decoded.

423ee7f

Reduced the dependence on IBC events.

0ae6f2b

murisi mentioned this pull request Jun 12, 2024

Updated the Namada SDK dependency and adjusted denominations. heliaxdev/hermes#34

Closed

murisi marked this pull request as ready for review June 12, 2024 19:34

murisi requested review from yito88, grarco and cwgoes June 12, 2024 19:34

Merge branch 'base' into murisi/masp-ibc-replay-protection-using-txda…

438a4ab

…ta-on-0.39.0

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch from 41d4415 to 438a4ab Compare June 12, 2024 20:57

cwgoes requested changes Jun 13, 2024

View reviewed changes

yito88 reviewed Jun 13, 2024

View reviewed changes

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch 2 times, most recently from aacbe26 to 80fc75d Compare June 14, 2024 11:23

Subdivided some functions involved in processing IBC packets.

08e5664

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch from 80fc75d to 08e5664 Compare June 14, 2024 11:29

murisi mentioned this pull request Jun 14, 2024

Draft #3377

Closed

cwgoes reviewed Jun 17, 2024

View reviewed changes

grarco reviewed Jun 17, 2024

View reviewed changes

crates/sdk/src/masp.rs Outdated Show resolved Hide resolved

grarco reviewed Jun 17, 2024

View reviewed changes

crates/namada/src/ledger/native_vp/masp.rs Outdated Show resolved Hide resolved

yito88 requested changes Jun 17, 2024

View reviewed changes

grarco reviewed Jun 17, 2024

View reviewed changes

crates/namada/src/ledger/native_vp/masp.rs Show resolved Hide resolved

grarco reviewed Jun 17, 2024

View reviewed changes

murisi added 4 commits June 19, 2024 08:37

Now map denominations to tokens using ibc_token instead of reverse_qu…

a2854e7

…ery_denom.

Removed is_any_shielded_action_balance_key and related code since tha…

6739f0b

…ts no longer used.

Simplify checking packet acknowledgement by assuming the uniqueness o…

18fe989

…f the success acknowledgement.

Added more comments and improved function naming.

8de2ec0

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch 4 times, most recently from 439e5a9 to fb0ed6f Compare June 19, 2024 11:10

Start using IBC ports to determine message formats.

0c999bd

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch from fb0ed6f to 0c999bd Compare June 19, 2024 11:26

Now charge gas in IBC denom query.

d0b4f3d

murisi requested review from yito88, grarco and cwgoes June 20, 2024 11:53

cwgoes reviewed Jun 20, 2024

View reviewed changes

Update balances in the MASP using a non-mutating style.

0a5d480

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch from 4de6577 to 0a5d480 Compare June 20, 2024 16:55

murisi added a commit that referenced this pull request Jun 20, 2024

Merge branch 'murisi/masp-ibc-replay-protection-using-txdata-on-0.39.…

8e8d046

…0' (#3409)

murisi mentioned this pull request Jun 20, 2024

Murisi/draft with masp ibc replay protection #3430

Closed

2 tasks

murisi added a commit that referenced this pull request Jun 20, 2024

Merge branch 'murisi/masp-ibc-replay-protection-using-txdata-on-0.39.…

808a950

…0' (#3409)

Added changelog entry.

7ff94bd

murisi added a commit that referenced this pull request Jun 20, 2024

Merge branch 'murisi/masp-ibc-replay-protection-using-txdata-on-0.39.…

76fab4c

…0' (#3409)

murisi mentioned this pull request Jun 21, 2024

Refine IBC parts for masp ibc replay protection #3422

Closed

2 tasks

murisi added the ready for draft label Jun 21, 2024

brentstone added the merged to draft label Jun 25, 2024

brentstone added a commit that referenced this pull request Jul 4, 2024

fixup! Merge branch 'murisi/masp-ibc-replay-protection-using-txdata-o…

dae8c35

…n-0.39.0' (#3409)

brentstone merged commit 672a3d2 into main Jul 5, 2024
11 of 19 checks passed

brentstone deleted the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch July 5, 2024 21:16

murisi mentioned this pull request Aug 13, 2024

Refund failed when IBC unshielded transfer failed #3619

Closed

Murisi/masp ibc replay protection using txdata on 0.39.0 #3409

Murisi/masp ibc replay protection using txdata on 0.39.0 #3409

Conversation

murisi commented Jun 12, 2024 • edited Loading

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yito88 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yito88 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yito88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yito88 Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murisi commented Jun 12, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov bot commented Jun 12, 2024 •

edited

Loading

yito88 Jun 20, 2024 •

edited

Loading

yito88 Jun 20, 2024 •

edited

Loading

yito88 Jun 18, 2024 •

edited

Loading