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

Check attestation bitlist length in aggregation to prevent panic #4876

Merged
merged 3 commits into from Feb 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion beacon-chain/core/helpers/attestation.go
Expand Up @@ -16,6 +16,10 @@ var (
// ErrAttestationAggregationBitsOverlap is returned when two attestations aggregation
// bits overlap with each other.
ErrAttestationAggregationBitsOverlap = errors.New("overlapping aggregation bits")

// ErrAttestationAggregationBitsDifferentLen is returned when two attestation aggregation bits
// have different lengths.
ErrAttestationAggregationBitsDifferentLen = errors.New("different bitlist lengths")
)

// AggregateAttestations such that the minimal number of attestations are returned.
Expand All @@ -32,7 +36,7 @@ func AggregateAttestations(atts []*ethpb.Attestation) ([]*ethpb.Attestation, err
}
for j := i + 1; j < len(atts); j++ {
b := atts[j]
if !a.AggregationBits.Overlaps(b.AggregationBits) {
if a.AggregationBits.Len() == b.AggregationBits.Len() && !a.AggregationBits.Overlaps(b.AggregationBits) {
var err error
a, err = AggregateAttestation(a, b)
if err != nil {
Expand All @@ -50,6 +54,11 @@ func AggregateAttestations(atts []*ethpb.Attestation) ([]*ethpb.Attestation, err
for i, a := range atts {
for j := i + 1; j < len(atts); j++ {
b := atts[j]

if a.AggregationBits.Len() != b.AggregationBits.Len() {
continue
}

if a.AggregationBits.Contains(b.AggregationBits) {
// If b is fully contained in a, then b can be removed.
atts = append(atts[:j], atts[j+1:]...)
Expand All @@ -74,6 +83,9 @@ var signatureFromBytes = bls.SignatureFromBytes

// AggregateAttestation aggregates attestations a1 and a2 together.
func AggregateAttestation(a1 *ethpb.Attestation, a2 *ethpb.Attestation) (*ethpb.Attestation, error) {
if a1.AggregationBits.Len() != a2.AggregationBits.Len() {
return nil, ErrAttestationAggregationBitsDifferentLen
}
if a1.AggregationBits.Overlaps(a2.AggregationBits) {
return nil, ErrAttestationAggregationBitsOverlap
}
Expand Down
29 changes: 29 additions & 0 deletions beacon-chain/core/helpers/attestation_test.go
Expand Up @@ -62,6 +62,22 @@ func TestAggregateAttestation_OverlapFails(t *testing.T) {
}
}

func TestAggregateAttestation_DiffLengthFails(t *testing.T) {
tests := []struct {
a1 *ethpb.Attestation
a2 *ethpb.Attestation
}{
{a1: &ethpb.Attestation{AggregationBits: bitfield.Bitlist{0x0F}},
a2: &ethpb.Attestation{AggregationBits: bitfield.Bitlist{0x11}}},
}
for _, tt := range tests {
_, err := helpers.AggregateAttestation(tt.a1, tt.a2)
if err != helpers.ErrAttestationAggregationBitsDifferentLen {
t.Error("Did not receive wanted error")
}
}
}

func bitlistWithAllBitsSet(length uint64) bitfield.Bitlist {
b := bitfield.NewBitlist(length)
for i := uint64(0); i < length; i++ {
Expand Down Expand Up @@ -167,6 +183,19 @@ func TestAggregateAttestations(t *testing.T) {
{0b00000011, 0b1},
},
},
{
name: "attestations with different bitlist lengths",
inputs: []bitfield.Bitlist{
{0b00000011, 0b10},
{0b00000111, 0b100},
{0b00000100, 0b1},
},
want: []bitfield.Bitlist{
{0b00000011, 0b10},
{0b00000111, 0b100},
{0b00000100, 0b1},
},
},
}

var makeAttestationsFromBitlists = func(bl []bitfield.Bitlist) []*ethpb.Attestation {
Expand Down