fix: Reject ECDSA signatures with high-s values and v != 27/28 (#18032)

As defined in EIP2, we do not support ECDSA signatures with s-values in
the upper half of the curve on our contracts:


https://github.com/OpenZeppelin/openzeppelin-contracts/blob/51ab591cd7a47446293a0d5e285792f63cbeb1ea/contracts/utils/cryptography/ECDSA.sol#L176-L184

However, we do allow them when verifying them offchain. This can lead to
a proposer collecting attestations, verifying their signatures
successfully, but then having them rejected when trying to submit a tx.

This PR changes the default behaviour of our Secp256k1 ECDSA utils so
that we reject a signature as invalid if it's in the high-S values. It
also adds a safeguard around web3signer so, if it emits an invalid
signature, it flips it to the correct side.

This PR also makes `recover` to also reject signatures with v-values
other than 27/28, since these are rejected by ECRECOVER. Removing this
check would've meant that an attestor could have supplied a signature
with yParity bit instead of v, which would've been accepted by the node,
posted on chain, and then made the block eligible for invalidation.

Fixes A-182

Author: spalladino

l1-contracts/src/core/libraries/Errors.sol

@@ -211,4 +211,32 @@ library Errors {
 
   // SlashPayloadLib
   error SlashPayload_ArraySizeMismatch(uint256 expected, uint256 actual);
+
+  // OpenZeppelin dependencies
+
+  // ECDSA
+  error ECDSAInvalidSignature();
+  error ECDSAInvalidSignatureLength(uint256 length);
+  error ECDSAInvalidSignatureS(bytes32 s);
+
+  // Ownable
+  error OwnableUnauthorizedAccount(address account);
+  error OwnableInvalidOwner(address owner);
+
+  // Checkpoints
+  error CheckpointUnorderedInsertion();
+
+  // ERC20
+  error ERC20InsufficientBalance(address sender, uint256 balance, uint256 needed);
+  error ERC20InvalidSender(address sender);
+  error ERC20InvalidReceiver(address receiver);
+  error ERC20InsufficientAllowance(address spender, uint256 allowance, uint256 needed);
+  error ERC20InvalidApprover(address approver);
+  error ERC20InvalidSpender(address spender);
+
+  // SafeCast
+  error SafeCastOverflowedUintDowncast(uint8 bits, uint256 value);
+  error SafeCastOverflowedIntToUint(int256 value);
+  error SafeCastOverflowedIntDowncast(uint8 bits, int256 value);
+  error SafeCastOverflowedUintToInt(uint256 value);
 }

yarn-project/end-to-end/src/e2e_l1_publisher/e2e_l1_publisher.test.ts

@@ -21,7 +21,7 @@ import { range } from '@aztec/foundation/array';
 import { Buffer32 } from '@aztec/foundation/buffer';
 import { times, timesParallel } from '@aztec/foundation/collection';
 import { SecretValue } from '@aztec/foundation/config';
-import { SHA256Trunc, Secp256k1Signer, sha256ToField } from '@aztec/foundation/crypto';
+import { SHA256Trunc, Secp256k1Signer, flipSignature, sha256ToField } from '@aztec/foundation/crypto';
 import { EthAddress } from '@aztec/foundation/eth-address';
 import { hexToBuffer } from '@aztec/foundation/string';
 import { TestDateProvider } from '@aztec/foundation/timer';
@@ -592,6 +592,52 @@ describe('L1Publisher integration', () => {
       );
     });
 
+    it('rejects flipped proposer signature', async () => {
+      const block = await buildSingleBlock();
+      const blockAttestations = validators.map(v => makeBlockAttestationFromBlock(block, v));
+      const attestations = orderAttestations(blockAttestations, committee!);
+
+      const canPropose = await publisher.canProposeAtNextEthBlock(new Fr(GENESIS_ARCHIVE_ROOT), proposer!);
+      expect(canPropose?.slot).toEqual(block.header.getSlot());
+      await publisher.validateBlockHeader(block.getCheckpointHeader());
+
+      const attestationsAndSigners = new CommitteeAttestationsAndSigners(attestations);
+      const attestationsAndSignersSignature = makeAndSignCommitteeAttestationsAndSigners(
+        attestationsAndSigners,
+        validators.find(v => v.address.equals(proposer!))!,
+      );
+
+      await expect(
+        publisher.enqueueProposeL2Block(block, attestationsAndSigners, flipSignature(attestationsAndSignersSignature)),
+      ).rejects.toThrow(/ECDSAInvalidSignatureS/);
+    });
+
+    it('rejects signature with invalid recovery value', async () => {
+      const block = await buildSingleBlock();
+      const blockAttestations = validators.map(v => makeBlockAttestationFromBlock(block, v));
+      const attestations = orderAttestations(blockAttestations, committee!);
+
+      const canPropose = await publisher.canProposeAtNextEthBlock(new Fr(GENESIS_ARCHIVE_ROOT), proposer!);
+      expect(canPropose?.slot).toEqual(block.header.getSlot());
+      await publisher.validateBlockHeader(block.getCheckpointHeader());
+
+      const attestationsAndSigners = new CommitteeAttestationsAndSigners(attestations);
+      const attestationsAndSignersSignature = makeAndSignCommitteeAttestationsAndSigners(
+        attestationsAndSigners,
+        validators.find(v => v.address.equals(proposer!))!,
+      );
+
+      logger.warn(`Original v value: ${attestationsAndSignersSignature.v}`);
+
+      // Move v-value from 27-28 to 0-1
+      const wrongV = attestationsAndSignersSignature.v - 27;
+      const wrongSig = new Signature(attestationsAndSignersSignature.r, attestationsAndSignersSignature.s, wrongV);
+
+      await expect(publisher.enqueueProposeL2Block(block, attestationsAndSigners, wrongSig)).rejects.toThrow(
+        /ECDSAInvalidSignature/,
+      );
+    });
+
     it('publishes a block invalidating the previous one', async () => {
       const badBlock = await buildSingleBlock();
 

yarn-project/foundation/src/crypto/secp256k1-signer/malleability.test.ts

@@ -0,0 +1,141 @@
+import { Buffer32 } from '@aztec/foundation/buffer';
+
+import { generatePrivateKey } from 'viem/accounts';
+
+import type { EthAddress } from '../../eth-address/index.js';
+import { Signature } from '../../eth-signature/eth_signature.js';
+import { Secp256k1Signer } from './secp256k1_signer.js';
+import {
+  Secp256k1Error,
+  flipSignature,
+  makeEthSignDigest,
+  normalizeSignature,
+  recoverAddress,
+  tryRecoverAddress,
+} from './utils.js';
+
+describe('ecdsa malleability', () => {
+  let privateKey: `0x${string}`;
+  let signer: Secp256k1Signer;
+  let expectedAddress: EthAddress;
+  let message: Buffer32;
+  let digest: Buffer32;
+  let originalSignature: Signature;
+
+  beforeEach(() => {
+    // Generate a random private key and signer
+    privateKey = generatePrivateKey();
+    signer = new Secp256k1Signer(Buffer32.fromBuffer(Buffer.from(privateKey.slice(2), 'hex')));
+    expectedAddress = signer.address;
+
+    // Create a random message and sign it
+    message = Buffer32.random();
+    digest = makeEthSignDigest(message);
+    originalSignature = signer.sign(digest);
+  });
+
+  it('recovers the same address from both original and flipped signatures', () => {
+    // Recover address from original signature
+    const recoveredFromOriginal = recoverAddress(digest, originalSignature);
+    expect(recoveredFromOriginal.toString()).toEqual(expectedAddress.toString());
+
+    // Flip the signature
+    const flippedSignature = flipSignature(originalSignature);
+
+    // Ensure the flipped signature is different
+    expect(flippedSignature.equals(originalSignature)).toBe(false);
+    expect(flippedSignature.r.equals(originalSignature.r)).toBe(true); // r should be the same
+    expect(flippedSignature.s.equals(originalSignature.s)).toBe(false); // s should be different
+    expect(flippedSignature.v).not.toEqual(originalSignature.v); // v should be different
+
+    // Recover address from flipped signature (must use allowMalleable: true)
+    const recoveredFromFlipped = recoverAddress(digest, flippedSignature, { allowMalleable: true });
+    expect(recoveredFromFlipped.toString()).toEqual(expectedAddress.toString());
+    expect(() => recoverAddress(digest, flippedSignature)).toThrow(Secp256k1Error);
+
+    // Both recovered addresses should match
+    expect(recoveredFromOriginal.equals(recoveredFromFlipped)).toBe(true);
+  });
+
+  it('flips signature back and forth correctly', () => {
+    // Flip once
+    const flipped = flipSignature(originalSignature);
+
+    // Flip back
+    const flippedBack = flipSignature(flipped);
+
+    // Should match the original
+    expect(flippedBack.equals(originalSignature)).toBe(true);
+  });
+
+  it('rejects malleable signatures by default in recoverAddress', () => {
+    // Original signature should work
+    expect(() => recoverAddress(digest, originalSignature)).not.toThrow();
+
+    // Flip the signature to make it malleable
+    const flippedSignature = flipSignature(originalSignature);
+
+    // Flipped signature should be rejected by default
+    expect(() => recoverAddress(digest, flippedSignature)).toThrow(Secp256k1Error);
+  });
+
+  it('accepts malleable signatures when allowMalleable is true', () => {
+    // Flip the signature to make it malleable
+    const flippedSignature = flipSignature(originalSignature);
+
+    // Should work with allowMalleable: true
+    const recoveredAddress = recoverAddress(digest, flippedSignature, { allowMalleable: true });
+    expect(recoveredAddress.toString()).toEqual(expectedAddress.toString());
+  });
+
+  it('rejects malleable signatures by default in tryRecoverAddress', () => {
+    // Original signature should work
+    expect(tryRecoverAddress(digest, originalSignature)).toBeDefined();
+
+    // Flip the signature to make it malleable
+    const flippedSignature = flipSignature(originalSignature);
+
+    // Flipped signature should return undefined by default
+    expect(tryRecoverAddress(digest, flippedSignature)).toBeUndefined();
+  });
+
+  it('accepts malleable signatures in tryRecoverAddress when allowMalleable is true', () => {
+    // Flip the signature to make it malleable
+    const flippedSignature = flipSignature(originalSignature);
+
+    // Should work with allowMalleable: true
+    const recoveredAddress = tryRecoverAddress(digest, flippedSignature, { allowMalleable: true });
+    expect(recoveredAddress).toBeDefined();
+    expect(recoveredAddress!.toString()).toEqual(expectedAddress.toString());
+  });
+
+  it('normalizes signature with high s-value correctly', () => {
+    // Flip the signature to create a high s-value signature
+    const highSSignature = flipSignature(originalSignature);
+
+    // Recover address using the high s-value signature with allowMalleable: true
+    const recoveredAddress = recoverAddress(digest, highSSignature, { allowMalleable: true });
+    expect(recoveredAddress.toString()).toEqual(expectedAddress.toString());
+
+    // Check that the signature is flipped back to low s-value when normalized
+    const normalizedSignature = flipSignature(highSSignature);
+    expect(normalizedSignature.equals(originalSignature)).toBe(true);
+  });
+
+  it('does not alter low s-value signatures when normalizing', () => {
+    // Recover address using the original low s-value signature
+    const recoveredAddress = recoverAddress(digest, originalSignature);
+    expect(recoveredAddress.toString()).toEqual(expectedAddress.toString());
+
+    // Normalize the signature (should remain unchanged)
+    const normalizedSignature = normalizeSignature(originalSignature);
+    expect(normalizedSignature.equals(originalSignature)).toBe(true);
+  });
+
+  it('rejects signatures with invalid v-value', () => {
+    const signature = new Signature(originalSignature.r, originalSignature.s, originalSignature.v - 27);
+    expect(() => recoverAddress(digest, signature)).toThrow(Secp256k1Error);
+    const recoveredAddress = recoverAddress(digest, signature, { allowYParityAsV: true });
+    expect(recoveredAddress.toString()).toEqual(expectedAddress.toString());
+  });
+});

yarn-project/foundation/src/crypto/secp256k1-signer/secp256k1_signer.test.ts

@@ -22,7 +22,7 @@ describe('Secp256k1Signer', () => {
     lightSigner = new Secp256k1Signer(Buffer32.fromBuffer(Buffer.from(privateKey.slice(2), 'hex')));
   });
 
-  it('Compare implementation against viem', async () => {
+  it('compares implementation against viem', async () => {
     const message = Buffer.from('0x0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef', 'hex');
     // Use to compare addresses at the end
     const accountAddress = viemSigner.address;