Cosmos Module - Elys Network

In the provided code snippet, there is a potential risk of the chain halting due to a panic that can occur when using the MustAccAddressFromBech32 function. Specifically, in the CollectGasFees function, the following line of code is problematic:

protocolRevenueAddress := sdk.MustAccAddressFromBech32(params.ProtocolRevenueAddress)

The MustAccAddressFromBech32 function is used to convert a Bech32 encoded address string to an account address. However, if the provided Bech32 string is invalid or cannot be properly decoded, this function will panic.

If the params.ProtocolRevenueAddress value is not a valid Bech32 encoded address, it will trigger a panic when passed to MustAccAddressFromBech32. Since this code is executed in the CollectGasFees function, which is likely called during the end block processing or related to fee distribution, a panic at this point can cause the entire chain to halt.

Panics in critical parts of the code, such as those related to block processing or consensus, can lead to a complete stoppage of the chain. All nodes running this code will encounter the panic and fail to proceed, effectively bringing the network to a halt.

To mitigate the risk of a chain halt due to a panic in MustAccAddressFromBech32, it is recommended to use the AccAddressFromBech32 function instead and handle any potential errors gracefully. Here's an example of how to modify the code:

protocolRevenueAddress, err := sdk.AccAddressFromBech32(params.ProtocolRevenueAddress)

if err != nil {

    // Handle the error appropriately, such as logging it and using a default address or skipping the fee distribution

    ctx.Logger().Error("Invalid protocol revenue address", "error", err)

    return

}

Remediation Plan

SOLVED: The Elys Network team solved the issue by using AccAddressFromBech32 as it was recommended.

In the CollectGasFees and CollectDEXRevenue functions of the keeper module, there is a potential issue that could lead to a chain halt if the protocol revenue address is included in the block list.

The CollectGasFees function transfers a portion of the collected gas fees to the protocol revenue address using the following code:

// Send coins to protocol revenue address
if protocolGasFeeCoins.IsAllPositive() {
    protocolRevenueAddress := sdk.MustAccAddressFromBech32(params.ProtocolRevenueAddress)
    err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, authtypes.FeeCollectorName, protocolRevenueAddress, protocolGasFeeCoins)
    if err != nil {
        panic(err)
    }
}

Similarly, the CollectDEXRevenue function transfers a portion of the DEX revenue to the protocol revenue address using the following code:

// Send coins to protocol revenue address
if protocolRevenueCoins.IsAllPositive() {
    protocolRevenueAddress := sdk.MustAccAddressFromBech32(params.ProtocolRevenueAddress)
    err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, protocolRevenueAddress, protocolRevenueCoins)
    if err != nil {
        panic(err)
    }
}

The issue arises when the SendCoinsFromModuleToAccount function is called. This function checks if the recipient address (in this case, the protocol revenue address) is included in the block list using the BlockedAddr function. If the address is blocked, it returns an error wrapped with sdkerrors.ErrUnauthorized.However, in both CollectGasFees and CollectDEXRevenue, if an error occurs during the coin transfer to the protocol revenue address, the code panics using panic(err). This means that if the protocol revenue address is blocked, it will trigger a panic and potentially halt the chain.

To address this issue and prevent a potential chain halt, the following recommendations can be considered:

1. Error Handling:
   - Instead of using panic(err) when an error occurs during the coin transfer to the protocol revenue address, the error should be properly handled and logged.
   - The function can return the error to the caller, allowing it to be propagated and handled at a higher level.

Remediation Plan

SOLVED: The Elys Network team solved the issue by removing the problematic panics.

In the AddExternalIncentive function of the keeper package, there is a potential integer overflow issue when calculating the total amount of incentives to be added. The specific line of concern is:

amount := msg.AmountPerBlock.Mul(sdk.NewInt(int64(msg.ToBlock - msg.FromBlock)))

In this line, the difference between msg.ToBlock and msg.FromBlock is being converted to an int64 value using int64(msg.ToBlock - msg.FromBlock). If the result of msg.ToBlock - msg.FromBlock exceeds the maximum value representable by int64, which is 9,223,372,036,854,775,807, it will lead to an integer overflow.

func (k msgServer) AddExternalIncentive(goCtx context.Context, msg *types.MsgAddExternalIncentive) (*types.MsgAddExternalIncentiveResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)
	sender := sdk.MustAccAddressFromBech32(msg.Sender)

	if msg.FromBlock < uint64(ctx.BlockHeight()) {
		return nil, status.Error(codes.InvalidArgument, "invalid from block")
	}
	if msg.FromBlock >= msg.ToBlock {
		return nil, status.Error(codes.InvalidArgument, "invalid block range")
	}
	if msg.AmountPerBlock.IsZero() {
		return nil, status.Error(codes.InvalidArgument, "invalid amount per block")
	}

	found := false
	params := k.GetParams(ctx)
	for _, rewardDenom := range params.SupportedRewardDenoms {
		if msg.RewardDenom == rewardDenom.Denom {
			found = true
			if msg.AmountPerBlock.Mul(
				sdk.NewInt(int64(msg.ToBlock - msg.FromBlock)),
			).LT(rewardDenom.MinAmount) {
				return nil, status.Error(codes.InvalidArgument, "too small amount")
			}
			break
		}
	}
	if !found {
		return nil, status.Error(codes.InvalidArgument, "invalid reward denom")
	}

	amount := msg.AmountPerBlock.Mul(sdk.NewInt(int64(msg.ToBlock - msg.FromBlock)))
	err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, sdk.Coins{sdk.NewCoin(msg.RewardDenom, amount)})
	if err != nil {
		return nil, err
	}

	k.Keeper.AddExternalIncentive(ctx, types.ExternalIncentive{
		Id:             0,
		RewardDenom:    msg.RewardDenom,
		PoolId:         msg.PoolId,
		FromBlock:      msg.FromBlock,
		ToBlock:        msg.ToBlock,
		AmountPerBlock: msg.AmountPerBlock,
		Apr:            math.LegacyZeroDec(),
	})

	return &types.MsgAddExternalIncentiveResponse{}, nil
}

To address the potential integer overflow issue, it is recommended to use the sdk.Int type provided by the Cosmos SDK for arithmetic operations involving large values. The sdk.Int type is designed to handle arbitrary-precision integers safely and prevents overflow issues.

Remediation Plan

SOLVED: The Elys Network team solved this issue by using int64 directly.

The code snippet delegations := k.Keeper.Keeper.GetDelegatorDelegations(ctx, delAddr, 1024) has a couple of issues related to delegation retrieval and error handling:

1. Hardcoded Limit:
   - The code uses a hardcoded limit of 1024 when retrieving delegations from the keeper.
   - While 1024 is a relatively large number, it assumes that the delegator will never have more than 1024 delegations.
   - If the delegator exceeds this limit, the code will fail to retrieve all the delegations, potentially leading to incomplete reward withdrawal.2. Lack of

1. Error Handling:
      - The code does not handle any errors that may occur during the retrieval of delegations using k.Keeper.Keeper.GetDelegatorDelegations.
      - If an error is returned by the keeper, it is not being captured or handled properly.
      - Ignoring errors can lead to unexpected behavior and make it difficult to diagnose and fix issues.

To address these issues, consider the following recommendations:

1. Use a Dynamic or Configurable Limit:
   - Instead of using a hardcoded limit, consider using a dynamic or configurable limit for retrieving delegations. (math.MaxUint16)
   - The limit can be determined based on factors such as the maximum number of delegations allowed by the system or a configuration parameter.
   - This allows for flexibility and adaptability to different scenarios and future changes.

2. Handle Errors Properly:
   - Check and handle any errors returned by k.Keeper.Keeper.GetDelegatorDelegations.
   - If an error occurs during delegation retrieval, log the error and return an appropriate error response to the caller.
   - Proper error handling helps in diagnosing issues and ensures that the system behaves correctly in case of failures.

Remediation Plan

SOLVED: The Elys Network team solved this issue by iterating delegations.

The code snippet provided contains several instances of misleading error messages and comments that do not accurately reflect the functionality of the code. This can lead to confusion and difficulty in understanding the purpose and behavior of the functions.

In the msgClaimRewards function:

1. The error message "failed to serialize stake" is incorrect. The function is actually serializing the response, not a stake.In the performMsgClaimRewards function:

1. The error message "invalid address" is not descriptive enough. It should specify that the error occurs when parsing the sender address.
2. The error message "failed validating msgMsgDelegate" is incorrect. The function is validating msgMsgClaimRewards, not msgMsgDelegate.
3. The error message "elys redelegation msg" is incorrect. The function is performing a claim rewards operation, not a redelegation.
4. The response message "Redelegation succeed!" is incorrect. It should indicate that the claim rewards operation succeeded. These misleading error messages and comments can make it difficult for developers to understand the purpose and behavior of the code, leading to confusion and potential misinterpretation.

func performMsgClaimRewards(f *masterchefkeeper.Keeper, ctx sdk.Context, contractAddr sdk.AccAddress, msgClaimRewards *types.MsgClaimRewards) (*wasmbindingstypes.RequestResponse, error) {
	if msgClaimRewards == nil {
		return nil, wasmvmtypes.InvalidRequest{Err: "Invalid claim rewards parameter"}
	}

	msgServer := masterchefkeeper.NewMsgServerImpl(*f)
	_, err := sdk.AccAddressFromBech32(msgClaimRewards.Sender)
	if err != nil {
		return nil, errorsmod.Wrap(err, "invalid address")
	}

	msgMsgClaimRewards := &types.MsgClaimRewards{
		Sender:  msgClaimRewards.Sender,
		PoolIds: msgClaimRewards.PoolIds,
	}

	if err := msgMsgClaimRewards.ValidateBasic(); err != nil {
		return nil, errorsmod.Wrap(err, "failed validating msgMsgDelegate")
	}

	_, err = msgServer.ClaimRewards(sdk.WrapSDKContext(ctx), msgMsgClaimRewards) // Discard the response because it's empty
	if err != nil {
		return nil, errorsmod.Wrap(err, "elys redelegation msg")
	}

	resp := &wasmbindingstypes.RequestResponse{
		Code:   paramtypes.RES_OK,
		Result: "Redelegation succeed!",
	}

	return resp, nil
}

To improve the clarity and maintainability of the code, it is recommended to update the error messages and comments to accurately reflect the functionality of the code. Here are the suggested changes:1. In the msgClaimRewards function:
   - Update the error message to: "failed to serialize response"2. In the performMsgClaimRewards function:
   - Update the error message to: "invalid sender address"
   - Update the error message to: "failed validating msgMsgClaimRewards"
   - Update the error message to: "failed to process claim rewards"
   - Update the response message to: "Claim rewards succeeded!"

Remediation Plan

SOLVED: The Elys Network team solved this issue by implementing the suggested recommendation.

The provided code snippet contains the implementation of the message handlers for various messages in the Cosmos SDK module. However, it does not emit any events during the execution of these message handlers. Event emission is a crucial aspect of Cosmos SDK modules as it allows other modules and external clients to react to state changes and perform specific operations based on the emitted events.

Other modules or external clients that rely on events emitted by this module will not be able to respond to state changes appropriately.
It becomes challenging to track and audit the execution of message handlers and state changes within the module.

The lack of event emission can lead to reduced transparency and auditability of the module's operations.

Consider implementing event emission in each message handler to ensure that other modules and external clients can react to state changes appropriately.

Remediation Plan

SOLVED: The Elys Network team solved this issue by implementing the suggested recommendation.

The spec file intends to outline the common structure for specifications within this directory. The masterchef/estaking 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

SOLVED: The Elys Network team solved this issue by adding specs on the module.

The Elys App Chain, which utilizes a vulnerable version of CometBFT as its consensus engine, has been identified to have a default configuration issue. The default values for the EvidenceParams.MaxAgeNumBlocks and EvidenceParams.MaxAgeDuration consensus parameters may not provide sufficient coverage for the entire unbonding period of the chain. This could potentially lead to the premature expiration of evidence and allow for unpunished Byzantine behavior if evidence is discovered outside the defined window.

Affected Component:

CometBFT

Affected Versions:

The specific version of CometBFT used by the Elys App Chain, as mentioned in the go.mod file:

```

github.com/cometbft/cometbft v0.37.4

```

It is recommended that chain ecosystems and their maintainers set the consensus parameters EvidenceParams.MaxAgeNumBlocks and EvidenceParams.MaxAgeDuration.

Remediation Plan

RISK ACCEPTED: The Elys Network team accepted the risk of the issue.

The Elys App Chain, which utilizes CometBFT as its consensus engine, has been identified to have a default configuration for BlockParams.MaxBytes that may be too large for its specific use case. This default value can potentially increase block times and affect consensus participation when fully utilized by chain participants. It is recommended that the Noble App Chain team consider their specific needs and evaluate the impact of having proposed blocks with the maximum allowed block size, especially on bandwidth usage and block latency.

To mitigate the potential impact of this default configuration, it is recommended that the Elys App Chain team take the following step:

1. Evaluate the specific needs and requirements of the Noble App Chain use case and determine an appropriate value for BlockParams.MaxBytes.

Remediation Plan

SOLVED: The Elys Network team solved the issue by adding the following lines:

consensusParams.Block.MaxBytes = NewMaxBytes				app.ConsensusParamsKeeper.ParamsStore.Set(ctx, consensusParams)

The Command Line Interface (CLI) for the module is currently lacking long descriptions, which are available in the Cosmos SDK. Long descriptions provide users with detailed information about the purpose and usage of CLI commands. The absence of these descriptions reduces the overall usability and user experience of the CLI, as users may face difficulty in understanding the functionality and proper usage of various commands.

To address the issue of missing long descriptions in the CLI, it is recommended to:

• Update the CLI to include long descriptions for all commands, following the guidelines and format provided by the Cosmos SDK.

• Ensure that these descriptions are comprehensive and accurately convey the purpose and usage of each command.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.

The ConsensusVersion() function in the AppModule of the estaking module is currently set to return a value of 1.

// ConsensusVersion is a sequence number for state-breaking change of the module. It should be incremented on each consensus-breaking change introduced by the module. To avoid wrong/empty versions, the initial version should be set to 1
func (AppModule) ConsensusVersion() uint64 { return 1 }

Review the estaking module and identify any consensus-breaking changes that have been introduced since the initial version. Consider incrementing it by one.

Remediation Plan

ACKNOWLEDGED: The Elys Network team acknowledged the issue.