From 50f074ddcab5dbafe5bad0ebcbfde8a9f91826e4 Mon Sep 17 00:00:00 2001 From: Mircea Nistor Date: Wed, 25 Aug 2021 14:00:35 +0200 Subject: [PATCH] fix(key-manager): handle eth_signTransaction with from field (#675) fixes #674 --- __tests__/shared/keyManager.ts | 48 ++++++++++++++++++- .../src/abstract-key-management-system.ts | 4 +- packages/key-manager/src/key-manager.ts | 36 ++++++++++---- .../kms-local/src/key-management-system.ts | 12 ++++- 4 files changed, 87 insertions(+), 13 deletions(-) diff --git a/__tests__/shared/keyManager.ts b/__tests__/shared/keyManager.ts index 7967e0736..a268a4d1c 100644 --- a/__tests__/shared/keyManager.ts +++ b/__tests__/shared/keyManager.ts @@ -1,6 +1,6 @@ import { IKey, TKeyType } from '@veramo/core' import { TAgent, IDIDManager, IKeyManager, IAgentOptions } from '../../packages/core/src' -import { serialize } from '@ethersproject/transactions' +import { serialize, computeAddress } from '@ethersproject/transactions' type ConfiguredAgent = TAgent @@ -200,6 +200,52 @@ export default (testContext: { expect(typeof rawTx).toEqual('string') }) + it('should allow signing EthTX with matching from', async () => { + const key = await agent.keyManagerCreate({ + kms: 'local', + type: 'Secp256k1', + }) + const keyAddress = computeAddress('0x' + key.publicKeyHex) + + const rawTx = await agent.keyManagerSignEthTX({ + kid: key.kid, + transaction: { + to: '0xce31a19193d4b23f4e9d6163d7247243bAF801c3', + from: keyAddress, + value: 300000, + gasLimit: 43092000, + gasPrice: 20000000000, + nonce: 1, + }, + }) + + expect(typeof rawTx).toEqual('string') + }) + + it('should NOT sign EthTX with mismatching from field', async () => { + expect.assertions(1) + const key = await agent.keyManagerCreate({ + kms: 'local', + type: 'Secp256k1', + }) + + await expect( + agent.keyManagerSignEthTX({ + kid: key.kid, + transaction: { + to: '0xce31a19193d4b23f4e9d6163d7247243bAF801c3', + from: '0xce31a19193d4b23f4e9d6163d7247243bAF801c3', + value: 300000, + gasLimit: 43092000, + gasPrice: 20000000000, + nonce: 1, + }, + }), + ).rejects.toThrowError( + 'invalid_arguments: keyManagerSignEthTX `from` field does not match the chosen key. `from` field should be omitted.', + ) + }) + it('Should Encrypt/Decrypt', async () => { const message = 'foo bar' const senderKey = await agent.keyManagerCreate({ diff --git a/packages/key-manager/src/abstract-key-management-system.ts b/packages/key-manager/src/abstract-key-management-system.ts index d7a426f71..2b535eb59 100644 --- a/packages/key-manager/src/abstract-key-management-system.ts +++ b/packages/key-manager/src/abstract-key-management-system.ts @@ -1,6 +1,6 @@ import { IKey, TKeyType } from '@veramo/core' import { arrayify, hexlify } from '@ethersproject/bytes' -import { serialize, Transaction } from '@ethersproject/transactions' +import { serialize } from '@ethersproject/transactions' import * as u8a from 'uint8arrays' export abstract class AbstractKeyManagementSystem { @@ -9,7 +9,7 @@ export abstract class AbstractKeyManagementSystem { /**@deprecated please use `sign({key, alg: 'eth_signTransaction', data: arrayify(serialize(transaction))})` instead */ async signEthTX({ key, transaction }: { key: IKey; transaction: object }): Promise { - const { v, r, s, type, ...tx } = transaction + const { v, r, s, from, ...tx } = transaction const data = arrayify(serialize(tx)) const algorithm = 'eth_signTransaction' const signedTxHexString = this.sign({ key, data, algorithm }) diff --git a/packages/key-manager/src/key-manager.ts b/packages/key-manager/src/key-manager.ts index 12678a5d4..31f3329d0 100644 --- a/packages/key-manager/src/key-manager.ts +++ b/packages/key-manager/src/key-manager.ts @@ -19,8 +19,11 @@ import { import * as u8a from 'uint8arrays' import { JWE, createAnonDecrypter, createAnonEncrypter, createJWE, decryptJWE, ECDH } from 'did-jwt' import { arrayify, hexlify } from '@ethersproject/bytes' +import { serialize, computeAddress } from '@ethersproject/transactions' import { toUtf8String, toUtf8Bytes } from '@ethersproject/strings' import { convertPublicKeyToX25519 } from '@stablelib/ed25519' +import Debug from 'debug' +const debug = Debug('veramo:key-manager') /** * Agent plugin that provides {@link @veramo/core#IKeyManager} methods @@ -138,9 +141,12 @@ export class KeyManager implements IAgentPlugin { /** {@inheritDoc @veramo/core#IKeyManager.keyManagerSignJWT} */ async keyManagerSignJWT({ kid, data }: IKeyManagerSignJWTArgs): Promise { - const key = await this.store.get({ kid }) - const kms = this.getKms(key.kms) - return kms.signJWT({ key, data }) + if (typeof data === 'string') { + return this.keyManagerSign({ keyRef: kid, data, encoding: 'utf-8' }) + } else { + const dataString = u8a.toString(data, 'base16') + return this.keyManagerSign({ keyRef: kid, data: dataString, encoding: 'base16' }) + } } /** {@inheritDoc @veramo/core#IKeyManager.keyManagerSign} */ @@ -164,9 +170,23 @@ export class KeyManager implements IAgentPlugin { /** {@inheritDoc @veramo/core#IKeyManager.keyManagerSignEthTX} */ async keyManagerSignEthTX({ kid, transaction }: IKeyManagerSignEthTXArgs): Promise { - const key = await this.store.get({ kid }) - const kms = this.getKms(key.kms) - return kms.signEthTX({ key, transaction }) + const { v, r, s, from, ...tx } = transaction + if (typeof from === 'string') { + debug('WARNING: executing a transaction signing request with a `from` field.') + const key = await this.store.get({ kid }) + if (key.publicKeyHex) { + const address = computeAddress('0x' + key.publicKeyHex) + if (address.toLowerCase() !== from.toLowerCase()) { + const msg = + 'invalid_arguments: keyManagerSignEthTX `from` field does not match the chosen key. `from` field should be omitted.' + debug(msg) + throw new Error(msg) + } + } + } + const data = serialize(tx) + const algorithm = 'eth_signTransaction' + return this.keyManagerSign({ keyRef: kid, data, algorithm, encoding: 'base16' }) } /** {@inheritDoc @veramo/core#IKeyManager.keyManagerSharedKey} */ @@ -178,11 +198,11 @@ export class KeyManager implements IAgentPlugin { myKey.type === theirKey.type || (['Ed25519', 'X25519'].includes(myKey.type) && ['Ed25519', 'X25519'].includes(theirKey.type)) ) { + const kms = this.getKms(myKey.kms) + return kms.sharedSecret({ myKey, theirKey }) } else { throw new Error('invalid_argument: the key types have to match to be able to compute a shared secret') } - const kms = this.getKms(myKey.kms) - return kms.sharedSecret({ myKey, theirKey }) } createX25519ECDH(secretKeyRef: string): ECDH { diff --git a/packages/kms-local/src/key-management-system.ts b/packages/kms-local/src/key-management-system.ts index aa944e314..2848dedb5 100644 --- a/packages/kms-local/src/key-management-system.ts +++ b/packages/kms-local/src/key-management-system.ts @@ -10,8 +10,8 @@ import { parse } from '@ethersproject/transactions' import { Wallet } from '@ethersproject/wallet' import { SigningKey } from '@ethersproject/signing-key' import { randomBytes } from '@ethersproject/random' -import Debug from 'debug' import { arrayify, hexlify } from '@ethersproject/bytes' +import Debug from 'debug' const debug = Debug('veramo:kms:local') export class KeyManagementSystem extends AbstractKeyManagementSystem { @@ -161,8 +161,16 @@ export class KeyManagementSystem extends AbstractKeyManagementSystem { * @returns a `0x` prefixed hex string representing the signed raw transaction */ private async eth_signTransaction(privateKeyHex: string, rlpTransaction: Uint8Array) { - const { v, r, s, ...tx } = parse(rlpTransaction) + const { v, r, s, from, ...tx } = parse(rlpTransaction) const wallet = new Wallet(privateKeyHex) + if (from) { + debug('WARNING: executing a transaction signing request with a `from` field.') + if (wallet.address.toLowerCase() !== from.toLowerCase()) { + const msg = "invalid_arguments: eth_signTransaction `from` field does not match the chosen key. `from` field should be omitted." + debug(msg) + throw new Error(msg) + } + } const signedRawTransaction = await wallet.signTransaction(tx) //HEX encoded string, 0x prefixed return signedRawTransaction