Skip to content

Latest commit

 

History

History
162 lines (105 loc) · 5.81 KB

086.md

File metadata and controls

162 lines (105 loc) · 5.81 KB

Great Aegean Turkey

Medium

Silent failures in SendCoinsToFeeCollector can lead to node crash

Summary

The SendCoinsToFeeCollector function in the mint keeper does not handle errors explicitly when the function is called to send coins to the fee Module account, ignoring potenial errors during the process. Instead, it silently returns errors from SendCoinsFromModuleToModule. Rather than returning an error that can be handled gracefully. The code doesn't have any error handling or recovery mechanism for these potential panics. And when the function is called on the BeginBlocker, the unhandled errors panics on the ABCI.

It is clearly documented in cosmos that: A panic inside an ABCI method (e.g., EndBlocker) will stop the chain. There should be no unanticipated panics in these methods.

https://secure-contracts.com/not-so-smart-contracts/cosmos/abci_panic/index.html

Root Cause

The root cause of this issue is that the SendCoinsToFeeCollector function ignores potential errors. And these errors and silent failures go unnoticed, until they extend up into the BeginBlocker when it tries to mint the block Provision for the current block. And the errors ignored, lead to panic inside the Begin Blocker due to the logic on the function.

func (k Keeper) SendCoinsToFeeCollector(ctx context.Context, coins sdk.Coins) error {
	return k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, k.feeCollectorName, coins)
	// @audit - no check for if err !=nil before returning the k.bankKeeper.SendCoinsFromModuleToModule...
}

As a result of that, When this function is called in the BeginBlocker (specifically in mintBlockProvision), any error returned by SendCoinsToFeeCollector causes a panic, which can crash the node and halt the chain.

// BeginBlocker updates the inflation rate, annual provisions, and then mints
// the block provision for the current block.
func BeginBlocker(ctx context.Context, k keeper.Keeper) {
	defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker)       

	maybeUpdateMinter(ctx, k)              
	mintBlockProvision(ctx, k)  //@@>>> here              
	setPreviousBlockTime(ctx, k)
}

And the func mintBlockProvision() has the logic below.

// mintBlockProvision mints the block provision for the current block.
func mintBlockProvision(ctx context.Context, k keeper.Keeper) {
	minter := k.GetMinter(ctx)
	if minter.PreviousBlockTime == nil {
		// exit early if previous block time is nil
		// this is expected to happen for block height = 1
		return
	}

	sdkCtx := sdk.UnwrapSDKContext(ctx)

	toMintCoin, err := minter.CalculateBlockProvision(sdkCtx.BlockTime(), *minter.PreviousBlockTime)
	if err != nil {
		panic(err)
	}
	toMintCoins := sdk.NewCoins(toMintCoin)

	err = k.MintCoins(ctx, toMintCoins)
	if err != nil {
		panic(err)
	}

	err = k.SendCoinsToFeeCollector(ctx, toMintCoins)       //>>>>> @barz this external fucntion doesn't handle errors, it silently handles errors, so if the error is silently handled
	if err != nil {             // the error would check here, and if its not equal to nil, it panics. halts 
		panic(err)
	}

	if toMintCoin.Amount.IsInt64() {
		defer telemetry.ModuleSetGauge(types.ModuleName, float32(toMintCoin.Amount.Int64()), "minted_tokens")          
	}

	sdkCtx.EventManager().EmitEvent(
		sdk.NewEvent(
			types.EventTypeMint,
			sdk.NewAttribute(types.AttributeKeyInflationRate, minter.InflationRate.String()),
			sdk.NewAttribute(types.AttributeKeyAnnualProvisions, minter.AnnualProvisions.String()),
			sdk.NewAttribute(sdk.AttributeKeyAmount, toMintCoin.Amount.String()),
		),
	)
}

The issue is that SendCoinsToFeeCollector does not wrap or log errors from SendCoinsFromModuleToModule and this leads to silent failures, if a failure occurs during the process.

The mintBlockProvision function calls SendCoinsToFeeCollector and panics if an error is returned. This panic can easily be prevented if error was initially handled on the SendCoinsToFeeCollector function in the x/mint/keeper/keeper.go`

This panic can crash the node and halt the chain, disrupting network operations.

RELEVANT LINKS https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/mint/keeper/keeper.go#L119 https://github.com/sherlock-audit/2024-12-babylon/blob/main/babylon/x/mint/abci.go#L15

Internal Pre-conditions

  • SendCoinsToFeeCollector does not handle or log errors.
  • mintBlockProvision panics on errors from SendCoinsToFeeCollector.
  • Dependency on SendCoinsFromModuleToModule without error handling.
  • BeginBlocker calls mintBlockProvision, which can panic.

External Pre-conditions

  • Insufficient balances in the module account.
  • Invalid or misconfigured module accounts.
  • Network or state corruption.
  • Upgrade or configuration issues affecting the mint or bank module.
  • Edge cases in block provision calculation.

Attack Path

Vulnerability path

  1. SendCoinsToFeeCollector is triggered to send coin to x/auth fee collector module account
  2. There's an insufficient balance to perform the sendCoins operation or an error associated with the operation.
  3. The operations is carried normally without triggering a transaction failure
  4. On the BeginBlocker, mintBlockProvision calls SendCoinsToFeeCollector, and if there's an error (on the beginBlocker) it panics
  5. Due to the error not handled earlier on the SendCoinsToFeeCollector function in the Keeper.go, the error results to a panic in the blocker leading to chain halt.

Impact

  1. Node Crashes because unhandled errors can lead to panics, crashing nodes.
  2. Chain Halts. If multiple nodes panic, the chain may stop producing blocks.

PoC

No response

Mitigation

Consider adding err check in SendCoinsToFeeCollector

Exaample

if err != nil {
    return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "failed to send coins: %s", err.Error())
}