stonejiajia - MustNewDecFromString Causing Critical ABCI Panics in Cosmos Blockchain

[sherlock-admin3]

stonejiajia

Medium

MustNewDecFromString Causing Critical ABCI Panics in Cosmos Blockchain

Security Report: MustNewDecFromString Causing Critical ABCI Panics in Cosmos Blockchain

Executive Summary

A comprehensive security audit using CodeQL static analysis has uncovered a severe vulnerability in our Cosmos blockchain implementation. The MustNewDecFromString function, when used within ABCI (Application Blockchain Interface) methods, poses a medium risk of causing panics that can lead to chain halts. This report details the findings, with a particular focus on how MustNewDecFromString impacts critical consensus methods like BeginBlocker and EndBlocker.

It is crucial to note that panics in ABCI methods are recognized as severe security issues in the Cosmos ecosystem. According to the official Cosmos security documentation (https://github.com/crytic/building-secure-contracts/blob/master/not-so-smart-contracts/cosmos/abci_panic/README.md), such panics can lead to consensus failures and network-wide disruptions. The discovery of MustNewDecFromString as a source of these panics highlights an urgent need for immediate action.

Key Findings

1. The MustNewDecFromString function contains a direct panic call that can be triggered within ABCI methods.
2. Multiple panic-inducing paths in BeginBlocker have been identified, often involving calls to MustNewDecFromString or similar functions.
3. These panics in ABCI methods pose a direct threat to blockchain stability and consensus.

3. Panic in MustNewDecFromString Function

Severity: High

Location: dec.go:94:3 https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/math/dec.go#L91

Description: The MustNewDecFromString function contains a direct panic call, which could be triggered if the input string cannot be parsed into a decimal.

Problematic Code:

func MustNewDecFromString(s string) Dec {
    ret, err := NewDecFromString(s)
    if err != nil {
        panic(err)
    }
    return ret
}

Risk: If this function is called within ABCI methods like BeginBlocker or EndBlocker, it could cause a chain halt if the input string is invalid.

4. Multiple Potential Panic Paths in BeginBlocker

Severity: High

Location: Multiple paths within BeginBlocker function, https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/mint/module/abci.go#L105

Description: CodeQL analysis has revealed multiple paths that could lead to panics in the BeginBlocker function. These paths involve calls to various functions that might trigger panics, including:

• GetParamsBlocksPerMonth
• GetParams
• DefaultParams
• MustNewDecFromString

BeginBlocker code path to MustNewDecFromString

BeginBlocker -> GetParamsBlocksPerMonth ----> GetParams ---------> DefaultParams -------------> MustNewDecFromString

The following is the code path cause panic find by codeql

Problematic Code Snippet:

func BeginBlocker(ctx context.Context, k keeper.Keeper) error {
    // ... [other code]
    params, err := k.Params.Get(ctx)
    if err != nil {
        return err
    }
    // ... [other code]
    blocksPerMonth, err := k.GetParamsBlocksPerMonth(ctx)
    if err != nil {
        return err
    }
    // ... [other potentially panic-inducing calls]
}

Risk: Multiple points in this critical consensus method could lead to panics, potentially causing abrupt chain halts and network-wide disruptions.

Additional Recommendations

In light of these new findings, we recommend the following additional measures:

8. Refactor MustNewDecFromString Function:

   • Replace the panic with an error returning mechanism.
   • Consider renaming the function to indicate it may return an error, e.g., NewDecFromStringWithError.

9. Comprehensive Audit of BeginBlocker:

   • Conduct a line-by-line review of the BeginBlocker function.
   • Identify all potential panic points and refactor to use proper error handling.

10. Implement Panic Recovery in ABCI Methods:

    • Wrap all ABCI method calls (including BeginBlocker and EndBlocker) with a panic recovery mechanism.
    • Log recovered panics and attempt graceful degradation where possible.

11. Review and Refactor Utility Functions:

    • Audit all utility functions called within ABCI methods (e.g., GetParamsBlocksPerMonth, DefaultParams).
    • Ensure these functions use error returns instead of panics for exceptional cases.

12. Enhanced Static Analysis:

    • Expand the use of CodeQL and other static analysis tools to regularly scan for potential panic points.
    • Implement automated checks in the CI/CD pipeline to catch panic-inducing code before it reaches production.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

MustNewDecFromString can cause panic()

[relyt29]

➜  allora-chain git:(dev) find . -name "*.go" ! -name "*test.go" ! -path "./test*" -exec grep "MustNewDecFromString" {} \;
        maxDistance, err := alloraMath.OneDec().Mul(alloraMath.MustNewDecFromString("-1")) // Initialize with an impossible value
            dcoeff := alloraMath.MustNewDecFromString("0.001")
                dcoeff = alloraMath.MustNewDecFromString("-0.001")
    return alloraMath.MustNewDecFromString("0.173286795139986"), nil
    zeroPointOne := alloraMath.MustNewDecFromString("0.1")
    zeroPointFour := alloraMath.MustNewDecFromString("0.4")
    zeroPointFive := alloraMath.MustNewDecFromString("0.5")
        MinTopicWeight:                  alloraMath.MustNewDecFromString("100"),        // total weight for a topic < this => don't run inference solicatation or loss update
        BetaEntropy:                     alloraMath.MustNewDecFromString("0.25"),       // controls resilience of reward payouts against copycat workers
        LearningRate:                    alloraMath.MustNewDecFromString("0.05"),       // speed of gradient descent
        MaxGradientThreshold:            alloraMath.MustNewDecFromString("0.001"),      // gradient descent stops when gradient falls below this
        MinStakeFraction:                alloraMath.MustNewDecFromString("0.5"),        // minimum fraction of stake that should be listened to when setting consensus listening coefficients
        EpsilonReputer:                  alloraMath.MustNewDecFromString("0.01"),       // a small tolerance quantity used to cap reputer scores at infinitesimally close proximities
        TopicRewardStakeImportance:      alloraMath.MustNewDecFromString("0.5"),        // importance of stake in determining rewards for a topic
        TopicRewardFeeRevenueImportance: alloraMath.MustNewDecFromString("0.5"),        // importance of fee revenue in determining rewards for a topic
        TopicRewardAlpha:                alloraMath.MustNewDecFromString("0.5"),        // alpha for topic reward calculation; coupled with blocktime, or how often rewards are calculated
        TaskRewardAlpha:                 alloraMath.MustNewDecFromString("0.1"),        // alpha for task reward calculation used to calculate  ~U_ij, ~V_ik, ~W_im
        ValidatorsVsAlloraPercentReward: alloraMath.MustNewDecFromString("0.25"),       // 25% rewards go to cosmos network validators
        CRewardInference:                alloraMath.MustNewDecFromString("0.75"),       // fiducial value for rewards calculation
        CRewardForecast:                 alloraMath.MustNewDecFromString("0.75"),       // fiducial value for rewards calculation
        CNorm:                           alloraMath.MustNewDecFromString("0.75"),       // fiducial value for inference synthesis
        TopicFeeRevenueDecayRate:        alloraMath.MustNewDecFromString("0.0025"),     // rate at which topic fee revenue decays over time
        MinEffectiveTopicRevenue:        alloraMath.MustNewDecFromString("0.00000001"), // we no stop dripping from the topic's effective revenue when the topic's effective revenue is below this
    if msg.PNorm.Lt(alloraMath.MustNewDecFromString("2.5")) || msg.PNorm.Gt(alloraMath.MustNewDecFromString("4.5")) {
        alloraMath.MustNewDecFromString("2.28"),
        alloraMath.MustNewDecFromString("15.87"),
        alloraMath.MustNewDecFromString("50"),
        alloraMath.MustNewDecFromString("84.13"),
        alloraMath.MustNewDecFromString("97.72"),
    eightPointTwoFive := alloraMath.MustNewDecFromString("8.25")
    v6Point75OverP, err := alloraMath.MustNewDecFromString("6.75").Quo(pNorm)
    v8Point25OverP, err := alloraMath.MustNewDecFromString("8.25").Quo(pNorm)
    v17Point25OverP, err := alloraMath.MustNewDecFromString("17.25").Quo(pNorm)
    if err := k.SetPreviousPercentageRewardToStakedReputers(ctx, alloraMath.MustNewDecFromString("0.3")); err != nil {
// MustNewDecFromString returns a new Dec from a given string. It panics if the string
func MustNewDecFromString(s string) Dec {
    hundred := MustNewDecFromString("100")

Static analysis tools are great, but you still should check the outputs of them, this is a false positive, the panic will never be executed.

[relyt29]

also this is not a duplicate - the issue that seems duplicate to this has to do with catching error returns, we will fix that other issue

[mystery0x]

invalid because report is false positive and also too general/vague