BeforeSend Hook impact analysis on the Incentives module

[ljah8]

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:

If the smart contract would be written so that some accounts can't send coins but can receive them, distribution of native tokens in the incentives module would cause a panic in AfterEpochEnd.

AfterEpochEnd:

osmosis/x/incentives/keeper/hooks.go

Lines 42 to 45 in f09305e

	_, err := k.Distribute(ctx, distrGauges)
	if err != nil {
	panic(err)
	}

doDistributionSends - send coins from the module account to various recipients:

osmosis/x/incentives/keeper/distribute.go

Lines 200 to 207 in f09305e

	err := k.bk.SendCoinsFromModuleToManyAccounts(
	ctx,
	types.ModuleName,
	distrs.idToDecodedAddr,
	distrs.idToDistrCoins)
	if err != nil {
	return err
	}

Concerns:

• Tokens in gauges, which are distributed, can be native, which could cause the hook in the SendCoinsFromModuleToManyAccounts function to trigger. If an error is returned, it will panic the incentives module in the AfterEpochEnd function.

Facts:

• In case the smart contract is configured so that the incentives module account is blacklisted for sending coins only, the gauge with native coins can be created and rewards added to it without errors, but at the end of the epoch, when the distribution starts, it causes an error in the smart contract because the incentives module account is blacklisted for sending and AfterEpochEnd will panic.
• Creating gauge and adding to gauge in the incentive module can work with native tokens and get an error message from the BeforeSend hook, but not a panic
• There are some places in the incentives module that call send coins in the bank module, but don't trigger the BeforeSend hook: sending synthetic OSMO from the superfluid module, sending pool share tokens from the gamm module and adding distributed minted OSMO coins to gauge.

Conclusion:
It seems that exists a potential risk in this module for a new feature. Smart contract could be written to cause a panic in the module, so it is necessary to consider how to protect it.

[dalmirel]

Comments from our 09/02 sync meeting with @ValarDragon and @sunnya97:

The needed semantic for this problematic place would be to only log the error received upon the triggering of the hook.
Sending should be done in case there are no errors of type != BeforeSend hook error type,
for the denominations that could be sent.

There are two ways of approaching the problematic places - not sure if any approach is nice enough

1. to suppress and ignore errors for the denominations that can not be sent
2. to change the API of the sending to many accounts function to return the list of the errors received for each to be able to analyze the result...