[Fuzzing] Crash on nfuzz_attestation
mratsim opened this issue · 8 comments
See the fuzzing input pinned in the fuzzing discord channel.
attach files here, note version?
A bit annoying I can't attach the ssz directly:
nim_attestation_crash.ssz
This is a BeaconState + Attestation ssz container that, when passed to nfuzz_attestation, triggers the following assertion:
nim_attestation_trace.txt
I haven't investigated this scenario, but I assume this highlights a gap in our consensus tests.
Am I correct in that assumption?
Also, can someone add me to the fuzzing discord?
I think I've narrowed it down:
I can't see any equivalent in nim-beacon-chain for the following
assert data.index < get_committee_count_at_slot(state, data.slot) (https://github.com/ethereum/eth2.0-specs/blob/dev/specs/core/0_beacon-chain.md#attestations).
Because a larger index is allowed through, index > count is possible in compute_committee, and thus an endIdx > index_count.
@djrtwo I'm not certain re concensus tests, as this is effectively a "crash" of nim-beacon-chain, when it should return false (allowing nimbus to continue running).
If it's possible for the consensus tests to differentiate between an error return and a crash, then this could be a gap in the tests.
In pyspec, this would be triggering an AssertionError anyway. If the above understanding of the bug is correct, an equivalent bug in the pyspec would be for assert data.index < get_committee_count_at_slot(state, data.slot) to be missing from process_attestation, and the input triggers a different Assert in compute_shuffled_index: assert index < index_count
If the tests are only checking whether an assert is triggered, they would not be able to differentiate between these.
Can someone confirm whether my understanding is correct?
Interesting, I see.
That said, this codepath is clearly not triggered in Nimbus during the consensus tests. It seems likely that we can and should craft a consensus test that would trigger this code path.
I'll monitor this thread and make a call when y'all post a fix
In theory, this should be fixed in devel now.
Feel free to re-open if this isn't, in fact, fixed.