Beware of Using `inv-check-period` to Launch Nodes

[Hellobloc]

Problem Description

https://github.com/TruStory/truchain/blob/b9271e16769de35b07d0cf77087be51afec25d35/Makefile#L68

The current startup script of this project uses the inv-check-period parameter, which means that the project will crash when invariant checks fail. This poses a significant risk, especially since some native Cosmos modules have invariant checks that can be exploited by malicious actors.

For instance, the ModuleAccountInvariant function ensures that module account coins reflect the sum of the deposit amounts held in the store. A mismatch here can lead to an inconsistent, and utilizing inv-check-period means a node panic upon detection of such inconsistencies.

Relevant Code Example:

Cosmos SDK Example

func ModuleAccountInvariant(keeper *Keeper, bk types.BankKeeper) sdk.Invariant {
    return func(ctx sdk.Context) (string, bool) {
        var expectedDeposits sdk.Coins

        err := keeper.Deposits.Walk(ctx, nil, func(key collections.Pair[uint64, sdk.AccAddress], value v1.Deposit) (stop bool, err error) {
            expectedDeposits = expectedDeposits.Add(value.Amount...)
            return false, nil
        })
        if err != nil {
            panic(err)
        }

        macc := keeper.GetGovernanceAccount(ctx)
        balances := bk.GetAllBalances(ctx, macc.GetAddress())

        // Require that the deposit balances are <= than the x/gov module's total
        // balances. We use the <= operator since external funds can be sent to x/gov
        // module's account and so the balance can be larger.
        broken := !balances.IsAllGTE(expectedDeposits)

        return sdk.FormatInvariant(types.ModuleName, "deposits",
            fmt.Sprintf("\tgov ModuleAccount coins: %s\n\tsum of deposit amounts:  %s\n",
                balances, expectedDeposits)), broken
    }
}

Suggested Solution

Given that the x/crisis functionality has been removed by Cosmos, We recommend discontinuing the use of inv-check-period. Additionally, the code should be updated to remove any constraints that inv-check-period being non-zero.

This will help prevent node crashes due to invariant check failures, thereby improving system stability and mitigating potential security risks.

References

Broken Bookkeeping in Cosmos

Cosmos SDK Remove x/crisis