Open ljah8 opened 2 years ago
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
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:
Context in which audit took place
BeforeSend hook Facts:
Expectations:
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: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/incentives/keeper/hooks.go#L42-L45
doDistributionSends - send coins from the module account to various recipients: https://github.com/osmosis-labs/osmosis/blob/f09305e60b5b23fec761ea14446ee33556313e69/x/incentives/keeper/distribute.go#L200-L207
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:
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.