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

x/sequencers: fix DOS/inefficiency in getSequencerDecreasingBonds #1065

Closed
Tracked by #1055
danwt opened this issue Aug 9, 2024 · 2 comments · Fixed by #1142
Closed
Tracked by #1055

x/sequencers: fix DOS/inefficiency in getSequencerDecreasingBonds #1065

danwt opened this issue Aug 9, 2024 · 2 comments · Fixed by #1142
Assignees

Comments

@danwt
Copy link
Contributor

danwt commented Aug 9, 2024

// getSequencerDecreasingBonds returns the bond reduction item given sequencer address
func (k Keeper) getSequencerDecreasingBonds(ctx sdk.Context, sequencerAddr string) (bds []types.BondReduction) {
prefixKey := types.DecreasingBondQueueKey
store := prefix.NewStore(ctx.KVStore(k.storeKey), prefixKey)
iterator := sdk.KVStorePrefixIterator(store, []byte{})
defer iterator.Close() // nolint: errcheck
for ; iterator.Valid(); iterator.Next() {
var bd types.BondReduction
k.cdc.MustUnmarshal(iterator.Value(), &bd)
if bd.SequencerAddress == sequencerAddr {
bds = append(bds, bd)
}
}
return
}

iteration over ever reducing sequencer in end block and in slash and in the message handler is a DOS/easy way to raise other gas price?

@danwt
Copy link
Contributor Author

danwt commented Sep 25, 2024

reopened : Attackers can leverage the bond reduction mechanism to perform DoS

In x/sequencer/keeper/bond_reductions.go:13-26, the HandleBondReduction function executed in the EndBlocker of the sequencer module
22
DRAFT – NOT INTENDED TO BE SHARED
retrieves bondReductionIDs using the GetMatureDecreasingBondIDs method and processes the resulting list in a loop.
An attacker could exploit this by spamming the MsgDecreaseBond message with small amounts repeatedly, thereby generating a large list of bondReductionIDs.
Since there is no limit on the size of this list, the iteration through the bondReductionIDs can become computationally expensive. This could result in significant delays during block processing, or in the worst case, a complete halt of the chain, leading to Denial of Service.
Similarly in x/sequencer/keeper/unbond.go:148-155, the unbond function, executed in the EndBlocker of the sequencer module, is responsible for removing a sequencer and cleaning up its associated state. Part of this process involves looping through and deleting any active bondReductionIDs entries tied to the sequencer.
Since the unbond function is also used for jailing a sequencer in the EndBlocker of the rollapp module, an attacker could manipulate the system by creating an excessively large list of bondReduction entries. This could delay or prevent jailing and instead lead to a chain halt, effectively weaponizing the bond reduction process to avoid punishment.
Recommendation
We recommend implementing a minimum unbond amount and a cap of unbondings per sequencer like in the canonical staking module.

@danwt
Copy link
Contributor Author

danwt commented Oct 16, 2024

superseded by #1328

@danwt danwt closed this as completed Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants