Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix is_valid_indexed_attestation() to check for high attestation indices #1214

Merged
merged 1 commit into from Jun 22, 2020

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Jun 22, 2020

Addresses #1207

One obvious question here is whether this is just an artifact of the back-end expecting sanitized data, which would ordinarily be guaranteed, but at least we use is_valid_indexed_attestation() in places, such as isValidAttestation(), where it's directly exposed to arbitrary external input, and it looks like via process_operations() -> process_attester_slashing() -> is_valid_indexed_attestation(), this could slip through without having been checked even in the direct spec pseudocode.

if indices.asSeq != sorted(toHashSet(indices.asSeq).toSeq, system.cmp):
notice "indexed attestation: indices not sorted"
let indices = indexed_attestation.attesting_indices.asSeq
if len(indices) == 0 or not is_sorted_and_unique(indices):
Copy link
Member

@arnetheduck arnetheduck Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, but I'd move the len(indices) == 0 into is_sorted_and_unique

Copy link
Contributor Author

@tersec tersec Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of that factoring was that this template might be moved elsewhere, and this particular check on 0-length is specific to how is_valid_indexed_attribution() as opposed to the other sorted-and-unique-checks, of which there's a couple elsewhere in the spec.

This was a minimal, local change, but with a slightly broader vision.

Copy link
Member

@arnetheduck arnetheduck Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point was more that a 0-length seq is by definition sorted and unique so it's safer to keep the check in there (if it does indeed get copied to some generic function - as opposed to crashing with a Defect when [0-1] happens

Copy link
Contributor Author

@tersec tersec Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the point of that special case from the spec is to reject the trivially sorted and unique 0-length sequence, despite it being sorted and unique. It's not redundant, and it's not part of an actual is_sorted_and_unique (exact name aside) sort of function. It has the opposite polarity.

https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/beacon-chain.md#is_valid_indexed_attestation shows:

Check if ``indexed_attestation`` is not empty, has sorted and unique indices and has a valid aggregate signature.

...

if len(indices) == 0 or not indices == sorted(set(indices)):
  return False

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants