BeforeSend Hook impact analysis on the Lockup 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 coins, unlocking matured locks in the lockup module would cause the unlockFromIterator function, i.e. the EndBlocker function, to panic.

EndBlocker: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/lockup/abci.go#L28

unlockFromIterator - unlocks all matured locks and panics if an error occurs: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/lockup/keeper/iterator.go#L196-L212

unlockMaturedLockInternalLogic - send coins back to owner from lockup module: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/lockup/keeper/lock.go#L332-L335

Concerns:

• Native tokens can be locked in the lockup module which could cause the hook in the SendCoinsFromModuleToAccount function to trigger. If an error is returned, it will panic the lockup module in the unlockFromIterator function.

  Facts:

• In case the smart contract is configured so that the lockup module account is blacklisted for sending coins only, lock can be created with native tokens and native tokens can be added to an existing lock, but when the EndBlocker is run, it causes an error in the smart contract because the lockup module account is blacklisted for sending and unlockFromIterator function will panic.
• Creating lock and adding to existing lock in the lockup module can work with native tokens and get an error message from the BeforeSend hook, but not a panic.
• Before slashing the validator in the superfluid module, tokens from synthetic lock are sent to the community pool, but only synthetic OSMO tokens.

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:

This is a situation where there should not be any panic - it would be enough to only log the error instead of: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/lockup/keeper/iterator.go#L206