Full token factory Before Send Hook impact on IBC

[dalmirel]

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:

The IBC applications that directly or indirectly call Send functions in the bank module, which implement the Before Send hook, are Transfer (ics-20), and on the other hand, Interchain Accounts (ics-27) only manipulates with accounts using the Auth module.

Potential problematic places:

were analyzed in relay.go

Concerns:

• The packet that is sent in the Transfer application causes panic,

• During packet handling on IBC Transfer application an error that comes from smart contract occurs and it is not handled properly.

  Facts:

• Coin refundation (caused by timeout or an error in acknowledgment, in refundPacketToken)
  • “Sink side case” - coin refundation can lead to panic if coins of certain denom are sent from the transfer module account to the account that initiated the transfer. This is the case when the chain from which the transfer is initiated is labeled as sink so that coin denom surely contains the prefix of some chain (i.e "chainB/denom"). According to that, this won't lead to a problem: ibc-go code
  • “Source side case”: error is handled properly when refunding coins by sending from the escrow account to the sender - here we could be refunding token factory native denoms and hook could be triggered. Error could be misleading: ibc-go code however.
• All the places that have the possibility to send a token created in the Token Factory module are correctly covered in the case of error, for the cases of source and sink chain side in:
  • SendTransfer()
  • onRecvPacket() - for the sink side chain case: Error could be misleading:ibc-go code

Conclusion: Impact on IBC is not an issue from the point of:

• error handling
• possible panicking in IBC
• packages are sent for one type of denomination (sdk.Coin).

However, If it is possible to blacklist the escrow account as blockedFROM address in the smart contract, that would mean that: sending through IBC will work (escrow accounts are ok to receive coins) but, The entire mechanism of refunds will be blocked, for certain denomination(s). So, if sending fails for whatever reason (timeout let's say), refunds will not be possible to the sender.

This behavior should be considered, and:

• prohibit blacklisting escrow account from the smart contract, or:
• document this as expected when sending funds to osmosis via IBC.

[dalmirel]

Tagging audit collaboration team, to review issues as agreed. @ValarDragon @sunnya97

[ljah8]

After analyzing the ForceTransfer/BurnFrom features, we concluded that the impact on IBC is the same as the impact of Before Send hook.