M20 security record

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - app/app.go In the initialization of app: ```go=234 // module accounts that are allowed to receive tokens allowedReceivingModAcc = map[string]bool{ distrtypes.ModuleName: true, ``` The `allowedReceivingModAcc` is used to hardcode the module account address allowing receiving coins from the ordinary account address. Here the `DistributionAccount` is set which would break the invariant check against the balance between distribution module and the `FeePoolCommunity`. The chain node would crash due to this invariant broken scenario and if this node is under the continuously attack with full of the crisis broken transactions. It could not even to serve the node for accepting valid transactions at the same time. Furthermore, the invariant is broken when there is a scenario to export the genesis state for hard fork upgrading, it could not work due to this checker would fail forever.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - contracts/ERC20MinterBurnerDecimals.sol In the contract `ERC20MinterBurnerDecimals.sol`, the role `MINTER_ROLE` has the authority over the function `mint()` and the role `BURNER_ROLE` has the authority over the function `burnCoins()`. Any compromise to the `MINTER_ROLE` and `BURNER_ROLE` accounts may allow a hacker to take advantage of this authority and mint/burn tokens at will. - mint any amount of tokens to any account at will. - burn anyone's token at will.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - app/ante/ante.go - app/ante/handler_option.go In `anteHanlder` there is one named `NewGasWantedDecorator` to check the gas limit input by user. In the project, this `NewGasWantedDecorator` is import from the repo `github.com/evmos/ethermint` with version of `v0.20.0-rc4` where no validation for the gasLimit input .i.e `gasWanted` against the block gas limit of the chain. Hence, even the transaction input with gasLimit greater than the block gas limit, it could be accepted into the mempool by passing `checkTx`, though there could be refreshment after the block committed within the underlying `mempool` to remove these transactions. There is a risk associated with dump of malicious transactions together within a block interval. This can cause a disturbance, affecting the normal inclusion of transactions in the block.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - x/vesting/types/msg.go The struct `MsgCreateClawbackVestingAccount` is declared in the tx.pb.go files as below: ```go type MsgCreateClawbackVestingAccount struct { FromAddress string `protobuf:"bytes,1,opt,name=from_address,json=fromAddress,proto3" json:"from_address,omitempty"` ToAddress string `protobuf:"bytes,2,opt,name=to_address,json=toAddress,proto3" json:"to_address,omitempty"` StartTime time.Time `protobuf:"bytes,3,opt,name=start_time,json=startTime,proto3,stdtime" json:"start_time"` LockupPeriods github_com_cosmos_cosmos_sdk_x_auth_vesting_types.Periods `protobuf:"bytes,4,rep,name=lockup_periods,json=lockupPeriods,proto3,castrepeated=github.com/cosmos/cosmos-sdk/x/auth/vesting/types.Periods" json:"lockup_periods"` VestingPeriods github_com_cosmos_cosmos_sdk_x_auth_vesting_types.Periods `protobuf:"bytes,5,rep,name=vesting_periods,json=vestingPeriods,proto3,castrepeated=github.com/cosmos/cosmos-sdk/x/auth/vesting/types.Periods" json:"vesting_periods"` Merge bool `protobuf:"varint,6,opt,name=merge,proto3" json:"merge,omitempty"` } ``` However, the `ValidateBasic` method did not cover all the fields in the struct, the `amount` sub field of the `period` is not validated at all. This would lead to panic or error on the proceeding part of the chain node.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - contracts/ERC20MinterBurnerDecimals.sol In the contract `ERC20MinterBurnerDecimals.sol`, the role `PAUSER_ROLE` has the authority to update the status of the `_paused` and further pause/resume the functionality of the `transferFrom()`, `safeTransferFrom()` and `safeTransferFrom()` functions, which effectively impact token transfers. Any compromise to the private key of the `PAUSER_ROLE` may allow hackers to take advantage of this authority and allow/prevent external user access to token transfer functionalities.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - app/export.go According to the current implementation, the function `prepForZeroHeightGenesis()` retrieves an invalid validator address from the KV store, which may cause the export of the genesis file to failure. The function `prepForZeroHeightGenesis()` is intended to export the genesis state from the chain, which includes getting and exporting the validator information from the KV store. ```go=181 // Iterate through validators by power descending, reset bond heights, and // update bond intra-tx counters. store := ctx.KVStore(app.keys[stakingtypes.StoreKey]) iter := sdk.KVStoreReversePrefixIterator(store, stakingtypes.ValidatorsKey) counter := int16(0) for ; iter.Valid(); iter.Next() { addr := sdk.ValAddress(iter.Key()[1:]) validator, found := app.StakingKeeper.GetValidator(ctx, addr) if !found { panic("expected validator, not found") } validator.UnbondingHeight = 0 if applyAllowedAddrs && !allowedAddrsMap[addr.String()] { validator.Jailed = true } app.StakingKeeper.SetValidator(ctx, validator) counter++ } iter.Close() if _, err := app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx); err != nil { panic(err) } ... ``` In Line 186, the validator address is fetched via `iter.Key()[1:]`, which excludes the `ValidatorsKey`, but it still includes the prefix, the length of the address. This can be demonstrated in the function `GetValidatorKey()` in the cosmos sdk: **cosmos/cosmos-sdk/x/staking/types/keys.go** ```go // GetValidatorKey creates the key for the validator with address // VALUE: staking/Validator func GetValidatorKey(operatorAddr sdk.ValAddress) []byte { return append(ValidatorsKey, address.MustLengthPrefix(operatorAddr)...) } ``` Therefore, the `iter.Key()[1:]` returns the bytes of the length of the address plus the address, and it cannot be correctly converted to the validator address via the function `sdk.ValAddress()`. In this case, it will cause panic because it cannot be found in the staking keeper.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - cmd/config/config.go The file `config.go` defines the default denom and displayed denom. `1 mcoin= 1^18 amcoin` ```go const ( // DisplayDenom defines the denomination displayed to users in client applications. DisplayDenom = "mcoin" // BaseDenom defines to the default denomination used in Evmos (staking, EVM, governance, etc.) BaseDenom = "amcoin" ) //... // RegisterDenoms registers the base and display denominations to the SDK. func RegisterDenoms() { if err := sdk.RegisterDenom(DisplayDenom, sdk.OneDec()); err != nil { panic(err) } if err := sdk.RegisterDenom(BaseDenom, sdk.NewDecWithPrec(1, ethermint.BaseDenomUnit)); err != nil { panic(err) } } ``` However, there are many places using `mcoin` instead of `amcoin` as the `BaseDenom`, which will enlarge the amount 1e18 times. - app/upgrades/v9/upgrades.go - app/upgrades/v9_1/upgrades.go In the upgrade process, 1e18 times of tokens will be returned back to the input account. ```go func ReturnFundsFromCommunityPoolToAccount(ctx sdk.Context, dk distrKeeper.Keeper, account string, amount sdkmath.Int) error { to := sdk.MustAccAddressFromBech32(account) balance := sdk.Coin{ Denom: "mcoin", Amount: amount, } //... } ``` - types/utils.go ```go=22 const ( // MainnetChainID defines the Evmos EIP155 chain ID for mainnet MainnetChainID = "evmos_9001" // TestnetChainID defines the Evmos EIP155 chain ID for testnet TestnetChainID = "evmos_9000" // BaseDenom defines the Evmos mainnet denomination BaseDenom = "mcoin" ) ``` - x/claims/types/params.go ```go=15 var ( // DefaultClaimsDenom is mcoin DefaultClaimsDenom = "mcoin" // DefaultDurationUntilDecay is 1 month = 30.4375 days DefaultDurationUntilDecay = 2629800 * time.Second ``` Moreover, some statements still reference the original base denom from evmos. - ibc/testing/app.go ```go=69 import ( //.. evmos "github.com/evmos/evmos/v10/types" ) func SetupWithGenesisValSet(t *testing.T, valSet *tmtypes.ValidatorSet, genAccs []authtypes.GenesisAccount, chainID string, balances ...banktypes.Balance) ibcgotesting.TestingApp { //... stakingParams.BondDenom = evmos.BaseDenom stakingGenesis := stakingtypes.NewGenesisState(stakingParams, validators, delegations) genesisState[stakingtypes.ModuleName] = app.AppCodec().MustMarshalJSON(stakingGenesis) totalSupply := sdk.NewCoins() for _, b := range balances { // add genesis acc tokens and delegated tokens to total supply totalSupply = totalSupply.Add(b.Coins.Add(sdk.NewCoin(evmos.BaseDenom, bondAmt))...) } // add bonded amount to bonded pool module account balances = append(balances, banktypes.Balance{ Address: authtypes.NewModuleAddress(stakingtypes.BondedPoolName).String(), Coins: sdk.Coins{sdk.NewCoin(evmos.BaseDenom, bondAmt)}, }) ``` - ibc/testing/chain.go ```go import ( //.. evmos "github.com/evmos/evmos/v10/types" ) func NewTestChain(t *testing.T, coord *ibcgotesting.Coordinator, chainID string) *ibcgotesting.TestChain { //... balance := banktypes.Balance{ Address: acc.GetAddress().String(), Coins: sdk.NewCoins(sdk.NewCoin(evmos.BaseDenom, amount)), } //... } ```

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - go.mod Recently, a high-severity issue impacting cosmos-sdk was reported to core developers. This issue impacts chains running on v0.46 and v0.47, and chains on v0.45 may be vulnerable if they backport features of cosmos-sdk modules to their respective forks. Refer to the link [**Cosmos-sdk Security Advisory Barberry**](https://forum.cosmos.network/t/cosmos-sdk-security-advisory-barberry/10825) for details.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - contracts/ERC20MaliciousDelayed.sol Per the comments, the contracts `ERC20MaliciousDelayed` and `ERC20DirectBalanceManipulation` seem to be implementations of evil tokens, what's the usage of these two tokens? The auditing team would like to confirm with the M20 team that it is designed as expected.

Repository: - mcoin-evm Commit hash: - [`557f8d07f7b84fb77002e4d39e7c701ebc1f9416`](https://github.com/metherdev/mcoin-evm/tree/557f8d07f7b84fb77002e4d39e7c701ebc1f9416) Files: - local_node.sh - x/claims/keeper/claim.go The `claims` module is enabled according to the genesis.json file, which allows the users to get airdropped tokens after interacting with the M20 chain. ```json "claims": { "params": { "enable_claims": true, "airdrop_start_time": "2023-08-16T05:18:00Z", "duration_until_decay": "100000s", "duration_of_decay": "1000000s", "claims_denom": "mcoin", "authorized_channels": [ "channel-0", "channel-3" ], "evm_channels": [ "channel-2" ] }, ``` In the file `local_node.sh`, L90 is commented, which means the initial balance of the module account `claims` is zero. ```bash #jq -r --arg amount_to_claim "$amount_to_claim" '.app_state["bank"]["balances"] += [{"address":"pose1zmfvrprhl57jt4h20xdnmempx4d07t5a59rzt5","coins":[{"denom":"amcoin", "amount":$amount_to_claim}]}]' "$GENESIS" >"$TMP_GENESIS" && mv "$TMP_GENESIS" "$GENESIS" ``` Since the module account `claims` is not allowed to receive funds, the token balance of the module account `claims` will be zero forever. Thus, Performing an invocation on `SendCoinsFromModuleToAccount()` to send tokens from the escrowed module account `claims` to the user will always fail. x/claims/keeper/claim.go ```go=44 if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, addr, claimedCoins); err != nil { return sdk.ZeroInt(), err } ``` The error message is as below: > ERR failed to claim Vote action error="0mcoin is smaller than 2500mcoin: insufficient funds" address=mcoin14ws4ncu0gv80yjptt2r4dwz7csx24naty5rv9h module=x/claims The auditing team would like to confirm with the M20 team that if it is designed as expected.