fix: enforce eip-2

src/chains/ethereum/ethereum/src/wallet.ts

@@ -14,7 +14,7 @@ import {
   WEI
 } from "@ganache/utils";
 import { privateToAddress } from "ethereumjs-util";
-import secp256k1 from "@ganache/secp256k1";
+import secp256k1, { SECP256K1_N } from "@ganache/secp256k1";
 import { mnemonicToSeedSync } from "bip39";
 import { alea } from "seedrandom";
 import crypto from "crypto";
@@ -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,42 @@ 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) {
+        while (BigInt(`0x${fakePrivateKey.toString("hex")}`) >= SECP256K1_N) {
+          // 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

@@ -3,7 +3,13 @@ import getProvider from "../../helpers/getProvider";
 import compile from "../../helpers/compile";
 import { join } from "path";
 import EthereumProvider from "../../../src/provider";
-import { EthereumProviderOptions } from "@ganache/ethereum-options";
+import {
+  EthereumOptionsConfig,
+  EthereumProviderOptions
+} from "@ganache/ethereum-options";
+import Wallet from "../../../src/wallet";
+import { SECP256K1_N } from "@ganache/secp256k1";
+import { Data, Quantity } from "@ganache/utils";
 
 describe("api", () => {
   describe("eth", () => {
@@ -318,6 +324,38 @@ describe("api", () => {
           const balance0_2 = await p.send("eth_getBalance", [accounts[0]]);
           assert.strictEqual(BigInt(balance0_1) + 123n, BigInt(balance0_2));
         });
+
+        it("generates an EIP-2 compliant private key", async () => {
+          // https://eips.ethereum.org/EIPS/eip-2
+
+          const options = EthereumOptionsConfig.normalize({});
+          const wallet = new Wallet(options.wallet);
+
+          function makeKeys(address: string) {
+            const addressBuf = Data.from(address).toBuffer();
+            const pk = BigInt(wallet.createFakePrivateKey(address).toString());
+            const naivePk = Quantity.from(
+              Buffer.concat([addressBuf, addressBuf.slice(0, 12)])
+            ).toBigInt();
+            return { naivePk, pk };
+          }
+
+          // sanity test. the small key doesn't trigger the secp256k1 upper
+          // limit
+          const smallKey = makeKeys(
+            "0xfffffffffffffffffffffffffffffffebaaedce5"
+          );
+          assert.strictEqual(smallKey.pk, smallKey.naivePk);
+          assert(smallKey.pk < SECP256K1_N);
+
+          // this is first (smallest) key that will trigger the secp256k1 upper
+          // limit code path
+          const largeKey = makeKeys(
+            "0xfffffffffffffffffffffffffffffffebaaedce6"
+          );
+          assert.notStrictEqual(largeKey.pk, largeKey.naivePk);
+          assert(largeKey.pk < SECP256K1_N);
+        });
       });
     });
   });

src/chains/ethereum/transaction/package.json

@@ -26,7 +26,7 @@
   },
   "scripts": {
     "tsc": "tsc --build",
-    "test": "nyc npm run mocha",
+    "test": "nyc --reporter lcov npm run mocha",
     "mocha": "cross-env TS_NODE_FILES=true mocha --timeout 5000 --exit --check-leaks --throw-deprecation --trace-warnings --require ts-node/register 'tests/**/*.test.ts'"
   },
   "bugs": {

src/chains/ethereum/transaction/src/legacy-transaction.ts

@@ -8,8 +8,8 @@ import {
 } from "@ganache/utils";
 import { Address } from "@ganache/ethereum-address";
 import type Common from "@ethereumjs/common";
-import { ecsign } from "ethereumjs-util";
-import { encodeRange, digest } from "@ganache/rlp";
+import { ECDSASignature, ECDSASignatureBuffer, ecsign } from "ethereumjs-util";
+import { encodeRange, digest, EncodedPart } from "@ganache/rlp";
 import { BN } from "ethereumjs-util";
 import { RuntimeTransaction } from "./runtime-transaction";
 import { TypedRpcTransaction } from "./rpc-transaction";
@@ -47,13 +47,8 @@ export class LegacyTransaction extends RuntimeTransaction {
       if (!extra) {
         // TODO(hack): Transactions that come from the database must not be
         // validated since they may come from a fork.
-        const {
-          from,
-          serialized,
-          hash,
-          encodedData,
-          encodedSignature
-        } = this.computeIntrinsics(this.v, this.raw, this.common.chainId());
+        const { from, serialized, hash, encodedData, encodedSignature } =
+          this.computeIntrinsics(this.v, this.raw, this.common.chainId());
 
         this.from = from;
         this.serialized = serialized;
@@ -170,20 +165,33 @@ export class LegacyTransaction extends RuntimeTransaction {
       );
     }
 
-    const chainId = this.common.chainId();
-    const raw: LegacyDatabasePayload = this.toEthRawTransaction(
-      Quantity.from(chainId).toBuffer(),
-      BUFFER_EMPTY,
-      BUFFER_EMPTY
-    );
-    const data = encodeRange(raw, 0, 6);
-    const dataLength = data.length;
-
-    const ending = encodeRange(raw, 6, 3);
-    const msgHash = keccak(
-      digest([data.output, ending.output], dataLength + ending.length)
-    );
-    const sig = ecsign(msgHash, privateKey, chainId);
+    // only legacy transactions can work with EIP-155 deactivated.
+    // (EIP-2930 and EIP-1559 made EIP-155 obsolete and this logic isn't needed
+    // for those transactions)
+    const eip155IsActive = this.common.gteHardfork("spuriousDragon");
+    let chainId: Buffer;
+    let raw: LegacyDatabasePayload;
+    let data: EncodedPart;
+    let dataLength: number;
+    let sig: ECDSASignature | ECDSASignatureBuffer;
+    if (eip155IsActive) {
+      chainId = this.common.chainIdBN().toArrayLike(Buffer);
+      raw = this.toEthRawTransaction(chainId, BUFFER_EMPTY, BUFFER_EMPTY);
+      data = encodeRange(raw, 0, 6);
+      dataLength = data.length;
+
+      const ending = encodeRange(raw, 6, 3);
+      const msgHash = keccak(
+        digest([data.output, ending.output], dataLength + ending.length)
+      );
+      sig = ecsign(msgHash, privateKey, chainId);
+    } else {
+      raw = this.toEthRawTransaction(BUFFER_EMPTY, BUFFER_EMPTY, BUFFER_EMPTY);
+      data = encodeRange(raw, 0, 6);
+      dataLength = data.length;
+      const msgHash = keccak(digest([data.output], dataLength));
+      sig = ecsign(msgHash, privateKey);
+    }
     this.v = Quantity.from(sig.v);
     this.r = Quantity.from(sig.r);
     this.s = Quantity.from(sig.s);

src/chains/ethereum/transaction/src/transaction-factory.ts

@@ -20,6 +20,28 @@ import { decode } from "@ganache/rlp";
 import { CodedError } from "@ganache/ethereum-utils";
 import { TypedTransaction } from "./transaction-types";
 import { EIP1559FeeMarketTransaction } from "./eip1559-fee-market-transaction";
+import { SECP256K1_MAX_PRIVATE_KEY_DIV_2 } from "@ganache/secp256k1";
+
+/**
+ * @param common
+ * @param tx
+ * @throws
+ */
+function assertValidTransactionSValue(common: Common, tx: TypedTransaction) {
+  // Transaction signatures whose s-value is greater than secp256k1n/2 are
+  // invalid after EIP-2 hardfork (homestead). See: https://eips.ethereum.org/EIPS/eip-2
+  if (
+    tx.s &&
+    tx.s.toBigInt() >= SECP256K1_MAX_PRIVATE_KEY_DIV_2 &&
+    // EIP-2 is in homestead, but we can't use isActivatedEIP(2) because
+    // Common doesn't have that information for this hardfork.
+    common.gteHardfork("homestead")
+  ) {
+    throw new Error(
+      "Invalid Signature: s-values greater than secp256k1n/2 are considered invalid"
+    );
+  }
+}
 
 const UNTYPED_TX_START_BYTE = 0xc0; // all txs with first byte >= 0xc0 are untyped
 
@@ -32,7 +54,7 @@ export enum TransactionType {
 export class TransactionFactory {
   public tx: TypedTransaction;
   constructor(raw: Buffer, common: Common) {
-    const [txData, extra] = (decode(raw) as any) as [
+    const [txData, extra] = decode(raw) as any as [
       TypedDatabaseTransaction,
       GanacheRawExtraTx
     ];
@@ -162,7 +184,9 @@ export class TransactionFactory {
   ) {
     const txType = this.typeOfRPC(txData);
 
-    return this._fromData(txData, txType, common, extra);
+    const tx = this._fromData(txData, txType, common, extra);
+    assertValidTransactionSValue(common, tx);
+    return tx;
   }
   /**
    * Create a transaction from a `txData` object
@@ -218,6 +242,7 @@ export class TransactionFactory {
     let data = Data.from(txData).toBuffer();
     const type = data[0];
     const txType = this.typeOf(type);
+    let tx: TypedTransaction;
     if (common.isActivatedEIP(2718)) {
       let raw: TypedDatabasePayload;
       try {
@@ -227,16 +252,20 @@ export class TransactionFactory {
       } catch (e: any) {
         throw new Error("Could not decode transaction: " + e.message);
       }
-      return this._fromData(raw, txType, common);
+      tx = this._fromData(raw, txType, common);
     } else {
       let raw: TypedDatabasePayload;
       try {
         raw = decode<LegacyDatabasePayload>(data);
       } catch (e: any) {
         throw new Error("Could not decode transaction: " + e.message);
       }
-      return this._fromData(raw, TransactionType.Legacy, common);
+      tx = this._fromData(raw, TransactionType.Legacy, common);
     }
+
+    assertValidTransactionSValue(common, tx);
+
+    return tx;
   }
 
   private static typeOf(type: number) {

src/chains/ethereum/transaction/tests/index.test.ts

@@ -18,6 +18,7 @@ import Wallet from "../../ethereum/src/wallet";
 import { decode } from "@ganache/rlp";
 import { EthereumOptionsConfig } from "../../options";
 import { BUFFER_EMPTY, Quantity } from "@ganache/utils";
+import { SECP256K1_N } from "@ganache/secp256k1";
 
 describe("@ganache/ethereum-transaction", async () => {
   const common = Common.forCustomChain(
@@ -169,6 +170,65 @@ describe("@ganache/ethereum-transaction", async () => {
         const txFromRpc = TransactionFactory.fromRpc(tempAccessListTx, common);
         assert.strictEqual(txFromRpc.type.toString(), "0x0");
       });
+
+      describe("EIP-2", () => {
+        it("rejects transactions with too-high S values only when EIP-2 is active", () => {
+          const genesis = Common.forCustomChain(
+            "mainnet",
+            {
+              name: "ganache",
+              chainId: 1,
+              comment: "Local test network",
+              bootstrapNodes: []
+            },
+            // EIP-2 was in homestead, the first hardfork, so we need to create
+            // a common at the genesis (aka chainstart) so we can test at a fork
+            // where it is NOT active:
+            "chainstart"
+          );
+
+          const tx = <LegacyTransaction>(
+            TransactionFactory.fromRpc(typedLegacyTx, genesis)
+          );
+          tx.nonce = Quantity.from(1);
+          tx.signAndHash(privKeyBuf);
+          // Use `tx` to create a new transaction with "flipped" s and v values.
+          // This is called transaction "malleability". By changing the
+          // signature this way we still have a valid signature for the same
+          // address! EIP-2 was introduced to forbid this.
+          // See: https://ethereum.stackexchange.com/questions/55245/why-is-s-in-transaction-signature-limited-to-n-21
+
+          // flip the `v` value:
+          tx.v = Quantity.from(tx.v.toBigInt() === 28n ? 27n : 28n);
+
+          // flip the `s` value:
+          tx.s = Quantity.from(SECP256K1_N - tx.s.toBigInt());
+
+          // convert to a JSON-RPC transaction:
+          const flipTx = JSON.parse(JSON.stringify(tx.toJSON(genesis)));
+
+          // make sure chainstart allows it (implicitly - by not throwing):
+          const flipTxInstance = TransactionFactory.fromRpc(flipTx, genesis);
+
+          // convert to a RAW transaction:
+          const flipRaw = `0x${flipTxInstance.serialized.toString("hex")}`;
+
+          // make sure chainstart allows it
+          assert.doesNotThrow(() =>
+            TransactionFactory.fromString(flipRaw, genesis)
+          );
+
+          const message =
+            "Invalid Signature: s-values greater than secp256k1n/2 are considered invalid";
+          // now check it against a common with a hardfork _after_ EIP-2
+          assert.throws(() => TransactionFactory.fromRpc(flipTx, common), {
+            message
+          });
+          assert.throws(() => TransactionFactory.fromString(flipRaw, common), {
+            message
+          });
+        });
+      });
     });
 
     describe("EIP2930AccessListTransaction type from factory", () => {

src/packages/secp256k1/index.ts

@@ -7,6 +7,11 @@
 
 import { dirname } from "path";
 
+export const SECP256K1_N =
+  0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141n;
+export const SECP256K1_MAX_PRIVATE_KEY_DIV_2 =
+  0x7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0n; // (SECP256K1_N - 1n) / 2n
+
 let secp256k1: {
   ecdsaRecover: (
     output: Uint8Array,