CosmWasm / CosmWasm/wasmd

Tokens are burnt when smart contract is instantiated

Open
#1,763 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
416
Forks
538
PR merge metrics
No merged PRs in 30d

Description

Hello team,

Recently, you published official article on what was discussed on HackerOne - token burning when smart contract is instantiated: https://medium.com/cosmwasm/dev-note-5-specifying-how-to-handle-funds-when-pruning-a-vesting-account-2090076929e3. Now, we may discuss it publicly.

I find your decision to do nothing, to be very very very bad...

Total supply is extremely important part of the blockchain tocenomy (at least for some chains). Imagine that some chain tokenizes stocks, and there is some number of tokens put on paper by notary and agreed by SEC. That number cannot change! There is no option for permissionlessly changing the total supply by any user of the chain. It's simply out of discussion. It means that none of the modules can decide to burn tokens because it's an easy action to take for the module implementers.

In the article you wrote:

> One reason for keeping the current behavior (burn tokens) by default is that it’s simple and clean, correctly keeping track of the supply.

It's "simple and clean" for you to implement, for some blockchains it's a horrible vulnerability.

You also wrote:

> There are at least two other ways to burn tokens by an attacker, changing the total supply along the way:
> - Send tokens to a contract, which then uses BankMsg::Burn in CosmWasm.
> - Use the [MsgBurn]

It's not an argument. Blockchains requiring special permission to burn tokens will just wrap call to `MsgBurn` to ensure that only the appropriate person is allowed to execute that message.

What you did is not a feature, it's a bug.

To be constructive I present potential solutions we discussed in my team over past month:

## Block contract from being instantiated

Our first approach was to simply block the contract instantiation if the underlying account already exist.
The problem we found is that factory smart contracts (those instantiating other smart contracts) could be completely blocked it they use `Instantiate` method to instantiate the smart contract. Attacker could predict the address of the contract meaning that factory contract is blocked until someone creates new contract using `Instantiate` method (to increment the internal index). But then attack could be repeated.

## Deprecate `Instantiate`

Solution could be to deprecate and remove `Instantiate` method, forcing everyone to use `Instantiate2`. `Instantiate2` requires salt so address cannot be predicted by the attacker. Factory contract should receive the salt as an argument from the user and pass it to `Instantiate2`.

Problem with this approach is that tons of available smart contract could not operate on our chain because everyone uses `Instantiate` method.

## Redirect `Instantiate` to `Instantiate2`

We wanted to address the legacy issue by internally redirecting `Instantiate` calls to `Instantiate2`. To do this we would have to magically generate salt. My idea was to:

- take the hash of previous block
- combine it with the hashes of previous transactions executed in the current block (we would have to track it)
- combine it with the hash of the current transaction

This way we would be able to compute deterministic salt which cannot be predicted by the attacker in advance (before complete block is proposed by the validator).

This is my favourite solution so far, we discuss it internally as a solution to go with.

## Accept the vesting account as an address for smart contract

We also tested what happens if e just allow to instantiate the smart contract on top of the vesting account.
We found that:
- smart contract works
- vesting works

So my question here is: Why did you decide to burn those funds in the first place? I walked through all your related PRs and haven't fund the motivation to do it.

It could even be a feature, that funds might be vested to the smart contract.

# Summary

So far, your decision to do nothing and keep burning as is. Is the worst possible action you could have taken. It introduces very unexpected behaviour which is weird and for some chains is dangerous. The fact that it's "easy" to do doesn't mean it's correct. Please let's discuss proposed solutions and take actions to prevent problems rather than ignoring them.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.