DeAI-Artist/Linkis

Missing Validation of Priorities in Validateset in Header and Many Other Known Issues in Cometbft are Unsolved

Opened this issue · 3 comments

The header hash was generated with a missing hash of the validateset's priorities information. A malicious user could modify the priorities without causing a state hash validation error. Remarkably this is a known issue in Cometbft that breaks the state hash validation for priorities.

func (vals *ValidatorSet) Hash() []byte {
bzs := make([][]byte, len(vals.Validators))
for i, val := range vals.Validators {
bzs[i] = val.Bytes()
}
return merkle.HashFromByteSlices(bzs)
}

/types/validator.go
type Validator struct {
	Address     Address       `json:"address"`
	PubKey      crypto.PubKey `json:"pub_key"`
	VotingPower int64         `json:"voting_power"`

	ProposerPriority int64 `json:"proposer_priority"`
}
...
types/validator_set.go
func (vals *ValidatorSet) Hash() []byte {
	bzs := make([][]byte, len(vals.Validators))
	for i, val := range vals.Validators {
		bzs[i] = val.Bytes()
	}
	return merkle.HashFromByteSlices(bzs)
}
...
/types/validator.go
func (v *Validator) Bytes() []byte {
	pk, err := ce.PubKeyToProto(v.PubKey)
	if err != nil {
		panic(err)
	}

	pbv := cmtproto.SimpleValidator{
		PubKey:      &pk,
		VotingPower: v.VotingPower,
	}//missing ProposerPriority

	bz, err := pbv.Marshal()
	if err != nil {
		panic(err)
	}
	return bz
}

This project implemented its own consensus protocol using cometbft's fork project, but many of the flaws that were fixed in cometbft were not fixed by that project, and this issue is one of them.
More information is shown below:
Other Unsolved issues' Fix PR and Commits:
cometbft/cometbft#3984
cometbft/cometbft#3369
cometbft/cometbft@d766d20
cometbft/cometbft#890
cometbft/cometbft#865

@Hellobloc Thanks for raising this. I think we can fix the issue by including ProposerPriority in the validator's Bytes() function.

func (v *Validator) Bytes() []byte {
    pk, err := ce.PubKeyToProto(v.PubKey)
    if err != nil {
        panic(err)
    }
    pbv := tmproto.SimpleValidator{
        PubKey:          &pk,
        VotingPower:     v.VotingPower,
        ProposerPriority: v.ProposerPriority,
    }
    bz, err := pbv.Marshal()
    if err != nil {
        panic(err)
    }
    return bz
}

how about just combine the fix pr from original repo cometbft ? i am worry about this fix method you proposal may introduce new issues. for example how new node verify the old statehash because you update the statehash calculate method?
and i think we should assess all of the previous security fixs in cometbft i submit in this issue to figure out wether they could impact this repo