Impact of BeforeSend hook on Distribution module - FeePool.CommunityPool distribution

[dalmirel]

Impact on the distribution module was analyzed for the critical places (possible panics), but also for the situations where sending of more denominations in loops could be stopped because sending of certain token factory created denom is frozen - in this issue, when handling the execution of a community pool spend proposal.

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:

Once voting on the CommunityPoolSpendProposal has passed, distribution of funds from the feepool.CommunityPool will be activated.

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

Lines 20 to 23 in 5c9a51c

	err := k.DistributeFromFeePool(ctx, p.Amount, recipient)
	if err != nil {
	return err
	}

Now we are analyzing denominations that could be added to the Community pool:

but in the context of later distributing those funds to the recipient, specified with a proposal:

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

Lines 10 to 30 in 5c9a51c

	func (k Keeper) DistributeFromFeePool(ctx sdk.Context, amount sdk.Coins, receiveAddr sdk.AccAddress) error {
	feePool := k.GetFeePool(ctx)
	// NOTE the community pool isn't a module account, however its coins
	// are held in the distribution module account. Thus the community pool
	// must be reduced separately from the SendCoinsFromModuleToAccount call
	newPool, negative := feePool.CommunityPool.SafeSub(sdk.NewDecCoinsFromCoins(amount...))
	if negative {
	return types.ErrBadDistribution
	}
	feePool.CommunityPool = newPool
	err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, receiveAddr, amount)
	if err != nil {
	return err
	}
	k.SetFeePool(ctx, feePool)
	return nil
	}

Concerns:

• If a community pool holds denominations created with a token factory - we expect the distribution to pass for those denoms which are not blocked for sending by the hook execution. For others, distribution should be skipped.
  The error could be obtained for sending the amount from the distribution module account to the recipient, with SendCoinsFromModuleToAccount.

Facts:

Funding for the community pool could be done from:

• GAMM module - fees in uosmo are charged for creating a pool
• Lockup module - Superfluid module sends only synthetic denoms for slashing.
  In SlashSyntheticLock: SlashingTokensFromLockByID is called on lockup module, and synthetic tokens are sent to CommunityPool.
• Mint module - Mints only uosmo coins, and sends the amount to the community pool
• Pool incentives module - amount of the minted coins are distributed to the community pool, so there is no problem because "uosmo" is configured for minting in the mint module.
• Token factory module - funds community pool with osmo tokens when being charged for creation of token-factory denom

But, there is a possibility to fund the community pool externally... This function is exposed on CLI and REST

cosmos-sdk/x/distribution/types/msg.go

Lines 126 to 131 in 5c9a51c

	func NewMsgFundCommunityPool(amount sdk.Coins, depositor sdk.AccAddress) *MsgFundCommunityPool {
	return &MsgFundCommunityPool{
	Amount: amount,
	Depositor: depositor.String(),
	}
	}

Here, a depositor could send any type of denominations to community pool. We have not seen any validations that could exclude token-factory created denoms.

Conclusion:
Analysis showed that there were no modules that will fund the community pool with denominations other than native osmo.

However, it seems token factory created denoms could be sent to the pool.
If there are situations (that we could have possibly overseen, because of the large osmosis codebase) when the pool is funded with token factory-created denominations, we will not have the desired behavior.

We would need to:

• distribute the amount from the distribution module account (community pool) in a loop
• analyze the error code returned from the SendCoinsFromModuleToAccount, in order to ignore error code for "BeforeSend hook freezing the send" type of error, and to continue the distribution.

[dalmirel]

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

We should make sure that governance proposals will only revert if there is a token factory created denom in the community pool, and this denom needs to be distributed with the proposal.

The current implementation in governance modules' EndBlocker works as expected - there will be no panic in this case, only error logged, and state returned.
If the error is returned upon handler executed for a passed proposal:

cosmos-sdk/x/gov/abci.go

Lines 82 to 86 in 870132d

	} else {
	proposal.Status = types.StatusFailed
	tagValue = types.AttributeValueProposalFailed
	logMsg = fmt.Sprintf("passed, but failed on execution: %s", err)
	}