E Money security record

Repository: - `EmoneyChain` Commit hash: - [`b91404fefb81a9c653d1d508feeff13abcc66332`](https://github.com/EMoney-Network/EMoneyChain/tree/b91404fefb81a9c653d1d508feeff13abcc66332) Files: - `x/regulation/keeper/ante.go` - `x/regulation/keeper/keeper.go` The regulation module implements KYC (Know Your Customer) functionality by authorizing a designated group of members, namely the authorities within the regulation keeper. Only users designated as `Allowed Addresses` are permitted to perform transactions on the blockchain. And the user status can be updated by the authorities via: - AllowAddress - DisallowAddress Any unauthorized attempts will be rejected during the `anteHandler` by both the Cosmos and Ethereum stacks. See the code snippet on the `CosmosRegulationTxDecorator` as below: **`x/regulation/keeper/ante.go`** ```go=19 func (esc CosmosRegulationTxDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { for _, msg := range tx.GetMsgs() { addrs := msg.GetSigners() for _, addr := range addrs { if err = esc.keeper.AssertAllowed(ctx, addr); err != nil { return ctx, err } } } return next(ctx, tx, simulate) } ``` as well as the `EthermintRegulationTxDecorator` as below: **`x/regulation/keeper/ante.go`** ```go=40 func (e EthermintRegulationTxDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { for _, msg := range tx.GetMsgs() { msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx) if !ok { return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "invalid message type %T, expected %T", msg, (*evmtypes.MsgEthereumTx)(nil)) } if err = e.keeper.AssertAllowed(ctx, msgEthTx.GetFrom()); err != nil { return ctx, err } } return next(ctx, tx, simulate) } ``` The regulatory authority is managed by a group of members in a centralized manner, originating from the genesis state on the mainnet launch. The following operations are performed for maintenance: - `TransferAuthority`: Transfers authority between users. - Adding or removing authorities through internal governance: - `ProposeAdd` - `ProposeBlock` - `Vote` This internal governance pattern, which is based on on-chain voting collections, introduces risks related to concurrent operations accessing the voting store within a single block duration. This could lead to race condition problems that are susceptible to indeterminacy, even during the consensus process. Furthermore this internal structure could introduce centralization risks. Any compromise of the authority account can enable a hacker to exploit this authority, either by allowing/disallowing address, transferring authority or proposing malicious voting actions. If a group of hackers accumulates enough authority to reach the designated threshold, they can remove and add any accounts at will, just the below code snippet shown: ```go=586 func (k Keeper) ApplyVote(ctx sdk.Context, kycAuthority sdk.AccAddress, id uint64, vote bool) error { ``` if voting.VotesForCounter >= threshold { if voting.Type == types.VotingType_BLOCK_VOTING { // Finalize voting for blocking authority k.FinalizeBlock(ctx, id) } else { k.FinalizeAdd(ctx, id) } } else if voting.VotesAgainstCounter >= threshold { k.DeleteVoting(ctx, id) } ``` It would potentially dominate the entire blockchain. Unfortunately, there is no effective way to reverse this situation. --- Updated with commit [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f), the KYB module also relies on the authorities in regulation modules to vote for staking related operations.

Repository: - `EmoneyChain` Commit hash: - [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f) Files: - `x/lock/keeper/genesis.go` In Lock module, the `InitGenesis` function is introduced to import the existing locks states, however, the following concerns are raised based on the update: 1. In the updated version, the lock will not be removed when the delegator withdraws the lock tokens. Instead, the lock status will be set as `LockStatus_COINS_WITHDRAWN`. Since during the genesis, the delegator is asked to deposit again, if the lock is already set as `LockStatus_COINS_WITHDRAWN`, the delegator cannot withdraw the token since lock status validation failed: ```go if lock.Status != types.LockStatus_COINS_LOCKED { return sdkerrors.ErrInvalidRequest.Wrapf("coins for this lock have already been withdrawn") } ``` 2. The genesis requires the lock delegator to deposit the token again. However, such a balance may already be reflected in the module account of Lock, especially when the previous status already exists (like upgrade/reboot), which may cause duplicated deduction as well as the invariant broken issue that the total lock tokens mismatch with that of the deposits from the delegators after importing genesis states. ```go err = k.bankKeeper.SendCoinsFromAccountToModule( ctx, delegator, types.ModuleName, sdk.NewCoins(lock.Amount), ) ```

Repository: - `EmoneyChain` Commit hash: - [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f) Files: - `x/kyb/keeper/keeper.go` - `x/lock/keeper/keeper.go` - `x/regulation/keeper/keeper.go` During normal message handling within the `Lock`, `Regulation`, and `KYB` modules, the `cdc.MustUnmarshal` method is utilized for data deserialization, such as below code snippet: ```go k.cdc.MustUnmarshal(bz, voting) ``` The `MustUnmarshal` calls Unmarshal and panics if error is returned, e.g. in amino codec shown as below **`codec/amino.go`** ```go func (cdc *LegacyAmino) MustUnmarshal(bz []byte, ptr interface{}) { err := cdc.Unmarshal(bz, ptr) if err != nil { panic(err) } } ``` While this approach is acceptable during node startup, where encountering a fatal error with the data, such as genesis data, necessitates halting the process. It poses risks during regular message handling in the keeper. Using `cdc.MustUnmarshal` can lead to service instability due to panic situations, even though middleware may allow for recovery. Therefore, it is recommended to employ the standard `cdc.Unmarshal` method alongside appropriate error handling in these scenarios.

Repository: - `EMoneyChain` Commit hash: - [`b91404fefb81a9c653d1d508feeff13abcc66332`](https://github.com/EMoney-Network/EMoneyChain/tree/b91404fefb81a9c653d1d508feeff13abcc66332) Files: - `x/regulation/keeper/keeper.go` In regulation module, the KYC authority update may happen in two ways: 1. Transfer authority 2. Vote for block/add authority However, there are some potential inconsistency in the address status setup in current implementation: 1. While transferring authority - the original authority will be removed from authority list - the new authority will be added to the authority list, and if the new authority not `IsAllowed`, its address status will be added `AddressStatus_STATUS_ALLOWED`. 2. While voting for block/add authority - when block authority - the original authority will be removed from authority list and set status to `AddressStatus_STATUS_BLOCKED` - when add authority - the new authority will be added to the authority list but not set address status to `AddressStatus_STATUS_ALLOWED`

Repository: - `EmoneyChain` Commit hash: - [`b91404fefb81a9c653d1d508feeff13abcc66332`](https://github.com/EMoney-Network/EMoneyChain/tree/b91404fefb81a9c653d1d508feeff13abcc66332) Files: - `cmd/config/config.go` The `DisplayDenom` is set to `emyc`, while the `BaseDenom` is set to `uemyc` in the configuration file during initialization. Literally, the `DisplayDenom` maintains a decimal of 6, corresponding to the representation of the `BaseDenom`, i.e. 1 `emyc` = $$1 \times 10^{6}$$ `uemyc`. **cmd/config/config.go** ```go=49 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, within the `RegisterDenoms`, the decimal is inherited from `Ethermint`, which has a precision of 18. That is actually 1 `emyc` = $$1 \times 10^{18}$$ `uemyc`. The auditing team seeks clarification regarding the decimal of the denominations conversion to eliminate any potential confusion.

Repository: - `EmoneyChain` Commit hash: - [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f) Files: - `x/kyb/keeper/keeper.go` Based on commit [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f) is introduced to add additional validation on addresses with KYB. When removing the address from KYB certified, the corresponding delegator will be forced cashed out in the Lock module, which raises the corresponding concerns on the workflow: - The code currently doesn't check vesting status when updating KYB status. This could cause issues during removal since the `GetDelegatorByValidator` call may fail since the address is not vested and a potential DoS scenario is raised. For example, if the address X passed KYB but is **not** vested in Lock, and authorities want to remove it from KYB now, the transaction would fail since no vesting/delegator information is present. ```go // Get Account User which bound coins for Validator accAddr, err := k.lockKeeper.GetDelegatorByValidator(ctx, valAddr) //@note would return err if not vested if err != nil { return err } ``` - the current implementation in `FinalizeRemove()` directly calls `WithdrawAndReset()` on the lock keeper, which could bypass intended lock periods.

Repository: - `EMoneyChain` Commit hash: - [`b91404fefb81a9c653d1d508feeff13abcc66332`](https://github.com/EMoney-Network/EMoneyChain/tree/b91404fefb81a9c653d1d508feeff13abcc66332) Files: - `x/lock/keeper/keeper.go` The current implementation of the x/lock module in the EMoneyChain codebase has an inconsistency in handling the vested validator status. When a user vests coins for a validator using the Vest function, the validator is marked as vested using the `SetVestedValidator` function. **`x/lock/keeper/keeper.go`** ```go=23 func (k Keeper) Vest(ctx sdk.Context, from sdk.AccAddress, to sdk.ValAddress) error { // ... k.SetLock(ctx, &types.Lock{ Address: from.String(), Amount: p.LockAmount, UnlockTime: ctx.BlockTime().Add(p.LockDuration), }) k.SetVestedValidator(ctx, to) // ... } ``` However, when the user withdraws their locked coins using the `Withdraw` function, the vested validator status is not updated or removed. **`x/lock/keeper/keeper.go`** ```go=52 func (k Keeper) Withdraw(ctx sdk.Context, vester sdk.AccAddress) error { // ... k.RemoveLock(ctx, vester) return nil } ``` Furthermore, the `SetVestedValidator` function only stores the validator address as a key **without** any associated value or expiration time. This lack of expiration time makes it difficult to determine when a validator's vested status should be considered invalid or expired. **`x/lock/keeper/keeper.go`** ```go=118 func (k Keeper) SetVestedValidator(ctx sdk.Context, val sdk.ValAddress) { prefix.NewStore(ctx.KVStore(k.storeKey), types.VestedValidatorsKey).Set(val, []byte{}) } ``` As a result, a validator will be valid forever as long as one vested performed. --- Updated with commit [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f), the new mapping `ValidatorToDelegatorMap` is introduced to present the relationship between validator and delegator, however the concerns about expiry time for validator after unvested still remain. Also, the updated code base introduces the reset functionality with `WithdrawAndReset`, which will be invoked by the KYB module solely, and for the Lock module, there is no way to reset the lock and vest status, which we want to check with the team if this is intended as well.

Repository: - `EmoneyChain` Commit hash: - [`b91404fefb81a9c653d1d508feeff13abcc66332`](https://github.com/EMoney-Network/EMoneyChain/tree/b91404fefb81a9c653d1d508feeff13abcc66332) Files: - `app/sigverify.go` The `EmoneyChain` is a derivative of the Evmos project, integrating an EVM application over the Cosmos SDK. This results in the use of two types of signature algorithms: `secp256k1` and `ed25519`. The `SigVerificationGasConsumer` configures different gas consumption on signature verification parameters for these algorithms, as demonstrated in the code snippet below: **`app/sigverify.go`** ```go=33 func SigVerificationGasConsumer( meter sdk.GasMeter, sig signing.SignatureV2, params authtypes.Params, ) error { pubkey := sig.PubKey switch pubkey := pubkey.(type) { case *ethsecp256k1.PubKey: // Ethereum keys meter.ConsumeGas(secp256k1VerifyCost, "ante verify: eth_secp256k1") return nil case *ed25519.PubKey: // Validator keys meter.ConsumeGas(params.SigVerifyCostED25519, "ante verify: ed25519") return sdkerrors.Wrap(sdkerrors.ErrInvalidPubKey, "ED25519 public keys are unsupported") case multisig.PubKey: // Multisig keys multisignature, ok := sig.Data.(*signing.MultiSignatureData) if !ok { return fmt.Errorf("expected %T, got, %T", &signing.MultiSignatureData{}, sig.Data) } return ConsumeMultisignatureVerificationGas(meter, multisignature, pubkey, params, sig.Sequence) default: return sdkerrors.Wrapf(sdkerrors.ErrInvalidPubKey, "unrecognized/unsupported public key type: %T", pubkey) } } ``` However, the difference in gas consumption between them is significant, with approximately a 40-fold variation: - For `secp256k1`, the gas cost, denoted as `secp256k1VerifyCost`, is set at `21000`; - For `ed25519`, the default parameter is set at only `590`, as the auth params shown below ```shell emoneyd query auth params max_memo_characters: "256" sig_verify_cost_ed25519: "590" sig_verify_cost_secp256k1: "1000" tx_sig_limit: "7" tx_size_cost_per_byte: "10" ```

Repository: - `EMoneyChain` Commit hash: - [`b91404fefb81a9c653d1d508feeff13abcc66332`](https://github.com/EMoney-Network/EMoneyChain/tree/b91404fefb81a9c653d1d508feeff13abcc66332) Files: - `x/regulation/keeper/keeper.go` The `ProcessVoting` is designed to finalize the vote when either `VotesForCounter` or `VotesAgainstCounter` reaches the threshold. However, the corresponding finalization logic exists in the `ApplyVote` function as well, which will be triggered during the Vote. Since the voting threshold may changed during the time, the `ProcessVoting` will be used to finalized, however, the function will always return an error. ```go // Return error by default, meaning that we couldn't manage to process the voting return sdkerrors.ErrInvalidRequest.Wrapf("Voting with %s id can't be processed at the moment", votingId) ``` --- Update on commit [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f), the similar concerns also raised from KYB module.

Repository: - `EmoneyChain` Commit hash: - [`8c0cf7dd5ff999c31e75774eb84c00f807f0425f`](https://github.com/EMoney-Network/EMoneyChain/tree/8c0cf7dd5ff999c31e75774eb84c00f807f0425f) Files: - `x/kyb/keeper/ante.go` In the `KYBDecorator`, certain message types, particularly those related to the validator, are filtered in the `anteHandler` to verify whether they fall under the KYB authority. **`x/kyb/keeper/ante.go`** ```go=21 func (kd KYBDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { for _, msg := range tx.GetMsgs() { var valAddr sdk.ValAddress switch sdk.MsgTypeURL(msg) { case "/cosmos.staking.v1beta1.MsgCreateValidator": valMsg := msg.(*stakingtypes.MsgCreateValidator) valAddr = utils.MustValAddressFromBech32(valMsg.ValidatorAddress) case "/cosmos.staking.v1beta1.MsgEditValidator": valMsg := msg.(*stakingtypes.MsgEditValidator) valAddr = utils.MustValAddressFromBech32(valMsg.ValidatorAddress) case "/cosmos.staking.v1beta1.MsgDelegate": valMsg := msg.(*stakingtypes.MsgDelegate) valAddr = utils.MustValAddressFromBech32(valMsg.ValidatorAddress) case "/cosmos.staking.v1beta1.MsgBeginRedelegate": valMsg := msg.(*stakingtypes.MsgBeginRedelegate) valAddr = utils.MustValAddressFromBech32(valMsg.ValidatorDstAddress) case "/cosmos.staking.v1beta1.MsgCancelUnbondingDelegation": valMsg := msg.(*stakingtypes.MsgCancelUnbondingDelegation) valAddr = utils.MustValAddressFromBech32(valMsg.ValidatorAddress) case "/cosmos.slashing.v1beta1.MsgUnjail": valMsg := msg.(*slashingtypes.MsgUnjail) valAddr = utils.MustValAddressFromBech32(valMsg.ValidatorAddr) default: valAddr = nil } ``` Almost all message types pertaining to the validator are included, even the `MsgUnjail`. However, the `MsgUndelegate` in staking appears to be excluded.