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

refactor(x/distribution): audit changes #16785

Merged
merged 2 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion tests/integration/distribution/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ func TestGRPCDelegationRewards(t *testing.T) {
delTokens := sdk.TokensFromConsensusPower(2, sdk.DefaultPowerReduction)
validator, issuedShares := val.AddTokensFromDel(delTokens)
delegation := stakingtypes.NewDelegation(delAddr, f.valAddr, issuedShares)
f.stakingKeeper.SetDelegation(f.sdkCtx, delegation)
assert.NilError(t, f.stakingKeeper.SetDelegation(f.sdkCtx, delegation))
assert.NilError(t, f.distrKeeper.DelegatorStartingInfo.Set(f.sdkCtx, collections.Join(validator.GetOperator(), delAddr), types.NewDelegatorStartingInfo(2, math.LegacyNewDec(initialStake), 20)))

// setup validator rewards
Expand Down
8 changes: 4 additions & 4 deletions tests/integration/distribution/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
assert.NilError(t, err)
validator.DelegatorShares = math.LegacyNewDec(100)
validator.Tokens = sdk.NewInt(1000000)
f.stakingKeeper.SetValidator(f.sdkCtx, validator)
assert.NilError(t, f.stakingKeeper.SetValidator(f.sdkCtx, validator))

// set module account coins
initTokens := f.stakingKeeper.TokensFromConsensusPower(f.sdkCtx, int64(1000))
Expand All @@ -196,7 +196,7 @@ func TestMsgWithdrawDelegatorReward(t *testing.T) {
delTokens := sdk.TokensFromConsensusPower(2, sdk.DefaultPowerReduction)
validator, issuedShares := validator.AddTokensFromDel(delTokens)
delegation := stakingtypes.NewDelegation(delAddr, validator.GetOperator(), issuedShares)
f.stakingKeeper.SetDelegation(f.sdkCtx, delegation)
require.NoError(t, f.stakingKeeper.SetDelegation(f.sdkCtx, delegation))
err = f.distrKeeper.DelegatorStartingInfo.Set(f.sdkCtx, collections.Join(validator.GetOperator(), delAddr), distrtypes.NewDelegatorStartingInfo(2, math.LegacyOneDec(), 20))
require.NoError(t, err)
// setup validator rewards
Expand Down Expand Up @@ -896,8 +896,8 @@ func TestMsgDepositValidatorRewardsPool(t *testing.T) {

// mint a non-staking token and send to an account
amt := sdk.NewCoins(sdk.NewInt64Coin("foo", 500))
f.bankKeeper.MintCoins(f.sdkCtx, distrtypes.ModuleName, amt)
f.bankKeeper.SendCoinsFromModuleToAccount(f.sdkCtx, distrtypes.ModuleName, addr, amt)
require.NoError(t, f.bankKeeper.MintCoins(f.sdkCtx, distrtypes.ModuleName, amt))
require.NoError(t, f.bankKeeper.SendCoinsFromModuleToAccount(f.sdkCtx, distrtypes.ModuleName, addr, amt))

bondDenom, err := f.stakingKeeper.BondDenom(f.sdkCtx)
require.NoError(t, err)
Expand Down
7 changes: 4 additions & 3 deletions x/distribution/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ func BeginBlocker(ctx sdk.Context, k keeper.Keeper) error {
// TODO this is Tendermint-dependent
// ref https://github.com/cosmos/cosmos-sdk/issues/3095
if ctx.BlockHeight() > 1 {
k.AllocateTokens(ctx, previousTotalPower, ctx.VoteInfos())
if err := k.AllocateTokens(ctx, previousTotalPower, ctx.VoteInfos()); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
return err
}
}

// record the proposer for when we payout on the next block
consAddr := sdk.ConsAddress(ctx.BlockHeader().ProposerAddress)
k.SetPreviousProposerConsAddr(ctx, consAddr)
return nil
return k.SetPreviousProposerConsAddr(ctx, consAddr)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
}
4 changes: 3 additions & 1 deletion x/distribution/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data types.GenesisState) {
}
}

k.SetPreviousProposerConsAddr(ctx, previousProposer)
if err = k.SetPreviousProposerConsAddr(ctx, previousProposer); err != nil {
panic(err)
}

for _, rew := range data.OutstandingRewards {
valAddr, err := sdk.ValAddressFromBech32(rew.ValidatorAddress)
Expand Down
3 changes: 1 addition & 2 deletions x/distribution/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,7 @@ func (h Hooks) AfterDelegationModified(ctx context.Context, delAddr sdk.AccAddre

// record the slash event
func (h Hooks) BeforeValidatorSlashed(ctx context.Context, valAddr sdk.ValAddress, fraction sdkmath.LegacyDec) error {
h.k.updateValidatorSlashFraction(ctx, valAddr, fraction)
return nil
return h.k.updateValidatorSlashFraction(ctx, valAddr, fraction)
}

func (h Hooks) BeforeValidatorModified(_ context.Context, _ sdk.ValAddress) error {
Expand Down
4 changes: 3 additions & 1 deletion x/distribution/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,9 @@ func (k msgServer) DepositValidatorRewardsPool(ctx context.Context, msg *types.M
// Allocate tokens from the distribution module to the validator, which are
// then distributed to the validator's delegators.
reward := sdk.NewDecCoinsFromCoins(msg.Amount...)
k.AllocateTokensToValidator(ctx, validator, reward)
if err = k.AllocateTokensToValidator(ctx, validator, reward); err != nil {
return nil, err
}

logger := k.Logger(ctx)
logger.Info(
Expand Down
4 changes: 2 additions & 2 deletions x/distribution/keeper/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ func (k Keeper) GetPreviousProposerConsAddr(ctx context.Context) (sdk.ConsAddres
}

// set the proposer public key for this block
func (k Keeper) SetPreviousProposerConsAddr(ctx context.Context, consAddr sdk.ConsAddress) {
func (k Keeper) SetPreviousProposerConsAddr(ctx context.Context, consAddr sdk.ConsAddress) error {
store := k.storeService.OpenKVStore(ctx)
bz := k.cdc.MustMarshal(&gogotypes.BytesValue{Value: consAddr})
store.Set(types.ProposerKey, bz)
return store.Set(types.ProposerKey, bz)
}

// historical reference count (used for testcases)
Expand Down
46 changes: 0 additions & 46 deletions x/distribution/types/msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,37 +21,19 @@ func NewMsgSetWithdrawAddress(delAddr, withdrawAddr sdk.AccAddress) *MsgSetWithd
}
}

// Return address that must sign over msg.GetSignBytes()
func (msg MsgSetWithdrawAddress) GetSigners() []sdk.AccAddress {
delegator, _ := sdk.AccAddressFromBech32(msg.DelegatorAddress)
return []sdk.AccAddress{delegator}
}

func NewMsgWithdrawDelegatorReward(delAddr sdk.AccAddress, valAddr sdk.ValAddress) *MsgWithdrawDelegatorReward {
return &MsgWithdrawDelegatorReward{
DelegatorAddress: delAddr.String(),
ValidatorAddress: valAddr.String(),
}
}

// Return address that must sign over msg.GetSignBytes()
func (msg MsgWithdrawDelegatorReward) GetSigners() []sdk.AccAddress {
delegator, _ := sdk.AccAddressFromBech32(msg.DelegatorAddress)
return []sdk.AccAddress{delegator}
}

func NewMsgWithdrawValidatorCommission(valAddr sdk.ValAddress) *MsgWithdrawValidatorCommission {
return &MsgWithdrawValidatorCommission{
ValidatorAddress: valAddr.String(),
}
}

// Return address that must sign over msg.GetSignBytes()
func (msg MsgWithdrawValidatorCommission) GetSigners() []sdk.AccAddress {
valAddr, _ := sdk.ValAddressFromBech32(msg.ValidatorAddress)
return []sdk.AccAddress{sdk.AccAddress(valAddr)}
}

// NewMsgFundCommunityPool returns a new MsgFundCommunityPool with a sender and
// a funding amount.
func NewMsgFundCommunityPool(amount sdk.Coins, depositor sdk.AccAddress) *MsgFundCommunityPool {
Expand All @@ -61,27 +43,6 @@ func NewMsgFundCommunityPool(amount sdk.Coins, depositor sdk.AccAddress) *MsgFun
}
}

// GetSigners returns the signer addresses that are expected to sign the result
// of GetSignBytes.
func (msg MsgFundCommunityPool) GetSigners() []sdk.AccAddress {
depositor, _ := sdk.AccAddressFromBech32(msg.Depositor)
return []sdk.AccAddress{depositor}
}

// GetSigners returns the signer addresses that are expected to sign the result
// of GetSignBytes.
func (msg MsgUpdateParams) GetSigners() []sdk.AccAddress {
authority, _ := sdk.AccAddressFromBech32(msg.Authority)
return []sdk.AccAddress{authority}
}

// GetSigners returns the signer addresses that are expected to sign the result
// of GetSignBytes, which is the authority.
func (msg MsgCommunityPoolSpend) GetSigners() []sdk.AccAddress {
authority, _ := sdk.AccAddressFromBech32(msg.Authority)
return []sdk.AccAddress{authority}
}

// NewMsgDepositValidatorRewardsPool returns a new MsgDepositValidatorRewardsPool
// with a depositor and a funding amount.
func NewMsgDepositValidatorRewardsPool(depositor sdk.AccAddress, valAddr sdk.ValAddress, amount sdk.Coins) *MsgDepositValidatorRewardsPool {
Expand All @@ -91,10 +52,3 @@ func NewMsgDepositValidatorRewardsPool(depositor sdk.AccAddress, valAddr sdk.Val
ValidatorAddress: valAddr.String(),
}
}

// GetSigners returns the signer addresses that are expected to sign the result
// of GetSignBytes, which is the depositor.
func (msg MsgDepositValidatorRewardsPool) GetSigners() []sdk.AccAddress {
depositor, _ := sdk.AccAddressFromBech32(msg.Depositor)
return []sdk.AccAddress{depositor}
}