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

fix!: prevent denom DOS #931

Merged
merged 14 commits into from
May 15, 2023
26 changes: 17 additions & 9 deletions app/provider/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,14 @@ var (

// module account permissions
maccPerms = map[string][]string{
authtypes.FeeCollectorName: nil,
distrtypes.ModuleName: nil,
minttypes.ModuleName: {authtypes.Minter},
stakingtypes.BondedPoolName: {authtypes.Burner, authtypes.Staking},
stakingtypes.NotBondedPoolName: {authtypes.Burner, authtypes.Staking},
govtypes.ModuleName: {authtypes.Burner},
ibctransfertypes.ModuleName: {authtypes.Minter, authtypes.Burner},
authtypes.FeeCollectorName: nil,
distrtypes.ModuleName: nil,
minttypes.ModuleName: {authtypes.Minter},
stakingtypes.BondedPoolName: {authtypes.Burner, authtypes.Staking},
stakingtypes.NotBondedPoolName: {authtypes.Burner, authtypes.Staking},
govtypes.ModuleName: {authtypes.Burner},
ibctransfertypes.ModuleName: {authtypes.Minter, authtypes.Burner},
providertypes.ConsumerRewardsPool: nil,
}
)

Expand Down Expand Up @@ -308,12 +309,12 @@ func New(
maccPerms,
)

// Remove the fee-pool from the group of blocked recipient addresses in bank
// Remove the ConsumerRewardsPool from the group of blocked recipient addresses in bank
// this is required for the provider chain to be able to receive tokens from
// the consumer chain
bankBlockedAddrs := app.ModuleAccountAddrs()
delete(bankBlockedAddrs, authtypes.NewModuleAddress(
authtypes.FeeCollectorName).String())
providertypes.ConsumerRewardsPool).String())

app.BankKeeper = bankkeeper.NewBaseKeeper(
appCodec,
Expand Down Expand Up @@ -409,6 +410,8 @@ func New(
app.SlashingKeeper,
app.AccountKeeper,
app.EvidenceKeeper,
app.DistrKeeper,
app.BankKeeper,
authtypes.FeeCollectorName,
)

Expand Down Expand Up @@ -777,6 +780,11 @@ func (app *App) GetTestDistributionKeeper() testutil.TestDistributionKeeper {
return app.DistrKeeper
}

// GetTestAccountKeeper implements the ProviderApp interface.
func (app *App) GetTestAccountKeeper() testutil.TestAccountKeeper {
return app.AccountKeeper
}

// TestingApp functions

// GetBaseApp implements the TestingApp interface.
Expand Down
7 changes: 7 additions & 0 deletions proto/interchain_security/ccv/consumer/v1/consumer.proto
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ message Params {
// can opt out of running the consumer chain without being punished. For example, a
// value of 0.05 means that the validators in the bottom 5% of the set can opt out
string soft_opt_out_threshold = 10;

// Reward denoms. These are the denominations which are allowed to be sent to the provider as rewards.
repeated string reward_denoms = 11;

// Provider-originated reward denoms. These are denoms coming from the provider
// which are allowed to be used as rewards. e.g. "uatom"
repeated string provider_reward_denoms = 12;
}

// LastTransmissionBlockHeight is the last time validator holding
Expand Down
5 changes: 5 additions & 0 deletions proto/interchain_security/ccv/provider/v1/provider.proto
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import "ibc/core/client/v1/client.proto";
import "ibc/lightclients/tendermint/v1/tendermint.proto";
import "tendermint/crypto/keys.proto";
import "cosmos/evidence/v1beta1/evidence.proto";
import "cosmos/base/v1beta1/coin.proto";

// ConsumerAdditionProposal is a governance proposal on the provider chain to spawn a new consumer chain.
// If it passes, then all validators on the provider chain are expected to validate the consumer chain at spawn time
Expand Down Expand Up @@ -136,6 +137,10 @@ message Params {
// The maximum amount of throttled slash or vsc matured packets
// that can be queued for a single consumer before the provider chain halts.
int64 max_throttled_packets = 8;

// The fee required to be paid to add a reward denom
cosmos.base.v1beta1.Coin consumer_reward_denom_registration_fee = 9
[(gogoproto.nullable) = false];
}

message HandshakeMetadata {
Expand Down
12 changes: 12 additions & 0 deletions proto/interchain_security/ccv/provider/v1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ service Query {
returns (QueryThrottledConsumerPacketDataResponse) {
option (google.api.http).get = "/interchain_security/ccv/provider/pending_consumer_packets";
}

// QueryRegisteredConsumerRewardDenoms returns a list of consumer reward denoms that are registered
rpc QueryRegisteredConsumerRewardDenoms(QueryRegisteredConsumerRewardDenomsRequest)
returns (QueryRegisteredConsumerRewardDenomsResponse) {
option (google.api.http).get = "/interchain_security/ccv/provider/registered_consumer_reward_denoms";
}
}

message QueryConsumerGenesisRequest { string chain_id = 1; }
Expand Down Expand Up @@ -169,3 +175,9 @@ message ThrottledPacketDataWrapper {
interchain_security.ccv.v1.VSCMaturedPacketData vsc_matured_packet = 2;
}
}

message QueryRegisteredConsumerRewardDenomsRequest {}

message QueryRegisteredConsumerRewardDenomsResponse {
repeated string denoms = 1;
}
17 changes: 16 additions & 1 deletion proto/interchain_security/ccv/provider/v1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import "google/protobuf/any.proto";
// Msg defines the Msg service.
service Msg {
rpc AssignConsumerKey(MsgAssignConsumerKey) returns (MsgAssignConsumerKeyResponse);
rpc RegisterConsumerRewardDenom(MsgRegisterConsumerRewardDenom) returns (MsgRegisterConsumerRewardDenomResponse);
}

message MsgAssignConsumerKey {
Expand All @@ -27,4 +28,18 @@ message MsgAssignConsumerKey {
string consumer_key = 3;
}

message MsgAssignConsumerKeyResponse {}
message MsgAssignConsumerKeyResponse {}

// MsgRegisterConsumerRewardDenom allows an account to register
// a consumer reward denom, i.e., add it to the list of denoms
// accepted by the provider as rewards.
message MsgRegisterConsumerRewardDenom {
option (gogoproto.equal) = false;
option (gogoproto.goproto_getters) = false;

string denom = 1;
string depositor = 2;
}

// MsgRegisterConsumerRewardDenomResponse defines the Msg/RegisterConsumerRewardDenom response type.
message MsgRegisterConsumerRewardDenomResponse {}
2 changes: 2 additions & 0 deletions tests/difference/core/driver/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,8 @@ func (b *Builder) createConsumerGenesis(client *ibctmtypes.ClientState) *consume
consumertypes.DefaultHistoricalEntries,
b.initState.UnbondingC,
"0", // disable soft opt-out
[]string{},
[]string{},
)
return consumertypes.NewInitialGenesisState(client, providerConsState, valUpdates, params)
}
Expand Down
30 changes: 30 additions & 0 deletions tests/e2e/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -1160,6 +1160,36 @@ func (tr TestRun) registerRepresentative(
wg.Wait()
}

type registerConsumerRewardDenomAction struct {
chain chainID
from validatorID
denom string
}

func (tr TestRun) registerConsumerRewardDenom(action registerConsumerRewardDenomAction, verbose bool) {
//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
bz, err := exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[action.chain].binaryName,
"tx", "provider", "register-consumer-reward-denom", action.denom,

`--from`, `validator`+fmt.Sprint(action.from),
`--chain-id`, string(action.chain),
`--home`, tr.getValidatorHome(action.chain, action.from),
`--node`, tr.getValidatorNode(action.chain, action.from),
`--gas`, "9000000",
`--keyring-backend`, `test`,
`-b`, `block`,
`-y`,
).CombinedOutput()

if verbose {
fmt.Println("redelegate cmd:", string(bz))
}

if err != nil {
log.Fatal(err, "\n", string(bz))
}
}

// Creates an additional node on selected chain
// by copying an existing validator's home folder
//
Expand Down
21 changes: 14 additions & 7 deletions tests/e2e/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,19 @@ func DefaultTestRun() TestRun {
}
}

func DemocracyTestRun() TestRun {
func DemocracyTestRun(allowReward bool) TestRun {
consumerGenChanges := ".app_state.ccvconsumer.params.blocks_per_distribution_transmission = \"20\" | " +
".app_state.gov.voting_params.voting_period = \"10s\" | " +
".app_state.slashing.params.signed_blocks_window = \"2\" | " +
".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\""

if allowReward {
// This allows the consumer chain to send rewards in the stake denom
consumerGenChanges += " | .app_state.ccvconsumer.params.reward_denoms = [\"stake\"]"
}

return TestRun{
name: "democracy",
containerConfig: ContainerConfig{
Expand Down Expand Up @@ -253,12 +265,7 @@ func DemocracyTestRun() TestRun {
binaryName: "interchain-security-cdd",
ipPrefix: "7.7.9",
votingWaitTime: 20,
genesisChanges: ".app_state.ccvconsumer.params.blocks_per_distribution_transmission = \"20\" | " +
".app_state.gov.voting_params.voting_period = \"10s\" | " +
".app_state.slashing.params.signed_blocks_window = \"2\" | " +
".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\"",
genesisChanges: consumerGenChanges,
},
},
tendermintConfigOverride: `s/timeout_commit = "5s"/timeout_commit = "1s"/;` +
Expand Down
5 changes: 4 additions & 1 deletion tests/e2e/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ func main() {

testRuns := []testRunWithSteps{
{DefaultTestRun(), happyPathSteps},
{DemocracyTestRun(), democracySteps},
{DemocracyTestRun(true), democracySteps},
{DemocracyTestRun(false), rewardDenomConsumerSteps},
{SlashThrottleTestRun(), slashThrottleSteps},
}
if includeMultiConsumer != nil && *includeMultiConsumer {
Expand Down Expand Up @@ -143,6 +144,8 @@ func (tr *TestRun) runStep(step Step, verbose bool) {
tr.waitForSlashThrottleDequeue(action, verbose)
case startHermesAction:
tr.startHermes(action, verbose)
case registerConsumerRewardDenomAction:
tr.registerConsumerRewardDenom(action, verbose)
default:
log.Fatalf("unknown action in testRun %s: %#v", tr.name, action)
}
Expand Down
50 changes: 39 additions & 11 deletions tests/e2e/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,18 @@ import (
type State map[chainID]ChainState

type ChainState struct {
ValBalances *map[validatorID]uint
Proposals *map[uint]Proposal
ValPowers *map[validatorID]uint
RepresentativePowers *map[validatorID]uint
Params *[]Param
Rewards *Rewards
ConsumerChains *map[chainID]bool
AssignedKeys *map[validatorID]string
ProviderKeys *map[validatorID]string // validatorID: validator provider key
ConsumerChainQueueSizes *map[chainID]uint
GlobalSlashQueueSize *uint
ValBalances *map[validatorID]uint
Proposals *map[uint]Proposal
ValPowers *map[validatorID]uint
RepresentativePowers *map[validatorID]uint
Params *[]Param
Rewards *Rewards
ConsumerChains *map[chainID]bool
AssignedKeys *map[validatorID]string
ProviderKeys *map[validatorID]string // validatorID: validator provider key
ConsumerChainQueueSizes *map[chainID]uint
GlobalSlashQueueSize *uint
RegisteredConsumerRewardDenoms *[]string
}

type Proposal interface {
Expand Down Expand Up @@ -167,6 +168,11 @@ func (tr TestRun) getChainState(chain chainID, modelState ChainState) ChainState
chainState.ConsumerChainQueueSizes = &consumerChainQueueSizes
}

if modelState.RegisteredConsumerRewardDenoms != nil {
registeredConsumerRewardDenoms := tr.getRegisteredConsumerRewardDenoms(chain)
chainState.RegisteredConsumerRewardDenoms = &registeredConsumerRewardDenoms
}

return chainState
}

Expand Down Expand Up @@ -640,6 +646,28 @@ func (tr TestRun) getConsumerChainPacketQueueSize(consumerChain chainID) uint {
return uint(size)
}

func (tr TestRun) getRegisteredConsumerRewardDenoms(chain chainID) []string {
//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
cmd := exec.Command("docker", "exec", tr.containerConfig.instanceName, tr.chainConfigs[chain].binaryName,

"query", "provider", "registered-consumer-reward-denoms",
`--node`, tr.getQueryNode(chain),
`-o`, `json`,
)
bz, err := cmd.CombinedOutput()
if err != nil {
log.Fatal(err, "\n", string(bz))
}

denoms := gjson.Get(string(bz), "denoms").Array()
rewardDenoms := make([]string, len(denoms))
for i, d := range denoms {
rewardDenoms[i] = d.String()
}

return rewardDenoms
}

func (tr TestRun) getValidatorNode(chain chainID, validator validatorID) string {
return "tcp://" + tr.getValidatorIP(chain, validator) + ":26658"
}
Expand Down
8 changes: 8 additions & 0 deletions tests/e2e/steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ var democracySteps = concatSteps(
stepsDemocracy("democ"),
)

var rewardDenomConsumerSteps = concatSteps(
// democracySteps requires a transfer channel
stepsStartChains([]string{"democ"}, true),
// delegation needs to happen so the first VSC packet can be delivered
stepsDelegate("democ"),
stepsRewardDenomConsumer("democ"),
)

var multipleConsumers = concatSteps(
stepsStartChains([]string{"consu", "densu"}, false),
stepsMultiConsumerDelegate("consu", "densu"),
Expand Down
41 changes: 41 additions & 0 deletions tests/e2e/steps_democracy.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,47 @@ func stepsDemocracy(consumerName string) []Step {
},
},
},
{
action: relayRewardPacketsToProviderAction{
consumerChain: chainID(consumerName),
providerChain: chainID("provi"),
port: "transfer",
channel: 1,
},
state: State{
chainID("provi"): ChainState{
// Check that tokens are not distributed before the denom has been registered
Rewards: &Rewards{
IsRewarded: map[validatorID]bool{
validatorID("alice"): false,
validatorID("bob"): false,
validatorID("carol"): false,
},
IsIncrementalReward: false,
IsNativeDenom: false,
},
// Check that the denom is not registered on provider chain
RegisteredConsumerRewardDenoms: &[]string{},
},
},
},
{
action: registerConsumerRewardDenomAction{
chain: chainID("provi"),
from: validatorID("bob"),
denom: "ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9",
},
state: State{
chainID("provi"): ChainState{
// Check that the denom is registered on provider chain
RegisteredConsumerRewardDenoms: &[]string{"ibc/3C3D7B3BE4ECC85A0E5B52A3AEC3B7DFC2AA9CA47C37821E57020D6807043BE9"},
shaspitz marked this conversation as resolved.
Show resolved Hide resolved
ValBalances: &map[validatorID]uint{
// make sure that bob's account was debited
validatorID("bob"): 9490000000,
},
},
},
},
{
action: relayRewardPacketsToProviderAction{
consumerChain: chainID(consumerName),
Expand Down
Loading