cosmos / interchain-security

Interchain Security is an open sourced IBC application which allows cosmos blockchains to lease their proof-of-stake security to one another.
https://cosmos.github.io/interchain-security/
Other
156 stars 126 forks source link

Consumer sends invalid validator updates to tendermint, resulting in `tm.verifyRemovals` fail #230

Closed danwt closed 2 years ago

danwt commented 2 years ago

This tendermint code checks if the validator set updates returned in EndBlock are valid

https://github.com/tendermint/tendermint/blob/85870def7b628effad73af942e638bbddf2ba8fd/types/validator_set.go#L538-L544

func verifyRemovals(deletes []*Validator, vals *ValidatorSet) (votingPower int64, err error) {
    removedVotingPower := int64(0)
    for _, valUpdate := range deletes {
        address := valUpdate.Address
        _, val := vals.GetByAddress(address)
        if val == nil {
            return removedVotingPower, fmt.Errorf("failed to find validator %X to remove", address)

The check can fail on consumer because the passed updates are not filtered correctly.

https://github.com/cosmos/interchain-security/blob/657aaea0a6ed97f3a04d906186d353c56bef8d5e/x/ccv/consumer/module.go#L187-L195

The data needs to be modified in here

https://github.com/cosmos/interchain-security/blob/657aaea0a6ed97f3a04d906186d353c56bef8d5e/x/ccv/consumer/keeper/validators.go#L16-L52

See the very closely related spec issue that was fixed already

I can make a PR for this soon.

Found with diff testing.

sainoe commented 2 years ago

The check can fail on consumer because the passed updates are not filtered correctly.

So Tendermint errors when ValUpdates contains a change that has an unexisting validator in the consumer valset and a voting power equals to 0 ?

danwt commented 2 years ago

Yes. Tendermint interprets this as trying to delete a non-existing validator.

The pattern is:

A. Send valUpdate(val=val0, power=0) to tendermint (consumer side) B. Send valUpdate(val=val0, power=1) to consumer (but consumer does not process it) C. Send valUpdate(val=val0, power=0) to consumer (but consumer does not process it) D. Consumer process updates from B,C. They are aggregated to valUpdate(val=val0, power=0) E. Consumer sends update from D to tendermint