AppChain Modules - EMoney

The RegisterLegacyAminoCodec method in the AppModuleBasic struct of the regulation module is currently empty. This means that the module's types are not being registered with the LegacyAmino codec, which could lead to serialization and deserialization issues for messages and types used in this module.

The empty implementation is as follows:

// RegisterLegacyAminoCodec registers the regulation module's types on the LegacyAmino codec.
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {}

This oversight could potentially cause problems with backwards compatibility, especially if there are other parts of the system or external tools that still rely on Amino encoding.

Implement the RegisterLegacyAminoCodec method to register all relevant types used in the regulation module.

Remediation Plan

SOLVED : The Emoney team solved the issue by registering codec.

The current implementation of the NewHandler function in the regulation module is missing handlers for several message types that are defined in the msgServer struct. Specifically, the handlers for MsgProposeBlock, MsgProposeAdd, MsgVote, and MsgProcessVoting are not included in the handler function. This omission means that these message types cannot be processed by the module, potentially breaking critical functionality related to voting and proposal processes.

The current handler only processes the following message types:

- MsgTransferAuthority

- MsgAllowAddress

- MsgDisallowAddress

However, the msgServer struct implements handlers for additional message types that are not being routed in the NewHandler function.

The missing handlers prevent the module from processing important messages related to proposing blocks, proposing additions, voting, and processing voting results. This severely limits the functionality of the regulation module and could lead to a breakdown of the governance and regulatory processes that depend on these message types.

Update the NewHandler function to include all message types implemented in the msgServer struct. Here's an example of how the updated function should look:

func NewHandler(k keeper.Keeper) sdk.Handler {

    msgServer := keeper.NewMsgServerImpl(k)

    return func(dsadasctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) {

        ctx = ctx.WithEventManager(sdk.NewEventManager())

        switch msg := msg.(type) {

        case *types.MsgTransferAuthority:

            res, err := msgServer.TransferAuthority(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        case *types.MsgAllowAddress:

            res, err := msgServer.AllowAddress(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        case *types.MsgDisallowAddress:

            res, err := msgServer.DisallowAddress(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        case *types.MsgProposeBlock:

            res, err := msgServer.ProposeBlock(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        case *types.MsgProposeAdd:

            res, err := msgServer.ProposeAdd(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        case *types.MsgVote:

            res, err := msgServer.Vote(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        case *types.MsgProcessVoting:

            res, err := msgServer.ProcessVoting(sdk.WrapSDKContext(ctx), msg)

            return sdk.WrapServiceResult(ctx, res, err)

        default:

            return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized regulation message type: %T", msg)

        }

    }

}

Remediation Plan

SOLVED : The Emoney team solved the issue by registering message types.

A thorough review of the regulation module's Keeper implementation reveals several instances of inadequate or missing error handling. These issues can lead to silent failures, unexpected behavior, or potential security vulnerabilities.

Key Issues:

1. Ignored Errors:

Several functions ignore returned errors, potentially leading to incorrect state or execution flow.

2. Panics Instead of Error Returns:

Some functions panic on error conditions instead of returning errors, which can lead to blockchain instability.

3. Inconsistent Error Handling:

Error handling is not consistent across the module, making the code harder to maintain and debug.

4. Missing Error Checks:

Some operations that could potentially fail are not checked for errors.

Detailed Findings:

1. AddressStatus:

- No error handling for potential store access failures.

2. AllowAddress and DisallowAddress:

- Proper error handling implemented.

3. SetAddressStatus:

- No error handling for potential store access failures.

4. TransferAuthority:

- Proper error handling implemented.

5. RemoveAuthority and AddAuthority:

- Uses panic instead of returning errors.

- Example:

     if err != nil {

         panic(err)

     }

6. ProposeVoting:

- Proper error handling implemented.

7. Vote:

- Error from GetVotingById is properly handled.

8. ProcessVoting:

- Error from GetVotingById is properly handled.

- Error from GetVotingThreshold is ignored.

9. GetVotingById:

- Returns error, but uses MustUnmarshal which can panic.

10. GetVotingThreshold:

- Returns error, but it's often ignored in calling functions.

11. ApplyVote:

- Error from GetVotingThreshold is checked but not properly handled.

- Uses MustUnmarshal which can panic.

12. FinalizeBlock and FinalizeAdd:

- Ignore error from GetVotingById.

1. Consistent Error Handling:

Implement a consistent error handling strategy across all functions.

2. Replace Panics with Error Returns:

Update functions like RemoveAuthority and AddAuthority to return errors instead of panicking.

3. Check All Returned Errors:

Ensure all functions that can return errors have their results checked and handled appropriately.

4. Use Unmarshal Instead of MustUnmarshal:

Replace instances of MustUnmarshal with Unmarshal and handle potential errors.

5. Improve Error Context:

Use error wrapping to provide more context when returning errors.

6. Add Logging:

Implement logging for error conditions, especially in cases where execution continues after an error.

Remediation Plan

SOLVED : The Emoney team solved the issue by adding error handlers.

The regulation module in the Scallop Bank chain does not fully adhere to the Cosmos SDK upgrade procedures. Specifically, the module's ConsensusVersion() method is static and does not reflect proper version management as recommended by Cosmos SDK. This could lead to issues during chain upgrades and may prevent proper execution of upgrade handlers.

Key observations:

1. The ConsensusVersion() method returns a hard-coded value of 2:

   func (AppModule) ConsensusVersion() uint64 { return 2 }

2. There's no implementation of upgrade handlers or migration scripts.

3. The module doesn't seem to track or update its version based on changes or upgrades.

Implement upgrade handlers:

- Create upgrade handlers for each version change.

- Ensure these handlers perform necessary state migrations.

Remediation Plan

RISK ACCEPTED: The Emoney team accepted the risk of the issue.

A vulnerability was discovered with the implementation of the HTTP/2 protocol in Golang prior to 1.21.9 and 1.22.2 versions. The current version used in app chain is 1.20.

An attacker can cause the HTTP/2 endpoint to read arbitrary amounts of header data by sending an excessive number of CONTINUATION frames. This cause excessive CPU consumption of the receiver device since there is no sufficient limitation on the amount of frames. Thus, It could be exploited to cause DoS.

Please note that, many HTTP/2 implementations (including Golang) did not properly limit the amount of CONTINUATION frames within a single stream.

References:

https://kb.cert.org/vuls/id/421644

https://www.cve.org/CVERecord?id=CVE-2023-45288

The issue is fixed in both Golang 1.21.9 and 1.22.2. However, If you are intending to use 1.21.X, I would recommend upgrading to 1.21.11 (the latest of 1.21.X) since it has some other security/bug fixes in net/http package.

Remediation Plan

SOLVED : The Emoney team solved the issue by updating golang version.

The spec file intends to outline the common structure for specifications within this directory. The regulation/lock module is missing spec. This documentation is segmented into developer-focused messages and end-user-facing messages. These messages may be shown to the end user (the human) at the time that they will interact with the module.

It is recommended that modules are fully annotated using spec for all available functionalities.

Remediation Plan

RISK ACCEPTED: The Emoney team accepted the risk of the issue.

The Emoney AppChain system lacks comprehensive CosmosSDK simulations and invariants for its modules. More complete use of the simulation feature would make it easier to fuzz test the entire blockchain and help ensure that invariants hold.

Eventually, extend the simulation module to cover all operations that can occur in a real Emoney Chain deployment, along with all possible error states, and run it many times before each release.

Make sure of the following:

- All module operations are included in the simulation module.

- The simulation uses some accounts (e.g., between 5 and 20) to increase the likelihood of an interesting state change.

- The simulation uses the currencies/tokens that will be used in the production network.

- The simulation continues to run when a transaction fails.

- All paths of the transaction code are executed. (Enable code coverage to see how often individual lines are executed.)

Remediation Plan

RISK ACCEPTED: The Emoney team accepted the risk of the issue.

The Dockerfile for the Emoney app chain application is configured to run the container as the root user. This is evident from the use of the '/root' directory as the working directory and the absence of any user-switching commands.

FROM golang:stretch
RUN apt update
RUN apt install ca-certificates jq -y
WORKDIR /root
COPY --from=build-env /go/src/github.com/ScallopBank/chain/build/emoneyd /usr/bin/emoneyd
CMD ["emoneyd"]

Modify the Dockerfile to create and use a non-root user for running the application. Here's an example of how to modify the Dockerfile:

FROM golang:stretch AS build-env

WORKDIR /go/src/github.com/ScallopBank/chain

RUN apt update && apt install git -y

COPY . .

RUN make build

FROM golang:stretch

RUN apt update && apt install ca-certificates jq -y

# Create a non-root user

RUN useradd -m appuser

WORKDIR /home/appuser

# Copy the binary

COPY --from=build-env /go/src/github.com/ScallopBank/chain/build/emoneyd /usr/local/bin/emoneyd

# Change ownership of the binary

RUN chown appuser:appuser /usr/local/bin/emoneyd

# Switch to non-root user

USER appuser

CMD ["emoneyd"]

Remediation Plan

RISK ACCEPTED: The Emoney team accepted the risk of the issue.

The current implementation of the msgServer methods within the keeper package lacks event emissions for various message handling functions. Events are crucial for enabling off-chain services and clients to track and respond to state changes within the blockchain. The absence of these events makes it difficult for external systems to react to important actions such as authority transfers, address allowance/disallowance, proposals, and votes.

To address this issue, it is recommended to emit appropriate events in each method of the msgServer.

Remediation Plan

SOLVED : The Emoney team solved the issue by adding events on the modules.

In the current implementation of the keeper package, the utility function utils.MustAccAddressFromBech32 is used to convert Bech32 encoded addresses to sdk.AccAddress in multiple locations (e.g., SaveVoting, FinalizeBlock, FinalizeAdd, DeleteVoting). The MustAccAddressFromBech32 function can panic if the provided Bech32 address is invalid.

Replace MustAccAddressFromBech32 with AccAddressFromBech32.

Remediation Plan

RISK ACCEPTED: The Emoney team accepted the risk of the issue.

In the provided keeper code for the regulation module, two important constants are hardcoded:

1. MIN_AMOUNT = 20000 * 10

2. VOTING_PERIOD = 86400 * 14 (14 days in seconds)

These values are likely intended to be governance parameters that could be adjusted through governance proposals. Hardcoding these values reduces the flexibility of the system and makes it difficult to adjust these parameters without a code change and chain upgrade.

Impact:

1. Lack of flexibility: Changes to these parameters require code modifications and potentially a chain upgrade.

2. Governance limitations: The community cannot adjust these parameters through standard governance processes.

3. Reduced adaptability: The system cannot easily adapt to changing requirements or market conditions.

1. Convert these hardcoded constants into governance parameters:

   type Params struct {

       MinAmount    sdk.Int

       VotingPeriod time.Duration

   }

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.

The ValidatorCommissionDecorator struct and its associated methods are marked with a TODO comment indicating that they should be removed once the Cosmos SDK is upgraded to v0.46. However, these components are still present in the codebase, despite the project's go.mod file showing that it already depends on Cosmos SDK v0.46.13.

The relevant code snippet is:

// TODO: remove once Cosmos SDK is upgraded to v0.46

// ValidatorCommissionDecorator validates that the validator commission is always

// greater or equal than the min commission rate

type ValidatorCommissionDecorator struct {

    cdc codec.BinaryCodec

}

1. Remove the ValidatorCommissionDecorator struct and all associated methods (NewValidatorCommissionDecorator, AnteHandle, validateAuthz, validateMsg).

Remediation Plan

SOLVED : The Emoney team solved the issue by deleting redundant code.

The current voting mechanism within the regulation module does not provide an option for KYC authorities to abstain from voting. As a result, authorities are forced to vote either "for" or "against" a proposal. This limitation may lead to situations where authorities who prefer to remain neutral or are unsure about a proposal's implications are compelled to make a binary choice, potentially skewing the results.

The absence of an abstain option can lead to biased voting outcomes, as authorities might vote against their true preference just to avoid choosing the opposite option.

To address this issue, it is recommended to introduce an abstain option in the voting mechanism. This will allow KYC authorities to explicitly choose to abstain from voting without influencing the "for" or "against" counts.

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.

In the current implementation of the InitGenesis and ExportGenesis functions, there's a risk of losing historical vote data. The InitGenesis function resets all vote counters to zero when importing genesis state, while the ExportGenesis function only includes votes for active voting. This approach could lead to a loss of important historical data and potentially affect the integrity of the voting system.

Specifically:

1. In InitGenesis:

   voting.VotesForCounter = 0

   voting.VotesAgainstCounter = 0
   

This resets all vote counters, effectively erasing the voting history.

2. In ExportGenesis:

     activeVotings := k.GetActiveVotings(ctx) 

This only exports vote for currently active voting, potentially losing data from completed voting.

This behavior could lead to inconsistencies if the chain needs to be restarted from a genesis state, as all historical voting data would be lost.

Modify the ExportGenesis function to include all votings, not just active ones. This ensures no historical data is lost during export.

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.

The regulation module's keeper package has inconsistencies between the package declaration and the import statements. This suggests that the code may have been copied from a different project (ScallopBank) without proper adaptation to the EMoney-Network repository.

1. Package Declaration:

The file is declared as package keeper, which is correct for its location in the EMoney-Network repository.

2. Import Statements:

Several import statements reference a "ScallopBank" project, which is inconsistent with the EMoney-Network repository:

   "github.com/ScallopBank/chain/utils"

   "github.com/ScallopBank/chain/x/regulation/types"

   emoneyconfig "github.com/ScallopBank/chain/cmd/config"

Correct all import statements to use the appropriate paths for the EMoney-Network project.

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.

The project is using an outdated version of the CometBFT module (github.com/cometbft/cometbft). This older version is affected by multiple security vulnerabilities of varying severity.

Identified Vulnerabilities:

1. ASA-2024-008 (GHSA-hg58-rf2h-6rr7)

- Severity: Moderate

- Description: Instability during blocksync when syncing from malicious peer

- Published: June 27, 2024

2. ASA-2024-004 (GHSA-555p-m4v6-cqxv)

- Severity: Low

- Description: Default configuration param for Evidence may limit window of validity

- Published: February 28, 2024

3. ASA-2024-001 (GHSA-qr8r-m495-7hc4)

- Severity: High

- Description: Validation of VoteExtensionsEnableHeight can cause chain halt

- Published: January 18, 2024

4. ASA-2023-002 (GHSA-hq58-p9mv-338c)

- Severity: Not specified (likely Moderate)

- Description: Default for BlockParams.MaxBytes consensus parameter may increase block times and affect consensus participation

- Published: September 28, 2023

1. Identify the current version of CometBFT in use.

2. Determine the latest stable version of CometBFT that addresses all mentioned vulnerabilities.

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.

The EMoney-Network project is currently using an outdated version of the ibc-go module (v6.1.1) which is vulnerable to a critical security issue (ASA-2024-007). This vulnerability allows potential reentrancy attacks using timeout callbacks in ibc-hooks.

If the EMoney-Network chain satisfies the following conditions, it is vulnerable:

1. IBC-enabled (which it is, given the use of ibc-go).

2. CosmWasm-enabled and allows code uploads for wasm contracts.

3. Utilizes the ibc-hooks middleware and wraps ICS-20 transfer application.

Upgrade the ibc-go module to at least version 6.3.0 or the latest stable version in the v6.x.x series.

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.

The EMoney Appchain project is currently using an outdated version of the Cosmos SDK (v0.46.13). This version is affected by multiple security vulnerabilities of varying severity.

Identified Vulnerabilities:

1. ASA-2024-002 (GHSA-2557-x9mg-76w8)

- Severity: Moderate

- Description: Default PrepareProposalHandler may produce invalid proposals when used with default SenderNonceMempool

- Published: February 20, 2024

2. ASA-2024-003 (GHSA-4j93-fm92-rp4m)

- Severity: Moderate

- Description: Missing BlockedAddressed Validation in Vesting Module

- Published: February 20, 2024

3. ASA-2024-005 (GHSA-86h5-xcpx-cfqc)

- Severity: Low

- Description: Potential slashing evasion during re-delegation

- Published: February 27, 2024

Update the Cosmos SDK to the latest stable version that addresses all these vulnerabilities.

Remediation Plan

ACKNOWLEDGED: The Emoney team acknowledged the issue.