Skip to content

Commit

Permalink
chore: fix gosec (#2230)
Browse files Browse the repository at this point in the history
* handle errors

* exclude G115 from gosec: integer overflow conversion int issues
  • Loading branch information
mpoke authored Sep 6, 2024
1 parent 7fa04b5 commit a17a385
Show file tree
Hide file tree
Showing 17 changed files with 117 additions and 53 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/gosec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@ jobs:
- name: Run Gosec Security Scanner
uses: securego/gosec@master
with:
args: -exclude-dir=tests -exclude-dir=app -exclude-generated ./...
args: "-exclude=G115 -exclude-dir=tests -exclude-dir=testutil -exclude-dir=app -exclude-generated ./..."
9 changes: 6 additions & 3 deletions tests/integration/distribution.go
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,8 @@ func (s *CCVTestSuite) TestAllocateTokensToConsumerValidators() {
consuVals, err := providerKeeper.GetConsumerValSet(ctx, consumerId)
s.Require().NoError(err)
providerKeeper.DeleteConsumerValSet(ctx, consumerId)
providerKeeper.SetConsumerValSet(ctx, consumerId, consuVals[0:tc.consuValLen])
err = providerKeeper.SetConsumerValSet(ctx, consumerId, consuVals[0:tc.consuValLen])
s.Require().NoError(err)
consuVals, err = providerKeeper.GetConsumerValSet(ctx, consumerId)
s.Require().NoError(err)

Expand Down Expand Up @@ -1003,10 +1004,12 @@ func (s *CCVTestSuite) TestAllocateTokensToConsumerValidatorsWithDifferentValida
// have not been consumer validators for `GetNumberOfEpochsToStartReceivingRewards * GetBlocksPerEpoch` blocks
consuVals[2].JoinHeight = 2
consuVals[3].JoinHeight = 2
providerKeeper.SetConsumerValSet(ctx, consumerId, consuVals)
err = providerKeeper.SetConsumerValSet(ctx, consumerId, consuVals)
s.Require().NoError(err)

providerKeeper.DeleteConsumerValSet(ctx, consumerId)
providerKeeper.SetConsumerValSet(ctx, consumerId, consuVals)
err = providerKeeper.SetConsumerValSet(ctx, consumerId, consuVals)
s.Require().NoError(err)
consuVals, err = providerKeeper.GetConsumerValSet(ctx, consumerId)
s.Require().NoError(err)

Expand Down
13 changes: 7 additions & 6 deletions tests/integration/slashing.go
Original file line number Diff line number Diff line change
Expand Up @@ -397,36 +397,37 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() {

// Expect no error if validator does not exist
_, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
suite.Require().NoError(err, "no error expected")
suite.Require().NoError(err)

// Check expected behavior for handling SlashPackets for double signing infractions
slashPacketData.Infraction = stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN
ackResult, err := providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
suite.Require().NoError(err, "no error expected")
suite.Require().NoError(err)
suite.Require().Equal(ccv.V1Result, ackResult, "expected successful ack")

// Check expected behavior for handling SlashPackets for downtime infractions
slashPacketData.Infraction = stakingtypes.Infraction_INFRACTION_DOWNTIME

// Expect packet to be handled if the validator didn't opt in
ackResult, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
suite.Require().NoError(err, "no error expected")
suite.Require().NoError(err)
suite.Require().Equal(ccv.SlashPacketHandledResult, ackResult, "expected successful ack")

providerKeeper.SetConsumerValidator(ctx, firstBundle.ConsumerId, providertypes.ConsensusValidator{
err = providerKeeper.SetConsumerValidator(ctx, firstBundle.ConsumerId, providertypes.ConsensusValidator{
ProviderConsAddr: validAddress,
})
suite.Require().NoError(err)

// Expect the packet to bounce if the slash meter is negative
providerKeeper.SetSlashMeter(ctx, math.NewInt(-1))
ackResult, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
suite.Require().NoError(err, "no error expected")
suite.Require().NoError(err)
suite.Require().Equal(ccv.SlashPacketBouncedResult, ackResult, "expected successful ack")

// Expect the packet to be handled if the slash meter is positive
providerKeeper.SetSlashMeter(ctx, math.NewInt(0))
ackResult, err = providerKeeper.OnRecvSlashPacket(ctx, packet, *slashPacketData)
suite.Require().NoError(err, "no error expected")
suite.Require().NoError(err)
suite.Require().Equal(ccv.SlashPacketHandledResult, ackResult, "expected successful ack")
}

Expand Down
3 changes: 2 additions & 1 deletion tests/mbt/driver/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,8 @@ func (s *Driver) ConfigureNewPath(consumerChain, providerChain *ibctesting.TestC

considerAll := func(providerAddr providertypes.ProviderConsAddress) bool { return true }
nextValidators := s.providerKeeper().FilterValidators(s.providerCtx(), string(consumerChainId), stakingValidators, considerAll)
s.providerKeeper().SetConsumerValSet(s.providerCtx(), string(consumerChainId), nextValidators)
err = s.providerKeeper().SetConsumerValSet(s.providerCtx(), string(consumerChainId), nextValidators)
require.NoError(s.t, err)

err = s.providerKeeper().SetConsumerGenesis(
providerChain.GetContext(),
Expand Down
5 changes: 4 additions & 1 deletion x/ccv/provider/keeper/consumer_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,10 @@ func (k Keeper) MakeConsumerGenesis(

// need to use the bondedValidators, not activeValidators, here since the chain might be opt-in and allow inactive vals
nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, powerShapingParameters, minPower)
k.SetConsumerValSet(ctx, consumerId, nextValidators)
err = k.SetConsumerValSet(ctx, consumerId, nextValidators)
if err != nil {
return gen, nil, fmt.Errorf("unable to set consumer validator set in MakeConsumerGenesis: %s", err)
}

// get the initial updates with the latest set consumer public keys
initialUpdatesWithConsumerKeys := DiffValidators([]types.ConsensusValidator{}, nextValidators)
Expand Down
3 changes: 2 additions & 1 deletion x/ccv/provider/keeper/distribution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,15 @@ func TestComputeConsumerTotalVotingPower(t *testing.T) {
// set 5 validators to the consumer chain
for i := 0; i < 5; i++ {
val := createVal(int64(i))
keeper.SetConsumerValidator(
err := keeper.SetConsumerValidator(
ctx,
chainID,
providertypes.ConsensusValidator{
ProviderConsAddr: val.Address,
Power: val.VotingPower,
},
)
require.NoError(t, err)

expTotalPower += val.VotingPower
}
Expand Down
5 changes: 4 additions & 1 deletion x/ccv/provider/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,10 @@ func (k Keeper) InitGenesisValUpdates(ctx sdk.Context) []abci.ValidatorUpdate {
reducedValSet[i] = consensusVal
}

k.SetLastProviderConsensusValSet(ctx, reducedValSet)
err = k.SetLastProviderConsensusValSet(ctx, reducedValSet)
if err != nil {
panic(fmt.Errorf("setting the provider consensus validator set: %w", err))
}

valUpdates := make([]abci.ValidatorUpdate, len(reducedValSet))
for i, val := range reducedValSet {
Expand Down
6 changes: 4 additions & 2 deletions x/ccv/provider/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,12 @@ func TestQueryConsumerValidators(t *testing.T) {
require.Empty(t, res)

// set consumer valset
pk.SetConsumerValSet(ctx, consumerId, []types.ConsensusValidator{
err = pk.SetConsumerValSet(ctx, consumerId, []types.ConsensusValidator{
consumerValidator1,
consumerValidator2,
consumerValidator3,
})
require.NoError(t, err)

expRes.Validators = append(expRes.Validators, &types.QueryConsumerValidatorsValidator{
ProviderAddress: providerAddr3.String(),
Expand Down Expand Up @@ -329,7 +330,7 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) {
}

// set `providerAddr` as a consumer validator on first consumer chain
pk.SetConsumerValidator(ctx, consumerIds[0], types.ConsensusValidator{
err := pk.SetConsumerValidator(ctx, consumerIds[0], types.ConsensusValidator{
ProviderConsAddr: providerAddr.ToSdkConsAddr(),
Power: 1,
PublicKey: &crypto.PublicKey{
Expand All @@ -338,6 +339,7 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) {
},
},
})
require.NoError(t, err)

// set `providerAddr` as an opted-in validator on third consumer chain
pk.SetOptedIn(ctx, consumerIds[2], providerAddr)
Expand Down
3 changes: 2 additions & 1 deletion x/ccv/provider/keeper/key_assignment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,8 @@ func TestSimulatedAssignmentsAndUpdateApplication(t *testing.T) {
valSet, error := k.GetConsumerValSet(ctx, CHAINID)
require.NoError(t, error)
updates = providerkeeper.DiffValidators(valSet, nextValidators)
k.SetConsumerValSet(ctx, CHAINID, nextValidators)
err := k.SetConsumerValSet(ctx, CHAINID, nextValidators)
require.NoError(t, err)

consumerValset.apply(updates)
// Simulate the VSCID update in EndBlock
Expand Down
8 changes: 4 additions & 4 deletions x/ccv/provider/keeper/provider_consensus.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ import (
func (k Keeper) SetLastProviderConsensusValidator(
ctx sdk.Context,
validator types.ConsensusValidator,
) {
k.setValidator(ctx, types.LastProviderConsensusValsPrefix(), validator)
) error {
return k.setValidator(ctx, types.LastProviderConsensusValsPrefix(), validator)
}

// SetLastProviderConsensusValSet resets the stored last validator set sent to the consensus engine on the provider
// to the provided `nextValidators`.
func (k Keeper) SetLastProviderConsensusValSet(ctx sdk.Context, nextValidators []types.ConsensusValidator) {
k.setValSet(ctx, types.LastProviderConsensusValsPrefix(), nextValidators)
func (k Keeper) SetLastProviderConsensusValSet(ctx sdk.Context, nextValidators []types.ConsensusValidator) error {
return k.setValSet(ctx, types.LastProviderConsensusValsPrefix(), nextValidators)
}

// DeleteLastProviderConsensusValidator removes the validator with `providerConsAddr` address
Expand Down
15 changes: 10 additions & 5 deletions x/ccv/provider/keeper/provider_consensus_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ func TestSetLastProviderConsensusValidator(t *testing.T) {
PublicKey: &crypto.PublicKey{},
}

providerKeeper.SetLastProviderConsensusValidator(ctx, validator)
err := providerKeeper.SetLastProviderConsensusValidator(ctx, validator)
require.NoError(t, err)

// Retrieve the stored validator
vals, err := providerKeeper.GetLastProviderConsensusValSet(ctx)
Expand Down Expand Up @@ -49,7 +50,8 @@ func TestSetLastProviderConsensusValSet(t *testing.T) {

nextValidators := []types.ConsensusValidator{validator1, validator2}

providerKeeper.SetLastProviderConsensusValSet(ctx, nextValidators)
err := providerKeeper.SetLastProviderConsensusValSet(ctx, nextValidators)
require.NoError(t, err)

// Retrieve the stored validator set
storedValidators, err := providerKeeper.GetLastProviderConsensusValSet(ctx)
Expand All @@ -67,7 +69,8 @@ func TestDeleteLastProviderConsensusValidator(t *testing.T) {
PublicKey: &crypto.PublicKey{},
}

providerKeeper.SetLastProviderConsensusValidator(ctx, validator)
err := providerKeeper.SetLastProviderConsensusValidator(ctx, validator)
require.NoError(t, err)

// Delete the stored validator
providerKeeper.DeleteLastProviderConsensusValidator(ctx, types.NewProviderConsAddress(validator.ProviderConsAddr))
Expand Down Expand Up @@ -96,7 +99,8 @@ func TestDeleteLastProviderConsensusValSet(t *testing.T) {

nextValidators := []types.ConsensusValidator{validator1, validator2}

providerKeeper.SetLastProviderConsensusValSet(ctx, nextValidators)
err := providerKeeper.SetLastProviderConsensusValSet(ctx, nextValidators)
require.NoError(t, err)

// check that the set is not empty
storedValidators, err := providerKeeper.GetLastProviderConsensusValSet(ctx)
Expand Down Expand Up @@ -126,7 +130,8 @@ func TestGetLastTotalProviderConsensusPower(t *testing.T) {
PublicKey: &crypto.PublicKey{},
}
nextValidators := []types.ConsensusValidator{validator1, validator2}
providerKeeper.SetLastProviderConsensusValSet(ctx, nextValidators)
err := providerKeeper.SetLastProviderConsensusValSet(ctx, nextValidators)
require.NoError(t, err)
// Get the total power of the last stored validator set
totalPower, err := providerKeeper.GetLastTotalProviderConsensusPower(ctx)
require.NoError(t, err, "failed to get total power")
Expand Down
10 changes: 8 additions & 2 deletions x/ccv/provider/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ func (k Keeper) ProviderValidatorUpdates(ctx sdk.Context) ([]abci.ValidatorUpdat
}

// store the validator set we will send to consensus
k.SetLastProviderConsensusValSet(ctx, nextValidators)
err = k.SetLastProviderConsensusValSet(ctx, nextValidators)
if err != nil {
return []abci.ValidatorUpdate{}, fmt.Errorf("setting the last provider consensus validator set: %w", err)
}

valUpdates := DiffValidators(currentValidators, nextValidators)

Expand Down Expand Up @@ -251,7 +254,10 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) error {
nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, powerShapingParameters, minPower)

valUpdates := DiffValidators(currentValidators, nextValidators)
k.SetConsumerValSet(ctx, consumerId, nextValidators)
err = k.SetConsumerValSet(ctx, consumerId, nextValidators)
if err != nil {
return fmt.Errorf("setting consumer validator set, consumerId(%s): %w", consumerId, err)
}

// check whether there are changes in the validator set
if len(valUpdates) != 0 {
Expand Down
20 changes: 13 additions & 7 deletions x/ccv/provider/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,15 @@ func TestQueueVSCPacketsDoesNotResetConsumerValidatorsHeights(t *testing.T) {
PublicKey: &valAPubKey,
JoinHeight: 123456789,
}
providerKeeper.SetConsumerValidator(ctx, "consumerId", consumerValidatorA)
err := providerKeeper.SetConsumerValidator(ctx, "consumerId", consumerValidatorA)
require.NoError(t, err)

// Opt in validator B. Note that validator B is not a consumer validator and hence would become a consumer
// validator for the first time after the `QueueVSCPackets` call.
providerKeeper.SetOptedIn(ctx, "consumerId", providertypes.NewProviderConsAddress(valBConsAddr))

// set power shaping params
err := providerKeeper.SetConsumerPowerShapingParameters(ctx, "consumerId", providertypes.PowerShapingParameters{})
err = providerKeeper.SetConsumerPowerShapingParameters(ctx, "consumerId", providertypes.PowerShapingParameters{})
require.NoError(t, err)

err = providerKeeper.QueueVSCPackets(ctx)
Expand Down Expand Up @@ -161,9 +162,10 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) {
providerKeeper.SetValsetUpdateBlockHeight(ctx, packetData.ValsetUpdateId, uint64(15))

// Set consumer validator
providerKeeper.SetConsumerValidator(ctx, "chain-1", providertypes.ConsensusValidator{
err := providerKeeper.SetConsumerValidator(ctx, "chain-1", providertypes.ConsensusValidator{
ProviderConsAddr: packetData.Validator.Address,
})
require.NoError(t, err)

// Set slash meter to negative value and assert a bounce ack is returned
providerKeeper.SetSlashMeter(ctx, math.NewInt(-5))
Expand All @@ -172,9 +174,10 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) {
require.NoError(t, err)

// Set consumer validator
providerKeeper.SetConsumerValidator(ctx, "chain-2", providertypes.ConsensusValidator{
err = providerKeeper.SetConsumerValidator(ctx, "chain-2", providertypes.ConsensusValidator{
ProviderConsAddr: packetData.Validator.Address,
})
require.NoError(t, err)

// Also bounced for chain-2
ackResult, err = executeOnRecvSlashPacket(t, &providerKeeper, ctx, "channel-2", 2, packetData)
Expand All @@ -185,7 +188,8 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) {
providerKeeper.SetSlashMeter(ctx, math.NewInt(5))

// Set the consumer validator
providerKeeper.SetConsumerValidator(ctx, "chain-1", providertypes.ConsensusValidator{ProviderConsAddr: packetData.Validator.Address})
err = providerKeeper.SetConsumerValidator(ctx, "chain-1", providertypes.ConsensusValidator{ProviderConsAddr: packetData.Validator.Address})
require.NoError(t, err)

// Mock call to GetEffectiveValPower, so that it returns 2.
providerAddr := providertypes.NewProviderConsAddress(packetData.Validator.Address)
Expand Down Expand Up @@ -463,7 +467,8 @@ func TestHandleSlashPacket(t *testing.T) {
// Setup consumer address to provider address mapping.
require.NotEmpty(t, tc.packetData.Validator.Address)
providerKeeper.SetValidatorByConsumerAddr(ctx, chainId, consumerConsAddr, providerConsAddr)
providerKeeper.SetConsumerValidator(ctx, chainId, providertypes.ConsensusValidator{ProviderConsAddr: providerConsAddr.Address.Bytes()})
err := providerKeeper.SetConsumerValidator(ctx, chainId, providertypes.ConsensusValidator{ProviderConsAddr: providerConsAddr.Address.Bytes()})
require.NoError(t, err)

// Execute method and assert expected mock calls.
providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData)
Expand Down Expand Up @@ -731,7 +736,8 @@ func TestProviderValidatorUpdates(t *testing.T) {
// consensusVals is now [removedValidator, validator 2, validator 1]

// Set the last provider consensus validator set
providerKeeper.SetLastProviderConsensusValSet(ctx, consensusVals)
err = providerKeeper.SetLastProviderConsensusValSet(ctx, consensusVals)
require.NoError(t, err)

// Set the max number of validators
maxProviderConsensusValidators := int64(2)
Expand Down
6 changes: 5 additions & 1 deletion x/ccv/provider/keeper/staking_keeper_interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper

import (
"context"
"fmt"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -34,11 +35,14 @@ func (k Keeper) TotalBondedTokens(ctx context.Context) (math.Int, error) {
// iterate through the bonded validators
totalBondedTokens := math.ZeroInt()

k.IterateBondedValidatorsByPower(ctx, func(_ int64, validator stakingtypes.ValidatorI) (stop bool) {
err := k.IterateBondedValidatorsByPower(ctx, func(_ int64, validator stakingtypes.ValidatorI) (stop bool) {
tokens := validator.GetBondedTokens()
totalBondedTokens = totalBondedTokens.Add(tokens)
return false
})
if err != nil {
return math.Int{}, fmt.Errorf("iteration inside TotalBondedTokens failed: %w", err)
}

return totalBondedTokens, nil
}
Expand Down
8 changes: 4 additions & 4 deletions x/ccv/provider/keeper/validator_set_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ func (k Keeper) SetConsumerValidator(
ctx sdk.Context,
consumerId string,
validator types.ConsensusValidator,
) {
k.setValidator(ctx, k.GetConsumerChainConsensusValidatorsKey(ctx, consumerId), validator)
) error {
return k.setValidator(ctx, k.GetConsumerChainConsensusValidatorsKey(ctx, consumerId), validator)
}

// SetConsumerValSet resets the current consumer validators with the `nextValidators` computed by
// `FilterValidators` and hence this method should only be called after `FilterValidators` has completed.
func (k Keeper) SetConsumerValSet(ctx sdk.Context, consumerId string, nextValidators []types.ConsensusValidator) {
k.setValSet(ctx, k.GetConsumerChainConsensusValidatorsKey(ctx, consumerId), nextValidators)
func (k Keeper) SetConsumerValSet(ctx sdk.Context, consumerId string, nextValidators []types.ConsensusValidator) error {
return k.setValSet(ctx, k.GetConsumerChainConsensusValidatorsKey(ctx, consumerId), nextValidators)
}

// DeleteConsumerValidator removes consumer validator with `providerAddr` address
Expand Down
Loading

0 comments on commit a17a385

Please sign in to comment.