Skip to content

signer/core: fix complex typed data signing (EIP-712) #24102

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.

Sign up for GitHub

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

Closed
pranksteess wants to merge 4 commits into from
Closed

signer/core: fix complex typed data signing (EIP-712) #24102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
222fe34
fix: complex typed data sign (EIP712)
pranksteess Dec 13, 2021
3f473ea
fix: go mod
pranksteess Dec 14, 2021
e01bfb4
reset: go mod
pranksteess Dec 14, 2021
ff400b9
feat: pass the code checker
pranksteess Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion signer/core/signed_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ func (typedData *TypedData) HashStruct(primaryType string, data TypedDataMessage

// Dependencies returns an array of custom types ordered by their hierarchical reference tree
func (typedData *TypedData) Dependencies(primaryType string, found []string) []string {
primaryType = strings.TrimSuffix(primaryType, "[]")
Copy link
Copy Markdown
Contributor

@holiman holiman Dec 14, 2021

Choose a reason for hiding this comment

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

Isn't this a bit naive?
If we use this approach, then it only solves Foobar[], but not Foobar[8] or Foobar[][].
I'm not saying it's optimal as it is right now, but I'm kind of wondering how far to go with this.

Copy link
Copy Markdown
Contributor Author

@pranksteess pranksteess Dec 14, 2021

Choose a reason for hiding this comment

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

Foobar[8] is a data, which specific to a item in type Foobar[], here primaryType means a type not a data

In my mind, the EIP-712 does not support 2d (or more) arrays?

includes := func(arr []string, str string) bool {
for _, obj := range arr {
if obj == str {
Expand Down Expand Up @@ -471,7 +472,7 @@ func (typedData *TypedData) EncodeData(primaryType string, data map[string]inter
if err != nil {
return nil, err
}
arrayBuffer.Write(encodedData)
arrayBuffer.Write(crypto.Keccak256(encodedData))
Copy link
Copy Markdown
Contributor

@holiman holiman Dec 14, 2021

Choose a reason for hiding this comment

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

Could you explain this change?
It looks like something that was either completely broken earlier, or is completely broken now. If it's the former, does any test show how it was broken?

Copy link
Copy Markdown
Contributor Author

@pranksteess pranksteess Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why hash before write

According to the describe on EIP-712 , when EncodeData on a type of array, it needs to hash the result of each element, and then append these results together, hash the combination again.

It may look like this:

sample = [{'k': 'a', 'v': 'b'}, {'k': 'c', 'v': 'd'}]
# old version
data = hash(hash(a) + hash(b) + hash(c) + hash(d))
# fixed
data = hash(hash(hash(a) + hash(b)) + hash(hash(c) + hash(d)))

Curious: I see Line494 correctly use

buffer.Write(crypto.Keccak256(encodedData))

instead of

buffer.Write(encodedData)

Why not do the same in this line? It's in the same condition I think (both are returned from deeper EncodeData)

Why the former test not broken

Reason is simple, cause the test datas in signed_data_test.go are not in full coverage. No array in any test samples. I add a test case with an array in signed_data_test.go of this PR.

More

I have done the test with MetaMask

Copy link
Copy Markdown
Contributor Author

@pranksteess pranksteess Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How to test via MetaMask

Get signature from MetaMask

Open Developer Tools with Chrome, switch to Console tab, connect to MetaMask (use BSC network), paste code below after replacing <your bsc addr> with your own BSC addr:

const typedData = {
    types: {
        EIP712Domain: [
            { name: 'chainId', type: 'uint256' },
            { name: 'name', type: 'string' },
            { name: 'verifyingContract', type: 'address' },
            { name: 'version', type: 'string' }
        ],
	Action:[
            { name:"action", type:"string" },
            { name:"params", type:"string" }
        ],
        Cell:[
            { name:"capacity", type:"string" },
            { name:"lock", type:"string" },
            { name:"type", type:"string" },
            { name:"data", type:"string" },
            { name:"extraData", type:"string" }
        ],
        Transaction: [
            { name: 'DAS_MESSAGE', type: 'string' },
            { name: 'inputsCapacity', type: 'string' },
            { name: 'outputsCapacity', type: 'string' },
            { name: 'fee', type: 'string' },
	    { name:"action", type:"Action" },
	    { name:"inputs", type:"Cell[]" },
            { name:"outputs", type:"Cell[]" },
            { name:"digest", type:"bytes32" }
        ]
    },
    primaryType: 'Transaction',
    domain: {
        chainId: 56,
        name: 'da.systems',
        verifyingContract: '0x0000000000000000000000000000000020210722',
        version: '1'
    },
    message: {
        "DAS_MESSAGE": "SELL mobcion.bit FOR 100000 CKB",
        "inputsCapacity": "1216.9999 CKB",
        "outputsCapacity": "1216.9998 CKB",
        "fee": "0.0001 CKB",
        "digest": "0x53a6c0f19ec281604607f5d6817e442082ad1882bef0df64d84d3810dae561eb",
        "action": {
            "action": "start_account_sale",
            "params": "0x00"
        },
        "inputs": [
            {
                "capacity": "218 CKB",
                "lock": "das-lock,0x01,0x051c152f77f8efa9c7c6d181cc97ee67c165c506...",
                "type": "account-cell-type,0x01,0x",
                "data": "{ account: mobcion.bit, expired_at: 1670913958 }",
                "extraData": "{ status: 0, records_hash: 0x55478d76900611eb079b22088081124ed6c8bae21a05dd1a0d197efcc7c114ce }"
            }
        ],
        "outputs": [
            {
                "capacity": "218 CKB",
                "lock": "das-lock,0x01,0x051c152f77f8efa9c7c6d181cc97ee67c165c506...",
                "type": "account-cell-type,0x01,0x",
                "data": "{ account: mobcion.bit, expired_at: 1670913958 }",
                "extraData": "{ status: 1, records_hash: 0x55478d76900611eb079b22088081124ed6c8bae21a05dd1a0d197efcc7c114ce }"
            },
            {
                "capacity": "201 CKB",
                "lock": "das-lock,0x01,0x051c152f77f8efa9c7c6d181cc97ee67c165c506...",
                "type": "account-sale-cell-type,0x01,0x",
                "data": "0x1209460ef3cb5f1c68ed2c43a3e020eec2d9de6e...",
                "extraData": ""
            }
        ]
    }
}

const from = '<your bsc addr>'
ethereum.sendAsync({
    method: 'eth_signTypedData_v4',
    params: [from, JSON.stringify(typedData)],
    from
}, (err, res) => {
    if (err) {
        console.error(err)
        return
    }
    console.log(res.result)
})

This will print the signatureA in Console after clicked the "SIGN" button.

Get signature from go-ethereum

Run TestComplexTypedData in signed_data_test.go in this PR, get the message to be signed: 0x42b1aca82bb6900ff75e90a136de550a58f1a220a071704088eabd5e6ce20446

Then use crypto.Sign("0x42b1aca82bb6900ff75e90a136de550a58f1a220a071704088eabd5e6ce20446", key) to get the signatureB (key is the private key used in MetaMask)

Compare

Make sure signatureA == signatureB, ignore the recId (the last byte in signature ±27)

} else {
bytesValue, err := typedData.EncodePrimitiveValue(parsedType, item, depth)
if err != nil {
Expand Down
150 changes: 150 additions & 0 deletions signer/core/signed_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,3 +531,153 @@ func TestGnosisCustomData(t *testing.T) {
t.Fatalf("Error, got %x, wanted %x", sighash, expSigHash)
}
}

var complexTypedData = `
{
"types": {
"EIP712Domain": [
{
"name": "chainId",
"type": "uint256"
},
{
"name": "name",
"type": "string"
},
{
"name": "verifyingContract",
"type": "address"
},
{
"name": "version",
"type": "string"
}
],
"Action": [
{
"name": "action",
"type": "string"
},
{
"name": "params",
"type": "string"
}
],
"Cell": [
{
"name": "capacity",
"type": "string"
},
{
"name": "lock",
"type": "string"
},
{
"name": "type",
"type": "string"
},
{
"name": "data",
"type": "string"
},
{
"name": "extraData",
"type": "string"
}
],
"Transaction": [
{
"name": "DAS_MESSAGE",
"type": "string"
},
{
"name": "inputsCapacity",
"type": "string"
},
{
"name": "outputsCapacity",
"type": "string"
},
{
"name": "fee",
"type": "string"
},
{
"name": "action",
"type": "Action"
},
{
"name": "inputs",
"type": "Cell[]"
},
{
"name": "outputs",
"type": "Cell[]"
},
{
"name": "digest",
"type": "bytes32"
}
]
},
"primaryType": "Transaction",
"domain": {
"chainId": "56",
"name": "da.systems",
"verifyingContract": "0x0000000000000000000000000000000020210722",
"version": "1"
},
"message": {
"DAS_MESSAGE": "SELL mobcion.bit FOR 100000 CKB",
"inputsCapacity": "1216.9999 CKB",
"outputsCapacity": "1216.9998 CKB",
"fee": "0.0001 CKB",
"digest": "0x53a6c0f19ec281604607f5d6817e442082ad1882bef0df64d84d3810dae561eb",
"action": {
"action": "start_account_sale",
"params": "0x00"
},
"inputs": [
{
"capacity": "218 CKB",
"lock": "das-lock,0x01,0x051c152f77f8efa9c7c6d181cc97ee67c165c506...",
"type": "account-cell-type,0x01,0x",
"data": "{ account: mobcion.bit, expired_at: 1670913958 }",
"extraData": "{ status: 0, records_hash: 0x55478d76900611eb079b22088081124ed6c8bae21a05dd1a0d197efcc7c114ce }"
}
],
"outputs": [
{
"capacity": "218 CKB",
"lock": "das-lock,0x01,0x051c152f77f8efa9c7c6d181cc97ee67c165c506...",
"type": "account-cell-type,0x01,0x",
"data": "{ account: mobcion.bit, expired_at: 1670913958 }",
"extraData": "{ status: 1, records_hash: 0x55478d76900611eb079b22088081124ed6c8bae21a05dd1a0d197efcc7c114ce }"
},
{
"capacity": "201 CKB",
"lock": "das-lock,0x01,0x051c152f77f8efa9c7c6d181cc97ee67c165c506...",
"type": "account-sale-cell-type,0x01,0x",
"data": "0x1209460ef3cb5f1c68ed2c43a3e020eec2d9de6e...",
"extraData": ""
}
]
}
}
`

func TestComplexTypedData(t *testing.T) {
var td core.TypedData
err := json.Unmarshal([]byte(complexTypedData), &td)
if err != nil {
t.Fatalf("unmarshalling failed '%v'", err)
}
_, sighash, err := sign(td)
if err != nil {
t.Fatal(err)
}
expSigHash := common.FromHex("0x42b1aca82bb6900ff75e90a136de550a58f1a220a071704088eabd5e6ce20446")
if !bytes.Equal(expSigHash, sighash) {
t.Fatalf("Error, got %x, wanted %x", sighash, expSigHash)
}
}