Cross chain execution and payments via UserOps #1

lalexgap · 2023-11-10T16:28:04Z

TODO

Add more thorough testing
Implement proper nonce support
Turn PR description into readme.md

lalexgap · 2023-11-12T18:02:32Z

contracts/SCBridgeWallet.sol

+    if (
+      validateSignature(userOpHash, ownerSig, owner) != SIG_VALIDATION_SUCCEEDED
+    ) {
+      return SIG_VALIDATION_FAILED;


The ERC 4337 spec specifies that the wallet SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch, so I've changed this from a revert to return SIG_VALIDATION_FAILED

bitwiseguy · 2023-11-14T20:12:21Z

Thanks for all of the details in the PR description. It might be useful to turn this PR description into a .md file within the repo. That way the documentation is not lost when this PR is merged and it is easy to leave comments on specific parts of the writeup as we are reviewing

contracts/SCBridgeWallet.sol

lalexgap · 2023-11-14T22:56:07Z

Thanks for all of the details in the PR description. It might be useful to turn this PR description into a .md file within the repo. That way the documentation is not lost when this PR is merged and it is easy to leave comments on specific parts of the writeup as we are reviewing

8babf50

contracts/SCBridgeWallet.sol

docs/cross-chain-payments-and-execution.md

geoknee · 2023-11-15T12:48:37Z

docs/cross-chain-payments-and-execution.md

+
+# Multihop
+
+Since a UserOp can contain multiple `PaymentChainInfo`s and `ExecuteChainInfos`, this approach can be used for multi-hop execution or payments. We just require the `owner` and `intermediary` of every BridgeWallet involved to sign the UserOp.


When we go beyond a single hop, we have "intermediary-intermediary" channels. Is this a bit awkward with the "owner" / "intermediary" roles of the bridge wallet? Will one of the parties just arbitrarily take the role of "owner"?

I think right now it might be a bit awkward, as once intermediary would play the role of the owner and one intermediary would play the role of intermediary in the BridgeWallet.

To solve this we could implement a specific Intermediary<->Intermediary mode (or separate contract?) for the BridgeWallet that handles the "intermediary-intermediary" use case.

geoknee · 2023-11-15T12:51:21Z

docs/cross-chain-payments-and-execution.md

+
+It's important that we're not vulnerable to replay attacks, where a `UserOp` is submitted again using a different entrypoint or chain.
+
+If we're working on one chain, it's fairly easy to prevent replay attacks. The `userOpHash` [provided by the Entrypoint](https://github.com/magmo/Bridge-Wallet/blob/ad6d24fa2435f449751d1b61e24d12faff1f83a9/contracts/core/EntryPoint.sol#L298) to `validateUserOp` is hashed against the current chain and entrypoint. This means that if the UserOp is run against a different chain or entrypoint, there will be a different `userOpHash` causing all the signature checks to fail.


This feels a little suboptimal. I suppose the entrypoint has not been designed with cross chain applications in mind, meaning that we have to make multiple hashes and a signature on each one (and as you say validate all of those combinations).

A "cross chain entrypoint" would ease this, but would be a departure from 4337 norms (I guess?).

With this change I don't think it's too bad. Now instead of generating a unique hash for each chain, all participants just sign the hash based on the first chain. This means we can generate one hash and check all the signatures against that.

I think it's safe to have everyone sign the same UserOpHash since we perform a separate check on the chainId and entrypoint based on the calldata in the UserOp. If you try submitting the UserOp, it will only work if the chain and entrypoint match a ExecuteChainInfo or PaymentChainInfo.

So the cost of validateCrossChain would be generating one hash, and then checking 2 signatures for every BridgeWallet involved. IE: If we have a multi-hop UserOp with Alice,Irene,Iris,Bob we'd expect the UserOp to have the following signatures: [AliceSig,IreneSig,IreneSig,IrisSig,IrisSig,BobSig]. We'd generate one hash and check the6 signatures against that hash.

It would also be an easy optimization to omit or skip duplicate signatures so we could get the cost down to generating one hash and checking 1 signature per participant.

geoknee · 2023-11-15T13:01:17Z

test/SCBridgeWallet.test.ts

+        ownerSig,
+        intermediarySig,


Should we extend the test to work with two different chains (and chain ids) somehow?

Yup, I've created #4 for that. We'll need to rig up a test that works with multiple chains.

geoknee · 2023-11-15T13:03:46Z

contracts/SCBridgeWallet.sol

+    // Only the entrypoint should trigger this by excecuting a UserOp
+    require(msg.sender == entrypoint, "account: not EntryPoint");


Curious if this is necessary -- should we have had this check before? Should all 4337 wallets have this check?

It also seems to be replicated below?

This is needed since we want to enforce that crossChain is only triggered through submitting a UserOp. Otherwise anyone could call with crossChain, using a transaction from an EoA, and whatever PaymentChainInfo or ExecutionChainInfo they wanted. Since challenge is allowed to be called at any time, someone could call crossChain with a bogus unsigned state and trigger a challenge using that.

Should all 4337 wallets have this check?

It depends on the function and the 4337 wallet! In a lot of cases there will be functions restricted to only some owner or entrypoint(IE: A function to send user funds)

require( msg.sender == entrypoint || msg.sender == owner, "account: not Owner or EntryPoint" );

This allows the owner of the wallet to call the function using either an EoA account or through submitting a UserOp.

In our case we need to be a bit stricter, we only want to trigger crossChain if a UserOp has been submitted signed by all the participants. We're relying on the signature validation of the UserOp to protect the call to crossChain.

It also seems to be replicated below?

I think that's in a separate execute function, which can be triggered outside of crossChain. So we need to implement the check in both functions.

contracts/SCBridgeWallet.sol

geoknee · 2023-11-15T13:09:34Z

Adding a multi-chain sequence diagram would be really good.

docs/cross-chain-payments-and-execution.md

bitwiseguy · 2023-11-15T15:58:42Z

docs/cross-chain-payments-and-execution.md

+
+# Payments
+
+We embed payment information in the UserOp using a `PaymentChainInfo`


Could the structs be renamed to PaymentInfo and ExecuteInfo for simplicity? Or does the use of Chain in those names help clarify what they are?

I think the use of Chain helps clarify that info in these structs is specific to a given chain, and should only be honoured on that chain. I don't feel too strongly about it though so I'm happy to change it if others feel differently.

docs/cross-chain-payments-and-execution.md

contracts/SCBridgeWallet.sol

Co-authored-by: Colin Kennedy <NiloCK@users.noreply.github.com> Co-authored-by: George C. Knee <georgeknee@googlemail.com> Co-authored-by: Sam Stokes <35908605+bitwiseguy@users.noreply.github.com>

lalexgap · 2023-11-15T19:34:00Z

Adding a multi-chain sequence diagram would be really good.

Add 2 hop RPC example

lalexgap force-pushed the cross-chain-exec branch from dbbc4c5 to 3281226 Compare November 10, 2023 21:48

lalexgap marked this pull request as ready for review November 10, 2023 22:08

lalexgap closed this Nov 10, 2023

lalexgap reopened this Nov 10, 2023

lalexgap marked this pull request as draft November 10, 2023 22:37

lalexgap marked this pull request as ready for review November 10, 2023 23:48

lalexgap commented Nov 12, 2023

View reviewed changes

lalexgap changed the title ~~Cross chain execution using signed UserOps~~ Cross chain execution and payments via UserOps Nov 12, 2023

lalexgap marked this pull request as draft November 13, 2023 00:12

lalexgap force-pushed the cross-chain-exec branch from 81dc9f0 to 17bada2 Compare November 14, 2023 18:33

lalexgap added 2 commits November 14, 2023 10:38

run formatter

4689e09

implement cross-chain execution and payment

4644179

lalexgap force-pushed the cross-chain-exec branch from 17bada2 to 4644179 Compare November 14, 2023 18:42

lalexgap marked this pull request as ready for review November 14, 2023 18:43

lalexgap commented Nov 14, 2023

View reviewed changes

contracts/SCBridgeWallet.sol Show resolved Hide resolved

lalexgap mentioned this pull request Nov 14, 2023

Challenge expires immediately with no HTLCs #3

Closed

move pr desc to markdown

8babf50

lalexgap added 6 commits November 14, 2023 15:08

add a bit more detail

c7bcf39

move test

b917704

add test for cross chain execute

66dbb9c

add nonce info

473e08c

Add link

bf20e33

add nonce key support

aebbd8b

geoknee approved these changes Nov 15, 2023

View reviewed changes

bitwiseguy reviewed Nov 15, 2023

View reviewed changes

docs/cross-chain-payments-and-execution.md Outdated Show resolved Hide resolved

bitwiseguy reviewed Nov 15, 2023

View reviewed changes

docs/cross-chain-payments-and-execution.md Outdated Show resolved Hide resolved

NiloCK reviewed Nov 15, 2023

View reviewed changes

contracts/SCBridgeWallet.sol Outdated Show resolved Hide resolved

lalexgap and others added 2 commits November 15, 2023 10:35

Doc tweaks

36259fe

Co-authored-by: Colin Kennedy <NiloCK@users.noreply.github.com> Co-authored-by: George C. Knee <georgeknee@googlemail.com> Co-authored-by: Sam Stokes <35908605+bitwiseguy@users.noreply.github.com>

just generate hash once

c4829f5

lalexgap force-pushed the cross-chain-exec branch from 3b6d304 to c4829f5 Compare November 15, 2023 19:16

lalexgap added 2 commits November 15, 2023 11:19

clarify readme

fd2842d

update code snippets to solidity

a9545dc

lalexgap and others added 7 commits November 15, 2023 11:48

rename to _challenge

437c20f

add link

3cc0fa4

add .gitattributes file

694d432

emphasize that the chainid must match

7be2234

typo

7d74d7c

add 2 hop example and diagram

a1e230c

Merge pull request #5 from magmo/gk-cross-chain-exec

ded0c82

Add 2 hop RPC example

lalexgap merged commit 99c7c3c into main Nov 29, 2023
1 check passed

lalexgap deleted the cross-chain-exec branch November 29, 2023 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross chain execution and payments via UserOps #1

Cross chain execution and payments via UserOps #1

lalexgap commented Nov 10, 2023 •

edited

Loading

lalexgap Nov 12, 2023 •

edited

Loading

bitwiseguy commented Nov 14, 2023 •

edited

Loading

lalexgap commented Nov 14, 2023

geoknee Nov 15, 2023

lalexgap Nov 16, 2023 •

edited

Loading

geoknee Nov 15, 2023

lalexgap Nov 15, 2023 •

edited

Loading

geoknee Nov 15, 2023

lalexgap Nov 15, 2023

geoknee Nov 15, 2023

geoknee Nov 15, 2023

lalexgap Nov 15, 2023 •

edited

Loading

geoknee commented Nov 15, 2023

bitwiseguy Nov 15, 2023

lalexgap Nov 15, 2023

lalexgap commented Nov 15, 2023 •

edited

Loading


		# Multihop

		Since a UserOp can contain multiple `PaymentChainInfo`s and `ExecuteChainInfos`, this approach can be used for multi-hop execution or payments. We just require the `owner` and `intermediary` of every BridgeWallet involved to sign the UserOp.


		It's important that we're not vulnerable to replay attacks, where a `UserOp` is submitted again using a different entrypoint or chain.

		If we're working on one chain, it's fairly easy to prevent replay attacks. The `userOpHash` [provided by the Entrypoint](https://github.com/magmo/Bridge-Wallet/blob/ad6d24fa2435f449751d1b61e24d12faff1f83a9/contracts/core/EntryPoint.sol#L298) to `validateUserOp` is hashed against the current chain and entrypoint. This means that if the UserOp is run against a different chain or entrypoint, there will be a different `userOpHash` causing all the signature checks to fail.

		// Only the entrypoint should trigger this by excecuting a UserOp
		require(msg.sender == entrypoint, "account: not EntryPoint");


		# Payments

		We embed payment information in the UserOp using a `PaymentChainInfo`

Cross chain execution and payments via UserOps #1

Cross chain execution and payments via UserOps #1

Conversation

lalexgap commented Nov 10, 2023 • edited Loading

TODO

lalexgap Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

bitwiseguy commented Nov 14, 2023 • edited Loading

lalexgap commented Nov 14, 2023

Choose a reason for hiding this comment

lalexgap Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalexgap Nov 15, 2023 • 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

lalexgap Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

geoknee commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalexgap commented Nov 15, 2023 • edited Loading

lalexgap commented Nov 10, 2023 •

edited

Loading

lalexgap Nov 12, 2023 •

edited

Loading

bitwiseguy commented Nov 14, 2023 •

edited

Loading

lalexgap Nov 16, 2023 •

edited

Loading

lalexgap Nov 15, 2023 •

edited

Loading

lalexgap Nov 15, 2023 •

edited

Loading

lalexgap commented Nov 15, 2023 •

edited

Loading