search

sigp / beacon-fuzz

Differential Fuzzer for Ethereum 2.0
MIT License
161 stars 25 forks source link

[FUZZ] beaconfuzz_v2 identify a mismatch between clients with attester_slashing #88

Closed pventuzelo closed 4 years ago

pventuzelo commented 4 years ago

beaconfuzz_v2 just found an attester_slashing that is interpreted differently by eth2 clients:

Info to Reproduce

Download: issue_pat_attester_slashing.zip

Run:

$ unzip issue_pat_attester_slashing.zip
$ cd issue_pat_attester_slashing
$ ../beaconfuzz_v2 debug beacon.ssz attslash.ssz attesterslashing

[+] beaconfuzz_v2
[DEBUG] beaconstate_path = beacon.ssz
[DEBUG] beaconstate length = 2720473
[DEBUG] container_path = debug_file.ssz
[DEBUG] container length = 496
Using z.Allocator with starting ref: c5e0000000000000
[LIGHTHOUSE] SSZ decoding true
[LIGHTHOUSE] Ok(())
[LIGHTHOUSE] Processing true
[PRYSM] Processing true
[NIMBUS] Processing false
[TEKU] Processing false

Your Environment

michaelsproul commented 4 years ago

Lighthouse accepts that attestation because the fuzzer is running with VerifySignatures::False rather than fake_crypto. The difference is that, with fake crypto, verification will attempt to load the pubkeys from the state, whereas with VerifySignatures::False, the pubkeys won't be loaded at all and we won't detect the index out of bounds.

https://github.com/sigp/beacon-fuzz/blob/9404192347e59c448d19fa77cc58c20ca69ab43f/beaconfuzz_v2/libs/lighthouse/src/attester_slashing.rs#L19-L24

^For beacon_fuzz to use fake_crypto would require turning it on at the Cargo.toml, which might mean that you need two fuzz targets or something. I'll leave that design decision up to you. If it's really a hassle, we could consider modding Lighthouse so that it detects the index out of bounds even when VerifySignatures::False is set.

For debugging I added a transition-op command to lcli, but it's not very fleshed out so we shouldn't merge it. Something like that seems like it's planned for eth2diff?

https://github.com/sigp/lighthouse/tree/lcli-op

Compiling that branch with:

cargo install --path lcli --force --locked --features fake_crypto

And then running:

lcli transition-ops issue_pat_attester_slashing/beacon.ssz issue_pat_attester_slashing/attslash.ssz output.ssz

Yields the expected result:

Failed to run lcli: Failed to transition ops: State transition failed: AttesterSlashingInvalid { index: 0, reason: IndexedAttestation1Invalid(SignatureSetError(ValidatorUnknown(68116944363978752))) }

Idk about why Prysm is accepting the slashing, but it might be something similar

zedt3ster commented 4 years ago

Thanks, that lcli feature will indeed be quite handy for eth2diff. This issue looks the same as #61 (also described in detail in the latest beacon fuzz update).

This explains why Prysm accepts this AttesterSlashing:

When BLS signatures are disabled, PublicKeyFromBytes() returns an empty BLS signatures, regardless of the attesting indices provided (therefore not checking for out-of-range attesting indices).