monerium / smart-contracts

ERC20 compatible e-money deployed on Ethereum
https://monerium.com
Apache License 2.0
35 stars 21 forks source link

ERC-1271 (potentially ERC-6492) Account Ownership Verification #39

Closed kevtechi closed 2 months ago

kevtechi commented 12 months ago

Hi,

Kevin from Citizen Wallet here.

We use account abstraction (ERC4337) wallets for everything.

We were trying to set up Wallet Connect. Signing the "personal_sign" payload and sending it back to Monerium by using the address of the private key works. But attempting to sign by providing the address of our Smart Contract Wallet fails. We tried following a ERC191 version 0x00 signature, but the check always seems to use ecrecover.

Screenshot 2023-10-05 at 12 15 26

Is there something that I'm missing or do you not support Smart Contract Wallets?

kerma commented 12 months ago

hi @kev-techi ! We do support https://eips.ethereum.org/EIPS/eip-1271 however the amount of different contracts we've been able to test with has been limited so far. The main use case was to support multi-sigs.

Please try with posting "0x" as the signature (as per https://monerium.dev/api-docs#operation/profile-addresses multi signature address payload). Given you smart contract address implements 1271 it could work.

kevtechi commented 12 months ago

Ok, I'll give it a shot!

kerma commented 12 months ago

We actually do support smart contract 65 byte signatures in our contracts. But our backend cannot handle them correctly atm. We'll look into enabling this soon as well.

kevtechi commented 12 months ago

Ok, will you update here when it's enabled?

kerma commented 12 months ago

@kev-techi Can you give me an example address to check with? I just need the address part on any of our supported networks.

gislik commented 12 months ago

It’s actually already working for either a single signer or a 1-of-N multisig wallets

Here’s a link to an example using a Safe wallet on Görli.

IMG_2157

gislik commented 12 months ago

Here’s a link to the API docunentation to add a smart wallet. Make sure that you include the “chain” and “network” parameters.

IMG_2158

kevtechi commented 12 months ago

Hey, I'm doing this through Wallet Connect. I'm not calling your API directly, just responding to the session and signing requests I receive. This triggers your frontend to make those API calls automatically.

Screenshot 2023-10-06 at 23 12 35

I get a "personal_sign" request which I need to sign and send back. I can only send back a single string. This should be the signed version of what you sent me.

Normally, there should be an auth request from your side which I can respond to with a structured signature.

Hence my feeling that this flow was missing from your Wallet Connect implementation.

kevtechi commented 12 months ago

When I scan this from our wallet and approve the session, Monerium shows a "Connect wallet" page:

Screenshot 2023-10-06 at 23 25 38

The wallet address at the top is the smart contract wallet. Now, when I press the "Connect" button, I receive this:

{id: 1696627566032919, jsonrpc: 2.0, method: wc_sessionRequest, params: {request: {method: personal_sign, params: [0x4920686572656279206465636c6172652074686174204920616d207468652061646472657373206f776e65722e, 0x0eB87B2b50183FDa651130c9543E8b822F3ff2A7]}, chainId: eip155:1}}

"personal_sign" expects that I sign params[0] and send it back. When I send it back, the "Connect wallet" page makes the API call you referenced in your documentation. It calls https://monerium.app/api/emoney/profiles/{profileId}/addresses POST with this payload:

{
    "message": "I hereby declare that I am the address owner.",
    "address": "0x0eB87B2b50183FDa651130c9543E8b822F3ff2A7",
    "network": "mainnet",
    "chain": "ethereum",
    "signature": "0x12383418b499e8cc5fec663eb81dc8973dbe655c95a44e1d1a5b141a9237bfa27af5d8739b93cd17b6b7d266dc24038e2310351204a178673666df63ce2c1dfb2c",
    "accounts": [
        {
            "currency": "eur",
            "chain": "polygon",
            "network": "mainnet",
            "isVisible": true
        },
        {
            "currency": "eur",
            "chain": "ethereum",
            "network": "mainnet",
            "isVisible": true
        }
    ]
}

And your backend responds with:

{
    "code": 400,
    "status": "Bad Request",
    "message": "Invalid signature",
    "details": {
        "address": "0x0eB87B2b50183FDa651130c9543E8b822F3ff2A7",
        "message": "address recovered from signature does not match given address",
        "recovered": "0xafdbee37289279a160665da4acc573d37d965a9e"
    }
}

Screenshot 2023-10-06 at 23 26 31

kevtechi commented 12 months ago

If I set the address as the private key's address (and not the smart contract wallet), it works. From this, I know that I am following the Wallet connect docs correctly.

I can see from here that I should receive an Auth Request rather than "personal_sign". This would allow me to respond with something like this:

await web3Wallet.respondAuthRequest(
  id: args.id,
  iss: 'did:pkh:eip155:1:0x06C6A22feB5f8CcEDA0db0D593e6F26A3611d5fa',
  signature: CacaoSignature(t: CacaoSignature.EIP191, s: sig),
);
kevtechi commented 12 months ago

Wallet Connect supports Smart Contract Wallets and so does your API, so it might be just tweaking that "Connect wallet" page to send the proper request when I hit "Connect".

kerma commented 12 months ago

@kev-techi maybe a stupid question, but why you say https://etherscan.io/address/0x0eB87B2b50183FDa651130c9543E8b822F3ff2A7 is a smart contract address? There is no code associated with it on chain, so to me it looks like any other externally owned address.

kevtechi commented 11 months ago

I was giving an example just to explain the flow 🙂.

It is here: https://polygonscan.com/address/0x0eB87B2b50183FDa651130c9543E8b822F3ff2A7#tokentxns

kerma commented 11 months ago

Ok, that explains. What I can tell so far is that:

Two hints here, frontend connected address looks to be ethereum:

image

Payload includes eth mainnet (because it is using the connected wallet for information):

image

Can you switch the network once you land on the signature page or pick the polygon already while in your wallet? Atm it looks like you start your connection on eth mainnet, but you actually want to link polygon address.

kevtechi commented 11 months ago

In your wallet connect configuration, Ethereum is mandatory and Polygon is optional.

kevtechi commented 11 months ago

It would be great if I could simply only link the network I want.

gislik commented 11 months ago

Hey I noticed something similar when I was trying to link the Safe wallet.

Can you do me a favor? When you reach this screen, can you reload the page. Once I reloaded it changed page to only care about the network that I wanted. If you can confirm that to work for you then, we’d be able to fix on our end.

By the way. Is there a way for me to replicate what you’re doing on my side?

kevtechi commented 11 months ago

A wc_sessionPropose jsonrpc request is sent as soon as I scan the Wallet Connect QR code. This is Monerium specifying what rpc methods and chains it supports.

It always specifies a single required name space eip155:1 with methods [eth_sendTransaction, personal_sign]. Polygon and Gnosis are mentioned as optional.

These are the params:

{requiredNamespaces: {eip155: {chains: [eip155:1], methods: [eth_sendTransaction, personal_sign], events: [chainChanged, accountsChanged], rpcMap: {1: https://cloudflare-eth.com}}}, optionalNamespaces: {eip155: {chains: [eip155:137, eip155:100], methods: [eth_sendTransaction, personal_sign, eth_accounts, eth_requestAccounts, eth_sendRawTransaction, eth_sign, eth_signTransaction, eth_signTypedData, eth_signTypedData_v3, eth_signTypedData_v4, wallet_switchEthereumChain, wallet_addEthereumChain, wallet_getPermissions, wallet_requestPermissions, wallet_registerOnboarding, wallet_watchAsset, wallet_scanQRCode], events: [chainChanged, accountsChanged, message, disconnect, connect], rpcMap: {1: https://cloudflare-eth.com, 100: https://rpc.gnosischain.com, 137: https://polygon-rpc.com}}}, relays: [{protocol: irn}], proposer: {publicKey: 56f4e78a18f0ec25c04bd8455dbb72d06a24e5caefa06fee4d359b7a86f4ae44, metadata: {description: , url: https://monerium.app, icons: [https://monerium.app/icon.png], name: Accounts | Monerium}}}

It would be great if you could only request the relevant chain: https://specs.walletconnect.com/2.0/specs/clients/sign/namespaces

kevtechi commented 11 months ago

Hey I noticed something similar when I was trying to link the Safe wallet.

Can you do me a favor? When you reach this screen, can you reload the page. Once I reloaded it changed page to only care about the network that I wanted. If you can confirm that to work for you then, we’d be able to fix on our end.

By the way. Is there a way for me to replicate what you’re doing on my side?

reloading does not change anything since the request happens before this screen.

I tried it anyway and it still always defaults to ethereum.

kevtechi commented 11 months ago

Alright, I deployed an Account Factory and generated an ERC4337 Account Contract with 1271 verification: Account: 0xC19Ee7B6E4204dcA883D4EB8449Ee5F25cf0a0E4

Account Factory: 0xFE213c74e25505B232CE4C7f89647408bE6f71d2

When I use this, the choices are then limited to only Ethereum on Monerium.

Screenshot 2023-10-09 at 00 35 02

I was then able to sign as that contract and the connection was accepted.

It still shows all 3 chains in the dashboard.

Screenshot 2023-10-09 at 00 47 43

This is great, because it shows that it's possible 🎉 .

There is still the issue where it's not possible for me to do this for Polygon. I would have to deploy a contract under the same address on Ethereum as on Polygon and then sign on Ethereum.

Is it possible to fix this so that I can select and sign for Polygon?

kevtechi commented 11 months ago

Source code for the Account: https://github.com/citizenwallet/smartcontracts/blob/main/contracts/accounts/Account.sol

Source code for the Account Factory: https://github.com/citizenwallet/smartcontracts/blob/main/contracts/accounts/AccountFactory.sol

kevtechi commented 11 months ago

Hey @gislik, will you guys be able to look into a fix for the frontend of Monerium here?

It's going to be quite heavy to make Etherium Contract Wallets as well for people on Polygon.

einaralex commented 11 months ago

I'm looking into this @kev-techi . We are using RainbowKit which abstracts all of the WC namespace management, I should be able to detect the chainId in the wallet before rendering the modal. I'll do some testing.

einaralex commented 11 months ago

@kev-techi Quick question. Before you sign, are you unable to choose the chain logo in the upper left corner and choose Polygon, before clicking the Link button so that the signature request will be performed on Polygon?

I know this is not a solution to your problem, but should allow you to continue. Let me know and then we'll map out the next steps.

kevtechi commented 11 months ago
Screenshot 2023-10-14 at 15 04 45

Chain selection only comes after the QR code. The session proposal and approval are already sent back and forth with the wrong chain at that point.

Screenshot 2023-10-14 at 15 20 22

But if I try switching chains after, it does the trick. Then it connects!

Thanks for the tip @einaralex .

We would still appreciate there being some way that is more intuitive to be able to decide "Polygon" for the user. Either we send something to you when we scan the QR code (through Wallet Connect) or the user interface on Monerium guides them to pick a chain first (if you go with the second option and there is a route "/accounts/new?chain=137", we can then open that page).

We're onboarding non-crypto people here, so all these terms are foreign to them. Therefore, we're trying to remove as much complexity and crypto-speak as possible.

kevtechi commented 11 months ago

After the successful Wallet Connect connection, is it possible to get the IBAN number? Now that a Monerium account is configured, it would be great to show people the IBAN they can transfer to top receive EURe.

kevtechi commented 11 months ago

@einaralex any updates on the frontend fix?

einaralex commented 11 months ago

Hey @kev-techi, I plan to fix this in our downtime, we are currently in an active sprint cycle. Regarding displaying the IBAN, we are also in the process of updating our UI to make it more focused on the IBAN. Sorry for the late reply, there have been a lot of incidents recently with our growing customer base.

kevtechi commented 11 months ago

That makes sense, thanks for the response!

JonasBoury commented 10 months ago

Is this already fixed now?

einaralex commented 9 months ago

@kev-techi I removed Ethereum from being the enforced chain, it is still considered the required chain according to WC V2, but it should allow Polygon or Gnosis if the provider reads that from the wallet when connecting. Could you try again?

@JonasBoury I think the our new smart contracts are ready, but still in the process of being audited before we release them

kevtechi commented 9 months ago

Great news! I will check again.

einaralex commented 7 months ago

Did it work as expected?

kevtechi commented 6 months ago

Hey @einaralex , I just tried it the new interface (sorry for the delay) and it now shows "Polygon" correctly as the selected chain.

However, the flow that was working previously seems to not be working anymore. I can get up to this point when a personal_sign request is sent:

Screenshot 2024-03-12 at 00 59 05

But after signing the message and returning it, I don't get any response (no error, no success).

I tested my hash + signature pair and they seem to be correctly constructed (I am able to ecrecover properly).

I also tested directly against the isValidSignature function on my Smart Contract Account and it returns the proper magic value 0x1626ba7e.

You can try here: hash: 0xb77c35c892a1b24b10a2ce49b424e578472333ee8d2456234fff90626332c50f signature: 0x146be9fec7bd23c43c5c1d7377484657d21629be2bf8431ce798ea6b243e0d485f04a48d271a8f46e60b18d5920b93fa5882f06aeffb3c5a3a4e88f1b08f81981b

https://polygonscan.com/address/0x260965c708773E5754b5234d940AE8AbE0DF3d39#readProxyContract

Screenshot 2024-03-12 at 01 44 49

The contract's owner is 0xFcC70e5bCF685cD1342063144d4dcb6922c2c438, this can be recovered from the hash + signature.

I'm not sure what the issue could be. Is it possible to expose the error that is occurring (there's nothing in the Network tab, Console or returned on the App's side)?

kevtechi commented 6 months ago

If I return an invalid signature on purpose (I add an extra letter for example), I still don't receive any errors.

einaralex commented 6 months ago

@kev-techi Just to be sure, you are building on top of Safe, right? We just noticed a few days ago that something recently changed that disabled the Safe on-chain signing.

TL:DR: Our API only supports on-chain signatures and expects signatures to be 0x for smart contract wallets.


What is happening today: when trying to sign, an off-chain signature is requested; if all owners sign, a signature hash is acquired, and the request to our API is sent off. This does not work because we do not support off-chain signatures yet, and when using smart contract wallets, our backend always expects the signature to be 0x

How it should work: When a signature request is proposed, the request to our API is sent with the signature as 0x, and it will respond with a 202 Status. When all owners have signed, the account will be created.

I've been working on a fix for this, and it's almost thoroughly tested and ready to be released to production ideally this week. I'll notify you when it's ready.

kevtechi commented 6 months ago

This is an on-chain 4337 Smart Contract Account with ERC1271 support (https://eips.ethereum.org/EIPS/eip-1271) . It's not a Safe.

It was working a few months ago from your end, I was able to link it. And as you can see, the contract that I provided responds correctly to the hash + signature combination.

So, this could be another issue than the one you mentioned above.

We use ERC4337 account abstraction, this type of signature verification is necessary for our users to be able to use Monerium.

The main issue here is that there is no error message from Wallet Connect or from the Monerium interface. So, it's impossible to debug.

kevtechi commented 6 months ago

And this is indeed a signature with 0x.

kevtechi commented 6 months ago

Sorry, don't mean to come off as ranting. Just clarifying some points related to your response.

Hopefully we can resolve this.

einaralex commented 6 months ago

Hey @kev-techi , thanks for the explanation! I was able to connect this wallet by using the signature you shared.

I'm making changes so not all smart contract addresses are treated the same, yours should be sending the actual signature as signature to the EMS. Hopefully I can release it by the end of today, I'll keep you posted.

einaralex commented 6 months ago

Changes are live now, can you please test again 🙏

kevtechi commented 6 months ago
Screenshot 2024-03-21 at 10 34 23

Same issue, I am requested to sign.

I sign the way I mentioned above with a valid signature. And then nothing else happens.

I ecRecover locally before sending to double-check and it seems ok.

kevtechi commented 6 months ago

@einaralex I would highly recommend you trying to connect with a Smart Contract Account that is 1271 enabled in order to debug this better. Not a Gnosis Safe, but a 4337 account (or try using the Safe 4337 module).

Not that 4337 should make a difference to a 1271 implementation, but at least you'll be able to debug this faster rather than me testing this for you.

This is the new account abstraction standard that all new EVM accounts are using. The whole ecosystem is moving to account contracts as the primary way of interacting with chains.

Coinbase launched their new wallet with 4337 as well.

It has been months now of trying to integrate with Monerium and we would really like to start using the service, launching it in our app and promoting it. This really needs to be a higher priority.

kevtechi commented 6 months ago

We can also set up a call to get it working. I'm sure that synchronously, we could get this resolved much faster.

einaralex commented 6 months ago

Yeah this would be over quickly if I could get something to test with.

kevtechi commented 6 months ago

Do you not have a 1271 account contract setup to test internally?

kevtechi commented 6 months ago

Here is the account factory I used for my account: https://polygonscan.com/address/0x5bA08d9fC7b90f79B2b856bdB09FC9EB32e83616#writeContract

You can provide an owner (EOA) and salt=0 and it will publish an account for you.

That should allow you to test Wallet Connect and 1271 with the exact same implementation that we are using.

einaralex commented 6 months ago

Hey @kev-techi, I've created an account from your instructions, but it's now been days that I've not figured out a way to connect that account through WalletConnect. As I understand the Coinbase AA is only available on Base. I've been trying to set it up with Alchemy, but I haven't been able to get that to work properly, and it might take way more time than it already has.

So we can move on with this, could you please try linking and provide me with the payload. If the payload does not have a signature 0x, could you please copy the curl and try running it with the signature as 0x. If that works, I know what our next step should be.

kevtechi commented 6 months ago

It worked on 14/10/2024, just a few messages up: https://github.com/monerium/smart-contracts/issues/39#issuecomment-1762921436

We're still using the same ERC1271 compliant smart contract through Wallet Connect.

My only comment back then was to allow me to only pick Polygon if possible to make it less confusing for users.

einaralex commented 6 months ago

Yeah, ok, I understand; the problem with that setup is that it broke other wallets like Argent. I'm trying to find a more permanent solution for what we have now, but I'll do some patching, so it should be working by tomorrow afternoon.