Best Practice for Darwinia Security Review

This document is to describe the security concern developer need to take care of when coding, current it is organized by a list or security review issues

They can be classified into the following categories:

• A. Imbalance and OnUnbalance

• B. Overflow Checking

  • 1. Using checked arithmetics instead of direct arithmetics, e.g. check_add
  • 2. It the requirement need to use checked mul and max value to give a saturated mul operation over supported types, can use Saturating
  • 3. Refer Substrate's Safe Math document: https://substrate.dev/recipes/3-entrees/safemath.html

• C. Unique storage names to pallets

• D. Extrinsics must not cause a panic in runtime logic

  • e.g. Should avoid using unwrap() and assert! etc. in runtime calls, Refer Issue #153

  • Extrinsics must not cause a panic in the runtime logic or else the system becomes vulnerable to attacks where users can trigger computational execution without any punishment.

• E. Check all the side effects caused by Err

  • Issue 177

  • Example: https://github.com/darwinia-network/darwinia/pull/182

  • There are suggestions to prefix the methods that modify chain state. e.g. modification method in staking

  • https://substrate.dev/docs/en/overview/transaction-lifecycle

    • Remember that with Substrate, token transfers are not the only state transitions: any information can be written to a block. execute_block should never panic, and if a call to a module returns Err, it will be finalized and changes to storage will not be reverted. This is because reverting changes would require that each node copy storage at every block.

    • When you are going to modify storage, the best practice is to do every check necessary to ensure that the call will succeed, execute all the storage changes, and finally emit an event so you know that the function has not returned early. For an example, see the transfer (code) function in the balances module - all checks are performed before touching storage.

    • If you are designing your own modules, you have the responsibility to ensure that invalid extrinsics never panic and never modify module state when returning Err. A correctly implemented module may also secure a bond to penalize malicious parties from submitting invalid extrinsics in the first place (preventing DoS attacks).

  • https://substrate.dev/substrate-collectables-workshop/#/1/storing-a-value?id=result

  • Solution: Using utilities::with_transaction_result, sample

• F. unstable output functions:

  • https://github.com/darwinia-network/darwinia/issues/481

• G. Do not use Option in StorageIterator, example

• H. When some dispatch call want to some smart contract, should avoid possibilities of infinite loop, e.g. callback to some user defined contracts. Refer. https://github.com/darwinia-network/darwinia-common/issues/639

References: https://github.com/ParityAsia/wiki-faq/blob/master/audit-checklist.md