fix failed back transfer #83

taitruong · 2024-01-25T21:41:58Z

Bug description

an NFT gets transferred back from chain B to A
in case of failure (e.g. caused by incoming proxy or callback) nothing should happen
instead states for outgoing channel are not reverted.

Task to reproduce:

do NOT whitelist channel in incoming proxy on chain A -> this will cause an error on back transfer
transfer NFT from A to B
transfer NFT back from B to A

1. NFT (forward) transferred from chain A to B

Outcoume:

chain A: NFT escrowed by ICS721
chain B: NFT minted on newly, instantiated collection
state changes:
- entry added on chain A: OUTGOING_CLASS_TOKEN_TO_CHANNEL ("returning to source" identifier for back transfer)
- entry added on chain B: INCOMING_CLASS_TOKEN_TO_CHANNEL

2. Transfer NFT back from B to A

Outcome:

expected outcome on failure:
- NFTs unchanged on chain A (escrowed) and B (holder)
- no state changes
actual outcome on failure:
- NFTs unchanged (this is correct) on both chains
- states not reverted

Reason here states are updated in contract, but all other actions are handled in sub messages. So if sub messages fails, states in ics721 are not reverted. Reason here is, that on ibc receive TX must not fail for relaying Ack success or error.

What happened next: I WLed incoming proxy and did another back transfer, this time successfully, but with an unexpected result:

chain B: NFT burned
chain A: 2 collections:
- NFT minted on newly, instantiated collection
- "source/OG" NFT still escrowed by ICS721

Reason here is because of previous failure, removal of entry in outgoing channel got not reverted. So on next transfer ics721 on chain A:

does not recognise NFT as a back transfer
NFT on correct collection stays escrowed by ics721
a new collection is instanted
NFT is mint on new collection

This PR fixes this, by moving state changes for incoming and outgoing channel, away from ics721 contract to a dedicated sub message. So if other sub messages like callback or incoming proxy fails, this submessage is not executed.

- happy path on back transfer - edge case: transfer via unknown channel

…l sub messages have succesfully completed

…tion aggregator

taitruong · 2024-01-28T00:10:13Z

packages/ics721/src/ibc_packet_receive.rs

@@ -94,57 +121,117 @@ pub(crate) fn receive_ibc_packet(
                        // We previously sent this NFT out on this
                        // channel. Unlock the local version for the
                        // receiver.
-                        OUTGOING_CLASS_TOKEN_TO_CHANNEL.remove(deps.storage, key);


This here is the bug: state change is done here in ics721

taitruong · 2024-01-28T00:10:42Z

packages/ics721/src/ibc_packet_receive.rs

-                let local_prefix = get_endpoint_prefix(&packet.dest);
-                let local_class_id = ClassId::new(format!("{}{}", local_prefix, data.class_id));
-
-                INCOMING_CLASS_TOKEN_TO_CHANNEL.save(


This is the other bug: state change is done also in ics721.

taitruong · 2024-01-28T00:13:21Z

packages/ics721/src/ibc_packet_receive.rs

-    let submessage = action_aggregator.into_submessage(
-        env.contract.address,
-        receiver,
-        callback_msg,
-        incoming_proxy_msg,
-    )?;


Here is all the main logic: submessage itself contains a list of messags for:

action aggregrator: creates a callback msg with a list of more messages for create or redeemining NFTs

optional incoming proxy handling

optional callback handling

taitruong · 2024-01-28T00:15:17Z

packages/ics721/src/ibc_packet_receive.rs

-    let action_aggregator = data
+    // sub message holds 2 to 4 messages:
+    // - one message for voucher creation or redemption, another message for updating incoming or outgoing channel
+    let (is_redemption, voucher_and_channel_messages) = create_voucher_and_channel_messages(
+        deps.as_ref(),
+        env.clone(),
+        data.clone(),
+        maybe_local_class_id,
+        local_class_id.clone(),
+        packet.clone(),
+    )?;
+    // - one optional incoming proxy message
+    let incoming_proxy_msg =
+        get_incoming_proxy_msg(deps.as_ref().storage, packet.clone(), data.clone())?;
+    // - one optional callback message
+    let callback_msg = create_callback_msg(
+        deps.as_ref(),
+        &env,
+        &data,
+        is_redemption,
+        callback,
+        local_class_id,
+    )?;
+
+    let submessage = into_submessage(
+        env.contract.address,
+        voucher_and_channel_messages.0,
+        voucher_and_channel_messages.1,
+        callback_msg,
+        incoming_proxy_msg,
+    )?;
+
+    let response = if let Some(memo) = data.memo {
+        IbcReceiveResponse::default().add_attribute("ics721_memo", memo)
+    } else {
+        IbcReceiveResponse::default()
+    };
+
+    Ok(response
+        .add_submessage(submessage)
+        .add_attribute("method", "receive_ibc_packet")
+        .add_attribute("class_id", data.class_id)
+        .add_attribute("local_channel", packet.dest.channel_id)
+        .add_attribute("counterparty_channel", packet.src.channel_id))
+}
+
+fn create_voucher_and_channel_messages(
+    deps: Deps,
+    env: Env,
+    data: NonFungibleTokenPacketData,
+    maybe_local_class_id: Option<&str>,
+    local_class_id: ClassId,
+    packet: IbcPacket,
+) -> Result<(bool, (WasmMsg, WasmMsg)), ContractError> {
+    let token_count = data.token_ids.len();
+    let redemption_or_create = data
        .token_ids
        .into_iter()
        .zip_optional(data.token_uris)
        .zip_optional(data.token_data)
        .try_fold(
-            Vec::<Action>::with_capacity(token_count),
-            |mut messages, ((token_id, token_uri), token_data)| -> StdResult<_> {


old logic is super complicated for create a callaback submessage, using Action and Action Aggregator...

taitruong · 2024-01-28T00:17:14Z

packages/ics721/src/ibc_packet_receive.rs

+    let (is_redemption, voucher_and_channel_messages) = create_voucher_and_channel_messages(
+        deps.as_ref(),
+        env.clone(),
+        data.clone(),
+        maybe_local_class_id,
+        local_class_id.clone(),
+        packet.clone(),
+    )?;


...new logic leads to same result, by creating required messages - without using temporary action and aggregator objects

taitruong · 2024-01-28T00:24:34Z

packages/ics721/src/ibc_packet_receive.rs

+            let add_incoming_channels: Vec<((ClassId, TokenId), String)> = creation
+                .tokens
+                .clone()
+                .into_iter()
+                .map(|token| {
+                    (
+                        (local_class_id.clone(), token.id),
+                        packet.dest.channel_id.clone(),
+                    )
+                })
+                .collect();
+            let add_incoming_channels_msg = WasmMsg::Execute {
+                contract_addr: env.contract.address.to_string(),
+                msg: to_json_binary(&ExecuteMsg::Callback(
+                    CallbackMsg::AddIncomingChannelEntries(add_incoming_channels),
+                ))?,
+                funds: vec![],
+            };


... CallbackMsg::AddIncomingChannelEntries which then will be added to sub message.

taitruong · 2024-01-28T00:29:06Z

ts-relayer-tests/src/ics721.spec.ts

+  assertAckErrors(info.acksFromA);
+  // assert no change before and after relay
+  const afterWasmOutgoingClassTokenToChannelList = await outgoingChannels(
+    wasmClient,
+    wasmIcs721
+  );
+  const afterWasmIncomingClassTokenToChannelList = await incomingChannels(
+    wasmClient,
+    wasmIcs721
+  );
+  const afterWasmNftContractsToClassIdList = await nftContracts(
+    wasmClient,
+    wasmIcs721
+  );
+  t.deepEqual(
+    beforeWasmOutgoingClassTokenToChannelList,
+    afterWasmOutgoingClassTokenToChannelList,
+    `outgoing channels must be unchanged:
+- wasm before: ${JSON.stringify(beforeWasmOutgoingClassTokenToChannelList)}
+- wasm after: ${JSON.stringify(afterWasmOutgoingClassTokenToChannelList)}`
+  );
+  t.deepEqual(
+    beforeWasmIncomingClassTokenToChannelList,
+    afterWasmIncomingClassTokenToChannelList,
+    `incoming channels must be unchanged:
+- wasm before: ${JSON.stringify(beforeWasmIncomingClassTokenToChannelList)}
+- wasm after: ${JSON.stringify(afterWasmIncomingClassTokenToChannelList)}`
+  );
+  t.deepEqual(
+    beforeWasmNftContractsToClassIdList,
+    afterWasmNftContractsToClassIdList,
+    `nft contracts must be unchanged:
+- wasm before: ${JSON.stringify(beforeWasmNftContractsToClassIdList)}
+- wasm after: ${JSON.stringify(afterWasmNftContractsToClassIdList)}`
+  );
+  const afterOsmoOutgoingClassTokenToChannelList = await outgoingChannels(
+    osmoClient,
+    osmoIcs721
+  );
+  const afterOsmoIncomingClassTokenToChannelList = await incomingChannels(
+    osmoClient,
+    osmoIcs721
+  );
+  const afterOsmoNftContractsToClassIdList = await nftContracts(
+    osmoClient,
+    osmoIcs721
+  );
+  t.deepEqual(
+    beforeOsmoOutgoingClassTokenToChannelList,
+    afterOsmoOutgoingClassTokenToChannelList,
+    `outgoing channels must be unchanged:
+- osmo before: ${JSON.stringify(beforeOsmoOutgoingClassTokenToChannelList)}
+- osmo after: ${JSON.stringify(afterOsmoOutgoingClassTokenToChannelList)}`
+  );
+  t.deepEqual(
+    beforeOsmoIncomingClassTokenToChannelList,
+    afterOsmoIncomingClassTokenToChannelList,
+    `incoming channels must be unchanged:
+- osmo before: ${JSON.stringify(beforeOsmoIncomingClassTokenToChannelList)}
+- osmo after: ${JSON.stringify(afterOsmoIncomingClassTokenToChannelList)}`
+  );
+  t.deepEqual(
+    beforeOsmoNftContractsToClassIdList,
+    afterOsmoNftContractsToClassIdList,
+    `nft contracts must be unchanged:
+- osmo before: ${JSON.stringify(beforeOsmoNftContractsToClassIdList)}
+- osmo after: ${JSON.stringify(afterOsmoNftContractsToClassIdList)}`
+  );


This is the ts relayer test, checking states before and after a failed back transfer

taitruong · 2024-01-28T00:31:41Z

ts-relayer-tests/src/ics721.spec.ts

+  tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId);
+  t.is(wasmAddr, tokenOwner.owner);
+
+  // ==== test transfer NFT to osmo chain via channel WLed ONLY on osmo incoming proxy and back to wasm chain ====


This test reproduces back transfer on failure and success:

taitruong · 2024-01-28T00:32:24Z

ts-relayer-tests/src/ics721.spec.ts

+  tokenOwner = await ownerOf(osmoClient, osmoCw721, tokenId);
+  t.is(osmoAddr, tokenOwner.owner);
+
+  // test back transfer NFT to wasm chain, where onlyOsmoIncomingChannel is not WLed on wasm chain


test failed back transfer

taitruong · 2024-01-28T00:33:11Z

ts-relayer-tests/src/ics721.spec.ts


-    info = await otherChannel.link.relayAll();
+  // ==== WL channel on wasm chain and test back transfer again ====


migrate and WL channel for testing succesful back transfer, now NFT is unescrowd on existing collection - and NOT minted on a new collection

taitruong · 2024-01-29T14:45:08Z

packages/ics721/src/msg.rs

+    AdminCleanAndBurnNft {
+        owner: String,
+        token_id: String,
+        class_id: String,
+        collection: String,
+    },
+
+    /// Admin msg in case something goes wrong.
+    /// As a minimum it clean up state (outgoing channel), and transfer NFT if exists.
+    /// - transfer NFT if exists
+    AdminCleanAndUnescrowNft {
+        recipient: String,
+        token_id: String,
+        class_id: String,
+        collection: String,
+    },


Added 2 new messages for fixing "forked" NFTs due to back transfer bug.

Due to the bug, we have this invalid state on back transfer:
chain A:

OG NFT escrowed

new NFT on new collection (due to bug)
chain B:

NFT got burned due to bug

So on chain A, we need to:

burn newly minted NFT

unescrow OG NFT, transfer to recipient

These 2 msgs can only be executed by admin of ics721, once ics721 is immutable it cant be executed anymore! So in case of invalid we may fix specific NFTs.

taitruong · 2024-01-29T14:46:01Z

packages/ics721/src/execute.rs

+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn execute_admin_clean_and_burn_nft(


burn "child" nft: ...

taitruong · 2024-01-29T14:46:51Z

packages/ics721/src/execute.rs

+        INCOMING_CLASS_TOKEN_TO_CHANNEL
+            .remove(deps.storage, (child_class_id.clone(), token_id.clone()));
+        TOKEN_METADATA.remove(deps.storage, (child_class_id.clone(), token_id.clone()));


... cleanup state

taitruong · 2024-01-29T14:48:36Z

packages/ics721/src/execute.rs

+            let burn_msg = WasmMsg::Execute {
+                contract_addr: child_collection.to_string(),
+                msg: to_json_binary(&cw721::Cw721ExecuteMsg::Burn {
+                    token_id: token_id.clone().into(),
+                })?,
+                funds: vec![],
+            };
+            response = response.add_message(burn_msg);


... burn nft

taitruong · 2024-01-29T14:49:03Z

packages/ics721/src/execute.rs

+    }
+
+    #[allow(clippy::too_many_arguments)]
+    fn execute_admin_clean_and_unescrow_nft(


Unescrow "parent" NFT

taitruong · 2024-01-29T14:54:46Z

ts-relayer-tests/src/ics721.spec.ts

+  );
+  // assert NFT on chain A is owned by wasmAddr
+  tokenOwner = await ownerOf(wasmClient, wasmCw721, tokenId);
+  t.is(wasmAddr, tokenOwner.owner);


NFT transferred to owner

taitruong · 2024-01-29T14:56:17Z

ts-relayer-tests/src/ics721.spec.ts

+
+  // ==== test burn NFT on osmo chain ====
+  // we need to approve the contract to burn the NFT
+  t.log(`approve NFT on osmo chain`);


we need to approve nft, so ics721 can burn it

taitruong · 2024-01-29T14:56:26Z

ts-relayer-tests/src/ics721.spec.ts

+  await adminCleanAndBurnNft(
+    osmoClient,
+    osmoIcs721,
+    osmoAddr,
+    tokenId,
+    osmoClassId,
+    osmoCw721
+  );


Burn "child" NFT

taitruong · 2024-01-29T14:56:48Z

ts-relayer-tests/src/ics721.spec.ts

+  );
+  t.log(`- response: ${JSON.stringify(response, bigIntReplacer, 2)}`);
+  allNFTs = await allTokens(osmoClient, osmoCw721);
+  t.is(allNFTs.tokens.length, 0);


No NFTs in collection

taitruong · 2024-01-29T14:57:00Z

ts-relayer-tests/src/ics721.spec.ts

+    osmoClient,
+    osmoIcs721
+  );
+  t.deepEqual(


empty state

humanalgorithm

MrT and I went over these changes on a call

humanalgorithm · 2024-01-29T20:55:31Z

packages/ics721/src/ibc_packet_receive.rs

+        packet.clone(),
+    )?;
+    // - one optional incoming proxy message
+    let incoming_proxy_msg =


thanks for refactoring this out to functions 👍

taitruong force-pushed the fix_back_transfer branch from 3c2437b to ff96919 Compare January 25, 2024 21:44

test backtransfer to banned recipient, but also to to regular recpient

fb52da8

taitruong force-pushed the fix_back_transfer branch from ff96919 to fb52da8 Compare January 25, 2024 22:01

taitruong added 9 commits January 26, 2024 15:08

e2e ts relayer test:

be897b8

- happy path on back transfer - edge case: transfer via unknown channel

fix back transfer, remove entries in outgoing channel only in case al…

ae6c7d3

…l sub messages have succesfully completed

rename

2afbce7

simplify receive_ibc_packet(), create sub message directly, remove ac…

6357673

…tion aggregator

fix move to sub message for saving incoming channel entries

f903de4

move to dedicated functions

f43f9c0

docs

31ab7b5

cleanup

dec10c1

cargo schema

94557a5

taitruong commented Jan 28, 2024

View reviewed changes

shanev approved these changes Jan 28, 2024

View reviewed changes

taitruong added 4 commits January 29, 2024 12:22

2 new admin msgs for fixing forked NFTs

c5a6548

docs

80995ed

test admin msgs

dc32c5f

cargo schema

545909f

taitruong commented Jan 29, 2024

View reviewed changes

humanalgorithm self-requested a review January 29, 2024 20:48

humanalgorithm approved these changes Jan 29, 2024

View reviewed changes

humanalgorithm reviewed Jan 29, 2024

View reviewed changes

jhernandezb approved these changes Jan 31, 2024

View reviewed changes

taitruong added this pull request to the merge queue Jan 31, 2024

Merged via the queue into public-awesome:main with commit 1d96023 Jan 31, 2024
1 check passed

taitruong mentioned this pull request Feb 18, 2024

Final audit revision #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix failed back transfer #83

fix failed back transfer #83

taitruong commented Jan 25, 2024 •

edited

Loading

taitruong Jan 28, 2024

taitruong Jan 28, 2024

taitruong Jan 28, 2024

taitruong Jan 28, 2024 •

edited

Loading

taitruong Jan 28, 2024 •

edited

Loading

taitruong Jan 28, 2024

taitruong Jan 28, 2024

taitruong Jan 28, 2024

taitruong Jan 28, 2024

taitruong Jan 28, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

taitruong Jan 29, 2024

humanalgorithm left a comment

humanalgorithm Jan 29, 2024


		info = await otherChannel.link.relayAll();
		// ==== WL channel on wasm chain and test back transfer again ====

fix failed back transfer #83

fix failed back transfer #83

Conversation

taitruong commented Jan 25, 2024 • edited Loading

Bug description

1. NFT (forward) transferred from chain A to B

2. Transfer NFT back from B to A

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong Jan 28, 2024 • edited Loading

Choose a reason for hiding this comment

taitruong Jan 28, 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

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

humanalgorithm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taitruong commented Jan 25, 2024 •

edited

Loading

taitruong Jan 28, 2024 •

edited

Loading

taitruong Jan 28, 2024 •

edited

Loading