Extend pallet-xcm to pay teleport with non teleported asset by metricaez · Pull Request #266 · paritytech/trappist

metricaez · 2023-08-24T21:18:11Z

Intro

First approach to an extension for pallet-xcm to allow a native asset Teleport from Trappist (or any para) into Asset Hub.

Issue

Our main issue at the moment is that even though we are able to register HOP (Trappist native asset) as a foreign asset on Asset Hub, we cannot teleport it into AH. This is related to how InitiateTeleport call of XCM works, which is the one that also pallet-xcm uses for its limited and unlimited teleports. Said instruction triggers a ReceiveTeleportedAssets on destiny and this “minted” assets are used for buying execution on destiny, however, AH doesn’t allow (yet) to buy execution with foreign assets and doesn’t recognize sibling parachains as ROC teleporters so we cannot send ROC on the Assets vector of the Teleport to pay the fees. A further description can be found here.

Workaround

We presented a workaround with batching multiple instruction calls through xcm pallet, this can also be found here but the main logic is:

Burn HOP on Trappist
Withdraw ROC from an already funded account on AH
Buy execution with ROC
ReceiveTeleported (mint) HOP on AH
Deposit to Beneficiary

Proposed Solution

I see that an abstraction of this process that might become useful also for other use cases is to create a call into a pallet-xcm extension that allows users to define an asset that is not being teleported to pay for execution in destiny This implies that the account that buys execution on destiny must be previously funded with said non-teleported asset, in this case we would be funding the account on AH with ROC before sending this call.

This first iteration presents a simple approach in which we only allow user to teleport native tokens out.

Disclaimer

I prioritized speed to start receiver feedback on the logic itself as soon as possible so there are some consciously TODOs, for example weights and weight limit.

Same goes for implementation, so please, do drop any suggestions.

I am particularly interested on your feedback on how to define the account to withdraw ROC from on AH, on this case I am using Root to force withdraw from Sovereign Account but this does introduce evident vulnerabilities.

We can add ensure root to request sudo and turn it into a privileged call to get foreign assets into AH and from there distribute or fund pools but really eager to get your thoughts on this.

Thanks.

…target_dir=trappist

IkerAlus · 2023-08-28T08:08:20Z

pallets/proxy-teleport/src/lib.rs

+		dest: Box<VersionedMultiLocation>,
+		beneficiary: Box<VersionedMultiLocation>,
+		native_asset_amount: u128,
+		proxy_asset: Box<VersionedMultiAssets>,


is this input parameter actually required?

My understanding is that it is always going to be the Relay chain/system parachain native asset hence the user would always set the same input.

Hey, I see your point, I left it as a parameter as I thought that there might be an use case in which you would like to buy execution with other asset that is handeld but is not the Relay/SP native. For example buying with execution with xUSD on AH. What do you think?

good point, I agree it makes sense to leave this flexibility for the future. Only thing, it would be good to document this, particularly that the sovereign account in AH will need to have enough balance of the relevant asset for this to work (apart from it having a liquidity pool vs the native asset etc)

pallets/proxy-teleport/src/lib.rs

IkerAlus · 2023-08-28T08:38:13Z

pallets/proxy-teleport/src/lib.rs

+		//Build the message to execute on origin.
+		let assets: MultiAssets = assets.into();
+		let message: Xcm<<T as frame_system::Config>::RuntimeCall> = Xcm(vec![
+			// Withdraw drops asset so is used as burn mechanism


I was wondering, is it possible to mimic what InitiateTeleport does so that we don't leave the native asset trapped in the holding?

This way we could directly pass assets in line 172 instead of the hardcoded foreing_assets.

I like this, on it! Thanks

Update:

I have implemented the reanchor , foreign_assets is no longer hardcoded.

I have also added a BurnAssets to mimic this part of the InitiateTeleport instruction

Currently trying to solve this check_out logic for this proxy call to track teleports too.

Good idea to use BurnAsssets instruction. In my opinion this is a solution close to as clean as we can get it. This way there is no trapped assets in the holding whereas the counterpart is expected to be minted in the destination chain. I think the check out logic is then secondary.

IkerAlus

I left some general comments around the new pallet for the proxy teleport, great work!

stiiifff · 2023-08-28T15:52:59Z

pallets/proxy-teleport/src/lib.rs

+		let xcm_message: Xcm<()> = Xcm(vec![
+			WithdrawAsset(proxy_asset),
+			BuyExecution { fees, weight_limit },
+			ReceiveTeleportedAsset(foreing_assets.clone()),


I don't get the need for the ReceiveTeleportedAsset instruction .. can you explain ?

Hey! Sure.

We must take in consideration that:

InitiateTeleport instruction imprints a ReceiveTeleportedAsset as first instruction of the xcm message that it sends.

The Barrier that we are targeting for in Asset Hub is AllowTopLevelPaidExecutionFrom, this Barrier to pass imposes a certain sequence of Instructions to be received in order as detailed here.

That is why do_teleport_assets fn from pallet-xcm sets BuyExecution as the first instruction of the xcm message that is sent as parameter to InitiateTeleport, therefore the Instruction order to be receive would be:
-- Receive -> ClearOrigin (from InitiateTeleport)
-- BuyExecution (from the xcm sent as parameter of InitiateTeleport)
Leading to the Barrier compliant sequence of ReceiveTele... -> ClearOrigin -> BuyExecution

However, on Asset Hub we cannot BuyExecution with HOP or teleport ROC into it which would trigger the ReceiveTeleportedAssets progression with ROC.

There is no Instruction that allows me to prepend a Withdraw (ROC) + BuyExecution (ROC) together with the InitiateTeleport logic.

Therefore I am forced to split the logic of the call in two parts: What should be executed on Trappist and what should be sent to be executed on Asset Hub. I am trying to handcraft the logic as close as possible to the one from InitiateTeleport but adding the extra instructions that are needed for the use case. This leads to:

A execute_xcm for taking the assets to be teleported on origin via Withdraw + Burn

A send_xcm for paying execution with ROC and apply the "teleport" logic on Asset Hub. This is done by the Withdraw + BuyExecution + ReceiveTele... + DepositAsset that is sent to be executed on Asset Hub.

With these Instructions on this combination I try to approach to the do_teleport_assets logic but sneaking an execution buy with ROC on AH.

I hope this explains both this approach and the rest of the comments, please do let me know if you have any further question or if something doesn't make sense since this is just how I figured out how to work around this issue

stiiifff · 2023-08-28T15:54:37Z

pallets/proxy-teleport/src/lib.rs

+		let message: Xcm<<T as frame_system::Config>::RuntimeCall> = Xcm(vec![
+			// Withdraw drops asset so is used as burn mechanism
+			WithdrawAsset(assets.clone()),
+			BurnAsset(assets),


Indeed, shouldn't it be a InitiateTeleport instruction instead of the BurnAsset one ?

Addressed here

stiiifff · 2023-08-28T15:57:24Z

pallets/proxy-teleport/src/lib.rs

+			root_origin.try_into().map_err(|_| pallet_xcm::Error::<T>::InvalidOrigin)?;
+		//TODO: Check this Error population
+		let message_id = BaseXcm::<T>::send_xcm(interior, dest, xcm_message.clone())
+			.map_err(|_| Error::<T>::SendError)?;


Shouldn't you use execute_xcm instead of send_xcm, to initiate the teleport port operation on the local chain ?

Addressed here

pallets/proxy-teleport/src/lib.rs

franciscoaguirre · 2023-09-01T12:29:33Z

pallets/proxy-teleport/src/lib.rs

+		//Unbox proxy asset
+		let proxy_asset: MultiAssets =
+			(*proxy_asset).try_into().map_err(|()| pallet_xcm::Error::<T>::BadVersion)?;


You might want to unbox this earlier. You're gonna use it anyway and reanchoring is more expensive than this conversion.

Done but would you care to explain further ?

You don't want to perform an expensive operation only to then find out your boxed value had a bad version

Understood and makes sense, feel free to resolve.

pallets/proxy-teleport/src/lib.rs

franciscoaguirre · 2023-09-01T13:05:56Z

pallets/proxy-teleport/src/lib.rs

+		Self::deposit_event(Event::Attempted { outcome });
+
+		// Use pallet-xcm send for sending message.
+		let root_origin = T::SendXcmOrigin::ensure_origin(frame_system::RawOrigin::Root.into())?;


The origin should be the account that has the funds it wants to teleport

I agree that the one paying for the execution with the proxy asset on destiny chain should be the SovereignAccount (SA from now on) of the sender account and not the SA of the chain.

As a quick recap, proxy asset indicates which asset should be used to buy execution on destiny, but the funding of the account that is going to pay for that execution with said asset must be done previously to this call.

For simplicity, let's focus on the use case of: origin chain is Trappist, destiny chain is Asset Hub and the proxy asset is ROC.

Funding the SA of Trappist is simple, you can obtain the SA address with this tool and the PARA ID. Then you do a simple transfer of ROC on Asset Hub to the address of Trappist's SA. Therefore why I currently set Root as origin.

However doing so for the SA of the caller origin account from Trappist in Asset Hub, let's call it Alice, is not straightforward. Two good solutions would be:

Creating a tool that exposes this to obtain the address given a certain MultiLocation and send ROC to it on AH.

As you suggested on our related talk, another approach would be doing a XCM TransferAsset of ROC to {1, X2(Trappist,Alice)} which is the SA of Trappist's Alice on AH (or any Sibling). However, atm, AH doesn't allow executions.

Moving forward, I implemented the HashedDescription on Trappist's LocationToAccountId and was able to get the address SA of Trappist's Alice, meaning the SA of {1, X2(Trappist,Alice)} MultiLocation.

This allowed me to send ROC on AH to this address and now the call buys execution successfully if we set the origin of the call to Alice instead of Root. However, {1, X2(Trappist,Alice)} is not considered a Trusted Teleporter by AH.

Using an alias of Alice's SA to Trappist's SA which is currently a TrustedTeleported would be a increment of privileges on Alice that I do not see fit and also AH doesn't accept any Aliasers.

I don't think the name "proxy asset" is the best to describe what's going on here. The idea of this pallet, and extrinsic, is to teleport one main asset to chain B but withdraw another one on B as well.
It might be called something like withdraw_and_teleport.
It's just the name "proxy" the one that seems misused, it could be called a "fee asset" and that might be better.

Regarding using Alice as the origin of the message, in the end, it's not needed, Alice will already pay up on the sender chain, in this case Trappist, with derivative assets, in this case derivative ROCs.

It's perfectly fine to Withdraw from Trappist's sovereign account since AH doesn't care about Trappist's internal bookkeeping, it only cares that the whole of Trappist has some amount of ROCs. InitiateReserveWithdraw handles it in that way.

On another note, if you only fund Trappist's sovereign account on AH, you're basically giving up power over those assets and are unable to use them again. You should instead use DepositReserveAsset. That will fund the SA but also send a message over to Trappist to mint derivative tokens and send them to Alice over there. That will allow Alice to use this extrinsic to teleport whatever she wants, along with withdrawing the real ROCs for fees.

Agreed on the wording, as logic evolved the proxy term that I initially used became outdated, will re work.

As for the DepositReserveAsset, will explore the suggestion as it would solve two main points:

Users or chain admins having to fund the SA with a transfer which requires them to obtain the address of the SA and adds an extra step of friction.

Most importantly, it diminishes vulnerabilities as it would take any incentive to drain the SA because users would be required to have the derivative on origin chain, and to do so they would have needed to reserve them, so basically it would be their funds.

As discussed on our following chat regarding this comment, withdrawn ROC derivatives on Trappist should also be burned and surplus of ROCs on Asset Hub could be refunded to avoid trapping unnecessary users funds.

From usability perspective, users would no longer need to fund SA through a transfer and they would need to ReserveAssetTransfer ROCs to their addresses on Trappist to be able to do this teleport.

Will implement and update on this thread.

#275 Shows a first approach.

franciscoaguirre · 2023-09-01T13:06:19Z

pallets/proxy-teleport/src/lib.rs

+			WithdrawAsset(proxy_asset),
+			BuyExecution { fees, weight_limit },
+			ReceiveTeleportedAsset(foreing_assets.clone()),
+			// Intentionally trap ROC to avoid exploit


I don't see why keeping this ROC around would be a potential exploit. Care to explain?

Yet to be explored but initially I was depositing All assets on beneficiary, and since at the time ROC is coming from pre funded Trappist's SovereignAccount on Asset Hub, there could be a case in which multiple calls drain the funds from the SovereignAccount into the beneficiary.

Limiting the amount of the proxy_asset, or demanding at least certain amount of the native_asset to be teleported could be a starter to avoid this. Atm, by not depositing all assets and only the foreign/native asset whose equivalent amount has been withdrawn on origin, users couldn't benefit from this exploit. However they could drain the funds by trapping all of them because some people just want to watch the world burn.

Changing origin to the from root to the sender would totally fix this as the funds of ROC (or any proxy_asset) would have been already owned by the caller.

I explained the way things work on my other comment. The funds are always coming from a user, since they pay on the sender chain (via burning). The only potential exploit there is minting more than is burnt. If the mechanism works well, Trappist's SA should never be arbitrarily drained, it will always be the result of derivative tokens being burnt.

Trappist's SA's balance in ROCs = sum(balance in derivative ROC for account in Trappist) <- If this holds then it all works fine.

Agreed that, if implemented, would solve this existing vulnerability.

pallets/withdraw-teleport/src/lib.rs

joepetrowski · 2023-09-06T13:50:18Z

pallets/withdraw-teleport/src/lib.rs

+			WithdrawAsset(fee_asset),
+			BuyExecution { fees, weight_limit },
+			ReceiveTeleportedAsset(foreing_assets.clone()),
+			// Intentionally trap ROC to avoid exploit of draining Sovereing Account


~~Doesn't look trapped?~~
IMO this is hacky. Just debit the user of ROC here.

I would need some more explanation here as I do not fully understand where are you suggesting to debit the ROC from. Thanks!

I mean here on Trappist. You have reserve-backed ROC here, right?

No initially, but yes after #275 , so shall we move forward and merge 275 which solves this ?

joepetrowski · 2023-09-06T13:50:31Z

pallets/withdraw-teleport/src/lib.rs

+			// aware of this and implement a mechanism to prevent draining.
+			WithdrawAsset(fee_asset),
+			BuyExecution { fees, weight_limit },
+			ReceiveTeleportedAsset(foreing_assets.clone()),


Suggested change

ReceiveTeleportedAsset(foreing_assets.clone()),

ReceiveTeleportedAsset(foreing_assets.clone()),

RefundSurplus,

I agree but in this case I didn't initially intend to refund used ROCs to avoid this exploit.

TL:DR since users would be calling this extrinsic from Trappist and the execution in Asset Hub is bought with ROC coming from Trappist's AH SovereignAccount, refunding the ROC surplus could be used to drain the SovAcc.

A solution that is based on Cisco and Iker's input is presented here but implies users reserve transferring ROC into their Trappist's account.

Yeah I see, but like 275 shows can't you burn the ROC here and then transfer the amount out (partly for fee payment and partly to beneficiary) on AH?

Indeed, so I think that #275 is establishing as the way to go.

joepetrowski · 2023-09-06T13:50:49Z

pallets/withdraw-teleport/src/lib.rs

+			ReceiveTeleportedAsset(foreing_assets.clone()),
+			// Intentionally trap ROC to avoid exploit of draining Sovereing Account
+			// by depositing withdrawn ROC on beneficiary.
+			DepositAsset { assets: MultiAssetFilter::Definite(foreing_assets), beneficiary },


Why not just AllOf whatever is left?

Also, this is depositing the Trappist token, but the comment says (and I believe is correct) that it should deposit ROC.

I think this is what you want.

Suggested change

DepositAsset { assets: MultiAssetFilter::Definite(foreing_assets), beneficiary },

DepositAsset { assets: AllCounted(2).into(), beneficiary }

Agreed and initially did it that way but changed to depositing only teleported foreign_assets because of this.

Co-authored-by: Steve Degosserie <steve@parity.io> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

pallets/withdraw-teleport/Cargo.toml

remove unused imports

stiiifff · 2023-09-15T15:39:23Z

LGTM ! Let's open additional PRs to add proper benchmarks, and tests (which most likely requires the integration of xcm-emulator instead of the current xcm-simulator).

José Molina and others added 6 commits August 24, 2023 12:15

feat: migrate to polkadot-v1.0.0

85bde1c

feat: add missing migrations for polkadot-v1.0.0

35e301e

Fix Benchmkaring

7f13ca7

".git/.scripts/commands/bench-all/bench-all.sh" --runtime=trappist --…

e484833

…target_dir=trappist

Fix compilation

900877f

add pallet for teleport foreign assets into ah

923b9f7

metricaez requested review from hbulgarini and stiiifff August 24, 2023 21:19

metricaez marked this pull request as draft August 24, 2023 21:20

metricaez requested a review from IkerAlus August 24, 2023 21:20

metricaez added 3 commits August 25, 2023 19:21

clean duplicated errors

74db417

cargo fmt

08a93cf

comments

38918dd

IkerAlus reviewed Aug 28, 2023

View reviewed changes

pallets/proxy-teleport/src/lib.rs Outdated Show resolved Hide resolved

IkerAlus reviewed Aug 28, 2023

View reviewed changes

metricaez added 4 commits August 28, 2023 08:18

fix typo

ac306c0

foreign asset by reanchoring

92e5940

Burn withdrawn assets on origin

fb6d26b

Merge branch 'main' into emi-proxy-teleport-pallet

935f233

stiiifff reviewed Aug 28, 2023

View reviewed changes

add comments and remove fee_asset_item

f280b8e

franciscoaguirre reviewed Sep 1, 2023

View reviewed changes

metricaez added 6 commits September 1, 2023 13:22

import through prelude

575e655

add foreign locations for LocationToAccountId

fa7b919

Merge branch 'main' into emi-proxy-teleport-pallet

42f9527

minor fixes and add execution check

3d28e3c

add obnoxious comments to avoid misusage

ab8bfbc

cargo fmt

41aea01

metricaez added 3 commits September 5, 2023 12:24

prelude wildcard

69fb3fe

cargo fmt

55fb0fe

dismiss "proxy" wording

7541ba1

stiiifff marked this pull request as ready for review September 6, 2023 11:15

stiiifff requested a review from joepetrowski September 6, 2023 11:15

joepetrowski reviewed Sep 6, 2023

View reviewed changes

pallets/withdraw-teleport/src/lib.rs Show resolved Hide resolved

joepetrowski reviewed Sep 6, 2023

View reviewed changes

pallets/withdraw-teleport/src/lib.rs Outdated Show resolved Hide resolved

joepetrowski reviewed Sep 6, 2023

View reviewed changes

typo

a5c8dd4

metricaez mentioned this pull request Sep 6, 2023

Withdraw derivative on origin #275

Merged

metricaez and others added 3 commits September 14, 2023 16:40

Withdraw derivative on origin (#275)

baefbba

Co-authored-by: Steve Degosserie <steve@parity.io> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Merge branch 'main' into emi-proxy-teleport-pallet

4477bb8

add hash of foreign location

6aca06b

stiiifff suggested changes Sep 15, 2023

View reviewed changes

pallets/withdraw-teleport/Cargo.toml Outdated Show resolved Hide resolved

metricaez added 3 commits September 15, 2023 10:19

typo and remove job limit on git workflow

3aeab64

use teleport fallback weight

8d26c7d

remove unused imports

add jobs 1 flag to check.yml

d2bac9e

stiiifff approved these changes Sep 15, 2023

View reviewed changes

stiiifff merged commit 479ffb2 into main Sep 15, 2023

stiiifff deleted the emi-proxy-teleport-pallet branch September 15, 2023 15:39

	ReceiveTeleportedAsset(foreing_assets.clone()),
	ReceiveTeleportedAsset(foreing_assets.clone()),
	RefundSurplus,

	DepositAsset { assets: MultiAssetFilter::Definite(foreing_assets), beneficiary },
	DepositAsset { assets: AllCounted(2).into(), beneficiary }

Conversation

metricaez commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intro

Issue

Workaround

Proposed Solution

Disclaimer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IkerAlus Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IkerAlus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metricaez Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

franciscoaguirre Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metricaez commented Aug 24, 2023 •

edited

Loading

IkerAlus Aug 29, 2023 •

edited

Loading

metricaez Aug 28, 2023 •

edited

Loading

franciscoaguirre Sep 1, 2023 •

edited

Loading

joepetrowski Sep 6, 2023 •

edited

Loading