You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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:
It's "simple and clean" for you to implement, for some blockchains it's a horrible vulnerability.
You also wrote:
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 usingInstantiate
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 useInstantiate2
.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 toInstantiate2
.Problem with this approach is that tons of available smart contract could not operate on our chain because everyone uses
Instantiate
method.Redirect
Instantiate
toInstantiate2
We wanted to address the legacy issue by internally redirecting
Instantiate
calls toInstantiate2
. To do this we would have to magically generate salt. My idea was to: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:
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.
The text was updated successfully, but these errors were encountered: