fix: ensure impersonated "large" accounts use compliant (fake) private keys

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 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);
   }

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: {