Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

XCM Transact should support using multiple accounts on the target chain #6612

Closed
lucasvo opened this issue Jan 23, 2023 · 30 comments
Closed
Labels
T6-XCM This PR/Issue is related to XCM.

Comments

@lucasvo
Copy link

lucasvo commented Jan 23, 2023

This originated in the following Polkadot Forum thread and describes a feature request and/or current challenge with how XCM Transact is implemented.

XCM Transact uses a single "origin" on the target chain to execute extrinsics which means that if multiple users on the sending chain interact with a target chain the sending chain has to implement "shadow accounting" to keep track of individual user's actions. I propose the creation of a new XCM Message TransactAsAccount that allows the sending chain to control multiple accounts on the target chain.

Example Use Case A: NFT User Alice needs to trade on DEXChain to buy an NFT on NFTChain

Alice uses NFTChain to buy an NFT. She has DOT on her NFTChain account but this particular NFT needs to be purchased with USDC. NFTChain integrates DEXChain to allow swapping of tokens to buy any NFT in any currency.

Under the hood NFTChain sends DOT to DEXChain and sends a message Transact(Swap, from=DOT, to=UDC, amount=100) before transferring back the proceeds. Bob, another NFTChain user also wants to trade DOT for USDC and does the same.

If you were to try to use Transact today what would have to happen:
NFTChain needs to keep track of what the exchange rate is, how much in fees was paid and how much USDC Alice should get as well as how much Bob should get and then send respective amounts from the account DEXChain/NFTChain (the account it controls through XCM Transact back to NFTChain/Alice as well as some to NFTChain/Bob.

With a simple transaction this shadow accounting can be done relatively easily but it's impossible to generalize it beyond an AMM. Imagine DEXChain decided they want to give rewards (possibly even randomly) to traders. Now how do you allocate these rewards to users? It would only work by implementing a lot of reading back & forth and building very specific pallets to deal with every XCM Transact.

Example Use Case B: DeFiChain wants to allow users to vote with protocol tokens

Alice uses DeFiChain to manage her portfolio of tokens. She owns some DOT, some GLMR, etc. It's convenient for her to holder tokens there because she can trade and lend them out at any time.

Because so many people start holding tokens on other parachains, DeFiChain wants to offer bridged assets to participate in governance. Alice who has 10 $PROTO should be able to vote with 10 votes on the protcol chain. Bob who has 5 $PROTO should vote with weight 5. Using XCM Transact, the only way to vote would be to vote either YES or NO with 15 $PROTO which is not actually a precise representation of the tokenholders votes.

If DeFiChain would instead of holding $PROTO in the single XCM Origin account split it up into 2 accounts (DeFiChain/Alice and DeFiChain/Bob) voting could be done in a way that represents the voters accurately.

Technical Implementation proposal

We define a new XCM Message TransactAs which would take an additional argument Origin. On the receiving chain the router would instead of setting origin=SenderChainId change it to: origin=hash(senderChainId+senderSpecifiedOrigin) allowing the sending chain to transact as multiple accounts on the target chain.

@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/multichain-friendly-account-abstraction/1298/17

@KiChjang
Copy link
Contributor

It would seem to me that this proposal is describing the differences between Physical Origin and Computed Origin, which the executor is already keeping track of. It is already possible to use XCM Transact using a destination chain account as the FRAME origin via the combination of the DescendOrigin instruction and a newly constructed origin/location converter that's similar to SignedAccountId32AsNative/AccountId32Alias -- the ORML repo may already have such an origin/location converter.

As for the ability of controlling multiple accounts, this is a security risk as this entails that one compromised account on one chain can instruct multiple accounts on another to perform unintended actions. An origin is not simply a mechanism to track where an XCM or an extrinsic is coming from, but is also used as an access control mechanism.

@lucasvo
Copy link
Author

lucasvo commented Jan 24, 2023

As for the ability of controlling multiple accounts, this is a security risk as this entails that one compromised account on one chain can instruct multiple accounts on another to perform unintended actions. An origin is not simply a mechanism to track where an XCM or an extrinsic is coming from, but is also used as an access control mechanism.

I disagree here. If we allow chain A to control account "A/x", "A/y", "A/z" but not control "B/x" then there are no further security implications for the parachain than just allowing "A" to control "A" but not "B". x, y, z are just "subaccounts" but it would give huge benefits of why you would want the sending chain to control multiple accounts on the target chain (these are the use cases I described).

@KiChjang
Copy link
Contributor

I am exactly describing the case where account A gets compromised, which leads to x, y and z being compromised as well as a result of transitivity. B is irrelevant to our discussion here. All of x, y and z would essentially be giving up their own account's sovereignty to a foreign chain's account.

In addition, there is still a big question on how exactly the receiving chain keeps track of which foreign account can control which local accounts. Since the XCM subsystem is independent of FRAME, we can't really use on-chain storage to fetch and store such kinds of data, and it does bring in the question of whether these data should be on-chain at all, as it can quickly balloon in size and take up precious storage space. Any solution that suggests that FRAME and XCM should be more tightly coupled together is unlikely to gain much traction, as the two subsystems are specifically designed for different purposes.

That said, I don't believe there is a huge problem if these functionalities are tested and experimented upon as an XCM dialect, however we still do not support the notion of dialects quite just yet.

@mustermeiszer
Copy link
Contributor

mustermeiszer commented Jan 27, 2023

I think you are slightly misunderstanding each other. I guess what @lucasvo wants is something like

pub struct RemoteControlledAccount<RuntimeOrigin>(PhantomData<(RuntimeOrigin)>);

impl<RuntimeOrigin: OriginTrait> ConvertOrigin<RuntimeOrigin>
for RemoteControlledAccount<Network, RuntimeOrigin>
    where
        RuntimeOrigin::AccountId: From<[u8; 32]>,
{
    fn convert_origin(
        origin: impl Into<MultiLocation>,
        kind: OriginKind,
    ) -> Result<RuntimeOrigin, MultiLocation> {
        let origin = origin.into();
        log::trace!(
			target: "xcm::origin_conversion",
			"RemoteControlledAccount origin: {:?}, kind: {:?}",
			origin, kind,
		);
        match (kind, origin) {
            (
                OriginKind::SovereignAccount,
                MultiLocation { parents: 1, interior: X2(Junction::Parachain(para_id), Junction::AccountId32 { id, network }) },
            ) =>
                Ok(RuntimeOrigin::signed((para_id, id).using_encoded(blake2_256)),
            (_, origin) => Err(origin),
        }
    }
}

Meaning if we have a XCM of kind

vec![
   DescendOrigin(X1(Junction::AccountId32(...))),
   Transact(...)
]
  • we could have "remote" controlled accounts on other parachains, which can ONLY be controlled by the respective account on the parachain (Assuming the sending parachain does the appropriate checks upfront).
  • accounts on Parachain-A can act as equal accounts on Parachain-B (Equally treated as "native" accounts).

I think this is something the parachains must disucss if they wanna support it.

Personally, I think it is a really good approach with no risk for the receiving parachain.

cc: @xlc

@mustermeiszer
Copy link
Contributor

The converter could of course also convert into signed origins that are comming from the relay chain.

@KiChjang
Copy link
Contributor

vec![
   DescendOrigin(X1(Junction::AccountId32(...))),
   Transact(...)
]

This is already possible, in fact if you look at the XCM pallet's send_xcm function (which is used by the send extrinsic), you'll see it does prepend a DescendOrigin instruction before sending the XCM over to another chain.

As for the RemoteControlledAccount, as I've mentioned in my previous comment, this should also already be possible to implement on your own chain, and on top of that, ORML might already contain a struct that does something similar to what you've described.

@mustermeiszer
Copy link
Contributor

mustermeiszer commented Jan 31, 2023

Sorry, my choice of words was unclear. I know that and I know that this is up to the parachains to support.

But for this to be useful, it would need to be implemented by many chains, including, statemint, other common-good parachains and the relay-chain, hence I posted it here as this discussion involves Parity.

@KiChjang
Copy link
Contributor

I'm sorry but I don't think this is really what the OP was suggesting. Title says "support using multiple accounts on the target chain", and what you're suggesting is simply reusing the same 32-byte account ID from the origin chain to produce a valid SS58 address on the destination chain. None of this implies "multiple accounts on the target chain".

But for this to be useful, it would need to be implemented by many chains, including, statemint, other common-good parachains and the relay-chain, hence I posted it here as this discussion involves Parity.

You mentioned that this is something that the parachains must discuss if they want to support such an origin converter. How is Parity's involvement here relevant? Do you simply want us to write code and export something similar to the RemoteControlledAccount origin converter?

@lucasvo
Copy link
Author

lucasvo commented Jan 31, 2023

@KiChjang This is indeed what I suggested and I discussed this with @mustermeiszer before he came up with his proposal.

what you're suggesting is simply reusing the same 32-byte account ID from the origin chain to produce a valid SS58 address on the destination chain

This is what would allow for a parachain to control multiple accounts on the destination chain and nicely solves the user stories I described in my initial issue. I posted this in https://github.com/paritytech/polkadot because ultimately this would be a standard several parachains need to decide to adopt and support and most importantly it would be very convenient to implement this in statemint/e.

Implementing this on statemint would allow a Moonbeam user that does not have an ss58 capable wallet to request a withdrawal of say USDC from a CEX to their Moonbeam controlled Statemint address (hash(moonbeam/alice)) to then in a single transaction using their Ethereum-wallet trigger the bridging of the USDC to their Moonbeam account. This would not be possible without the ability for Moonbeam to control multiple addresses and a user that didn't have a Statemint account (or wasn't able to sign statemint transactions for other reasons, e.g. differing crypto or lack of wallet support) to transact on Statemint. I think though this will be useful beyond just the usecase where the user doesn't have a statemint wallet for technical reasons, I simply think it's more elegant if the user never needed to bother transacting with Statemint at all.

cc @joepetrowski

@joepetrowski
Copy link
Contributor

I simply think it's more elegant if the user never needed to bother transacting with Statemint at all.

Yes, 100%, this is what we want to be designing for.

@mustermeiszer
Copy link
Contributor

You mentioned that this is something that the parachains must discuss if they want to support such an origin converter. How is Parity's involvement here relevant? Do you simply want us to write code and export something similar to the RemoteControlledAccount origin converter?

@KiChjang I definitely didn't wanted to imply Parity should build this. I am happy to submit a PR, but I think it should go to the polkadot repo instead of ORML, as as Lucas mentioned above, it is most useful when being adapted by a lot of chains and acts as kind of a "standarf"-converter.

@KiChjang
Copy link
Contributor

The notion of a "standard" is what I am pushing hard against.

We don't want to be seen as a source of centralization for the tech that parachains use. If parachains start using the tech because the relay chain contains it, then how decentralized do you think the entire XCM tech stack is? I know it is certainly not as decentralized as we want with Substrate as a development framework, but with XCM in particular, it is all the more important that we aren't the ultimate "authorities" or "standard setters" for cross-chain interactions. I think the reasoning behind this should be apparent.

Thus, if such a functionality already exists in ORML, then I am inclined to not add it to the Polkadot repository. ORML acts more of a consensus between parachain teams, and so it is a much better fit for the kind of concepts that have evolved from the community, especially when it's something that arose from discussions between themselves, as you have noted.

With this in mind, I believe the concrete ask here is to make it so that the relay chain and all common good parachains reuse the AccountId32 origin to create a local SS58 address. That is something we can consider adding to our chains, once we really go through all the security implications behind it, especially with the lessons learned from the Mangata governance attack.

But I have to stress again -- it is a really bad idea for us to be the entity that pushes some XCM tech for adoption.

@joepetrowski
Copy link
Contributor

I don't think which repo it goes in is that important, or even if Parity calls it a "standard" or not (lots of people call things standards that nobody uses, so they are therefore not standards). This is just one of many things that belongs in an XCM tutorial or XCM-by-example book, which I think are within the realm of Parity to create.

@lucasvo
Copy link
Author

lucasvo commented Feb 1, 2023

The reason I posted an issue in this repo and am asking about both the relay chain and common good parachains to support it is because XCM actually is a standard and not having interoperability across parachains is a huge blocker for the ecosystem to grow (ask any of the parachains about what are hurdles for growth and they'll definitely come up with xcm complexity and the fact that chains are separate as a major hurdle compared to EVM - of course it's also a major scalability advantage vs. a single chain but we're getting off topic).

Imagine Centrifuge actually starts building and deploying the above code and decides to convert origins using the following pseudocode: function deriveAddress(origin) { return Hash("polkadot/"+origin") } and 2mo later the relay chain decides to use function deriveAddress(origin) { return Hash("substrate/polkadot/"+origin") }. You now have two very different addresses meaning the same thing and everyone building around it has to add a check to make sure it's one of the two implementations.

I started this issue because I do think it makes sense for the community to agree on this and that unfortunately includes the bridgehub and statemint which are both implemented by Parity. You could claim this is centralization but I can tell you that for every parachain I talk to, it's out of the question that compatibility and integration into statemint is number 1 integration target.

But I have to stress again -- it is a really bad idea for us to be the entity that pushes some XCM tech for adoption.

Luckily it's not you that's pushing this but us - a parachain team. Do we want your feedback and buy in? Of course.

Before this becomes a philosophical discussion around the centralization of substrate core development, I'd love to have an actual discussion about the proposed feature which we're getting off-topic on.

@xlc
Copy link
Contributor

xlc commented Feb 1, 2023

This is a must have feature for many use cases. For example, the Karura Liquid Staking protocol is already operating multiple sub-accounts using utility.as_derivative on Kusama to perform staking action. This is required due to staking cap per account and better accounting practice.

Another use case could be asset routing. For example, if someone want to send DAI on Ethereum to HydraDX via wormhole on Acala, we could have the wormhole Ethereum => Acala transfer fund to Hash(HydraDX/Alice) address and then Alice on HydraDX can dispatch XCM to withdraw the fund on Acala to HydraDX. This means Alice does not need to perform any transaction on Acala but still use it to route assets. Similarly, this feature will be useful for the bridge hub chain as well.

@girazoki
Copy link
Contributor

girazoki commented Feb 1, 2023

I agree with the thread in general, there are several use cases in which we would want a computed origin (e.g., Alice in Moonbeam) to control an account in another consensus system. I also believe hashing is the best way for these accounts not to be manipulated accross different physical origins.

In this regard there already exists the Account32Hash structure in xcm-builder that does precisely this (it hashes the multilocation to get an account). This is something we are using in the Moonbase network already.

Is there any reason why we want to change from Hash(MultiLocation) to Hash(para_id || accountId)? If this is a compatibility concern (maybe multilocations will change in the future)? Do we know if this will not be attackable by cross-ecosystem messages (e.g., Alice from parachain 2023 in Polkadot sending a message to Karura might get the same derived account as Alice in Moonriver)

I also dont believe parity needs to be involved on this particular one, as this is a usecase that parachains are willing to support, not the relay. I think if it becomes use by sufficient number of chains it might get itself into xcm-builder.

On a further note, I think discrepancies on how different chains convert MultiLocations to AccountIds can be solved by just having a runtime-API that we can query on each chain.

@jak-pan
Copy link

jak-pan commented Feb 1, 2023

After some longer back and forth with @lucasvo I completely agree that this is a good addition which would already serve use-cases across the whole ecosystem incl. liquid staking, multiple parachain treasuries, DAO chains etc.

To make it clear I think this should be completely up to each parachain to handle logic behind this. It just allows easier account abstractions and use-cases. It is critical to make this secure and opt-in. In that case, I don't see any reason to push back.

@lucasvo
Copy link
Author

lucasvo commented Feb 1, 2023

Is there any reason why we want to change from Hash(MultiLocation) to Hash(para_id || accountId)? If this is a compatibility concern (maybe multilocations will change in the future)? Do we know if this will not be attackable by cross-ecosystem messages (e.g., Alice from parachain 2023 in Polkadot sending a message to Karura might get the same derived account as Alice in Moonriver)

I agree, a multilocation should probably be used, I understand substrate at a high level but this was mostly my ignorance (and need to simplify it). I think the address converter should:

  1. Ensure that the multilocation has the appropriate prefix (it matches the relaychain & parachain id where the xcm message is coming from) < this is the security critical part that ensures no funds in control by parachain A can be stolen by parachain B.
  2. Then use the MultiLocation to Account32Hash converter to determine the Account Id.

I also dont believe parity needs to be involved on this particular one, as this is a usecase that parachains are willing to support, not the relay. I think if it becomes use by sufficient number of chains it might get itself into xcm-builder.

I slightly disagree on this:

  1. @xlc mentioned a good use case that they already have on the relay chain that would help them
  2. For us, a big reason we want this functionality is to be able to manage different token balances and do stuff like instant withdrawals from say Kraken to an account centrifuge/alice and then have functionality on Centrifuge chain to trigger a bridge transaction. This makes removes the need to support to trigger XCM messages from any wallet to allow bridging of tokens, all you need to do is transfer a token to centrifuge/alice and it's clear for Centrifuge that alice on Centrifuge should be able to receive these tokens.

I do agree though, it's not critical for this to be implemented in the relay chain right away. But it doesn't hurt to have relaychain developers and bridgehub/statemint team weigh in on the design.

@joepetrowski joepetrowski added the T6-XCM This PR/Issue is related to XCM. label Feb 1, 2023
@mustermeiszer
Copy link
Contributor

I personally wouldn't use the Account32Hash in its current form.

  • Every change to the encoding of Multilocation will be a silent change that affects form of the resulting AccountId32
  • The current conversion does not have any checks on the inner structure of the MultiLocation

In order to push this forward I created a draft PR #6662, where we can discuss details.

Note

This is on the polkadot repo as

  • this makes most sense if being adopted by the common good parachains in the polkadot repo (Statemint, Statemine)
  • having an orml dependency in the polkadot repo creates a "circular" dependency between those two → upgrading polkadot will depend on orml, updating orml will depend on polkadot

@girazoki
Copy link
Contributor

girazoki commented Feb 2, 2023

I think Account32Hash is safe to use as long as there is no breaking change (i.e., as long as there is backwards compatibility). I would agree that there is no check on the inner structure, but you can add it yourself using a wrapper around it.

That being said I am not against the new conversion.

@mustermeiszer
Copy link
Contributor

mustermeiszer commented Feb 2, 2023

I think Account32Hash is safe to use as long as there is no breaking change (i.e., as long as there is backwards compatibility). I would agree that there is no check on the inner structure, but you can add it yourself using a wrapper around it.

Out of interest. Assuming the variant orders of Junction are changed in v4. pallet-xcm & orml-xtokens both use let des: latest::MultiLocation = MultiLocation::try_from(Box<VersionedMultiLocation>). Then the resulting

Account32Hash::convert(v3::MultiLocation) != Account32Hash::convert(v4::MultiLocation) although the try_from works just fine.

Or am I missing something or are you handling this differently on Moonbeam?

@gavofyork
Copy link
Member

gavofyork commented Feb 3, 2023

This is implemented as AccountId32Aliases, and allows accounts on specified networks to be able to transact over XCM as the same ID on the local chain.

The main issue against having this be widespread is cross-chain security. If one parachain's runtime logic becomes insecure and allows an unprivileged user to send arbitrary XCM or dispatch a call locally as though it's from a desired account, then instead of just compromising every account on that chain (which is the case at present), they would also compromise every account on every chain.

It is reasonable to use when the security is inherited or equivalent; since both Statemint and Encointer can only be upgraded by the Relay-chain it's not not unreasonable for them to allow account aliasing between each other and from the Relay-chain. The same cannot be said about community chains whose upgrade/governance logic is entirely arbitrary.

For this reason, an XCM from a (say) Acala origin which purported to be from account "GAV" on Acala could never control account "GAV" on Statemint or the Relay-chain. It's just too cheap for Acala to have its runtime upgraded maliciously and break the entire network.

I can see that Account32Hash is too volatile to be an effective solution here. I'd suggest something similar to it but which only worked for specific location patterns (e.g. Parent/Parachain(p)/AccountId32(a)) and was stable over XCM versions/encodings (e.g. hash(&(b"SiblingChainAccount32Hash", p, a).encode()), similar to what has already been proposed above). It would still result in a different account ID on each target chain though.

If a group of chains really want to actually allow account aliasing between them with the security implications it brings, I would recommend an opt-in approach, whereby an origin of Parent/Parachain(p)/AccountId32(a) could be mutated into AccountId32(a) if and only if preauthorised by a transaction from a mentioning p on the target chain (perhaps stored in a flag within XCM Pallet). There is already an issue open for an instruction which would make this possible with the appropriate configuration. See https://github.com/paritytech/polkadot/issues/4118 under Origin Aliasing: .

@mustermeiszer
Copy link
Contributor

I think this is not right. If a runtime upgrade leads to the said behaviour, it is true that the all Acala derived accounts in other parachains are also exposed, but this does not introduce any risk to this parachains "local" accounts.

Or what am I missing?

@gavofyork
Copy link
Member

gavofyork commented Feb 3, 2023

As long as it aliases to a different account on each target chain, then there is no immediate security issue. I was however drawing from @xlc's comment:

transfer fund to Hash(HydraDX/Alice) address and then Alice on HydraDX can dispatch XCM to withdraw the fund on Acala to HydraDX

If I understand the intent correctly, then HydraDX must be able to have control over Alice on Acala. This means if HydraDX suffered a buggy or malicious upgrade it could compromise ALL accounts on Acala.

There is also always the issue of whether you want a parachain to be able to trivialise a sybil attack. This is an argument against allowing derivative accounts on the Relay-chain for staking purposes. However, in this case I fear the cat is largely out of the bag.

@mustermeiszer
Copy link
Contributor

If I understand the intent correctly, then HydraDX must be able to have control over Alice on Acala. This means if HydraDX suffered a buggy or malicious upgrade it could compromise ALL accounts on Acala.

Ahh, yes. But I hope this is not what @xlc was intending. I thought he meant

  • transfer DAI from Ethereum-to-Acala to Alice-alias Acala account
  • Trigger withdraw from Alice-alias on Acala from Alice on Hydra

@xlc
Copy link
Contributor

xlc commented Feb 4, 2023

My intent is that Alice on HydraDX will have access Alice' on Acala, which could be address generated with ParaId(HydraDX) ++ Hash(Salt ++ Alice). This ensures compromised HydraDX will not be able to control accounts not originated from HydraDX.

@gavofyork
Copy link
Member

I would accept a PR bringing RemoteControlledAccount into the XCM codebase. The name could perhaps be more consistent with other implementations (perhaps mentioning SiblingChain), but the only important change would be introducing a label into the hash to secure against accidental account collisions (prepending b"SiblingChainAccount32Hash" into the encoding for the hash would be enough).

@mustermeiszer
Copy link
Contributor

@gavofyork and the others PR is here #6662

@KiChjang
Copy link
Contributor

Fixed by #6662.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

No branches or pull requests

9 participants