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

signer/core: handle gnosis safe problem with missing chain id #26309

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 5, 2022

This PR adds a check that the safetxhash that we sign corresponds to the one that is expected by the input. This PR also adds a case which is currently failing.

The current code calculates the safeTxHash for a certain json message differently than what is expected by the online safe tx machinery.

cc @rmeissner could you please help me figure out the difference here?

json:

{
  "safe": "0x899FcB1437DE65DC6315f5a69C017dd3F2837557",
  "to": "0x899FcB1437DE65DC6315f5a69C017dd3F2837557",
  "value": "0",
  "data": "0x0d582f13000000000000000000000000d3ed2b8756b942c98c851722f3bd507a17b4745f0000000000000000000000000000000000000000000000000000000000000005",
  "operation": 0,
  "gasToken": "0x0000000000000000000000000000000000000000",
  "safeTxGas": 0,
  "baseGas": 0,
  "gasPrice": "0",
  "refundReceiver": "0x0000000000000000000000000000000000000000",
  "nonce": 0,
  "executionDate": null,
  "submissionDate": "2022-02-23T14:09:00.018475Z",
  "modified": "2022-12-01T15:52:21.214357Z",
  "blockNumber": null,
  "transactionHash": null,
  "safeTxHash": "0x6f0f5cffee69087c9d2471e477a63cab2ae171cf433e754315d558d8836274f4",
  "executor": null,
  "isExecuted": false,
  "isSuccessful": null,
  "ethGasPrice": null,
  "maxFeePerGas": null,
  "maxPriorityFeePerGas": null,
  "gasUsed": null,
  "fee": null,
  "origin": "https://gnosis-safe.io",
  "dataDecoded": {
    "method": "addOwnerWithThreshold",
    "parameters": [
      {
        "name": "owner",
        "type": "address",
        "value": "0xD3Ed2b8756b942c98c851722F3bd507a17B4745F"
      },
      {
        "name": "_threshold",
        "type": "uint256",
        "value": "5"
      }
    ]
  },
  "confirmationsRequired": 4,
  "confirmations": [
    {
      "owner": "0x30B714E065B879F5c042A75Bb40a220A0BE27966",
      "submissionDate": "2022-03-01T14:56:22Z",
      "transactionHash": "0x6d0a9c83ac7578ef3be1f2afce089fb83b619583dfa779b82f4422fd64ff3ee9",
      "signature": "0x00000000000000000000000030b714e065b879f5c042a75bb40a220a0be27966000000000000000000000000000000000000000000000000000000000000000001",
      "signatureType": "APPROVED_HASH"
    },
    {
      "owner": "0x8300dFEa25Da0eb744fC0D98c23283F86AB8c10C",
      "submissionDate": "2022-12-01T15:52:21.214357Z",
      "transactionHash": null,
      "signature": "0xbce73de4cc6ee208e933a93c794dcb8ba1810f9848d1eec416b7be4dae9854c07dbf1720e60bbd310d2159197a380c941cfdb55b3ce58f9dd69efd395d7bef881b",
      "signatureType": "EOA"
    }
  ],
  "trusted": true,
  "signatures": null
}

Resulting preimage before keccak:

0x190119994026f194e6a8a9be0983c8c7fc49d8f1aa03b638577e35c4be0d082145c681d1e7162ad6fcb59199bcacdd11e8228484abcd2f5fde35a4b9be40e668bf24

Resulting hash:

0x6e6a5ede66d5838ec141746b6544a0140416bff184faff17cf62cab97f11f5ae

But the expectation is

  "safeTxHash": "0x6f0f5cffee69087c9d2471e477a63cab2ae171cf433e754315d558d8836274f4",

@mmv08
Copy link
Contributor

mmv08 commented Dec 5, 2022

@holiman code from the gnosis_safe.go signer

	var domainType = []apitypes.Type{{Name: "verifyingContract", Type: "address"}}
	if tx.ChainId != nil {
		domainType = append([]apitypes.Type{{Name: "chainId", Type: "uint256"}}, domainType[0])
	}

Relevant PR: #24231

Perhaps I should've added a comment there. So the safe you're using is version 1.3.0, which also factors in the chainId for the transaction hash. The JSON payload is missing the chainId parameter and therefore the function produces a mismatching hash.

@holiman
Copy link
Contributor Author

holiman commented Dec 5, 2022

The JSON payload is missing the chainId parameter and therefore the function produces a mismatching hash.

But why is that missing? It is coming directly from curl -X GET "https://safe-transaction-mainnet.safe.global/api/v1/safes/0x899FcB1437DE65DC6315f5a69C017dd3F2837557/multisig-transactions/?executed=false" -H "accept: application/json" -H "X-CSRFToken: oBRAVXzXeIzq0vfzp8bWjdPhZBTD25rj5vFzVKIYUZ4HwCJlXCuOWDJ7Vh333STp" | jq ".results[0]" .

@mmv08
Copy link
Contributor

mmv08 commented Dec 5, 2022

The JSON payload is missing the chainId parameter and therefore the function produces a mismatching hash.

But why is that missing? It is coming directly from curl -X GET "https://safe-transaction-mainnet.safe.global/api/v1/safes/0x899FcB1437DE65DC6315f5a69C017dd3F2837557/multisig-transactions/?executed=false" -H "accept: application/json" -H "X-CSRFToken: oBRAVXzXeIzq0vfzp8bWjdPhZBTD25rj5vFzVKIYUZ4HwCJlXCuOWDJ7Vh333STp" | jq ".results[0]" .

I think I've misphrased that, the payload is not missing it. Our services simply do not return it because we have a separate service per chain.

@holiman holiman marked this pull request as ready for review December 6, 2022 09:10
@holiman holiman changed the title signer/core: work on gnosis safe problems signer/core: handle gnosis safe problem with missing chain id Dec 10, 2022
@holiman holiman added this to the 1.11.0 milestone Dec 10, 2022
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@holiman holiman merged commit 502fa82 into ethereum:master Dec 14, 2022
@holiman holiman deleted the gnosis_woes branch December 14, 2022 09:34
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
…um#26309)

This PR adds a check that the safetxhash that we sign corresponds to the one that is expected by the input. If it differs, it tries again with the configured chainid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants