Ensure validator indices in attester slashings are valid #2348
Conversation
Avoid IndexOutOfBoundsException when looking up validator public keys.
|
Awesome, thanks @ajsutton.
Am I right in understanding that this bug would be triggerable when processing a malicious |
|
I don't think you could create a valid block with a malicious |
|
Thanks @ajsutton! Apologies, my wording in the previous comment was poor. What I meant was that nothing prevents a malicious proposer from including this |
|
Ok yes agreed with that. Except that panic probably isn't the right term for Teku. It's an unhandled exception which gets logged very noisily but it doesn't cause Teku to crash. Essentially any exception produced from processing messages (from gossip or requested blocks) just causes that message to be treated as invalid. It's good to fix things up so that we don't log misleading error messages but the observed behaviour of Teku will wind up correct for any exception thrown from the processing or validation code. |
|
Great to know, thanks for clarifying! |
| @@ -150,13 +147,17 @@ public static AttestationProcessingResult is_valid_indexed_attestation( | |||
| SSZList<UnsignedLong> indices = indexed_attestation.getAttesting_indices(); | |||
|
|
|||
| List<UnsignedLong> bit_0_indices_sorted = | |||
| indices.stream().sorted().distinct().collect(Collectors.toList()); | |||
| indices.stream().sorted().distinct().collect(toList()); | |||
There was a problem hiding this comment.
Oh, is it allowed by our codestyle? I mean methods static import.
(would be just happy is yes 👍)
There was a problem hiding this comment.
Yes it is as long as the method name isn't too generic (so this is fine but a static import of valueOf is not). I it passes error prone and aids readability go for it. :)
PR Description
AttestaterSlashingobjects include anIndexedAttestationopening up the possibility that the validator indices it specifies are out of bounds for the validator list. Normally this isn't an issue because we index the attestation ourselves so know the indices are valid.SignedAggregateAndProofalso includes a validator index which must be verified to be in range before looking up the validator public key.To ensure this category of errors is avoided,
ValidatorsUtil.getValidatorPubKeynow performs bounds checking and returns an emptyOptionalif the index is out of range. Callers are updated to handle this and fail validation of the public key is not available.Fixed Issue(s)
fix #2345
Documentation
documentationlabel to this PR if updates are required.