Fix bug in attestation verification

We were ensuring that a validator was present on the aggregation
bitfield before adding their signature to the agg pub

eth2/state_processing/src/per_block_processing/errors.rs

@@ -147,6 +147,8 @@ pub enum AttestationInvalid {
     ///
     /// (attestation_data_shard, attestation_data_slot)
     NoCommitteeForShard(u64, Slot),
+    /// The validator index was unknown.
+    UnknownValidator(u64),
     /// The attestation signature verification failed.
     BadSignature,
     /// The shard block root was not set to zero. This is a phase 0 requirement.

eth2/state_processing/src/per_block_processing/validate_attestation.rs

@@ -159,18 +159,16 @@ fn validate_attestation_signature_optional(
 
     if verify_signature {
         let attestation_epoch = attestation.data.slot.epoch(spec.slots_per_epoch);
-        verify!(
         verify_attestation_signature(
             state,
             committee,
             attestation_epoch,
+            &attestation.aggregation_bitfield,
             &attestation.custody_bitfield,
             &attestation.data,
             &attestation.aggregate_signature,
-                spec
-            ),
-            Invalid::BadSignature
-        );
+            spec,
+        )?;
     }
 
     // [TO BE REMOVED IN PHASE 1] Verify that `attestation.data.crosslink_data_root == ZERO_HASH`.
@@ -195,19 +193,33 @@ fn verify_attestation_signature(
     state: &BeaconState,
     committee: &[usize],
     attestation_epoch: Epoch,
+    aggregation_bitfield: &Bitfield,
     custody_bitfield: &Bitfield,
     attestation_data: &AttestationData,
     aggregate_signature: &AggregateSignature,
     spec: &ChainSpec,
-) -> bool {
+) -> Result<(), Error> {
     let mut aggregate_pubs = vec![AggregatePublicKey::new(); 2];
     let mut message_exists = vec![false; 2];
 
     for (i, v) in committee.iter().enumerate() {
-        let custody_bit = match custody_bitfield.get(i) {
+        let validator_signed = aggregation_bitfield.get(i).map_err(|_| {
+            Error::Invalid(Invalid::BadAggregationBitfieldLength(
+                committee.len(),
+                aggregation_bitfield.len(),
+            ))
+        })?;
+
+        if validator_signed {
+            let custody_bit: bool = match custody_bitfield.get(i) {
                 Ok(bit) => bit,
                 // Invalidate signature if custody_bitfield.len() < committee
-            Err(_) => return false,
+                Err(_) => {
+                    return Err(Error::Invalid(Invalid::BadCustodyBitfieldLength(
+                        committee.len(),
+                        custody_bitfield.len(),
+                    )));
+                }
             };
 
             message_exists[custody_bit as usize] = true;
@@ -216,10 +228,11 @@ fn verify_attestation_signature(
                 Some(validator) => {
                     aggregate_pubs[custody_bit as usize].add(&validator.pubkey);
                 }
-            // Invalidate signature if validator index is unknown.
-            None => return false,
+                // Return error if validator index is unknown.
+                None => return Err(Error::BeaconStateError(BeaconStateError::UnknownValidator)),
             };
         }
+    }
 
     // Message when custody bitfield is `false`
     let message_0 = AttestationDataAndCustodyBit {
@@ -251,5 +264,10 @@ fn verify_attestation_signature(
 
     let domain = spec.get_domain(attestation_epoch, Domain::Attestation, &state.fork);
 
-    aggregate_signature.verify_multiple(&messages[..], domain, &keys[..])
+    verify!(
+        aggregate_signature.verify_multiple(&messages[..], domain, &keys[..]),
+        Invalid::BadSignature
+    );
+
+    Ok(())
 }