BeforeSend Hook impact analysis on the Distribution module

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.

The distribution module was singled out as the only one of the Cosmos SDK Core modules that could have a negative impact. After the thorough analysis, it seems that problematic places are working only with native osmo tokens. **Here is the analysis, available for further discussion.**

Analysis Summary:

Potential problematic places:

1. Begin Blocker function

Fees are allocated from the fee collector account to the distribution module account within the AllocateTokens:

cosmos-sdk/x/distribution/keeper/allocation.go

Lines 31 to 34 in 5c9a51c

    
           err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, types.ModuleName, feesCollectedInt) 
        
           if err != nil { 
        
           	panic(err) 
        
           }

Concerns:

It was assumed that fees could be collected in different denominations, which means we could have the situation that the module would panic once the allocation of the fees in token factory native denom is frozen in a bank module.

Facts:

Osmosis TxFees module holds the implementation of the functionality that is changing the initial assumption:
DeductFeeDecorator is customized within Osmosis TxFees module, where fees are collected in separate module accounts with DeductFees function, depending on the denomination of the fee.
antehandler is initialized with this deductFeeDecorator.
On the end of each epoch, all collected non-osmo fees are swapped into osmo, and sent to the FeeCollector module account.
AfterEpochEnd hook is implemented

Conclusion:
It seems this place is not at the risk, since allocate tokens will work only with OSMO tokens - it is not possible to trigger the hook for the native osmosis token so we could not get the situation where the FeeCollector account or Distribution account could be blocked within any of the smart contracts implementing the BeforeSend hook.

2. FundCommunityPool function calls

There were two potential "panic" cases that could occur:

Mint module, implementation of the AfterEpochHook
Pool incentives, implementation of AfterDistributeMintedCoin hook

Concerns:
Minted coins could be of the native token type, which could cause the FundCommunityPool function to trigger the hook and return the error, where mint module and pool incentives modules would panic.

Facts:

Mint module only mints OSMO tokens.
In functions: distributeDeveloperRewards and DistributeMintedCoin - Community Pool will be funded with OSMO tokens.
Pool incentives will also allocate OSMO tokens to the community pool, with the implemented hook (see diagrams)

Conclusion:
According to the listed facts - not at the risk.

Conclusion for BeforeSend hook impact on Distribution module

It seems that a new feature has landed in the existing design with no introduction of unexpected behaviour.
However, it is necessary for the distribution module to work only with OSMO tokens in these specific places. If any change regarding this will be introduced, we would get panic.

Comments from our 09/02 sync meeting with @ValarDragon and @sunnya97:
we have discussed the results of this analysis, and it was concluded that it would be valuable to write down information about the modules and denominations they are working with.
It is important to add to osmosis documentation that the Before Send Hook impact was analyzed with assumptions that certain modules can not have Token Factory created denominations to work with in listed critical places.

Any change regarding denominations that could be present in module accounts can introduce issues with the hook impact.

	err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, types.ModuleName, feesCollectedInt)
	if err != nil {
	panic(err)
	}