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

[pranksteess]

I found when there is a xxx[] recursively appears in the type field, the HashStruct can not work well.
You can see more details in test file of signed_data_test.go on TestComplexTypedData func.
And I have fixed it in this PR

[holiman]

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?

[pranksteess]

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

[pranksteess]

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)

[holiman]

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.

[pranksteess]

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?

[fjl]

Closing because resubmit #24220 will be merged instead.