Skip to content

Commit 2f8d257

Browse files
authored
fix(network): cap attester set size to prevent DoS and uint16 overflow (#361)
* 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. * fix: make MaxAttesters a const, refactor test to not override - Changed MaxAttesters from var to const per review: protocol limits should be immutable - Refactored TestJoinAttesterSetMaxCap to verify the constant value fits in uint16 and test the happy path without overriding the const
1 parent ba94ff1 commit 2f8d257

3 files changed

Lines changed: 55 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+
const 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: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,41 @@ func TestJoinAttesterSet(t *testing.T) {
124124
}
125125
}
126126

127+
func TestJoinAttesterSetMaxCap(t *testing.T) {
128+
// Verify the constant is set to a sane value that is within uint16 range
129+
require.LessOrEqual(t, MaxAttesters, int(^uint16(0)),
130+
"MaxAttesters must fit in uint16 to avoid index overflow in BuildValidatorIndexMap")
131+
132+
t.Run("join succeeds under cap", func(t *testing.T) {
133+
sk := NewMockStakingKeeper()
134+
cdc := moduletestutil.MakeTestEncodingConfig().Codec
135+
keys := storetypes.NewKVStoreKeys(types.StoreKey)
136+
logger := log.NewTestLogger(t)
137+
cms := integration.CreateMultiStore(keys, logger)
138+
authority := authtypes.NewModuleAddress("gov")
139+
keeper := NewKeeper(cdc, runtime.NewKVStoreService(keys[types.StoreKey]), sk, nil, nil, authority.String())
140+
server := msgServer{Keeper: keeper}
141+
ctx := sdk.NewContext(cms, cmtproto.Header{ChainID: "test-chain", Time: time.Now().UTC(), Height: 10}, false, logger).
142+
WithContext(t.Context())
143+
144+
// With an empty set, join should succeed
145+
newAddr := sdk.ValAddress("new_attester")
146+
msg := &types.MsgJoinAttesterSet{
147+
Authority: newAddr.String(),
148+
ConsensusAddress: newAddr.String(),
149+
}
150+
151+
rsp, err := server.JoinAttesterSet(ctx, msg)
152+
require.NoError(t, err)
153+
require.NotNil(t, rsp)
154+
155+
// Verify the attester was added
156+
exists, err := keeper.AttesterSet.Has(ctx, newAddr.String())
157+
require.NoError(t, err)
158+
assert.True(t, exists)
159+
})
160+
}
161+
127162
var _ types.StakingKeeper = &MockStakingKeeper{}
128163

129164
type MockStakingKeeper struct {

0 commit comments

Comments
 (0)