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

TransferBurn Bug in Transfer() #10

Open
Jdkhnjggf opened this issue Jul 13, 2020 · 1 comment
Open

TransferBurn Bug in Transfer() #10

Jdkhnjggf opened this issue Jul 13, 2020 · 1 comment
Labels
bug Something isn't working

Comments

@Jdkhnjggf
Copy link

Describe The Bug
The self-transfer of any accounts lead to an unexpected coinsBurn of the asset handler located at /x/asset/keeper/keeper.go. Specifically, the Transfer() routine is designed to handle the KuTransfMsg message in order to transfer coins. However, the checks on the input message are not thorough. As a result, an accidental KuTransfMsg message which contains the same from and to account could lead to an unexpected coinsBurn behavior, causing asset losses to that user. In the following, we show the related code snippet.

Code Snippets (Optional)

159 func (a AssetKeeper) Transfer(ctx sdk.Context, from, to types.AccountID, amount types.Coins) error {
  		... ...
180		fromCoins, err := a.getCoins(ctx, from)
181		if err != nil {
182			return sdkerrors.Wrap(err, "get from coins")
183		}
184
185		toCoins, err := a.getCoins(ctx, to)
186		if err != nil {
187			return sdkerrors.Wrap(err, "get to coins")
188		}
189
190		coinSubed, hasNeg := fromCoins.SafeSub(amount)
191		if hasNeg {
192			return sdkerrors.Wrap(types.ErrAssetCoinNoEnough, "transfer")
193		}
194
195		if err := a.checkIsCanUseCoins(ctx, from, amount, fromCoins); err != nil {
196			return sdkerrors.Wrap(err, "transfer")
197		}
198
199		if err := a.setCoins(ctx, to, toCoins.Add(amount...)); err != nil {
200			return sdkerrors.Wrap(err, "set to coins")
201		}
202
203		if err := a.setCoins(ctx, from, coinSubed); err != nil {
204			return sdkerrors.Wrap(err, "set from coins")
205		}

Input/Output

  1. Craft a KuTransfMsg: '{"from":"kratos","to":"kratos","amount":[{"denom":"kratos/kts","amount":"100000000"}]}'
  2. Output: None

To Reproduce
Steps to reproduce the behavior:

  1. sudo ./scripts/boot-testnet.sh
  2. sudo ./build/ktscli query asset coins kratos
  3. sudo ./build/ktscli tx asset transfer kratos kratos kratos 100000000kratos/kts --keyring-backend test --chain-id testing --home /testing/cli/ --from kratos
  4. sudo ./build/ktscli query asset coins kratos

Expected Behavior
Returns an error "from account cannot be equal to to account".

Screenshots
self-transfer1
self-transfer2

Desktop (please complete the following information):

  • OS: [macOS High Sierra 10.13.6]

Additional Context (Optional)
None

Contact Information

Email - [email protected]

@cain42 cain42 added the bug Something isn't working label Jul 14, 2020
@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 P3.
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

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

No branches or pull requests

3 participants