From b29f33bd980c71756392e56587b23732a97d7d37 Mon Sep 17 00:00:00 2001 From: David Murdoch Date: Mon, 14 Mar 2022 18:24:16 -0400 Subject: [PATCH 1/3] fix: ensure a "large" address can be unlocked/used --- src/chains/ethereum/ethereum/src/wallet.ts | 39 ++++++++++++++- .../tests/api/eth/sendTransaction.test.ts | 50 +++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/wallet.ts b/src/chains/ethereum/ethereum/src/wallet.ts index 9fa26686e2..136f17f408 100644 --- a/src/chains/ethereum/ethereum/src/wallet.ts +++ b/src/chains/ethereum/ethereum/src/wallet.ts @@ -23,6 +23,8 @@ import { writeFileSync } from "fs"; import { EthereumInternalOptions } from "@ganache/ethereum-options"; import { Address } from "@ganache/ethereum-address"; +const TWELVE_255s = Buffer.allocUnsafe(12).fill(255); + //#region Constants const SCRYPT_PARAMS = { dklen: 32, @@ -626,7 +628,7 @@ export default class Wallet { public createFakePrivateKey(address: string) { let fakePrivateKey: Buffer; - const addressBuf = Data.from(address).toBuffer(); + const addressBuf = Address.from(address).toBuffer(); if (addressBuf.equals(ACCOUNT_ZERO)) { // allow signing with the 0x0 address... // always sign with the same fake key, a 31 `0`s followed by a single @@ -636,7 +638,40 @@ export default class Wallet { fakePrivateKey = Buffer.allocUnsafe(32).fill(0, 0, 31); fakePrivateKey[31] = 1; } else { - fakePrivateKey = Buffer.concat([addressBuf, addressBuf.slice(0, 12)]); + // Private keys must not be greater than *or equal to*: + // 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141, and + // if they are that large then signing of the transaction (ecdsaSign) will + // fail. + // Historically, we've just concatenated the address with part of itself, + // to make something long enough to use as a private key, so we can't + // change the default/legacy behavior now. But for addresses that would + // generate an invalid private key we must use a different approach. If + // the legacy way of generating a private key results in a key that is too + // large + // + const first12 = addressBuf.slice(0, 12); + fakePrivateKey = Buffer.concat([addressBuf, first12]); + // BigInt is slow, so only do it if we might need to, which we usually + // never will. + // Since we already have a slice of the first 12 bytes let's use it to + // help check if we might overflow the max secp256k1 key value. If the + // first 12 bytes, the most significant digits of the key, are all `255` + // then there is a chance that the fakePrivateKey will be too large. + // `Buffer.compare` is a native method in V8, so it should be pretty fast. + // example: the address `0xffffffffffff{anything can go here}` generates a + // bad fakePrivateKey but, `0xfffffffffffe{anything can go here}` never + // will. There are obviously many chances for a false positive, but the + // next condition in the `while` loop will catch those. + if (first12.compare(TWELVE_255s) === 0) { + // keccak returns a 32 byte hash of the input data, which is the exact + // length we need for a private key. + // note: if keccak can return it's own input as it's output, then this + // loops forever. The chances of this happening are impossibly low, so + // it's not worth the effort to check, but it would be interesting if + // someone reported an issue that can cause this for a specific + // address! + fakePrivateKey = keccak(fakePrivateKey); + } } return Data.from(fakePrivateKey); } diff --git a/src/chains/ethereum/ethereum/tests/api/eth/sendTransaction.test.ts b/src/chains/ethereum/ethereum/tests/api/eth/sendTransaction.test.ts index bfedf65b88..ebd9b1d590 100644 --- a/src/chains/ethereum/ethereum/tests/api/eth/sendTransaction.test.ts +++ b/src/chains/ethereum/ethereum/tests/api/eth/sendTransaction.test.ts @@ -265,6 +265,56 @@ describe("api", () => { ); }); + it("can send transactions from an unlocked \"large\" address", async () => { + // see https://github.com/trufflesuite/ganache/issues/2586 + const LARGE_ADDRESS = "0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE"; + const provider = await getProvider({ + miner: { + defaultGasPrice: 0 + }, + wallet: { + unlockedAccounts: [LARGE_ADDRESS] + }, + chain: { + // use berlin here because we just want to test if we can use a + // "large" address, and we do this by transferring value while + // setting the gasPrice to `0`. This isn't possible after the + // `london` hardfork currently, as we don't provide an option to + // allow for a 0 `maxFeePerGas` value. + // TODO: remove once we have a configurable `maxFeePerGas` + hardfork: "berlin" + } + }); + const [from] = await provider.send("eth_accounts"); + await provider.send("eth_subscribe", ["newHeads"]); + const initialZeroBalance = "0x1234"; + await provider.send("eth_sendTransaction", [ + { from: from, to: LARGE_ADDRESS, value: initialZeroBalance } + ]); + await provider.once("message"); + const initialBalance = await provider.send("eth_getBalance", [ + LARGE_ADDRESS + ]); + assert.strictEqual( + initialBalance, + initialZeroBalance, + "Zero address's balance isn't correct" + ); + const removeValueFromZeroAmount = "0x123"; + await provider.send("eth_sendTransaction", [ + { from: LARGE_ADDRESS, to: from, value: removeValueFromZeroAmount } + ]); + await provider.once("message"); + const afterSendBalance = BigInt( + await provider.send("eth_getBalance", [LARGE_ADDRESS]) + ); + assert.strictEqual( + BigInt(initialZeroBalance) - BigInt(removeValueFromZeroAmount), + afterSendBalance, + "Large address's balance isn't correct" + ); + }); + it("unlocks accounts via unlock_accounts (both string and numbered numbers)", async () => { const p = await getProvider({ wallet: { From d506017e0233f5f81c14957f8c54f6440e2a5f1a Mon Sep 17 00:00:00 2001 From: David Murdoch <187813+davidmurdoch@users.noreply.github.com> Date: Thu, 21 Apr 2022 12:01:14 -0400 Subject: [PATCH 2/3] Update src/chains/ethereum/ethereum/src/wallet.ts --- src/chains/ethereum/ethereum/src/wallet.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chains/ethereum/ethereum/src/wallet.ts b/src/chains/ethereum/ethereum/src/wallet.ts index 136f17f408..8f58500e56 100644 --- a/src/chains/ethereum/ethereum/src/wallet.ts +++ b/src/chains/ethereum/ethereum/src/wallet.ts @@ -665,7 +665,7 @@ export default class Wallet { if (first12.compare(TWELVE_255s) === 0) { // keccak returns a 32 byte hash of the input data, which is the exact // length we need for a private key. - // note: if keccak can return it's own input as it's output, then this + // note: if keccak can return its own input as its output, then this // loops forever. The chances of this happening are impossibly low, so // it's not worth the effort to check, but it would be interesting if // someone reported an issue that can cause this for a specific From dc3d662ab82f3ff34fce3c941a9a9fc23f3fad4d Mon Sep 17 00:00:00 2001 From: David Murdoch <187813+davidmurdoch@users.noreply.github.com> Date: Thu, 21 Apr 2022 12:02:41 -0400 Subject: [PATCH 3/3] Update src/chains/ethereum/ethereum/src/wallet.ts --- src/chains/ethereum/ethereum/src/wallet.ts | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/wallet.ts b/src/chains/ethereum/ethereum/src/wallet.ts index 8f58500e56..e67737eab9 100644 --- a/src/chains/ethereum/ethereum/src/wallet.ts +++ b/src/chains/ethereum/ethereum/src/wallet.ts @@ -663,14 +663,19 @@ export default class Wallet { // will. There are obviously many chances for a false positive, but the // next condition in the `while` loop will catch those. if (first12.compare(TWELVE_255s) === 0) { - // keccak returns a 32 byte hash of the input data, which is the exact - // length we need for a private key. - // note: if keccak can return its own input as its output, then this - // loops forever. The chances of this happening are impossibly low, so - // it's not worth the effort to check, but it would be interesting if - // someone reported an issue that can cause this for a specific - // address! - fakePrivateKey = keccak(fakePrivateKey); + while ( + BigInt(`0x${fakePrivateKey.toString("hex")}`) >= + SECP256K1_MAX_PRIVATE_KEY + ) { + // keccak returns a 32 byte hash of the input data, which is the exact + // length we need for a private key. + // note: if keccak can return its own input as its output, then this + // loops forever. The chances of this happening are impossibly low, so + // it's not worth the effort to check, but it would be interesting if + // someone reported an issue that can cause this for a specific + // address! + fakePrivateKey = keccak(fakePrivateKey); + } } } return Data.from(fakePrivateKey);