Skip to content

Commit 71016b7

Browse files
committed
security: cap attester set size to prevent DoS and uint16 overflow
The attester set was unbounded, allowing an attacker to register thousands of sybil identities. This caused: - EndBlocker stalling via O(n) iteration in BuildValidatorIndexMap - Silent uint16 index overflow when >65,535 attesters joined This commit: - Adds MaxAttesters=10,000 variable to cap the attester set size - Enforces the cap in JoinAttesterSet, rejecting new joins at capacity - Adds uint16 overflow guard in BuildValidatorIndexMap as defense-in-depth Adds 2 table-driven tests for cap enforcement.
1 parent ba94ff1 commit 71016b7

3 files changed

Lines changed: 82 additions & 0 deletions

File tree

modules/network/keeper/keeper.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,11 @@ func (k Keeper) GetAllAttesters(ctx sdk.Context) ([]string, error) {
185185
return attesters, nil
186186
}
187187

188+
// MaxAttesters is the maximum number of attesters allowed in the set.
189+
// This prevents unbounded growth, EndBlocker stalling, and uint16 index overflow
190+
// in BuildValidatorIndexMap.
191+
var MaxAttesters = 10_000
192+
188193
// BuildValidatorIndexMap rebuilds the validator index mapping
189194
func (k Keeper) BuildValidatorIndexMap(ctx sdk.Context) error {
190195
// Get all attesters instead of bonded validators
@@ -193,6 +198,12 @@ func (k Keeper) BuildValidatorIndexMap(ctx sdk.Context) error {
193198
return err
194199
}
195200

201+
// Guard against uint16 overflow — should not happen if MaxAttesters is enforced
202+
// at join time, but defense-in-depth
203+
if len(attesters) > int(^uint16(0)) {
204+
return fmt.Errorf("attester count %d exceeds uint16 max %d", len(attesters), ^uint16(0))
205+
}
206+
196207
// Clear existing indices and powers
197208
// The `nil` range clears all entries in the collection.
198209
if err := k.ValidatorIndex.Clear(ctx, nil); err != nil {

modules/network/keeper/msg_server.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,15 @@ func (k msgServer) JoinAttesterSet(goCtx context.Context, msg *types.MsgJoinAtte
157157
return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "consensus address already in attester set")
158158
}
159159

160+
// Enforce maximum attester set size to prevent unbounded growth and uint16 index overflow
161+
attesters, err := k.GetAllAttesters(ctx)
162+
if err != nil {
163+
return nil, sdkerr.Wrap(err, "get all attesters")
164+
}
165+
if len(attesters) >= MaxAttesters {
166+
return nil, sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, "attester set is full: %d/%d", len(attesters), MaxAttesters)
167+
}
168+
160169
// Store the attester information including pubkey (key by consensus address)
161170
attesterInfo := &types.AttesterInfo{
162171
Validator: msg.ConsensusAddress, // Use consensus address as primary key

modules/network/keeper/msg_server_test.go

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package keeper
22

33
import (
44
"context"
5+
"fmt"
56
"maps"
67
"slices"
78
"strings"
@@ -124,6 +125,67 @@ func TestJoinAttesterSet(t *testing.T) {
124125
}
125126
}
126127

128+
func TestJoinAttesterSetMaxCap(t *testing.T) {
129+
// Use a small cap for testing. We temporarily reduce MaxAttesters
130+
// to avoid creating 10,000 attesters in a test.
131+
const testCap = 3
132+
133+
specs := map[string]struct {
134+
existingCount int
135+
expErr error
136+
}{
137+
"under cap": {
138+
existingCount: testCap - 1,
139+
},
140+
"at cap - rejected": {
141+
existingCount: testCap,
142+
expErr: sdkerrors.ErrInvalidRequest,
143+
},
144+
}
145+
146+
for name, spec := range specs {
147+
t.Run(name, func(t *testing.T) {
148+
sk := NewMockStakingKeeper()
149+
cdc := moduletestutil.MakeTestEncodingConfig().Codec
150+
keys := storetypes.NewKVStoreKeys(types.StoreKey)
151+
logger := log.NewTestLogger(t)
152+
cms := integration.CreateMultiStore(keys, logger)
153+
authority := authtypes.NewModuleAddress("gov")
154+
keeper := NewKeeper(cdc, runtime.NewKVStoreService(keys[types.StoreKey]), sk, nil, nil, authority.String())
155+
server := msgServer{Keeper: keeper}
156+
ctx := sdk.NewContext(cms, cmtproto.Header{ChainID: "test-chain", Time: time.Now().UTC(), Height: 10}, false, logger).
157+
WithContext(t.Context())
158+
159+
// Pre-populate the attester set
160+
for i := range spec.existingCount {
161+
addr := sdk.ValAddress(fmt.Sprintf("val%05d", i))
162+
require.NoError(t, keeper.SetAttesterSetMember(ctx, addr.String()))
163+
}
164+
165+
// Now try to join with a new attester
166+
newAddr := sdk.ValAddress("new_attester")
167+
msg := &types.MsgJoinAttesterSet{
168+
Authority: newAddr.String(),
169+
ConsensusAddress: newAddr.String(),
170+
}
171+
172+
// Temporarily override MaxAttesters for test
173+
oldMax := MaxAttesters
174+
defer func() { MaxAttesters = oldMax }()
175+
MaxAttesters = testCap
176+
177+
rsp, err := server.JoinAttesterSet(ctx, msg)
178+
if spec.expErr != nil {
179+
require.ErrorIs(t, err, spec.expErr)
180+
require.Nil(t, rsp)
181+
return
182+
}
183+
require.NoError(t, err)
184+
require.NotNil(t, rsp)
185+
})
186+
}
187+
}
188+
127189
var _ types.StakingKeeper = &MockStakingKeeper{}
128190

129191
type MockStakingKeeper struct {

0 commit comments

Comments
 (0)