Impact analysis of Before Send Hook on Osmosis wasmbinding

[dalmirel]

The main task during the Informal Audit of Before Send hook design and implementation was to detect the possible area of impact and to see if there are places that could lead to panicking in modules affected the most.

Artifacts:

• osmosis/cosmos sdk fork at commit hash: 6fcd25f
• Full Token Factory branch holding new features

Context in which audit took place

**BeforeSend hook Facts:** - were added in several Send functions on the Bank module - registered only for token factory-created native denoms - Hook implementation in the TokenFactory module would lead us to the execution of a smart contract (logic not important in this phase of auditing) holding the information about whether or not the sending of certain native tokens should be frozen. If the sending should be frozen: sending of the tokens in the bank module will be aborted.

Expectations:

• All the modules should be able to continue to work or provide a reasonable error once the sending is frozen.
• Both module accounts and "plain" accounts could be blocklisted in the smart contract implementing the freeze logic.
• If there are manipulations with several denomination types in modules, and sending one token factory-created denom is frozen, the module should continue to work with the rest of them. No panic, in this case.

Analysis Summary:

osmosis/wasmbinding is containing cosmwasm integration points. Since smart contracts are admins of Token Factory-created native denoms, cosmwasm binding is controlling the minting and burning of those new tokens. Plugins registered within the wasmbinding are: gammkeeper, bankkeeper and tokenfactory keeper. For this impact analysis: bank and token factory function calls are of interest.

Potential problematic places:

Minting and Burning are done through token factory with: PerformMint : https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L113 https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L123 and PerformBurn functions: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L183 https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L192

Currently: minting can be done only for the Msg sender address - NewMsgMint is created Burning can be done from the sender address - NewMsgBurn is created

Concerns: Could we have a situation where for a certain denomination we could have the token factory module account blacklisted, smart contract, or account address that minting to/burning from is blacklisted? Also, certain denom could be frozen for minting/burning - by admin.

Facts:

• Minting and burning is done for sdk.Coin - one type of token factory denomination.

  • Mint: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L123
  • Burn: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L191

• Minting and burning of tokens could be frozen and errors will be returned in:

  • PerformMint, for:
    • minting the tokens on the token factory module account and transferring them to the contract account with: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L130-L133
    • and sending the necessary amount of minted tokens to the recipient account: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L134-L137
  • PerformBurn, when on token factory first coins are sent from the address to token factory module account and then the amount is burned from the module account - with bank.Burn function. https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L198-L202 Here, there is a risk of error when: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/tokenfactory/keeper/bankactions.go#L43-L49

• ValidateBasic() is called from PerformMint and PerformBurn functions for appropriate sdkMsgs

Conclusion

for BeforeSend hook impact:

• Triggering of Before Send Hook does not have any impact on proper functioning of wasmbinding integration with cosmwasm.

Suggestions for changes to adapt code to new full token factory features:

• ValidateBasic() is called from PerformMint and PerformBurn functions for appropriate sdkMsgs. Currently NewMsgMint and NewMsgBurn. ValidateBasic functions in tokenfactory module should for NewMsgMintTo, NewMsgBurnFrom contain checks for mintingTo and burningFrom addresses if not empty. Sender and mintTo/BurnFrom addr could be the same. Ignore - since if mintTo/BurnFrom are not sent, they are being set to sender within token factory.
• It seems that MsgForceTransfer and SetBeforeHook (?) should be registered in wasmbinging - in order to enable the full token factory from sc.
• PerformMint and PerformBurn functions in bindings should be adapted for MintTO and BurnFROM: If BurningFrom any account should be possible, then we don’t need this validation: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/wasmbinding/message_plugin.go#L184-L189

[dalmirel]

Tagging audit collaboration team, to review issues as agreed. @ValarDragon @sunnya97