The security check of "black list address" is best performed at the base module rather than the application module

[summerpro]

Summary

• The security check of "black list address" is best performed at the base module rather than the business module

Problem Definition

• "Module account" is the system account used by each module to manage module funds. The transfer of tokens by ordinary users to the blacklisted "module account" is beyond expectations and should be prohibited
• In some business functions, we need to use "recipient address" to receive payment, like this:

// MsgSetWithdrawAddress
// SetWithdrawAddr sets a new address that will receive the rewards upon withdrawal
func (k Keeper) SetWithdrawAddr(ctx sdk.Context, delegatorAddr sdk.AccAddress, withdrawAddr sdk.AccAddress) error {
	if k.blockedAddrs[withdrawAddr.String()] {
		return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive external funds", withdrawAddr)
	}

	if !k.GetWithdrawAddrEnabled(ctx) {
		return types.ErrSetWithdrawAddrDisabled
	}

	ctx.EventManager().EmitEvent(
		sdk.NewEvent(
			types.EventTypeSetWithdrawAddress,
			sdk.NewAttribute(types.AttributeKeyWithdrawAddress, withdrawAddr.String()),
		),
	)

	k.SetDelegatorWithdrawAddr(ctx, delegatorAddr, withdrawAddr)
	return nil
}

// CommunityPoolSpendProposal
// HandleCommunityPoolSpendProposal is a handler for executing a passed community spend proposal
func HandleCommunityPoolSpendProposal(ctx sdk.Context, k Keeper, p *types.CommunityPoolSpendProposal) error {
	if k.blockedAddrs[p.Recipient] {
		return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive external funds", p.Recipient)
	}

	recipient, addrErr := sdk.AccAddressFromBech32(p.Recipient)
	if addrErr != nil {
		return addrErr
	}

	err := k.DistributeFromFeePool(ctx, p.Amount, recipient)
	if err != nil {
		return err
	}

	logger := k.Logger(ctx)
	logger.Info("transferred from the community pool to recipient", "amount", p.Amount.String(), "recipient", p.Recipient)

	return nil
}

• The risk is that we need to remember to check the "blockedAddrs" when developing each new business function that needs to be passed in the "receipient address". This is a work that is easily forgotten. I suggest using the basic module such as "SendCoinsFromModuleToAccount" Do such a check.
• Especially for external developers when using cosmos-SDK, it is easier to miss the "black list address" check.

Proposal

• add security check of "black list address" in function "SendCoinsFromModuleToAccount"

func (k BaseKeeper) SendCoinsFromModuleToAccount(
	ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins,
) error {
	if k.BlockedAddr(recipientAddr) {
		return sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "Address <%s> in blacklist is not allowed", recipientAddr)
	}
	senderAddr := k.ak.GetModuleAddress(senderModule)
	if senderAddr == nil {
		panic(sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule))
	}

	return k.SendCoins(ctx, senderAddr, recipientAddr, amt)
}

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

The risk is that we need to remember to check the "blockedAddrs" when developing each new business function that needs to be passed in the "receipient address". This is a work that is easily forgotten. I suggest using the basic module such as "SendCoinsFromModuleToAccount" Do such a check.

We do not need to. The blacklist is used to prevent funds from being explicitly sent to via MsgSend and MsgMultiSend. These handlers already check the blacklist.

However, as you've pointed out, there are a few exceptions where a user can set an address to receive funds such as SetWithdrawAddr. I agree these methods should also check against the blacklist.

[summerpro]

SetWithdrawAddr. I agree these methods should also check against the blacklist.

Yes, your findings are great. I think that security checks should be based on more basic modules to prevent such things from happening, because it is difficult for new module development to avoid the omission of such checks from happening again.

[summerpro]

SetWithdrawAddr. I agree these methods should also check against the blacklist.

Yes, your findings are great. I think that security checks should be based on more basic modules to prevent such things from happening, because it is difficult for new module development to avoid the omission of such checks from happening again.

If we want to transfer funds to "module accounts", we only need to call "SendCoinsFromAccountToModule" or "SendCoinsFromModuleToModule" instead of "SendCoinsFromModuleToAccount". The design of the function has stipulated its scope of use.

[alexanderbez]

because it is difficult for new module development to avoid the omission of such checks from happening again.

Module developers just need to keep in mind and ask themselves, "Can a user explicitly and externally set an address to receive funds in this case?". If the answer is yes, then need to guard against the blacklist.

[summerpro]

because it is difficult for new module development to avoid the omission of such checks from happening again.

Module developers just need to keep in mind and ask themselves, "Can a user explicitly and externally set an address to receive funds in this case?". If the answer is yes, then need to guard against the blacklist.

If these are easy to remember in our mind, there will be no omissions in the check that occurred in "SetWithdrawAddr".

I think complex and generic inspections should be handed over to the basic module, and the considerations of the application module should be as simple as possible.