Review security: Burn module

[RaulBernal]

Hello Ignite devs & community!

I'm asking for an expert review of this burn module before to put in pre-production:

• https://github.com/BitCannaGlobal/bcna/tree/v047-burn-module/x/burn

It's a simple (but powerful) module that move an amount of coins from the signer to the bank account and then that amount of coins are burned.

Is based in this proof-of-concept made by my partner @atmoner & @locky10loumpi using Ignite:

• https://github.com/atmoner/ignite-burn

Specifically, my request is for an Ignite/Cosmos-SDK expert to review the main functions for security/design bugs.

• https://github.com/BitCannaGlobal/bcna/blob/9f86f1d33393b7d0f1eb8b4679a0f934e2ce19fd/x/burn/keeper/msg_server_burn_coins_action.go#L12
• https://github.com/BitCannaGlobal/bcna/blob/v047-burn-module/x/burn/types/expected_keepers.go
• https://github.com/BitCannaGlobal/bcna/blob/9f86f1d33393b7d0f1eb8b4679a0f934e2ce19fd/app/app.go#L195

[Victor118]

Hi, great work !
It seems that there's no control on denom, I think module should not burn IBC coins !?

[RaulBernal]

IBC burn would be an interesting use case for spam coins and undesirables airdrops but YES, very curious to see it in action, I'm going to make a test to see how relayers catch the messages and send the "feedback" to the native chain.

Thanks for sharing your thoughts 💭

[Victor118]

I think there's no feedback , tokens will be locked forever in the IBC module of native chain

[julienrbrt]

• You may want to deduplicate multiple coins with the same denom
• Instead of loop twice through coins, you could do it once with validity check and positiveness checks for each coins, or should call IsValid and IsAllPositive directly

Basically verify you have the same checks are here: https://github.com/cosmos/cosmos-sdk/blob/31c860c161153e6db37faa6f6ecaaa5b37e8a42f/x/bank/keeper/msg_server.go#L180

Otherwise, sending the coins to the module account and burning it afterwards (due to the burn function being able to burn only token on a module account before v0.51) makes sense to me.

[RaulBernal]

Thanks for your feedback @Victor118 & @julienrbrt

Based on your advice and in order to keep simplicity (as Cosmos-SDK v0.51 will incorporate this feature in the core) and security I am going to make the following changes for the v1 version of the module:

1. Create a parameter adjustable by governance to set the token to be burned by the function. By default it will be ubcna.

This way we remove the possibility of incorporating more than one denomination (including duplicates and IBC tokens because it is not the goal in this first version).

2. Take into consideration the design of the Burn function in SDK v0.51 and make calls to IsValid and IsAllPositive directly.

I'll post here the next draft with the module modifications. Thanks again.

[RaulBernal]

This is the point 1 so far:
https://github.com/BitCannaGlobal/bcna/compare/v047-burn-module...v047-burn-module-add-params?expand=1

[RaulBernal]

This commit shows how to change the list of accepted denoms by only one denom (that should match with the params.BurnDenom . We avoid multiple denoms in favor of the agreed denom.

BitCannaGlobal/bcna@5368c1d

[RaulBernal]

About the 2 point, related with @julienrbrt advices:

• BitCannaGlobal/bcna@4227bdd

• BitCannaGlobal/bcna@905d8f1

• BitCannaGlobal/bcna@6a7d280

• Complete here: https://github.com/BitCannaGlobal/bcna/compare/v047-burn-module...v047-burn-module-add-params?expand=1

[RaulBernal]

BitCannaGlobal/bcna#340