Refine IBC parts for masp ibc replay protection #3422

yito88 · 2024-06-18T12:32:04Z

Describe your changes

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

#3409
Diff: 76fab4c...5c3b3ab

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

…ncodings are consistent.

…ta-on-0.39.0

…ery_denom.

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

…ts no longer used.

…f the success acknowledgement.

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

murisi · 2024-06-21T07:01:31Z

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

-        // message cannot be found, then ignore this message. Though this check
-        // is done in the IBC VP, the test is repeated here to avoid making
-        // assumptions about how the IBC VP interprets the given message.
-        if self


Could we keep this check or have something with similar effect so that we avoid making assumptions about the execution of the IBC VP? See also #3409 (comment) .

murisi

Overall could the assumptions that the MASP VP makes about the execution of the IBC VP be reduced? See a more precise description of the potentially problematic assumptions here #3409 (comment) .

murisi · 2024-06-21T07:22:23Z

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

-            &packet.chan_id_on_a,
-            packet.seq_on_a,
-        );
-        if !keys_changed.contains(&ack_key) {


Putting aside assumptions about the behaviour/judgement of the IBC VP, is there a chance that an IBC packet that has been previously acknowledged in a previous state change could be replayed here successfully and be used to fool the MASP VP into, say, reshielding funds from the packet?

it is possible if IBC VP isn't triggered. I'll revert this.

murisi · 2024-06-21T07:36:17Z

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

            // Mirror how the IBC token is derived in
            // gen_ibc_shielded_transfer in the non-refund case
-            let ibc_denom = self.query_ibc_denom(


Discussion here: #3409 (comment) . Would the above comment about "mirror"ing still be correct with this change?

I changed it to check the packet receipt

yito88 · 2024-06-21T23:45:48Z

IBC E2E tests run_ledger_ibc_with_hermes and ibc_namada_gaia passed with the updated Hermes

yito88 · 2024-06-24T10:56:13Z

crates/sdk/src/rpc.rs

-        if let Address::Internal(InternalAddress::IbcToken(ibc_token_hash)) =
-            token
-        {
-            extract_base_token(context, ibc_token_hash.clone(), None).await


There is no guarantee that the base token of the source chain is the same as that of the destination(this) chain.
We should use zero denomination for all IbcTokens.

yito88 · 2024-06-24T14:36:13Z

Tested NFT transfers between local Namada and Stargaze testnet.

Stargeze -> Namada tnam -> Stargaze - (Shielding) -> Namada znam1 - (Shielded) -> Namada znam2 - (Unshielding) -> Stargaze

codecov · 2024-07-01T21:30:03Z

Codecov Report

Attention: Patch coverage is 9.62838% with 535 lines in your changes missing coverage. Please review.

Project coverage is 53.74%. Comparing base (879a326) to head (61db5a4).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/namada/src/ledger/native_vp/masp.rs	0.00%	376 Missing ⚠️
crates/ibc/src/trace.rs	41.55%	45 Missing ⚠️
crates/core/src/masp.rs	0.00%	41 Missing ⚠️
crates/ibc/src/lib.rs	0.00%	20 Missing ⚠️
crates/sdk/src/masp.rs	0.00%	10 Missing ⚠️
crates/vp_env/src/lib.rs	0.00%	10 Missing ⚠️
crates/ibc/src/context/common.rs	50.00%	9 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 ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3422      +/-   ##
==========================================
- Coverage   53.92%   53.74%   -0.19%     
==========================================
  Files         317      318       +1     
  Lines      107575   107887     +312     
==========================================
- Hits        58011    57980      -31     
- Misses      49564    49907     +343

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

yito88 · 2024-07-05T21:53:44Z

merged with #3444

murisi added 11 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

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

438a4ab

…ta-on-0.39.0

Subdivided some functions involved in processing IBC packets.

08e5664

yito88 mentioned this pull request Jun 18, 2024

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

Merged

2 tasks

Now map denominations to tokens using ibc_token instead of reverse_qu…

a2854e7

…ery_denom.

murisi reviewed Jun 19, 2024

View reviewed changes

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

murisi added 2 commits June 19, 2024 08:46

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.

murisi reviewed Jun 19, 2024

View reviewed changes

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

Added more comments and improved function naming.

8de2ec0

murisi force-pushed the murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch 3 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

yito88 force-pushed the yuji/murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch from 14fe5d5 to 05c584a Compare June 19, 2024 15:05

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

Added changelog entry.

7ff94bd

murisi reviewed Jun 21, 2024

View reviewed changes

murisi requested changes Jun 21, 2024

View reviewed changes

yito88 added 4 commits June 21, 2024 11:13

support NftTransfer

d44fc56

fix for apply_recv_msg

320e9ed

refactoring: add trace.rs

60ea876

reduce assumptions for IBC VP

a942176

yito88 force-pushed the yuji/murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 branch from a51f3eb to a942176 Compare June 21, 2024 11:50

yito88 added 3 commits June 21, 2024 14:31

add ibc trace file

9c51728

fix convert_to_address

fb19c25

fix tests

210f34b

yito88 changed the title ~~masp ibc replay protection for nft transfer~~ Refine IBC parts for masp ibc replay protection Jun 24, 2024

yito88 marked this pull request as ready for review June 24, 2024 07:41

murisi requested review from grarco and cwgoes June 24, 2024 09:32

yito88 commented Jun 24, 2024

View reviewed changes

fix the port and channel for is_receiving_success

5c3b3ab

yito88 requested a review from murisi June 25, 2024 07:32

yito88 mentioned this pull request Jul 1, 2024

Make the sender set MASP transaction to IBC memo #3444

Merged

2 tasks

yito88 changed the base branch from murisi/masp-ibc-replay-protection-using-txdata-on-0.39.0 to main July 1, 2024 20:56

add changelog

61db5a4

yito88 added the merged to draft label Jul 4, 2024

yito88 closed this Jul 5, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine IBC parts for masp ibc replay protection #3422

Refine IBC parts for masp ibc replay protection #3422

yito88 commented Jun 18, 2024 •

edited

Loading

murisi Jun 21, 2024

murisi left a comment

murisi Jun 21, 2024

yito88 Jun 21, 2024

murisi Jun 21, 2024

yito88 Jun 21, 2024

yito88 commented Jun 21, 2024

yito88 Jun 24, 2024

yito88 commented Jun 24, 2024

codecov bot commented Jul 1, 2024

yito88 commented Jul 5, 2024

Refine IBC parts for masp ibc replay protection #3422

Refine IBC parts for masp ibc replay protection #3422

Conversation

yito88 commented Jun 18, 2024 • edited Loading

Describe your changes

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

Checklist before merging to draft

murisi Jun 21, 2024

Choose a reason for hiding this comment

murisi left a comment

Choose a reason for hiding this comment

murisi Jun 21, 2024

Choose a reason for hiding this comment

yito88 Jun 21, 2024

Choose a reason for hiding this comment

murisi Jun 21, 2024

Choose a reason for hiding this comment

yito88 Jun 21, 2024

Choose a reason for hiding this comment

yito88 commented Jun 21, 2024

yito88 Jun 24, 2024

Choose a reason for hiding this comment

yito88 commented Jun 24, 2024

codecov bot commented Jul 1, 2024

Codecov Report

yito88 commented Jul 5, 2024

yito88 commented Jun 18, 2024 •

edited

Loading

Checklist before merging to `draft`