[Bug] Attacker has the ability to send funds to module address, potentially breaking blockchain invariants and halting the network

[stonejiajia]

Root Cause

A critical vulnerability in the bankMsgSend precompile function allows attackers to send funds directly to module address. This capability can potentially break blockchain invariants and lead to a network halt. The lack of proper checks in the function enables malicious actors to disrupt the expected state of the system, posing a severe threat to the network's stability and integrity.

The vulnerability is triggered through the following execution path:
FunTokenMethod_bankMsgSend -> bankMsgSend -> sendCoins -> sendCoins ( cosmos )

func (bk NibiruBankKeeper) SendCoins(
	ctx sdk.Context,
	fromAddr sdk.AccAddress,
	toAddr sdk.AccAddress,
	coins sdk.Coins,
) error {
	return bk.ForceGasInvariant(
		ctx,
		func(ctx sdk.Context) error {
			return bk.BaseKeeper.SendCoins(ctx, fromAddr, toAddr, coins)
		},
		func(ctx sdk.Context) {
			if findEtherBalanceChangeFromCoins(coins) {
				bk.SyncStateDBWithAccount(ctx, fromAddr)
				bk.SyncStateDBWithAccount(ctx, toAddr)
			}
		},
	)
}

The final step uses the Cosmos bank keeper function SendCoins. In Cosmos, there is no validation of the toAddr in SendCoins. Only the send function validates the toAddr

func (k msgServer) Send(ctx context.Context, msg *types.MsgSend) (*types.MsgSendResponse, error) {
        .....
	if k.BlockedAddr(to) {
		return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", msg.ToAddress)
	}

	err = k.SendCoins(ctx, from, to, msg.Amount)
	if err != nil {
		return nil, err
	}

	return &types.MsgSendResponse{}, nil
}

Document Explain that:

The x/bank module accepts a map of addresses that are considered blocklisted from directly and explicitly receiving funds.

Typically, these addresses are module accounts. If these addresses receive funds outside the expected rules of the state machine, invariants are likely to be broken and could result in a halted network.

Internal pre-conditions

1. The bankMsgSend precompile function needs to be active and accessible in the EVM environment.
2. The attacker needs to have sufficient funds to initiate a transfer.
3. The system needs to have at least one module account with a non-zero balance.

Attack Path:

1. Attacker identifies a module account address.
2. Attacker calls the bankMsgSend precompile function, specifying the module account as the recipient.
3. The function executes the transfer without proper checks.
4. Funds are transferred to the module account, potentially disrupting its balance and breaking system invariants.

Impact

The protocol suffers a potential loss of funds in module accounts, which could lead to broken invariants and network halts.

[stonejiajia]

I reported the same vulnerability on Code4rena, but SendCoins was not modified. I suspect this report was disregarded. However, I still believe that it's necessary to validate the toAddr within the SendCoins function

[Unique-Divine]

Why does it matter if you can send funds to a module account. That's perfectly valid

It looks like you're referring to an issue similar to this one from 6 years ago: cosmos/cosmos-sdk#4795.

Is there an example of a module account with the modern Cosmos-SDK where it's bad to send it funds?

[stonejiajia]

1. First , 6 years ago allow send funds to module address. In 2021 , there is patch that not allowed do this
   The security check of "black list address" is best performed at the base module rather than the application module cosmos/cosmos-sdk#8463

https://medium.com/@jjordanjjordan/150-000-evmos-vulnerability-through-reading-documentation-d26328590a7a
2. This blog mentions: if send funds to module address, It will cause CONSENSUS FAILURE by tendermint consensus mechanism. could result in a halted network.

[stonejiajia]

https://github.com/evmos/evmos/blob/main/app/app.go#L963
This is how evmos handle blocked address, Module address are not allowed

func initBankKeeper(appCodec codec.Codec, keys map[string]*sdk.KVStoreKey, keepers *KeeperCache, moduleAccPerms map[string][]string) *bankkeeper.BaseKeeper {
	bankK := bankkeeper.NewBaseKeeper(
		appCodec,
		keys[banktypes.StoreKey],
		GetKeeper[authkeeper.AccountKeeper](keepers),
		keepers.getSubspace(banktypes.ModuleName),
		maps.Filter(moduleAccountAddrs(moduleAccPerms), func(addr string, _ bool) bool {
			// we do not rely on internal balance tracking for invariance checks in the nexus module
			// (https://github.com/cosmos/cosmos-sdk/issues/12825 for more details on the purpose of the blocked list),
			// but the nexus module account must be able to send or receive coins to mint/burn them for cross-chain transfers,
			// so we exclude this address from the blocked list
			return addr != authtypes.NewModuleAddress(nexusTypes.ModuleName).String()
		}),
	)
	return &bankK
}

https://github.com/axelarnetwork/axelar-core/blob/main/app/keepers.go#L440
This is how axelarnetwork handle blocked address. They allowed some module address can receive funds

https://github.com/maticnetwork/heimdall/blob/master/bank/handler.go#L25

Since this is code from several years ago. Module address allowed recive funds

[Unique-Divine]

Looks like there's a simpler approach here than what's done on Axelar and Heimdall to factor in k.BlockedAddr(to). If we use the Bank msgServer.Send instead of the direct one on the keeper, it'll take into account blocked addresses.

[stonejiajia]

By the way，I have reported same report on code4rena. auther name is stonejiajia, If my report is classified as invalid, can it be reclassified as valid? The severity level is high.

[Unique-Divine]

By the way，I have reported same report on code4rena. auther name is stonejiajia, If my report is classified as invalid, can it be reclassified as valid? The severity level is high.

Yes, will communicate with the judges and Code4rena. This GitHub issue provides a nice summary for them to review as well.

Edit: Oh, I see why C4's marked it the way they did. The code you've highlighted in IFunToken.bankMsgSend is from outside of the audit scope because it was introduced after the audit in Nibiru#2135.

[stonejiajia]

Edit: Oh, I see why C4's marked it the way they did. The code you've highlighted in IFunToken.bankMsgSend is from outside of the audit scope because it was introduced after the audit in Nibiru#2135.

According to the principle of defense in depth, I believe SendCoins should be modified. SendCoins doesn't validate toAddr. In any case, I still believe my report is valid and reasonable. From this issue, it can be seen that you used SendCoins, which caused harm.

[Unique-Divine]

"I believe SendCoins should be modified. SendCoins doesn't validate toAddr. In any case, I still believe my report is valid and reasonable."

Yes, it's a valid issue. I think Code4rena only removed it from the competition because that code wasn't in the codebase for the audit's commit hash.

I'm still more in favor of using the MsgServer.Send here rather than modifying the NibiruBankKeeper.Send function because changing the latter would globally affect every module that uses it. We need the behavior of the NibiruBankKeeper's send methods to function as closely as possible to the Cosmos-SDK bank keeper.

As for the vulnerability, I'll discuss with the team do a bug bounty

[stonejiajia]

As for the vulnerability, I'll discuss with the team do a bug bounty

Thank you for your understanding , If you do provide bug bounties, my email is [email protected], please use this method to contact me.