osmosis-labs/cosmos-sdk

BurnFrom and ForceTransfer impact on Cosmos SDK Core modules

dalmirel opened this issue · 1 comments

dalmirel commented

Main task during the Informal Audit of Full token factory BurnFrom and ForceTransfer feature was to detect the possible area of impact.

Artifacts:

  • osmosis/cosmos sdk fork at commit hash: 6fcd25f
  • Full Token Factory branch holding new features
Context in which audit took place

New feature Facts:

  • BurnFrom, ForceTransfer and MintTo functions are introduced in the "full-powered" Token Factory module
  • Sender of the BurnFrom/ForceTransfer sdk.Msg should be the owner/admin of the denomination
  • These functions enable denom admin:
    • to burn desired amount from any address (until now only from it’s own address) - BurnFrom
    • to send desired amount of coins from/to any address in the system - ForceTransfer
  • MintTo feature enables admin of the token factory-created denom to mint tokens on any account (until now, only to it’s own address). By design, this is not possible for module accounts, which simplifies the impact of introduced functionality.

Expectations:

  • All the modules should be able to continue to work or provide a reasonable error once the burning from or forced transfer affected its behavior in an unexpected way.
  • It is expected that all accounts (both module and user/smart contract accounts) could be burned from addr (BurnFrom) or fromAddr/ToAddr (ForceTransfer).
  • All the modules should work with a global state (no local copies of expected amounts in accounts) in order to have valid balance information in the system and on accounts

Analysis Summary done for:

  • all the possible module accounts that could be set as BurnFrom address, from/to address in ForceTransfer
  • the impact that the introduced feature could have on the system if it is possible to change the amounts on module accounts
  • confirming modules are working with a global state that holds information that the amount/balance and supply of coins has changed due to the execution of new features
  • other structures (community pool and distribution module account, locks for osmosis modules… ) that hold some information about the number of coins expected to be present on other accounts

Potential problematic places

Combination of all places in core modules using send functions and results of full token factory new features execution.
FullTokenFactoryNewFunctions

Concerns:

  • Implicit assumptions that account balances can be changed only with sending coins - will the coins not existing no longer in accounts be an issue?
  • Error handling in the case of smaller amounts of the coins in the accounts - so that sending can not be done.
  • The possibility of sending from/to module accounts is introduced with this functionality - this could lead to module accounts working with token factory created denominations and before send hooks could be triggered in critical places analyzed with Before Send hook analysis.

Facts:

  • The same places are under risk as for the before send hook feature, since those places are using sending functions (transferring funds from one account to another) and those places are under risk if expecting certain amounts.
  • Manipulation with module accounts is risky. The possibility of transferring token factory-created denoms to the following accounts could be an issue:
    • x/auth FeeCollector account: this account should not be able to hold non-base denom coins - possible issues in AllocateTokens
    • osmosis x/txfees NonNativeFeeCollector account: if system has this feetoken registered, those coins will be swaped into osmo, but this account should not receive external funds -AnteHandler should be the entry point.
    • Distribution and Governance module account: assumption that module account holds only base denom
    • Vesting accounts:
      • work with base denoms
      • In the case of Burn From or ForceTransfer: locked coins can not be sent from vesting, since they are locked. For the amount greater than spendable coins - we will panic and delivering transaction will be aborted.

Conclusion:
(made with not entirely knowing business requirements for the introduced features)

  • Module accounts should not be used as a destination in ForceTransfer. If necessary, enable ForceTransfer from any account to TokenFactory module account, and hold those coins here if burning them is not an option. Later, coins could be sent from token factory module account to any account. in that case supply is being preserved and tokens distributed to accounts.
  • The only way for listed module accounts to hold token factory denominations is to transfer them with ForceTransfer. Osmosis works with an assumption that: staking, governance proposals are deposited /fees are collected/ coins are minted in base denom.
  • FeeCollector and NonNativeFeeCollectorName accounts should also be excluded from BurnFrom and ForceTransfer From options.
  • We concluded that MintTo could not introduce issues - since there are no expectations that some accounts should hold the exact amount of coins (that is, it should not be greater!)
dalmirel commented

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