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

ICS28: system aborted due to update aggregation logic #743

Closed
danwt opened this issue May 19, 2022 · 1 comment
Closed

ICS28: system aborted due to update aggregation logic #743

danwt opened this issue May 19, 2022 · 1 comment
Assignees
Labels
app Application layer.

Comments

@danwt
Copy link
Contributor

danwt commented May 19, 2022

Context

In the consumer EndBlock the received updates are aggregated and then processed

// aggregate the pending changes
changes = pendingChanges.Aggregate()
// Note: in the implementation, the aggregation is done directly
// when receiving a VSCPacket via the AccumulateChanges method.
// remove all pending changes
pendingChanges.RemoveAll()
// update validatorSet
UpdateValidatorSet(changes)

the aggregation is defined

// return an aggregated list of updates, i.e.,
// keep only the latest update per validator;
// the original list is not modified
Aggregate(): [ValidatorUpdate]

and the aggregated updates are processed:

// CCF: Consumer Chain Function
function UpdateValidatorSet(changes: [ValidatorUpdate]) {
foreach update IN changes {
addr := hash(update.pubKey)
if addr NOT IN validatorSet.Keys() {
// new validator bonded
abortSystemUnless(update.power > 0)

Scenario

If a validator v0 receives a power=0 update on the consumer in block A then the validator will be deleted

else if update.power == 0 {
// existing validator begins unbonding
validatorSet.Remove(addr)

but if subsequently the the pendingUpdates struct fills up with, e.g.

[(v0, 42), (v0, 0)]

then the aggregation will reduce this to

[ (v0, 0)]

which will trigger this branch

if addr NOT IN validatorSet.Keys() {
// new validator bonded
abortSystemUnless(update.power > 0)

causing a panic

@danwt danwt moved this to Todo in Replicated Security May 19, 2022
@danwt danwt changed the title ICS;028 ICS028 system aborted due to update aggregation logic May 19, 2022
@mpoke
Copy link
Contributor

mpoke commented May 19, 2022

@danwt That's true. Nice catch. Could you please open an issue on the implementation side also?

@mpoke mpoke changed the title ICS028 system aborted due to update aggregation logic ICS28: system aborted due to update aggregation logic May 19, 2022
@mpoke mpoke added the app Application layer. label May 19, 2022
@mpoke mpoke moved this from Todo to Waiting for review in Replicated Security May 19, 2022
@mpoke mpoke closed this as completed May 20, 2022
Repository owner moved this from Waiting for review to Done in Replicated Security May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Application layer.
Projects
Status: Backlog
Status: Done
Development

No branches or pull requests

2 participants