fix: Handle invalid ecdsa signatures

yarn-project/aztec-node/src/sentinel/sentinel.ts

@@ -321,8 +321,10 @@ export class Sentinel extends (EventEmitter as new () => WatcherEmitter) impleme
     // (contains the ones synced from mined blocks, which we may have missed from p2p).
     const block = this.slotNumberToBlock.get(slot);
     const p2pAttested = await this.p2p.getAttestationsForSlot(slot, block?.archive);
+    // Filter out attestations with invalid signatures
+    const p2pAttestors = p2pAttested.map(a => a.getSender()).filter((s): s is EthAddress => s !== undefined);
     const attestors = new Set(
-      [...p2pAttested.map(a => a.getSender().toString()), ...(block?.attestors.map(a => a.toString()) ?? [])].filter(
+      [...p2pAttestors.map(a => a.toString()), ...(block?.attestors.map(a => a.toString()) ?? [])].filter(
         addr => proposer.toString() !== addr, // Exclude the proposer from the attestors
       ),
     );

yarn-project/end-to-end/src/e2e_multi_validator/e2e_multi_validator_node.test.ts

@@ -139,7 +139,7 @@ describe('e2e_multi_validator_node', () => {
 
     expect(attestations.length).toBeGreaterThanOrEqual((COMMITTEE_SIZE * 2) / 3 + 1);
 
-    const signers = attestations.map(att => att.getSender().toString());
+    const signers = attestations.map(att => att.getSender()!.toString());
 
     expect(signers.every(s => validatorAddresses.includes(s))).toBe(true);
   });
@@ -196,7 +196,7 @@ describe('e2e_multi_validator_node', () => {
 
     expect(attestations.length).toBeGreaterThanOrEqual((COMMITTEE_SIZE * 2) / 3 + 1);
 
-    const signers = attestations.map(att => att.getSender().toString());
+    const signers = attestations.map(att => att.getSender()!.toString());
 
     expect(signers).toEqual(expect.arrayContaining(validatorAddresses.slice(0, COMMITTEE_SIZE)));
   });

yarn-project/end-to-end/src/e2e_p2p/gossip_network.test.ts

@@ -174,7 +174,7 @@ describe('e2e_p2p_network', () => {
     const attestations = block.attestations
       .filter(a => !a.signature.isEmpty())
       .map(a => new BlockAttestation(blockNumber, payload, a.signature, Signature.empty()));
-    const signers = await Promise.all(attestations.map(att => att.getSender().toString()));
+    const signers = await Promise.all(attestations.map(att => att.getSender()!.toString()));
     t.logger.info(`Attestation signers`, { signers });
 
     // Check that the signers found are part of the proposer nodes to ensure the archiver fetched them right

yarn-project/end-to-end/src/e2e_p2p/gossip_network_no_cheat.test.ts

@@ -227,7 +227,7 @@ describe('e2e_p2p_network', () => {
     const attestations = block.attestations
       .filter(a => !a.signature.isEmpty())
       .map(a => new BlockAttestation(blockNumber, payload, a.signature, Signature.empty()));
-    const signers = await Promise.all(attestations.map(att => att.getSender().toString()));
+    const signers = await Promise.all(attestations.map(att => att.getSender()!.toString()));
     t.logger.info(`Attestation signers`, { signers });
 
     // Check that the signers found are part of the proposer nodes to ensure the archiver fetched them right

yarn-project/end-to-end/src/e2e_p2p/preferred_gossip_network.test.ts

@@ -359,7 +359,7 @@ describe('e2e_p2p_preferred_network', () => {
     const attestations = block.attestations
       .filter(a => !a.signature.isEmpty())
       .map(a => new BlockAttestation(blockNumber, payload, a.signature, Signature.empty()));
-    const signers = await Promise.all(attestations.map(att => att.getSender().toString()));
+    const signers = await Promise.all(attestations.map(att => att.getSender()!.toString()));
     t.logger.info(`Attestation signers`, { signers });
 
     expect(signers.length).toEqual(validators.length);

yarn-project/foundation/src/crypto/secp256k1-signer/utils.ts

@@ -47,6 +47,7 @@ export function addressFromPrivateKey(privateKey: Buffer): EthAddress {
  * @param hash - The hash to recover the address from.
  * @param signature - The signature to recover the address from.
  * @returns The address.
+ * @throws Error if signature recovery fails.
  */
 export function recoverAddress(hash: Buffer32, signature: Signature): EthAddress {
   try {
@@ -59,6 +60,21 @@ export function recoverAddress(hash: Buffer32, signature: Signature): EthAddress
   }
 }
 
+/**
+ * Safely attempts to recover an address from a hash and a signature.
+ * @param hash - The hash to recover the address from.
+ * @param signature - The signature to recover the address from.
+ * @returns The address if recovery succeeds, undefined otherwise.
+ */
+export function tryRecoverAddress(hash: Buffer32, signature: Signature): EthAddress | undefined {
+  try {
+    const publicKey = recoverPublicKey(hash, signature);
+    return publicKeyToAddress(publicKey);
+  } catch {
+    return undefined;
+  }
+}
+
 /**
  * @attribution - viem
  * Converts a yParityOrV value to a recovery bit.

yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool_test_suite.ts

@@ -117,7 +117,7 @@ export function describeAttestationPool(getAttestationPool: () => AttestationPoo
     const retreivedAttestations = await ap.getAttestationsForSlotAndProposal(BigInt(slotNumber), archive.toString());
     expect(retreivedAttestations.length).toBe(1);
     expect(retreivedAttestations[0].toBuffer()).toEqual(attestations[0].toBuffer());
-    expect(retreivedAttestations[0].getSender().toString()).toEqual(signer.address.toString());
+    expect(retreivedAttestations[0].getSender()?.toString()).toEqual(signer.address.toString());
 
     // Try adding them on another operation and check they are still not duplicated
     await ap.addAttestations([attestations[0]]);
@@ -291,7 +291,7 @@ export function describeAttestationPool(getAttestationPool: () => AttestationPoo
       expect(retrievedProposal).toBeDefined();
       // Should have the second proposal
       expect(retrievedProposal!.toBuffer()).toEqual(proposal2.toBuffer());
-      expect(retrievedProposal!.getSender().toString()).toBe(signers[1].address.toString());
+      expect(retrievedProposal!.getSender()?.toString()).toBe(signers[1].address.toString());
     });
 
     it('should handle block proposals with different slots and same archive', async () => {

yarn-project/p2p/src/mem_pools/attestation_pool/kv_attestation_pool.ts

@@ -66,7 +66,19 @@ export class KvAttestationPool implements AttestationPool {
       for (const attestation of attestations) {
         const slotNumber = attestation.payload.header.slotNumber;
         const proposalId = attestation.archive;
-        const address = attestation.getSender().toString();
+        const sender = attestation.getSender();
+
+        // Skip attestations with invalid signatures
+        if (!sender) {
+          this.log.warn(`Skipping attestation with invalid signature for slot ${slotNumber.toBigInt()}`, {
+            signature: attestation.signature.toString(),
+            slotNumber,
+            proposalId,
+          });
+          continue;
+        }
+
+        const address = sender.toString();
 
         await this.attestations.set(this.getAttestationKey(slotNumber, proposalId, address), attestation.toBuffer());
 
@@ -176,7 +188,15 @@ export class KvAttestationPool implements AttestationPool {
       for (const attestation of attestations) {
         const slotNumber = attestation.payload.header.slotNumber;
         const proposalId = attestation.archive;
-        const address = attestation.getSender().toString();
+        const sender = attestation.getSender();
+
+        // Skip attestations with invalid signatures
+        if (!sender) {
+          this.log.warn(`Skipping deletion of attestation with invalid signature for slot ${slotNumber.toBigInt()}`);
+          continue;
+        }
+
+        const address = sender.toString();
         const key = this.getAttestationKey(slotNumber, proposalId, address);
 
         if (await this.attestations.hasAsync(key)) {

yarn-project/p2p/src/mem_pools/attestation_pool/memory_attestation_pool.ts

@@ -55,16 +55,26 @@ export class InMemoryAttestationPool implements AttestationPool {
       const slotNumber = attestation.payload.header.slotNumber;
 
       const proposalId = attestation.archive.toString();
-      const address = attestation.getSender();
+      const sender = attestation.getSender();
+
+      // Skip attestations with invalid signatures
+      if (!sender) {
+        this.log.warn(`Skipping attestation with invalid signature for slot ${slotNumber.toBigInt()}`, {
+          signature: attestation.signature.toString(),
+          slotNumber,
+          proposalId,
+        });
+        continue;
+      }
 
       const slotAttestationMap = getSlotOrDefault(this.attestations, slotNumber.toBigInt());
       const proposalAttestationMap = getProposalOrDefault(slotAttestationMap, proposalId);
-      proposalAttestationMap.set(address.toString(), attestation);
+      proposalAttestationMap.set(sender.toString(), attestation);
 
-      this.log.verbose(`Added attestation for slot ${slotNumber.toBigInt()} from ${address}`, {
+      this.log.verbose(`Added attestation for slot ${slotNumber.toBigInt()} from ${sender}`, {
         signature: attestation.signature.toString(),
         slotNumber,
-        address,
+        address: sender,
         proposalId,
       });
     }
@@ -147,9 +157,16 @@ export class InMemoryAttestationPool implements AttestationPool {
         const proposalId = attestation.archive.toString();
         const proposalAttestationMap = getProposalOrDefault(slotAttestationMap, proposalId);
         if (proposalAttestationMap) {
-          const address = attestation.getSender();
-          proposalAttestationMap.delete(address.toString());
-          this.log.debug(`Deleted attestation for slot ${slotNumber} from ${address}`);
+          const sender = attestation.getSender();
+
+          // Skip attestations with invalid signatures
+          if (!sender) {
+            this.log.warn(`Skipping deletion of attestation with invalid signature for slot ${slotNumber.toBigInt()}`);
+            continue;
+          }
+
+          proposalAttestationMap.delete(sender.toString());
+          this.log.debug(`Deleted attestation for slot ${slotNumber} from ${sender}`);
         }
       }
     }

yarn-project/p2p/src/msg_validators/attestation_validator/attestation_validator.ts

@@ -26,8 +26,14 @@ export class AttestationValidator implements P2PValidator<BlockAttestation> {
         return PeerErrorSeverity.HighToleranceError;
       }
 
-      // Verify the attester is in the committee for this slot
+      // Verify the signature is valid
       const attester = message.getSender();
+      if (attester === undefined) {
+        this.logger.warn(`Invalid signature in attestation for slot ${slotNumberBigInt}`);
+        return PeerErrorSeverity.LowToleranceError;
+      }
+
+      // Verify the attester is in the committee for this slot
       if (!(await this.epochCache.isInCommittee(slotNumberBigInt, attester))) {
         this.logger.warn(`Attester ${attester.toString()} is not in committee for slot ${slotNumberBigInt}`);
         return PeerErrorSeverity.HighToleranceError;

yarn-project/p2p/src/msg_validators/block_proposal_validator/block_proposal_validator.ts

@@ -14,6 +14,13 @@ export class BlockProposalValidator implements P2PValidator<BlockProposal> {
 
   async validate(block: BlockProposal): Promise<PeerErrorSeverity | undefined> {
     try {
+      // Check signature validity first - invalid signatures are a high-severity issue
+      const proposer = block.getSender();
+      if (!proposer) {
+        this.logger.debug(`Penalizing peer for block proposal with invalid signature`);
+        return PeerErrorSeverity.MidToleranceError;
+      }
+
       const { currentProposer, nextProposer, currentSlot, nextSlot } =
         await this.epochCache.getProposerAttesterAddressInCurrentOrNextSlot();
 
@@ -25,12 +32,11 @@ export class BlockProposalValidator implements P2PValidator<BlockProposal> {
       }
 
       // Check that the block proposal is from the current or next proposer
-      const proposer = block.getSender();
       if (slotNumberBigInt === currentSlot && currentProposer !== undefined && !proposer.equals(currentProposer)) {
         this.logger.debug(`Penalizing peer for invalid proposer for current slot ${slotNumberBigInt}`, {
           currentProposer,
           nextProposer,
-          proposer,
+          proposer: proposer.toString(),
         });
         return PeerErrorSeverity.MidToleranceError;
       }
@@ -39,7 +45,7 @@ export class BlockProposalValidator implements P2PValidator<BlockProposal> {
         this.logger.debug(`Penalizing peer for invalid proposer for next slot ${slotNumberBigInt}`, {
           currentProposer,
           nextProposer,
-          proposer,
+          proposer: proposer.toString(),
         });
         return PeerErrorSeverity.MidToleranceError;
       }

yarn-project/p2p/src/services/peer-manager/peer_manager.ts

@@ -1,5 +1,5 @@
 import type { EpochCacheInterface } from '@aztec/epoch-cache';
-import { makeEthSignDigest, recoverAddress } from '@aztec/foundation/crypto';
+import { makeEthSignDigest, tryRecoverAddress } from '@aztec/foundation/crypto';
 import type { EthAddress } from '@aztec/foundation/eth-address';
 import { Fr } from '@aztec/foundation/fields';
 import { createLogger } from '@aztec/foundation/log';
@@ -907,7 +907,14 @@ export class PeerManager implements PeerManagerInterface {
 
       const hashToRecover = authRequest.getPayloadToSign();
       const ethSignedHash = makeEthSignDigest(hashToRecover);
-      const sender = recoverAddress(ethSignedHash, peerAuthResponse.signature);
+      const sender = tryRecoverAddress(ethSignedHash, peerAuthResponse.signature);
+      if (!sender) {
+        this.logger.verbose(`Disconnecting peer ${peerId} due to failed auth handshake, invalid signature.`, logData);
+        this.markAuthHandshakeFailed(peerId);
+        this.markPeerForDisconnect(peerId);
+        return;
+      }
+
       const registeredValidators = await this.epochCache.getRegisteredValidators();
       const found = registeredValidators.find(v => v.toString() === sender.toString()) !== undefined;
       if (!found) {

yarn-project/stdlib/src/p2p/block_attestation.ts

@@ -1,5 +1,5 @@
 import { Buffer32 } from '@aztec/foundation/buffer';
-import { keccak256, recoverAddress } from '@aztec/foundation/crypto';
+import { keccak256, tryRecoverAddress } from '@aztec/foundation/crypto';
 import type { EthAddress } from '@aztec/foundation/eth-address';
 import { Signature } from '@aztec/foundation/eth-signature';
 import { Fr } from '@aztec/foundation/fields';
@@ -73,31 +73,19 @@ export class BlockAttestation extends Gossipable {
 
   /**
    * Lazily evaluate and cache the signer of the attestation
-   * @returns The signer of the attestation
+   * @returns The signer of the attestation, or undefined if signature recovery fails
    */
-  getSender(): EthAddress {
+  getSender(): EthAddress | undefined {
     if (!this.sender) {
       // Recover the sender from the attestation
       const hashed = getHashedSignaturePayloadEthSignedMessage(this.payload, SignatureDomainSeparator.blockAttestation);
       // Cache the sender for later use
-      this.sender = recoverAddress(hashed, this.signature);
+      this.sender = tryRecoverAddress(hashed, this.signature);
     }
 
     return this.sender;
   }
 
-  /**
-   * Tries to get the sender of the attestation
-   * @returns The sender of the attestation or undefined if it fails during recovery
-   */
-  tryGetSender(): EthAddress | undefined {
-    try {
-      return this.getSender();
-    } catch {
-      return undefined;
-    }
-  }
-
   /**
    * Lazily evaluate and cache the proposer of the block
    * @returns The proposer of the block
@@ -107,7 +95,7 @@ export class BlockAttestation extends Gossipable {
       // Recover the proposer from the proposal signature
       const hashed = getHashedSignaturePayloadEthSignedMessage(this.payload, SignatureDomainSeparator.blockProposal);
       // Cache the proposer for later use
-      this.proposer = recoverAddress(hashed, this.proposerSignature);
+      this.proposer = tryRecoverAddress(hashed, this.proposerSignature)!;
     }
 
     return this.proposer;

yarn-project/stdlib/src/p2p/block_proposal.ts

@@ -1,5 +1,5 @@
 import { Buffer32 } from '@aztec/foundation/buffer';
-import { keccak256, recoverAddress } from '@aztec/foundation/crypto';
+import { keccak256, tryRecoverAddress } from '@aztec/foundation/crypto';
 import type { EthAddress } from '@aztec/foundation/eth-address';
 import { Signature } from '@aztec/foundation/eth-signature';
 import { Fr } from '@aztec/foundation/fields';
@@ -100,12 +100,13 @@ export class BlockProposal extends Gossipable {
 
   /**Get Sender
    * Lazily evaluate the sender of the proposal; result is cached
+   * @returns The sender address, or undefined if signature recovery fails
    */
-  getSender() {
+  getSender(): EthAddress | undefined {
     if (!this.sender) {
       const hashed = getHashedSignaturePayloadEthSignedMessage(this.payload, SignatureDomainSeparator.blockProposal);
       // Cache the sender for later use
-      this.sender = recoverAddress(hashed, this.signature);
+      this.sender = tryRecoverAddress(hashed, this.signature);
     }
 
     return this.sender;

yarn-project/validator-client/src/block_proposal_handler.ts

@@ -100,6 +100,12 @@ export class BlockProposalHandler {
     const blockNumber = proposal.blockNumber;
     const proposer = proposal.getSender();
 
+    // Reject proposals with invalid signatures
+    if (!proposer) {
+      this.log.warn(`Received proposal with invalid signature for slot ${slotNumber}`);
+      return { isValid: false, reason: 'invalid_proposal' };
+    }
+
     const proposalInfo = { ...proposal.toBlockInfo(), proposer: proposer.toString() };
     this.log.info(`Processing proposal for slot ${slotNumber}`, {
       ...proposalInfo,

yarn-project/validator-client/src/metrics.ts

@@ -74,9 +74,10 @@ export class ValidatorMetrics {
   }
 
   public recordFailedReexecution(proposal: BlockProposal) {
+    const proposer = proposal.getSender();
     this.failedReexecutionCounter.add(1, {
       [Attributes.STATUS]: 'failed',
-      [Attributes.BLOCK_PROPOSER]: proposal.getSender().toString(),
+      [Attributes.BLOCK_PROPOSER]: proposer?.toString() ?? 'unknown',
     });
   }
 

yarn-project/validator-client/src/validator.test.ts

@@ -318,9 +318,10 @@ describe('ValidatorClient', () => {
 
       // We should emit WANT_TO_SLASH_EVENT
       const proposer = proposal.getSender();
+      expect(proposer).toBeDefined();
       expect(emitSpy).toHaveBeenCalledWith(WANT_TO_SLASH_EVENT, [
         {
-          validator: proposer,
+          validator: proposer!,
           amount: config.slashBroadcastedInvalidBlockPenalty,
           offenseType: OffenseType.BROADCASTED_INVALID_BLOCK_PROPOSAL,
           epochOrSlot: expect.any(BigInt),