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.

Sign up for GitHub

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

fix: Dedup Vote Extensions in ValidateVoteExtensions #18895

Merged
merged 5 commits into from
Dec 27, 2023
Merged

fix: Dedup Vote Extensions in ValidateVoteExtensions #18895

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19dd690
cache validators as ves are processed
davidterpay Dec 26, 2023
1977b78
test duplicates
davidterpay Dec 26, 2023
5015e54
changelog
davidterpay Dec 26, 2023
bbb4086
error on dup
davidterpay Dec 26, 2023
e230a6d
base app test fix
davidterpay Dec 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions baseapp/abci_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func ValidateVoteExtensions(
sumVP int64
)

cache := make(map[string]struct{})
for _, vote := range extCommit.Votes {
totalVP += vote.Validator.Power

Expand All @@ -96,7 +97,13 @@ func ValidateVoteExtensions(
return fmt.Errorf("vote extensions enabled; received empty vote extension signature at height %d", currentHeight)
}

// Ensure that the validator has not already submitted a vote extension.
valConsAddr := sdk.ConsAddress(vote.Validator.Address)
if _, ok := cache[valConsAddr.String()]; ok {
continue
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
}
cache[valConsAddr.String()] = struct{}{}

pubKeyProto, err := valStore.GetPubKeyByConsAddr(ctx, valConsAddr)
if err != nil {
return fmt.Errorf("failed to get validator %X public key: %w", valConsAddr, err)
Expand Down
35 changes: 35 additions & 0 deletions baseapp/abci_utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,41 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteAbsent() {
s.Require().NoError(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
}

// check ValidateVoteExtensions works with duplicate votes
func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() {
ext := []byte("vote-extension")
cve := cmtproto.CanonicalVoteExtension{
Extension: ext,
Height: 2,
Round: int64(0),
ChainId: chainID,
}

bz, err := marshalDelimitedFn(&cve)
s.Require().NoError(err)

extSig0, err := s.vals[0].privKey.Sign(bz)
s.Require().NoError(err)

ve := abci.ExtendedVoteInfo{
Validator: s.vals[0].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig0,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
}

llc := abci.ExtendedCommitInfo{
Round: 0,
Votes: []abci.ExtendedVoteInfo{
ve,
ve,
ve,
},
}
// expect fail (duplicate votes)
s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test function TestValidateVoteExtensionsDuplicateVotes correctly simulates the scenario of duplicate vote extensions and asserts that an error is expected. This aligns with the PR's objective to prevent duplicate vote extensions from being processed.

However, it would be beneficial to include a comment explaining why the error is expected and what specific error message or type is anticipated. This would improve the maintainability and clarity of the test for future developers.

+ // TestValidateVoteExtensionsDuplicateVotes ensures that an error is returned
+ // when duplicate vote extensions are detected. This is critical for preventing
+ // potential security vulnerabilities as described in issue #18893.
  func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() {
      ...
+     // The error should specifically indicate the presence of duplicate vote extensions.
      s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
  }
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// check ValidateVoteExtensions works with duplicate votes
func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() {
ext := []byte("vote-extension")
cve := cmtproto.CanonicalVoteExtension{
Extension: ext,
Height: 2,
Round: int64(0),
ChainId: chainID,
}
bz, err := marshalDelimitedFn(&cve)
s.Require().NoError(err)
extSig0, err := s.vals[0].privKey.Sign(bz)
s.Require().NoError(err)
ve := abci.ExtendedVoteInfo{
Validator: s.vals[0].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig0,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
}
llc := abci.ExtendedCommitInfo{
Round: 0,
Votes: []abci.ExtendedVoteInfo{
ve,
ve,
ve,
},
}
// expect fail (duplicate votes)
s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
}
// TestValidateVoteExtensionsDuplicateVotes ensures that an error is returned
// when duplicate vote extensions are detected. This is critical for preventing
// potential security vulnerabilities as described in issue #18893.
func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() {
ext := []byte("vote-extension")
cve := cmtproto.CanonicalVoteExtension{
Extension: ext,
Height: 2,
Round: int64(0),
ChainId: chainID,
}
bz, err := marshalDelimitedFn(&cve)
s.Require().NoError(err)
extSig0, err := s.vals[0].privKey.Sign(bz)
s.Require().NoError(err)
ve := abci.ExtendedVoteInfo{
Validator: s.vals[0].toValidator(333),
VoteExtension: ext,
ExtensionSignature: extSig0,
BlockIdFlag: cmtproto.BlockIDFlagCommit,
}
llc := abci.ExtendedCommitInfo{
Round: 0,
Votes: []abci.ExtendedVoteInfo{
ve,
ve,
ve,
},
}
// The error should specifically indicate the presence of duplicate vote extensions.
s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
}


// check ValidateVoteExtensions works when a single node has submitted a BlockID_Nil
func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteNil() {
ext := []byte("vote-extension")
Expand Down
Loading