Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document better Safe message hash calculation #1555

Closed
lucashenning opened this issue Jul 6, 2023 · 12 comments · Fixed by #1573
Closed

Document better Safe message hash calculation #1555

lucashenning opened this issue Jul 6, 2023 · 12 comments · Fixed by #1573
Assignees
Labels
enhancement New feature or request

Comments

@lucashenning
Copy link

What is needed?

The definition of POST /api/v1/safes/<addr>/messages in the Transaction Service API Definition doesn't specify the format of the Message variable. Looking at the python code, it looks like both, EIP712 objects as well as strings should be supported. However, when trying different formats, the API returns 400: '0x... is not an owner of the Safe'

A clear definition of the message format + encoding would be very helpful and should be added to the docs to avoid issues when signing messages.

Background

The following code should sign a message and send it to the tx service. The message is being sent but the transaction service returns an invalid owner, even though it is signed by one of the safe owners.

    public async signData(message: string): Promise<string> {
        const hash = ethers.hashMessage(message);
        const signature = await this.safeSigner.signMessage(hash);
        const url = SAFE_TX_SERVICE_URLS[this.chainId] + '/v1/safes/' + this.safeAddress + '/messages/';
        const response = await fetch(url, {
            method: 'POST',
            headers: {
                'Content-Type': 'application/json',
            },
            body: JSON.stringify({
                message,
                // safeAppId: 0,
                signature,
            }),
        });
        return response.json();
    }

Response (400):

{
        nonFieldErrors: [
          '0x04a3764E164C2763c5217a6A96c16D6B50677D8f is not an owner of the Safe'
        ]
}

The message was signed by a Safe owner. However, the returned address 0x04a... doesn't match the signer's address.

If the same message + signature are run through ethers.recoverAddress, the resulting address is correct (the address of the safe owner):

const hash = ethers.hashMessage(message);
const signature = await signer.signMessage(hash);
const address = ethers.recoverAddress(ethersv5.utils.arrayify(ethersv5.utils.hashMessage(hash)), signature);
// address = 0x479... = the actual address of the safe owner

Additional Info

I tried both, EIP712 typed data as well as plaintext messages but it doesn't seem to be working. Every time I get another 400 with a random address: '0x... is not an owner of the Safe'.

Is there a clear definition of the encoding of the message and signature variables or example code in JS?

@lucashenning lucashenning added the enhancement New feature or request label Jul 6, 2023
Uxio0 added a commit that referenced this issue Jul 18, 2023
@Uxio0
Copy link
Member

Uxio0 commented Jul 18, 2023

Service does not accept a message as a hash, you can send a string (EIP191 will be used) or a EIP712 object. You are totally right our documentation can be improved.

Do you think this PR adds enough info? #1573

@lucashenning
Copy link
Author

Thanks @Uxio0 - I appreciate you updating the docs, this is super helpful. I'm still facing this issue when signing an EIP191 message and I feel like there might be something missing.

You're saying EIP191 will be used. If that's the case, signing with signer.signMessage(message) should return the right signature (incl. the Ethereum Signed Message: prefix), please correct me if I'm wrong. I tried doing that but the returned address does not match the address of my signer, simplified code example:

  const message = "Hello World";
  const signature = await this.safeSigner.signMessage(message);

  console.log(await this.safeSigner.getAddress()); // output: 0x4790.......... <-- this is the actual signer

  const url = SAFE_TX_SERVICE_URLS[this.chainId] + '/v1/safes/' + this.safeAddress + '/messages/';

  const response = await fetch(url, {
      method: 'POST',
      headers: {
          'Content-Type': 'application/json',
      },
      body: JSON.stringify({
          message,
          // safeAppId: 0,
          signature,
      }),
  });

response.body:

      {
        nonFieldErrors: [
          '0xAc60AaFB573cBB6471e90237a0Dce07b21ead0c4 is not an owner of the Safe'
          // that's not the address of the signer (see above) 0x4790..........
        ]
      }

I think it would be good to add some more info to the docs on how to sign/encode this data because according to EIP191 signMessage(message) should work but it doesn't, it returns the wrong signer. Hope that makes sense, please let me know if I'm missing anything.

Thanks again for your help!

Uxio0 added a commit that referenced this issue Jul 21, 2023
@Uxio0
Copy link
Member

Uxio0 commented Jul 21, 2023

Every message hash is different depending on the Safe. You need to get the message hash for the concrete Safe (after you calculate the hash): https://github.com/safe-global/safe-eth-py/blob/master/gnosis/safe/safe.py#L847

We must complete the documentation, I will reopen the issue

@Uxio0 Uxio0 reopened this Jul 21, 2023
@Uxio0 Uxio0 changed the title POST /api/v1/safes/<addr>/messages returns "0x... is not an owner of the Safe" Document better message hash calculation Jul 21, 2023
@Uxio0 Uxio0 changed the title Document better message hash calculation Document better Safe message hash calculation Jul 21, 2023
slon2015 pushed a commit to GTON-capital/safe-transaction-service that referenced this issue Aug 1, 2023
@lucashenning
Copy link
Author

Thank you @Uxio0 for sharing the link - this is really helpful. I've been trying to create a JS version of the get_message_hash function.

However, when signing the result of this function with const signature = await this.safeSigner.signMessage(safeMessageHash);, the Gnosis Safe Transaction API is still not returning the correct signer address. Is there a library to make this easier or another way to come up with the right Safe hash to make this work?

 async getMessageHash(message: string): Promise<string> {
      const SAFE_MESSAGE_TYPEHASH = ethers.getBytes(
          "0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca"
      );

      const safeContract = new ethers.Contract(this.safeAddress, GnosisSafeL2Abi, this.ethAdapter as any);
      const domainSeperator = await safeContract.domainSeparator();
      console.log("domainSeperator: ", domainSeperator);

      let messageBytes: Uint8Array;
      
      if (typeof message === 'string') {
          messageBytes = ethers.toUtf8Bytes(message);
      } else {
          messageBytes = message;
      }

      const messageHash = ethers.keccak256(messageBytes);

      const safeMessageHash = ethers.keccak256(
          ethers.solidityPacked(
              ["bytes32", "bytes32"],
              [SAFE_MESSAGE_TYPEHASH, messageHash]
          )
      );

      const finalHash = ethers.keccak256(
          ethers.concat([
              ethers.getBytes("0x19"),
              ethers.getBytes("0x01"),
              ethers.getBytes(domainSeperator),
              ethers.getBytes(safeMessageHash),
          ])
      );

      return finalHash;
  }

@Uxio0
Copy link
Member

Uxio0 commented Aug 10, 2023

If you need a JavaScript implementation, you can take a look at the tests on https://github.com/safe-global/safe-contracts/blob/main/test/core/Safe.Signatures.spec.ts

@lucashenning
Copy link
Author

Thanks @Uxio0, this is helpful and I'm able to sign transactions using this functionality. However, when I try to use the same logic for message signing, the signing fails (POST /api/v1/safes/<addr>/messages rejects the result).

In other words, transactions are working fine, signatures are not. I can't seem to find a way to send a valid signature to the service. It keeps coming back with nonFieldErrors and random addresses for owner of the Safe:

      {
        nonFieldErrors: [
          '0xB1563042b684F27c10423319CcaA397FEd36eB41 is not an owner of the Safe'
        ]
      }

I'm using the logic of safeSignMessage() as called here. The problem is, in all tests the result is passed to executeTx() which works fine, but it's not what I want to do. I want to send the signature to the safe transaction service api.

My apologies for keeping you busy with this, I'm clearly missing something. If you could point me to an implementation that successfully signs a message and then sends it to the transaction service endpoint POST /api/v1/safes/<addr>/messages that would be awesome. Thanks again for your help!

@lucashenning
Copy link
Author

Hi all, quick update on this. Finally got to the bottom of this issue. The API seems to throw the error when a trailing / is added to the transaction URL, i.e. .../messages/ instead of .../messages

The correct URL is:

https://safe-transaction-goerli.safe.global/api/v1/safes/:safeAddress/messages

When adding a trailing / the API throws the following error:

      {
        nonFieldErrors: [
          '0xB1563042b684F27c10423319CcaA397FEd36eB41 is not an owner of the Safe'
        ]
      }

Video documenting the behavior:

gnosis.safe.tx.service.signing.mov

Thanks again for your help @Uxio0 - I think it would be great to add a better error message or somehow catch the trailing / in a different way.

@Uxio0
Copy link
Member

Uxio0 commented Aug 29, 2023

By using /messages you are getting redirected to /messages/, your request becomes a GET instead of a POST and you get that result

@lucashenning
Copy link
Author

You're correct, didn't realize there was a redirect happening. Because of the GET redirect, it returns the signatures of the signed messages created via WalletConnect. That does not solve my problem.

My problem remains the same, I'm not able to sign from an ethers.signer (owner of the safe) and send the signed message to the transaction service endpoint POST /api/v1/safes/:safeAddress/messages. I tried several different ways of message encoding, signing, etc. The result is:

      {
        nonFieldErrors: [
          '0xB1563042b684F27c10423319CcaA397FEd36eB41 is not an owner of the Safe'
        ]
      }

@Uxio0 thanks for pointing this out. If you have any idea how to solve this in JavaScript, I would greatly appreciate your input.

@Uxio0
Copy link
Member

Uxio0 commented Aug 29, 2023

Sorry, I cannot think on more insight to give you. The examples on the Safe contracts repository are typescript, they are tested and working, you should be able to adapt them to work. Also our frontend does message handling so you can take a look at: https://github.com/safe-global/safe-wallet-web

@moisses89
Copy link
Member

Adding messages doc here safe-global/safe-docs#235

@moisses89
Copy link
Member

The documentation was added here and also an example signing a message_hash calculated from Safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants