Analysis of BeforeSend hook impact on Osmosis modules

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:

Within this issue will be described the analysis of Before Send hook impact on the Osmosis app, gamm, superfluid and token factory modules.

The potential problem can appear in the gamm module when a user successfully joins the pool. When trying to exit the pool an error in the smart contract can appear which besides smart contract-specified token disables withdrawing tokens of the other denominations which exist in the same pool, but do not cause error. Similar problems can happen when swapping tokens.

ExitPool - checks if the amount of exit coins is correct: https://github.com/osmosis-labs/osmosis/blob/a9d1ad654ec68354c7665bc4088fdb15b91dc9ec/x/gamm/keeper/msg_server.go#L142-L145

applyExitPoolStateChange - sends tokens to the user that exits the pool: https://github.com/osmosis-labs/osmosis/blob/a9d1ad654ec68354c7665bc4088fdb15b91dc9ec/x/gamm/keeper/share.go#L32-L36

Concerns:

User can join the pool but can't exit because the Before Send hook triggers error,
Errors returned from the Send functions, after triggering the Before Send hook are not handled properly or cause panic.

Facts:

Gamm module:

Funding to the community pool is with OSMO tokens, sending from/to module is with pool share tokens (gamm/pool/poolId);
Joining and exiting the pool and swapping tokens can be done with native tokens, but errors are properly propagated - not leading to panic;

Superfluid module:

Several functions in this module call sending coins, delegating and undelegating, but only with OSMO/synthetic OSMO tokens;

Tokenfactory module:

If sending token factory coins leads to an error in the smart contract, the error will be properly propagated;

App:

notBondedTokensToBonded and bondedTokensToNotBonded functions from the staking module are called when exporting application state for genesis file and can cause panic. Those functions work with OSMO tokens so the Before Send hook will not trigger.

Conclusion for BeforeSend hook impact on Osmosis module

For the following modules, there is no negative impact:

Superfluid, Pool-incentives, Mint: modules will not work with native tokens created with Token Factory,
Epochs module is not using a function that holds the BeforeSend trigger

Potential negative impact is explained in separate issues:

osmosis-labs / osmosis

Analysis of BeforeSend hook impact on Osmosis modules #2613

Analysis Summary:

Conclusion for BeforeSend hook impact on Osmosis module