reown-com / reown_flutter

Reown is the onchain UX platform that provides toolkits built on top of the WalletConnect Network that enable builders to create onchain user experiences that make digital ownership effortless, intuitive, and secure.
https://docs.reown.com
Apache License 2.0
22 stars 17 forks source link

Address is not EIP-55 #45

Open rhamnett opened 3 days ago

rhamnett commented 3 days ago

Describe the bug

When I connect the appkit modal example to a wallet that uses the latest walletkit, the SIWE fails because the signing message does not use a checksummed address.

flutter: 0x958c690d6cb8d4feba4fe699xxxxx
flutter:
flutter: Welcome to AppKit 1.0.4 for Flutter. I further authorize the stated URI to perform the following actions on my behalf: (1) 'request': 'eth_sendTransaction', 'eth_sign', 'eth_signTransaction', 'eth_signTypedData', 'eth_signTypedData_v4', 'personal_sign', 'wallet_switchEthereumChain' for 'eip155'.
flutter:
flutter: URI: https://xxxxxxx
flutter: Version: 1
flutter: Chain ID: 324
flutter: Nonce: xxxxxxxx
flutter: Issued At: 2024-11-25T17:50:18.649Z
flutter: Resources:
flutter: - urn:recap:eyJhdHQiOnsiZWlwMTU1Ijp7InJlcXVlc3QvZXRoX3NlbmRUcmFuc2FjdGlvbiI6W3siY2hhaW5zIjpbImVpcDE1NToxIiwiZWlwMTU1OjQyNzkzIiwiZWlwMTU1OjEzNyIsImVpcDE1NTo0MjE2MSIsImVpcDE1NToxMCIsImVpcDE1NTo0MzExNCIsImVpcDE1NTo1NiIsImVpcDE1NTo0MjIyMCIsImVpcDE1NToxMDAiLCJla.......

Error:

flutter: [SIWESERVICE] verifyMessage() Response: {"error":"SIWE verification failed: Invalid SIWE message, Error: Address not conformant to EIP-55."}
flutter: Verification result: {error: SIWE verification failed: Invalid SIWE message, Error: Address not conformant to EIP-55.}

The address in the message should start 0x958C690D6Cb8d4.... note the capital C after 0x958....

Screenshot 2024-11-25 at 17 58 36
rhamnett commented 3 days ago

Perhaps this method can be updated? Although you may have a better suggestion.

key_service.dart

  ChainKey _eip155ChainKey(CryptoKeyPair keyPair) {
    final private = EthPrivateKey.fromHex(keyPair.privateKey);
    final address = private.address.hexEip55;   <<<HERE!!!

    final evmChainKey = ChainKey(
      chains: ChainsDataList.eip155Chains.map((e) => e.chainId).toList(),
      privateKey: keyPair.privateKey,
      publicKey: keyPair.publicKey,
      address: address,
    );
    debugPrint('[SampleWallet] evmChainKey ${evmChainKey.toString()}');
    return evmChainKey;
  }
rhamnett commented 3 days ago

I did provide a fix to my server side, which converts the siweAddress to checksummed, but don't you agree this should be something we send correctly from the client in the first place?

Cheers

quetool commented 3 days ago

Hello @rhamnett, can you provide steps to repro (specific wallet as well)? Coz I tried with our internal Swift wallet (which does support ERC-55) and I have no issues.

rhamnett commented 3 days ago

Hello, I am simply verifying the SIWE message in typescript using import { generateNonce, SiweMessage } from 'siwe'; . I have had to overwrite the address in the message created by reown as it does not conform strictly to adding the wallet address as a checksummed address.

In my opinion, the message should be transmitted with the correct type of checksummed address in the message, or the checksum is effectively not useful.

The message is created in Flutter using your util method:

createMessage: (SIWECreateMessageArgs args) {
          // Create SIWE message to be signed.
          // You can use our provided formatMessage() method of implement your own
          return SIWEUtils.formatMessage(args);
        },

Server code:

      let siweMessage: SiweMessage;
      try {
        try {
          siweMessage = new SiweMessage(message);
          siweMessage.address = ethers.utils.getAddress(siweMessage.address);       <<note this hack to fix the address
        } catch (error) {
          throw new Error(`Invalid SIWE message, ${error}`);
        }
        console.log(
          `[Auth] Parsed SIWE Message: ${JSON.stringify(siweMessage)} `
        );
        // Define actual domain
        console.log(`[Auth] Verification Domain: ${siweMessage.domain}`);

        // Perform verification
        try {
          const fields = await siweMessage.verify({
            signature,
            domain: siweMessage.domain,
            nonce: nonceFromToken,
          });

          if (!fields.success) {
            throw new Error('Signature verification failed');
          }
          console.log(
            `[Auth] SIWE message verified successfully: ${JSON.stringify(
              fields
            )}`
          );
        } catch (error) {
          throw new Error(`Signature verification failed: ${error}`);
        }

The above code will fail if the address isn't overwritten with error SIWE verification failed: Invalid SIWE message, Error: Address not conformant to EIP-55

I suppose you could also look to enhance the SIWEUtils.formatMessage() method.

Many thanks Rich

quetool commented 3 days ago

Yes, ERC-55 has still to be supported on flutter SDK, however this shouldn't be a reason for wallets to reject/invalidate the signing as you mention in the original comment:

When I connect the appkit modal example to a wallet that uses the latest walletkit, the SIWE fails because the signing message does not use a checksummed address.

This is what concerns me. 👆

Can you try by replacing this:

createMessage: (SIWECreateMessageArgs args) {
  // Create SIWE message to be signed.
  // You can use our provided formatMessage() method of implement your own
  return SIWEUtils.formatMessage(args);
},

With this:

createMessage: (SIWECreateMessageArgs args) {
  // Create SIWE message to be signed.
  // You can use our provided formatMessage() method of implement your own
  final authPayload = SessionAuthPayload.fromJson(args.toJson()).copyWith(
    chains: [args.chainId],
    aud: args.uri,
    type: args.type?.t ?? 'eip4361',
  );
  final account = NamespaceUtils.getAccount(args.address);
  final address = EthereumAddress.fromHex(account);
  return _appKitModal.appKit!.formatAuthMessage(
    iss: 'did:pkh:${args.chainId}:${address.hexEip55}',
    cacaoPayload: CacaoRequestPayload.fromSessionAuthPayload(
      authPayload,
    ),
  );
},
rhamnett commented 3 days ago

@quetool i think it's simply that the SIWE node implementation checks to ensure the wallet address is EIP-55 ( see https://github.com/spruceid/siwe/blob/940a66ac3dc7443b52d04fcdcebe5a6d889add9b/packages/siwe/lib/client.ts#L428 ) and fails hard.

On the flutter side, when the wallet is created from a seedphrase in the example code... the wallet is created with .hex and not .hexEip55.

I've added my own fix on both the flutter side and the server side to ensure that a checksummed wallet is generated & checked in the message.

I did ask ChatGPT and it says

The EIP-4361 standard specifies how the message is structured, but it does not explicitly mandate the use of a checksummed address.

So I guess this is up to you if you want to adjust your implementation to send a checksummed address. If not then please do feel free to close the ticket. It was simply a suggestion :)

quetool commented 3 days ago

We agree! I think there's a miscommunication here LMAO so let's try to strip this convo down:

  1. Yes, there's a lack of support of ERC-55 (checksummed addresses) on AppKit side for Flutter
  2. Yes, we are gonna add support for ERC-55 (checksummed addresses), it's in our bucket list (This is why I marked the issue as enhancement).
  3. However, in the meantime you should be able to override the current createMessage: (SIWECreateMessageArgs args) {} definition on host side (sample dapp) with the one I provided to overcome the server-side check. (this was the goal on my comments, not disagreeing with you)
  4. Finally, ideally wallets/dapps shouldn't reject non-checksummed addresses as backward compatibility is always appreciated.

We are on the same page! Thanks for the feedback! Keep it coming, please!

rhamnett commented 3 days ago

OK thanks :)

I certainly didn't take anything as if we were disagreeing, I was simply trying to clarify how I'd worked around (I'd changed the way the wallet key is imported to checksum it).

Your solution looks to do the job as well, in a different manner. Apologies if any crossed wires and thanks also for the feedback and great communication!

quetool commented 3 days ago

Let's keep it open as a reminder that we must support ERC-55! 💯