diff --git a/docs/nbc_audit_2020/README.md b/docs/nbc_audit_2020/README.md new file mode 100644 index 000000000..caddd0ee7 --- /dev/null +++ b/docs/nbc_audit_2020/README.md @@ -0,0 +1,41 @@ +# NBC Audit 2020 + +This folder contains the description, tasks and scope of Nimbus audit pre-mainnet launch. + +RFP: +- https://our.status.im/nimbus-eth2-0-security-audit-request-for-proposal + +The audit was done in 3 phases, related branches are: +- https://github.com/status-im/nimbus-eth2/tree/nbc-audit-2020-0 +- https://github.com/status-im/nimbus-eth2/tree/nbc-audit-2020-1 +- https://github.com/status-im/nimbus-eth2/tree/nbc-audit-2020-2 + +The audit involved 3 vendors: +- Consensys Diligence: https://consensys.net/diligence/ +- NCC: https://www.nccgroup.com/ +- Trail of Bits: https://www.trailofbits.com/ + +Outline + +| Module | Repository | Audit round | Category | +| -------------------------------------- | ----------------------------------------------- | ----------- | ----------------------------- | +| Wire encryption | nim-crypto, nim-libp2p, nim-bearssl | Round 1 | Network Core Audit | +| [Ethereum 2 Request/Response protocol](./eth2_spec_core/attestation_processing_and_production.md) | nimbus-eth2, nim-faststreams, nim-serialization | Round 1 | Network Core Audit | +| [Discover Protocol (discv5)](./network_core/discovery_protocol_discv5.md) | nim-eth | Round 1 | Network Core Audit | +| [SSZ - (De)serialization & tree hashing](./network_core/ssz_serialization_and_tree_hashing.md) | nimbus-eth2 | Round 2 | Network Core Audit | +| [Block/attestation signing](./validator_core/block_attestation_signing.md) | nimbus-eth2, nim-blscurve | Round 2 | Validator Core Audit | +| [Peer pool management](./eth2_spec_core/peer_pool_management.md) | nimbus-eth2 | Round 2 | ETH2 Specification Core Audit | +| [Block Synchronization](./eth2_spec_core/block_synchronization.md) | nimbus-eth2 | Round 2 | ETH2 Specification Core Audit | +| [Fork choice logic](./eth2_spec_core/fork_choice_logic.md) | nimbus-eth2 | Round 2 | ETH2 Specification Core Audit | +| [Reward processing](./eth2_spec_core/reward_processing.md) | nimbus-eth2 | Round 2 | ETH2 Specification Core Audit | +| [Eth1 data processing](./eth2_spec_core/eth1_data_processing.md) | nimbus-eth2, nim-web3 | Round 2 | ETH2 Specification Core Audit | +| [Epoch finalisation and justification](./eth2_spec_core/epoch_finalization_and_justification.md) | nimbus-eth2 | Round 2 | ETH2 Specification Core Audit | +| [Signature verification](./eth2_spec_core/signature_verification.md) | nimbus-eth2, nim-blscurve | Round 2 | ETH2 Specification Core Audit | +| [State transition logic](./eth2_spec_core/state_transition_logic.md) | nimbus-eth2 | Round 2 | ETH2 Specification Core Audit | +| [Publish/Subscribe protocol (gossipsub)](./network_core/publish_subscribe_gossipsub.md) | nim-libp2p | Round 4 | Network Core Audit | +| [Command Line Interface (CLI)](./validator_core/command_line_interface_CLI.md) | nimbus-eth2, nim-confutils | Round 3 | Validator Core Audit | +| [RPC API](./validator_core/rpc_api.md) | nimbus-eth2, nim-json-rpc | Round 3 | Validator Core Audit | +| [Accounts management & key storage](./validator_core/account_management_and_key_storage.md) | nimbus-eth2 | Round 3 | Validator Core Audit | +| [Slash-prevention mechanisms](./validator_core/slash_prevention_mechanisms.md) | nimbus-eth2 | Round 3 | Validator Core Audit | +| [Attestation processing and production](./eth2_spec_core/attestation_processing_and_production.md) | nimbus-eth2 | Round 3 | ETH2 Specification Core Audit | +| [Block processing and production](./eth2_spec_core/block_processing_and_production.md) | nimbus-eth2 | Round 3 | ETH2 Specification Core Audit | diff --git a/docs/nbc_audit_2020/eth2_spec_core/attestation_processing_and_production.md b/docs/nbc_audit_2020/eth2_spec_core/attestation_processing_and_production.md new file mode 100644 index 000000000..a04730a80 --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/attestation_processing_and_production.md @@ -0,0 +1,100 @@ +--- +title: "Attestation Processing & production" +code_owner: "(tersec) and Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 3" +category: "Validator Core Audit" +repositories: "nim-beacon-chain" +--- + +Related readings: + +- Honest validator spec: [https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md](https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md) +- Deposit contract spec: [https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/deposit-contract.md](https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/deposit-contract.md) + +## 0. Clarification of Scope + +There is a slight overlap between Trail of Bits tasks and NCC tasks in this round + +Trail of Bits will be in charge of the high-level attestation/block processing and production. + +This relies on BLS signatures and so secret keys which are under NCC scope. + +Concretely: +- NCC scope [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim) +- Trail of Bits scope: + + - [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim) + + - [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim) + +## 1. State after round 2 + +- Crypto.nim and nim-blscurve dependencies were audited by NCC in phase 1: + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/crypto.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/crypto.nim) + +- signatures.nim and dependencies which handles signature and verification were audited as well + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/signatures.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/signatures.nim) + +## 2. Explanation of the application flow + +The main application is the **beacon_node**, it has 2 mode of operations regarding accounts. With accounts managed in an integrated manner or managed in a split manner, something we call the VC/BN split (Validator Client / Beacon Node split). + +In both case, there is a need of a keystore to hold the validators' **signing keys.** + +### 2.1 Reminder on Ethereum secret keys + +As a reminder, the best explanation for the 2 kinds of secret keys (signing keys and **withdrawal keys**) used in Ethereum 2.0 is at [https://blog.ethereum.org/2020/05/21/keys/](https://blog.ethereum.org/2020/05/21/keys/). + +#### Risks + +A Beacon Node only requires the signing key which presents the following risks: someone who get holds of a signing keys can sign blocks and attestations, leading to double-signing and slashing. The slashing penalty is about 1 ETH (from the initial 32 ETH) but you are ejected when you reach below 16 ETH. An attacker cannot get your funds + +### 2.2 Validation in Nimbus + +2.2.1 ***Case: No Validator Client split*** + +**#### This part will be audited by Trail of Bits** + +Every start of a slot (every 12 seconds), "onSlotStart" will be scheduled: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L986-L987](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L986-L987) + +This will call "[handleValidator](https://github.com/status-im/nim-beacon-chain/blob/a84a8ba192e2f4c7bae22470b44b2db12f95ab0f/beacon_chain/beacon_node.nim#L516)Duties": [https://github.com/status-im/nim-beacon-chain/blob/a84a8ba192e2f4c7bae22470b44b2db12f95ab0f/beacon_chain/beacon_node.nim#L516](https://github.com/status-im/nim-beacon-chain/blob/a84a8ba192e2f4c7bae22470b44b2db12f95ab0f/beacon_chain/beacon_node.nim#L516) + +Then we get in the validator_duties.nim file which is the main consumer of the keystore: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L439-L440](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L439-L440) + +In particular when signing attestations: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L146](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L146) + +and blocks: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L295](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L295) + +**#### This part will be audited by NCC** + +The block and attestation signing functions need access to the secret keys and they are isolated in validator_pool.nim [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim) + +The AttachedValidator type is an abstraction over having the secret key in-memory or in a separate process: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node_types.nim#L73-L94](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node_types.nim#L73-L94) + +2.2.2 ***Case: With Validator Client split* + +A nbc-audit-2020-2 will be tagged with the** **validator client split. +**** + +**#### This part will be audited by Trail of Bits** + +When there is a VC/BN split, the Validator Client logic mirrors the main BeaconNode with an onSlotStart [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L102](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L102) + +Note: validator_client.nim is a stand-alone application + +**#### This part will be audited by NCC (in validator_pool.nim)** + +And signing block and attestations calls the same function as the no-split case: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L168](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L168) +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L144](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L144) + +with implementation in [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim) diff --git a/docs/nbc_audit_2020/eth2_spec_core/block_processing_and_production.md b/docs/nbc_audit_2020/eth2_spec_core/block_processing_and_production.md new file mode 100644 index 000000000..e59df9891 --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/block_processing_and_production.md @@ -0,0 +1,9 @@ +--- +title: "Block Processing & production" +code_owner: "(tersec) and Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 3" +category: "Validator Core Audit" +repositories: "nim-beacon-chain" +--- + +See [attestation_processing_and_production.md](./attestation_processing_and_production.md) diff --git a/docs/nbc_audit_2020/eth2_spec_core/block_synchronization.md b/docs/nbc_audit_2020/eth2_spec_core/block_synchronization.md new file mode 100644 index 000000000..e2aeb111a --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/block_synchronization.md @@ -0,0 +1,31 @@ +--- +title: "Block synchronization" +code_owner: "Eugene Kabanov (cheatfate)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain" +--- + +**Note:** Currently there is an issue where ingoing block sync is slow on Medalla testnet and being investigated. There are many changes being made. + +Spec: TODO + +# Ingoing Sync algorithms + +*syncing from remote peers* + +We have 2 kinds of ingoing sync, forward sync and backward sync + +forward sync: + +[https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_manager.nim](https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_protocol.nim) + +backward sync: + +[https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/request_manager.nim](https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/request_manager.nim) + +# Outgoing Sync algorithms + +*remote peers sync from us* + +[https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_protocol.nim](https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_protocol.nim) diff --git a/docs/nbc_audit_2020/eth2_spec_core/epoch_finalization_and_justification.md b/docs/nbc_audit_2020/eth2_spec_core/epoch_finalization_and_justification.md new file mode 100644 index 000000000..a0caeeba3 --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/epoch_finalization_and_justification.md @@ -0,0 +1,19 @@ +--- +title: "Epoch finalization and justification" +code_owner: "Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain" +--- + +Spec: [https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#justification-and-finalization](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#justification-and-finalization) + +Visual depiction: [https://github.com/protolambda/eth2-docs#justification-and-finalization](https://github.com/protolambda/eth2-docs#justification-and-finalization) + +![finalization.png](./finalization.png) + +Implementation: [https://github.com/status-im/nim-beacon-chain/blob/543cbdfc6b9bc16f0331df2797422b7dc4d61eeb/beacon_chain/spec/state_transition_epoch.nim#L102-L240](https://github.com/status-im/nim-beacon-chain/blob/543cbdfc6b9bc16f0331df2797422b7dc4d61eeb/beacon_chain/spec/state_transition_epoch.nim#L102-L240) + +Self-test: [https://github.com/status-im/nim-beacon-chain/blob/master/tests/spec_epoch_processing/test_process_justification_and_finalization.nim](https://github.com/status-im/nim-beacon-chain/blob/master/tests/spec_epoch_processing/test_process_justification_and_finalization.nim) + +EF test vectors: [https://github.com/status-im/nim-beacon-chain/blob/543cbdfc/tests/official/test_fixture_state_transition_epoch.nim#L57-L58](https://github.com/status-im/nim-beacon-chain/blob/543cbdfc6b9bc16f0331df2797422b7dc4d61eeb/tests/official/test_fixture_state_transition_epoch.nim#L57-L58) diff --git a/docs/nbc_audit_2020/eth2_spec_core/eth1_data_processing.md b/docs/nbc_audit_2020/eth2_spec_core/eth1_data_processing.md new file mode 100644 index 000000000..4e59c127f --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/eth1_data_processing.md @@ -0,0 +1,26 @@ +--- +title: "ETH1 data processing" +code_owner: "" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain, nim-web3" +--- + +ETH2 validators are added by locking ETH on a designated ETH1 contract. + +Monitoring the ETH1 deposit contract is done via the mainchain_monitor service +which connects to a ETH1 Web3 provider (Geth or Infura): +- [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/mainchain_monitor.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/mainchain_monitor.nim) +- Spec of the deposit contract (for reference): [https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/deposit-contract.md](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/deposit-contract.md) + +nim-web3 scope is significantly larger (and unimplemented) that the audit of nim-beacon-chain. The subset in scope is the one used by mainchain_monitor namely: + +- The types Web3, Sender, Subscription + +- The procedures + + - eth_getBlockByHash, eth_getBlockByNumber + +Processing the new entrants is done in the following state_transition function: +- [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition_block.nim#L125-L130](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition_block.nim#L125-L130) +- Specced at [https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#eth1-data](https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#eth1-data) diff --git a/docs/nbc_audit_2020/eth2_spec_core/finalization.png b/docs/nbc_audit_2020/eth2_spec_core/finalization.png new file mode 100644 index 000000000..ae5ba6336 Binary files /dev/null and b/docs/nbc_audit_2020/eth2_spec_core/finalization.png differ diff --git a/docs/nbc_audit_2020/eth2_spec_core/fork_choice_logic.md b/docs/nbc_audit_2020/eth2_spec_core/fork_choice_logic.md new file mode 100644 index 000000000..aad6862e0 --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/fork_choice_logic.md @@ -0,0 +1,35 @@ +--- +title: "Fork choice logic" +code_owner: "Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain" +--- + +Fork choice backend: +- [https://github.com/status-im/nim-beacon-chain/tree/devel/beacon_chain/fork_choice](https://github.com/status-im/nim-beacon-chain/tree/devel/beacon_chain/fork_choice) + +Fork choice is provided by the "attestation_pool" when "select_head" is called + +- [https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/attestation_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/attestation_pool.nim) + +Tests: +- [https://github.com/status-im/nim-beacon-chain/tree/master/tests/fork_choice](https://github.com/status-im/nim-beacon-chain/tree/master/tests/fork_choice) + +- [https://github.com/status-im/nim-beacon-chain/blob/master/tests/test_attestation_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/master/tests/test_attestation_pool.nim) + +Specs: +- [https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/fork-choice.md](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/fork-choice.md) + +- Explainer from Prysmatic: [https://hackmd.io/bABJiht3Q9SyV3Ga4FT9lQ?view](https://hackmd.io/bABJiht3Q9SyV3Ga4FT9lQ?view) + +Paper: +- Combining GHOST and Casper + Vitalik Buterin, Diego Hernandez, Thor Kamphefner, Khiem Pham, Zhi Qiao, Danny Ryan, Juhyeok Sin, Ying Wang, Yan X Zhang + [https://arxiv.org/abs/2003.03052](https://arxiv.org/abs/2003.03052) + +(Short) Implementations + +- [https://github.com/ethereum/research/blob/master/ghost/ghost.py](https://github.com/ethereum/research/blob/master/ghost/ghost.py) +- Original Proto-array (in Go): [https://github.com/protolambda/lmd-ghost/blob/master/eth2/fork_choice/choices/proto_array/proto_array.go](https://github.com/protolambda/lmd-ghost/blob/master/eth2/fork_choice/choices/proto_array/proto_array.go) +- Proto-array incorporating Lighthouse updates: [https://github.com/protolambda/eth2-py-hacks/blob/master/proto_array.py](https://github.com/protolambda/eth2-py-hacks/blob/master/proto_array.py) diff --git a/docs/nbc_audit_2020/eth2_spec_core/peer_pool_management.md b/docs/nbc_audit_2020/eth2_spec_core/peer_pool_management.md new file mode 100644 index 000000000..7e10ccf8d --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/peer_pool_management.md @@ -0,0 +1,19 @@ +--- +title: "Peer pool management" +code_owner: "Eugene Kabanov (cheatfate)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain" +--- + +Note: no spec + +location: + +- [https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/peer_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/peer_pool.nim) + +Created Issues from round 1: + +- [https://github.com/status-im/nim-beacon-chain/issues/1365](https://github.com/status-im/nim-beacon-chain/issues/1365) +- [https://github.com/status-im/nim-beacon-chain/issues/1364](https://github.com/status-im/nim-beacon-chain/issues/1364) +- [https://github.com/status-im/nim-beacon-chain/issues/1362](https://github.com/status-im/nim-beacon-chain/issues/1362) diff --git a/docs/nbc_audit_2020/eth2_spec_core/reward_processing.md b/docs/nbc_audit_2020/eth2_spec_core/reward_processing.md new file mode 100644 index 000000000..09de9519c --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/reward_processing.md @@ -0,0 +1,36 @@ +--- +title: "Reward processing" +code_owner: "(tersec)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain" +--- + +Spec: [https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md](https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md) +Code: [https://github.com/status-im/nim-beacon-chain/blob/bc6eefe3/beacon_chain/spec/state_transition_epoch.nim#L242-L440](https://github.com/status-im/nim-beacon-chain/blob/bc6eefe31ec60a7a1740b444e5c132fa55b4e859/beacon_chain/spec/state_transition_epoch.nim#L242-L440) + +Test: Missing ! For a long-time rewards and penalties tests were missing and they have not been integrated in NBC [https://github.com/status-im/nim-beacon-chain/blame/bc6eefe3/tests/official/test_fixture_state_transition_epoch.nim#L60-L63](https://github.com/status-im/nim-beacon-chain/blame/bc6eefe31ec60a7a1740b444e5c132fa55b4e859/tests/official/test_fixture_state_transition_epoch.nim#L60-L63) + +Fuzzing: Missing ! ([https://github.com/sigp/beacon-fuzz](https://github.com/sigp/beacon-fuzz)) + +Note: an unclear rewards/penalties spec led to non-finalization in the very first testnet Schlesi. + +Improvement in the spec test suite: [https://github.com/ethereum/eth2.0-specs/pull/1747](https://github.com/ethereum/eth2.0-specs/pull/1747) + +Writeup: + +*However, around 11 AM UTC on May 15th , it had been observed that Schlesi had +been nearly 50 epochs away from finality. While the initial cause of the finality delay has not been ascertained, they’re not rare in a test +network where participants are not obligated/incentivized to keep their +validators running. The lack of finality eventually lead to instability +in the chain, causing several temporary forks in the network.* + +![schlesi_rewards.png](./schlesi_rewards.png) + +*The block tree at the time of no finality.* + +*After almost 300 epochs of no finality, there was a block that split the +chain between Lighthouse and Prysm at slot 150496. A block was submitted that had an attester slashing, which surfaced differences in how all +clients tally rewards and penalties for slashed validators. This +revealed 2 different penalty calculation bugs in Prysm! After this chain split, it was decided that the best course of action would be to +restart with a fresh slate.* diff --git a/docs/nbc_audit_2020/eth2_spec_core/schlesi_rewards.png b/docs/nbc_audit_2020/eth2_spec_core/schlesi_rewards.png new file mode 100644 index 000000000..e046cdaae Binary files /dev/null and b/docs/nbc_audit_2020/eth2_spec_core/schlesi_rewards.png differ diff --git a/docs/nbc_audit_2020/eth2_spec_core/signature_verification.md b/docs/nbc_audit_2020/eth2_spec_core/signature_verification.md new file mode 100644 index 000000000..fd1fda717 --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/signature_verification.md @@ -0,0 +1,25 @@ +--- +title: "Signature verification" +code_owner: "Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain, nim-blscurve" +--- + +# Description + +Verify that cryptographic primitives (Miracl and BLST) are properly used at a high-level. +We assume that they are correctly implemented in the backend. + +Specs: +- [https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#bls-signatures](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#bls-signatures) + +For information, evaluation of backends: [https://notes.status.im/nim-bls-curve-backends](https://notes.status.im/nim-bls-curve-backends) + +# Links + +Links to the repositories and more information + +- [https://github.com/status-im/nim-blscurve](https://github.com/status-im/nim-blscurve) (low-level wrapper over crypto primitives, 2 backends) +- [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/crypto.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/crypto.nim) (high-level BLS signature types and serialization) +- [https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/spec/signatures.nim](https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/spec/signatures.nim) (abstraction over signing and verification of consensus objects) diff --git a/docs/nbc_audit_2020/eth2_spec_core/state_transition.png b/docs/nbc_audit_2020/eth2_spec_core/state_transition.png new file mode 100644 index 000000000..956f67a79 Binary files /dev/null and b/docs/nbc_audit_2020/eth2_spec_core/state_transition.png differ diff --git a/docs/nbc_audit_2020/eth2_spec_core/state_transition_logic.md b/docs/nbc_audit_2020/eth2_spec_core/state_transition_logic.md new file mode 100644 index 000000000..413479ef5 --- /dev/null +++ b/docs/nbc_audit_2020/eth2_spec_core/state_transition_logic.md @@ -0,0 +1,37 @@ +--- +title: "State transition logic" +code_owner: "(tersec)" +round: "Audit round 2" +category: "ETH2 Specification Core Audit" +repositories: "nim-beacon-chain" +--- + +Spec: [https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#beacon-chain-state-transition-function](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#beacon-chain-state-transition-function) + +Annotated spec from Vitalik: [https://github.com/ethereum/annotated-spec/blob/master/phase0/beacon-chain.md](https://github.com/ethereum/annotated-spec/blob/master/phase0/beacon-chain.md) +Annotated spec from Ben (from Consensys): [https://benjaminion.xyz/eth2-annotated-spec/phase0/beacon-chain](https://benjaminion.xyz/eth2-annotated-spec/phase0/beacon-chain) + +Visual depiction: [https://github.com/protolambda/eth2-docs#phase-0-beaconstate-transition](https://github.com/protolambda/eth2-docs#phase-0-beaconstate-transition) + +![state_transition.png](./state_transition.png) + +Implementation: +- block: [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition_block.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition.nim) +- epoch: [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition_epoch.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition.nim) +- High-level API: [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition.nim) and [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition_helpers.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/state_transition.nim) +Implementation follows the spec closely with an additional caching mechanism that avoids recomputing epoch invariants. + +Tests: +- Ethereum Foundation vectors are loaded here: [https://github.com/status-im/nim-beacon-chain/tree/master/tests/official](https://github.com/status-im/nim-beacon-chain/tree/master/tests/official) +- Self-tests were added for attestations, deposits and justification/finalization + - [https://github.com/status-im/nim-beacon-chain/tree/master/tests/spec_block_processing](https://github.com/status-im/nim-beacon-chain/tree/master/tests/spec_block_processing) + - [https://github.com/status-im/nim-beacon-chain/tree/master/tests/spec_epoch_processing](https://github.com/status-im/nim-beacon-chain/tree/master/tests/spec_epoch_processing) + +Fuzzing via [https://github.com/sigp/beacon-fuzz](https://github.com/sigp/beacon-fuzz) covers: +- attestation +- attester_slashing +- state transition for block +- deposit +- proposer slashing +- shuffling +- voluntary exit diff --git a/docs/nbc_audit_2020/network_core/discovery_protocol_discv5.md b/docs/nbc_audit_2020/network_core/discovery_protocol_discv5.md new file mode 100644 index 000000000..04b2f2d20 --- /dev/null +++ b/docs/nbc_audit_2020/network_core/discovery_protocol_discv5.md @@ -0,0 +1,18 @@ +--- +title: "Discovery Protocol (discv5)" +code_owner: "" +round: "Audit round 1" +category: "Network Core Audit" +repositories: "nim-eth" +--- + +## Discovery “discv5” + +- Protocol specification: [https://github.com/ethereum/devp2p/tree/master/discv5](https://github.com/ethereum/devp2p/tree/master/discv5) +- Note: the wire format might change ([https://github.com/ethereum/devp2p/issues/152](https://github.com/ethereum/devp2p/issues/152)) +- Part of `nim-eth` repository that holds the discovery v5 code: [https://github.com/status-im/nim-eth/tree/master/eth/p2p/discoveryv5](https://github.com/status-im/nim-eth/tree/master/eth/p2p/discoveryv5) +- In scope: + - All of `discoveryv5` directory + - All other modules in the `nim-eth` repository that the discoveryv5 code depends on, namely: `keys`, `rlp`, `async_utils`. It currently is also depending on and `trie/db` but as this is to be replaced, it can be considered out-of-scope. +- For additional information see also the [readme](https://github.com/status-im/nim-eth/blob/master/doc/discv5.md). +- [Open issues](https://github.com/status-im/nim-eth/issues?q=is%3Aissue+is%3Aopen+label%3Adiscoveryv5) and those that are likely to have [security implications](https://github.com/status-im/nim-eth/issues?q=is%3Aissue+is%3Aopen+label%3Adiscoveryv5+label%3Asecurity). diff --git a/docs/nbc_audit_2020/network_core/eth2_req_resp_protocol.md b/docs/nbc_audit_2020/network_core/eth2_req_resp_protocol.md new file mode 100644 index 000000000..0ad803a7a --- /dev/null +++ b/docs/nbc_audit_2020/network_core/eth2_req_resp_protocol.md @@ -0,0 +1,31 @@ +--- +title: "Ethereum 2 Request/Response protocol" +code_owner: "Zahary Karadjov (zah)" +round: "Audit round 1" +category: "Network Core Audit" +repositories: "nim-beacon-chain, nim-faststreams, nim-serialization" +--- + +The Eth2 Requests/Response protocols are specified here: +https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/p2p-interface.md + +Within nim-beacon-chain, we have a single module implementing the spec: +https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/eth2_network.nim + +The implementation sits on top of our LibP2P library: +https://github.com/status-im/nim-libp2p + +At the moment, Ethereum 2 defines a single high-level protocol based on the spec (the Beacon chain syncing protocol). Our implementation of this protocol can be found here: +https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_protocol.nim +https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_manager.nim + +Certain parts of the protocol implementation are procedurally generated through Nim's compile-time code generation features (i.e. macros). This is similar to using a RPC framework such as Apache Thrift or gRPC and the primary motivation is avoiding the manual writing of tedious and error-prone message serialization code. As a convenience, the build system provides an always up-to-date snapshot of the generated code here: +https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/sync_protocol.nim.generated.nim + +The generated code should be easy to read and I recommend focusing the review on it, but if studying the generator is also of interest, it can be found here: +https://github.com/status-im/nim-eth/blob/master/eth/p2p/p2p_protocol_dsl.nim + +Further explanations and more detailed rationale can be found here: +https://github.com/status-im/nim-beacon-chain/wiki/The-macro-skeptics-guide-to-the-p2pProtocol-macro + +Network flow outline: https://github.com/status-im/nim-beacon-chain/wiki/networking-flow diff --git a/docs/nbc_audit_2020/network_core/publish_subscribe_gossipsub.md b/docs/nbc_audit_2020/network_core/publish_subscribe_gossipsub.md new file mode 100644 index 000000000..75f4f5c3b --- /dev/null +++ b/docs/nbc_audit_2020/network_core/publish_subscribe_gossipsub.md @@ -0,0 +1,44 @@ +--- +title: "Publish/Subscribe protocol (gossipsub)" +code_owner: "Giovanni Petrantoni (sinkingsugar)" +round: "Audit round 4" +category: "Network Core Audit" +repositories: "nim-libp2p" +--- + +Seems that gossipsub might not be ready for Round 1 : libp2p/gossipsub1.1 + +Base spec here: [https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.0.md](https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.0.md) + +Gossip 1.1 here: [https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md](https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md) + +Published paper: [https://arxiv.org/pdf/2007.02754.pdf](https://arxiv.org/pdf/2007.02754.pdf) + +Gossip 1.1 is basically sitting on top of 1.0 so both specs knowledge is kind of necessary. + +### Code hot paths and major importance points + +Publishing: + +[https://github.com/status-im/nim-libp2p/blob/d75cc6edcaf1ebcb422d3f42633f58cde6a62592/libp2p/protocols/pubsub/gossipsub.nim#L1153](https://github.com/status-im/nim-libp2p/blob/d75cc6edcaf1ebcb422d3f42633f58cde6a62592/libp2p/protocols/pubsub/gossipsub.nim#L1153) + +RPC management: (within that we have validation, relevant for NBC) + +[https://github.com/status-im/nim-libp2p/blob/d75cc6edcaf1ebcb422d3f42633f58cde6a62592/libp2p/protocols/pubsub/gossipsub.nim#L1024](https://github.com/status-im/nim-libp2p/blob/d75cc6edcaf1ebcb422d3f42633f58cde6a62592/libp2p/protocols/pubsub/gossipsub.nim#L1024) + +Maintenance heartbeat: + +[https://github.com/status-im/nim-libp2p/blob/d75cc6edcaf1ebcb422d3f42633f58cde6a62592/libp2p/protocols/pubsub/gossipsub.nim#L737](https://github.com/status-im/nim-libp2p/blob/d75cc6edcaf1ebcb422d3f42633f58cde6a62592/libp2p/protocols/pubsub/gossipsub.nim#L737) + +---------------------------------------------------------------- + +here's a list of issues files for this workpackage: + +https://github.com/status-im/nimbus-eth2/issues/1885 +https://github.com/status-im/nimbus-eth2/issues/1878 +https://github.com/status-im/nimbus-eth2/issues/1877 +https://github.com/status-im/nimbus-eth2/issues/1876 +https://github.com/status-im/nimbus-eth2/issues/1875 +https://github.com/status-im/nimbus-eth2/issues/1874 +https://github.com/status-im/nimbus-eth2/issues/1873 +https://github.com/status-im/nimbus-eth2/issues/1871 diff --git a/docs/nbc_audit_2020/network_core/ssz_serialization_and_tree_hashing.md b/docs/nbc_audit_2020/network_core/ssz_serialization_and_tree_hashing.md new file mode 100644 index 000000000..467796e0e --- /dev/null +++ b/docs/nbc_audit_2020/network_core/ssz_serialization_and_tree_hashing.md @@ -0,0 +1,46 @@ +--- +title: "SSZ (De)Serialization & Tree hashing" +code_owner: "Zahary Karadjov (zah)" +round: "Audit round 2" +category: "Network Core Audit" +repositories: "nim-beacon-chain" +--- + +Relevant modules: + +SSZ spec implementation: +https://github.com/status-im/nim-beacon-chain/tree/master/beacon_chain/ssz + +Spec: +https://github.com/ethereum/eth2.0-specs/blob/dev/ssz/simple-serialize.md + +Lower-level layers: + +* Nim-serialization +https://github.com/status-im/nim-serialization/ +Implements compile-time reflection responsible for operations such as "List all serializable fields of an object", defines a generic protocol and a set of extensibility points (overloadable functions) that can be used to provide custom serialization for certain types. + +May be sensitive to bugs in the Nim compiler related to generic programming. + +* Nim-FastStreams +https://github.com/status-im/nim-faststreams/ +Defines low-level interfaces for working with various synchronous and asynchronous input streams. + +The SSZ implementation is tested against the official Eth2 test suite here: +https://github.com/status-im/nim-beacon-chain/blob/master/tests/official/test_fixture_ssz_consensus_objects.nim +https://github.com/status-im/nim-beacon-chain/blob/master/tests/official/test_fixture_ssz_generic_types.nim + +Run the tests with: +nim c -r tests/official/test_fixture_ssz_consensus_objects.nim + +Also, with some basic fuzzing tests here: +https://github.com/status-im/nim-beacon-chain/blob/master/scripts/run_ssz_fuzzing_test.nims +https://github.com/status-im/nim-beacon-chain/blob/master/tests/fuzzing/ssz_fuzzing.nim + +The fuzzing tests can be launched with: + +nim scripts/run_ssz_fuzzing_test.nims Attestation + +or + +nim scripts/run_ssz_fuzzing_test.nims --fuzzer:honggfuz Attestation diff --git a/docs/nbc_audit_2020/validator_core/account_management_and_key_storage.md b/docs/nbc_audit_2020/validator_core/account_management_and_key_storage.md new file mode 100644 index 000000000..e62ef226f --- /dev/null +++ b/docs/nbc_audit_2020/validator_core/account_management_and_key_storage.md @@ -0,0 +1,221 @@ +--- +title: "Account Management & Key storage" +code_owner: "Zahary Karadjov (zah) & Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 3" +category: "Validator Core Audit" +repositories: "nim-beacon-chain" +--- + + +Related readings: + +- Honest validator spec: [https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md](https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md) +- Deposit contract spec: [https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/deposit-contract.md](https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/deposit-contract.md) +- Explanation of signing and withdrawal keys: [https://blog.ethereum.org/2020/05/21/keys/](https://blog.ethereum.org/2020/05/21/keys/) +- Keystore EIP2386: [https://github.com/ethereum/EIPs/blob/4494da0966afa7318ec0157948821b19c4248805/EIPS/eip-2386.md](https://github.com/ethereum/EIPs/blob/4494da0966afa7318ec0157948821b19c4248805/EIPS/eip-2386.md) +- Key Derivation EIP2333: [https://eips.ethereum.org/EIPS/eip-2333](https://eips.ethereum.org/EIPS/eip-2333) + +A validator will do a deposit of 32 ETH to participate in proof of stake. Each secret key control 32 ETH, a single beacon node can handle hundreds to thousands of validators and so the same amount of secret keys. + +## 0. Scope clarification + +Beyond the signing and withdrawal keys, another type of keys is used in the application called "Network private keys". Those are application instance specific not user-specific and are used for P2P identity (secp256k1 secret keys), they do not secure money. + +They were reviewed as part of [https://github.com/status-im/nim-beacon-chain/issues/1320](https://github.com/status-im/nim-beacon-chain/issues/1320) + +and so are out of scope. See [https://github.com/status-im/nim-beacon-chain/pull/1533/files#diff-f64593cb13697c7f93a225b3b01a5921](https://github.com/status-im/nim-beacon-chain/pull/1533/files#diff-f64593cb13697c7f93a225b3b01a5921) to have an idea of where they are used + +## 1. State after round 2 + +- Crypto.nim and nim-blscurve dependencies were audited by NCC in phase 1: + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/crypto.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/crypto.nim) + +- signatures.nim and dependencies which handles signature and verification were audited as well + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/signatures.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/signatures.nim) + +## 2. Explanation of the application flow + +The main application is the **beacon_node**, it has 2 mode of operations regarding accounts. With accounts managed in an integrated manner or managed in a split manner, something we call the VC/BN split (Validator Client / Beacon Node split). + +In both case, there is a need of a keystore to hold the validators' **signing keys.** + +### 2.1 Reminder on Ethereum secret keys + +As a reminder, the best explanation for the 2 kinds of secret keys (signing keys and **withdrawal keys**) used in Ethereum 2.0 is at [https://blog.ethereum.org/2020/05/21/keys/](https://blog.ethereum.org/2020/05/21/keys/). + +#### Risks + +A Beacon Node only requires the signing key which presents the following risks: someone who get holds of a signing keys can sign blocks and attestations, leading to double-signing and slashing. The slashing penalty is about 1 ETH (from the initial 32 ETH) but you are ejected when you reach below 16 ETH. An attacker cannot get your funds + +### 2.2 Validation in Nimbus + +2.2.1 ***Case: No Validator Client split*** + +**#### This part will be audited by Trail of Bits** + +Every start of a slot (every 12 seconds), "onSlotStart" will be scheduled: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L986-L987](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L986-L987) + +This will call "[handleValidator](https://github.com/status-im/nim-beacon-chain/blob/a84a8ba192e2f4c7bae22470b44b2db12f95ab0f/beacon_chain/beacon_node.nim#L516)Duties": [https://github.com/status-im/nim-beacon-chain/blob/a84a8ba192e2f4c7bae22470b44b2db12f95ab0f/beacon_chain/beacon_node.nim#L516](https://github.com/status-im/nim-beacon-chain/blob/a84a8ba192e2f4c7bae22470b44b2db12f95ab0f/beacon_chain/beacon_node.nim#L516) + +Then we get in the validator_duties.nim file which is the main consumer of the keystore: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L439-L440](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L439-L440) + +In particular when signing attestations: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L146](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L146) + +and blocks: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L295](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L295) + +**#### This part will be audited by NCC** + +The block and attestation signing functions need access to the secret keys and they are isolated in validator_pool.nim [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim) + +The AttachedValidator type is an abstraction over having the secret key in-memory or in a separate process: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node_types.nim#L73-L94](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node_types.nim#L73-L94) + +2.2.2 ***Case: With Validator Client split* + +A nbc-audit-2020-2 will be tagged with the** **validator client split. +**** + +**#### This part will be audited by Trail of Bits** + +When there is a VC/BN split, the Validator Client logic mirrors the main BeaconNode with an onSlotStart [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L102](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L102) + +Note: validator_client.nim is a stand-alone application + +**#### This part will be audited by NCC (in validator_pool.nim)** + +And signing block and attestations calls the same function as the no-split case: + +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L168](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L168) +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L144](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim#L144) + +with implementation in [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim) + +2.2.3 ***Side-note on hardening*** + +We plan to make signing in a separate process ([https://github.com/status-im/nim-beacon-chain/issues/545](https://github.com/status-im/nim-beacon-chain/issues/545)) whether the Beacon Node and Validator Client are split or integrated. + +This was introduced in [https://github.com/status-im/nim-beacon-chain/pull/1522/files](https://github.com/status-im/nim-beacon-chain/pull/1522/files) +which adds a signing_process.nim file - it is a separate binary and for now the BN (or potentially the VC) talks to it through stdin/stdout to give it roots to sign and it echoes back the signed result. + +a nbc-audit-2020-2 branch which will have this PR will be tagged before the start of the audit + +Further hardening may consist of: + +- a hardened memory allocator [https://github.com/status-im/nim-beacon-chain/issues/563](https://github.com/status-im/nim-beacon-chain/issues/563) +- non-dumpable memory and encrypted memory: [https://github.com/status-im/nim-beacon-chain/issues/545](https://github.com/status-im/nim-beacon-chain/issues/545) + +## 3. Deposits creation + +Besides validation, deposits creation also involves keys, keystore and wallets. + +This is done in a dedicated binary `deposit_contract.nim` +which in particular asks for user private keys [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/deposit_contract.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/deposit_contract.nim) + +and creates the directory for each validator and their wallet and mnemonic private keys + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/deposit_contract.nim#L173](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/deposit_contract.nim#L173) + +The reference implementation is [https://github.com/ethereum/eth2.0-deposit-cli](https://github.com/ethereum/eth2.0-deposit-cli) + +User instructions are at: [https://status-im.github.io/nim-beacon-chain/create_wallet_and_deposit.html](https://status-im.github.io/nim-beacon-chain/create_wallet_and_deposit.html) + +#### Related audits + +Trail of Bits audit of the Ethereum Foundation deposit contract [https://github.com/ethereum/eth2.0-deposit-cli/issues?q=is%3Aissue+is%3Aopen+ToB+Audit](https://github.com/ethereum/eth2.0-deposit-cli/issues?q=is%3Aissue+is%3Aopen+ToB+Audit) + +## 3. Key management + +### 3.1 Loading the keys + +Keys are loaded in-memory by everything that calls `addLocalValidator` and `addLocalValidators` [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L44-L61](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L44-L61) + +Namely, on BeaconNode init: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L291](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L291) +Note: the out of process signing will be tagged later, see devel code: [https://github.com/status-im/nim-beacon-chain/blob/697bd23c/beacon_chain/beacon_node.nim#L280-L286](https://github.com/status-im/nim-beacon-chain/blob/697bd23c9bbdc76470d6ed1a01260c34617f60bd/beacon_chain/beacon_node.nim#L280-L286) + +The `validatorKeys` iterator which returns the validators secret signing keys is defined in `keystore_management.nim` [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/keystore_management.nim#L71-L93](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/keystore_management.nim#L71-L93) + +### 3.2 Saving keys + +The keys are saved in the following situations: + +- After the deposits process we provide: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/deposit_contract.nim#L179-L185](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/deposit_contract.nim#L179-L185) +- When someone stakes through the official Ethereum 2 launchpad: [https://medalla.launchpad.ethereum.org/](https://medalla.launchpad.ethereum.org/) +- When someone uses a third-party that happen to use Nimbus in the backend, typically a stacking pool or an exchange +- When keys are migrated from one client to the next, on both import or export. + +The `saveValidatorKey` procedure is a leftover of refactoring [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L37-L42](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L37-L42) + +### 3.3 Keystore & Wallet implementation + +EIP2386 [https://github.com/ethereum/EIPs/blob/4494da0966afa7318ec0157948821b19c4248805/EIPS/eip-2386.md](https://github.com/ethereum/EIPs/blob/4494da0966afa7318ec0157948821b19c4248805/EIPS/eip-2386.md) is implemented in [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/keystore.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/spec/keystore.nim) + +EIP2333 for key derivation [https://eips.ethereum.org/EIPS/eip-2333](https://eips.ethereum.org/EIPS/eip-2333) is implemented in [https://github.com/status-im/nim-blscurve/blob/master/blscurve/eth2_keygen/eth2_keygen.nim](https://github.com/status-im/nim-blscurve/blob/master/blscurve/eth2_keygen/eth2_keygen.nim) + +The `keystore.nim` has references to related spec EIP2334 and BIP39 + +UUID generation implementation is in [https://github.com/status-im/nim-eth/blob/master/eth/keyfile/uuid.nim](https://github.com/status-im/nim-eth/blob/master/eth/keyfile/uuid.nim) (already audited?) + +Importing keystores and keymanagement as explained to users: + +- import keystore link +- [https://status-im.github.io/nim-beacon-chain/medalla.html#key-management](https://status-im.github.io/nim-beacon-chain/medalla.html#key-management) + +## 4. Configuring Secrets + +Secrets are configured in `conf.nim` [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim) + +In particular: + +- validatorsDirFlag: "A directory containing validator keystores" +- secretsDirFlag: "A directory containing validator keystore passwords" +- walletsDirFlag: "A directory containing wallet files" +- validators: "Path to a validator keystore" +- inProcessValidators: "Disable the push model (the beacon node tells a signing process with the private keys of the validators what to sign and when) and load the validators in the beacon node itself". Actually the keys are currently loaded in-process by default (and not in the signing_process binary) but that should change in the future and then this switch will make sense. + +Everything related to wallets and generating/signing validator deposits: + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim#L259-L324](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim#L259-L324) + +Also impacting secrets is the nonInteractive flag as it will be used by people handling more than a couple of keys. + +CLI audit is in Consensys Diligence scope. + +### 5. Testing the APIs that handle secrets + +TODO, scripts to easily try the following scenarios +1. Launch a testnet +2. Join with an extra validator by passing credentials via + +- wallets +- commandline +- keystores + +3. Import 100 new validators + +4. Export 100 validators to use in another client + + And/or use the Medalla testnet: + +- provide ETH1 secret keys to go through the deposit process +- provide ETH2 keystore.json and/or mnemonic to go through their importing in Nimbus + + This is to avoid waiting a day due to ETH1_FOLLOW_DISTANCE to become a ETH2 validator + +User instructions to import a keystore: + +- [https://status-im.github.io/nim-beacon-chain/medalla.html#3-import-keystores](https://status-im.github.io/nim-beacon-chain/medalla.html#3-import-keystores) +- [https://status-im.github.io/nim-beacon-chain/medalla.html#key-management](https://status-im.github.io/nim-beacon-chain/medalla.html#key-management) +- Base files: [https://github.com/status-im/nim-beacon-chain/tree/nbc-audit-2020-1/docs/the_nimbus_book/src](https://github.com/status-im/nim-beacon-chain/tree/nbc-audit-2020-1/docs/the_nimbus_book/src) + +For now (for Nimbus to implement the script) + +- modifying the launch local testnet script [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim#L259-L324](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim#L259-L324) + - deposits: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/scripts/launch_local_testnet.sh#L182-L188](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/scripts/launch_local_testnet.sh#L182-L188) + - launching a node with its validators and their secret: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/scripts/launch_local_testnet.sh#L311-L316](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/scripts/launch_local_testnet.sh#L311-L316) diff --git a/docs/nbc_audit_2020/validator_core/block_attestation_signing.md b/docs/nbc_audit_2020/validator_core/block_attestation_signing.md new file mode 100644 index 000000000..485a03254 --- /dev/null +++ b/docs/nbc_audit_2020/validator_core/block_attestation_signing.md @@ -0,0 +1,38 @@ +--- +title: "Block/attestation signing" +code_owner: "Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 2" +category: "Validator Core Audit" +repositories: "nim-beacon-chain" +--- + +# Description + +Verify that cryptographic primitives (Miracl and BLST) are properly used at a high-level. +We assume that they are correctly implemented in the backend. Note, for this phase 2 we are concerned about the proper usage internal to nim-beacon-chain. Phase 3 will be about the end-user API (keystores and secret management). + +Specs: +- [https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#bls-signatures](https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/beacon-chain.md#bls-signatures) + +For information, evaluation of backends: [https://notes.status.im/nim-bls-curve-backends](https://notes.status.im/nim-bls-curve-backends) + +(phase 3) ETH2 has 2 kinds of keys +- a signing key which is used by validators to sign attestations or blocks. +The signing key is needed on a permanent basis by the validator client (audit phase 3). +Leaking this key puts the owner at risk of slashing (double-voting) +- a withdrawal key which is used by validators to retrieve their stake (~32 ETH). +The withdrawal key should be generated offline and stored offline. +Doc: [https://blog.ethereum.org/2020/05/21/keys/](https://blog.ethereum.org/2020/05/21/keys/) + +# Links + +Links to the repositories and more information + +- [https://github.com/status-im/nim-blscurve](https://github.com/status-im/nim-blscurve) (low-level wrapper over crypto primitives, 2 backends) +- [https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/crypto.nim](https://github.com/status-im/nim-beacon-chain/blob/master/beacon_chain/spec/crypto.nim) (high-level BLS signature types and serialization) +- [https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/spec/signatures.nim](https://github.com/status-im/nim-beacon-chain/blob/devel/beacon_chain/spec/signatures.nim) (abstraction over signing and verification of consensus objects) +- [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_pool.nim) (Pool of validators, currently includes secret keys in process) + - For information (not merged, not to be audited), PR to separate the key in another process: [https://github.com/status-im/nim-beacon-chain/pull/1522](https://github.com/status-im/nim-beacon-chain/pull/1522) +- High-level callers of signing primitives (for round 3 "validators"): + - [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim) + - [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_client.nim) diff --git a/docs/nbc_audit_2020/validator_core/command_line_interface_CLI.md b/docs/nbc_audit_2020/validator_core/command_line_interface_CLI.md new file mode 100644 index 000000000..9613325d8 --- /dev/null +++ b/docs/nbc_audit_2020/validator_core/command_line_interface_CLI.md @@ -0,0 +1,51 @@ +--- +title: "Command Line Interface (CLI)" +code_owner: "Zahary Karadjov (zah)" +round: "Audit round 3" +category: "Validator Core Audit" +repositories: "nim-beacon-chain, nim-confutils" +--- + +## Goals + +- Ensure that the CLI can handle malicious inputs (overflow, unicode, invalid characters, ...) +- Prevent users from shooting themselves in the foot: + - Avoid losing data + - Avoid losing funds/secrets (overlap with NCC work on secrets acknowledged, more eyes on this critical part is welcome) + - Clear instructions: + - The book [https://status-im.github.io/nim-beacon-chain/](https://status-im.github.io/nim-beacon-chain/) + - Audit freeze: [https://github.com/status-im/nim-beacon-chain/tree/nbc-audit-2020-1/docs/the_nimbus_book/src](https://github.com/status-im/nim-beacon-chain/tree/nbc-audit-2020-1/docs/the_nimbus_book/src) +- With a focus on the workflow to stake ETH + - See the book + - [https://medalla.launchpad.ethereum.org/](https://medalla.launchpad.ethereum.org/) + +## Code + +- All conf options: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/conf.nim) +- Makefile with preset build/run configuration: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/Makefile](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/Makefile) + +The key binaries with a CLI are: + +- beacon_node.nim which is configured by the BeaconConf type +and can be compiled with `make beacon_node` +- validator_client.nim which is configured by `make validator_client` +- deposit_contract.nim which allows to make a ETH1 deposit + +## Secret keys + +TODO + +- Address with Goerli ETH to test the deposit contract and ETH2 keystore generation +- ETH2 validator enabled on Medalla to test the beacon_node / validator_client + +## Related audit reports + +Trail of bits audit of the Ethereum Foundation deposit contract: + +- [https://github.com/ethereum/eth2.0-deposit-cli/issues?q=is%3Aissue+is%3Aopen+ToB+Audit](https://github.com/ethereum/eth2.0-deposit-cli/issues?q=is%3Aissue+is%3Aopen+ToB+Audit) + +---------------------------------------------------------------- + +https://github.com/status-im/nim-beacon-chain/issues/1320 ← also relevant for walletfile generation (even though the file is encrypted and usually in an ACL protected subpath (userdir); but its world readable + may leak info about wallets) + +https://github.com/status-im/nim-beacon-chain/issues/1687 diff --git a/docs/nbc_audit_2020/validator_core/rpc_api.md b/docs/nbc_audit_2020/validator_core/rpc_api.md new file mode 100644 index 000000000..4e1aef5b3 --- /dev/null +++ b/docs/nbc_audit_2020/validator_core/rpc_api.md @@ -0,0 +1,30 @@ +--- +title: "RPC API" +code_owner: "" +round: "Audit round 3" +category: "Validator Core Audit" +repositories: "nim-beacon-chain, nim-json_rpc" +--- + +RPC resources: + +- Official mandatory APIs: [https://ethereum.github.io/eth2.0-APIs/](https://ethereum.github.io/eth2.0-APIs/) +* OpenAPI description: [https://ethereum.github.io/eth2.0-APIs/beacon-node-oapi.yaml](https://ethereum.github.io/eth2.0-APIs/beacon-node-oapi.yaml) + +Code: + +- RpcServer in [https://github.com/status-im/nim-json-rpc](https://github.com/status-im/nim-json-rpc) +- Validator API handlers: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_api.nim#L177](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_api.nim#L177) +- init: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L259-L262](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L259-L262) +- Beacon Node API handlers: [https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L715-L833](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/beacon_node.nim#L715-L833) + +---------------------------------------------------------------- + +https://github.com/status-im/nim-json-rpc/issues/81 + +https://github.com/status-im/nim-beacon-chain/issues/1653 +https://github.com/status-im/nim-beacon-chain/issues/1652 +https://github.com/status-im/nim-beacon-chain/issues/1651 +https://github.com/status-im/nim-beacon-chain/issues/1650 + +recommendations/observations: [https://github.com/status-im/nim-beacon-chain/issues/1665](https://github.com/status-im/nim-beacon-chain/issues/1665) diff --git a/docs/nbc_audit_2020/validator_core/slash_prevention_mechanisms.md b/docs/nbc_audit_2020/validator_core/slash_prevention_mechanisms.md new file mode 100644 index 000000000..dd3a2fd6c --- /dev/null +++ b/docs/nbc_audit_2020/validator_core/slash_prevention_mechanisms.md @@ -0,0 +1,69 @@ +--- +title: "Slash Prevention Mechanisms" +code_owner: "Mamy André-Ratsimbazafy (mratsim)" +round: "Audit round 3" +category: "Validator Core Audit" +repositories: "nim-beacon-chain" +--- +Note: Slashing Protection is a very new feature and not merged/enabled by default yet in the codebase. + +For the moment it leaves in the PR: [https://github.com/status-im/nim-beacon-chain/pull/1643](https://github.com/status-im/nim-beacon-chain/pull/1643) + +---------------------- + +## Resources + +### Overview of slashing and how it ties in with the rest of Eth2.0 + +Phase 0 for humans - Validator responsibilities: + +- https://notes.ethereum.org/@djrtwo/Bkn3zpwxB#Validator-responsibilities + +Phase 0 spec - Honest Validator - how to avoid slashing + +- https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#how-to-avoid-slashing + +### In-depth reading on slashing conditions + +- Detecting slashing conditions https://hackmd.io/@n0ble/By897a5sH +- Open issue on writing a slashing detector https://github.com/ethereum/eth2.0-pm/issues/63 +- Casper the Friendly Finality Gadget, Vitalik Buterin and Virgil Griffith +https://arxiv.org/pdf/1710.09437.pdf +Figure 2 +An individual validator ν MUST NOT publish two distinct votes,〈ν,s1,t1,h(s1),h(t1) AND〈ν,s2,t2,h(s2),h(t2), +such that either: +I. h(t1) = h(t2). Equivalently, a validator MUST NOT publish two distinct votes for the same target height. + + OR + II. h(s1) < h(s2) < h(t2) < h(t1). + Equivalently, a validator MUST NOT vote within the span of its other votes. + +- Vitalik's annotated spec: https://github.com/ethereum/annotated-spec/blob/d8c51af84f9f309d91c37379c1fcb0810bc5f10a/phase0/beacon-chain.md#proposerslashing +1. A proposer can get slashed for signing two distinct headers at the same slot. +2. An attester can get slashed for signing +two attestations that together violate the Casper FFG slashing conditions. +- https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#ffg-vote +- The "source" is the current_justified_epoch +- The "target" is the current_epoch + +### Reading on weak subjectivity + +- https://notes.ethereum.org/@adiasg/weak-subjectvity-eth2 +- https://www.symphonious.net/2019/11/27/exploring-ethereum-2-weak-subjectivity-period/ +- https://ethresear.ch/t/weak-subjectivity-under-the-exit-queue-model/5187 + +### Reading of interop serialization format + +- Import/export format: https://hackmd.io/@sproul/Bk0Y0qdGD +- Tests: https://github.com/eth2-clients/slashing-protection-interchange-tests + +## Implementation + +In `validator_slashing_protection.nim` (TODO merge PR and tag an audit branch) +and used in `validator_duties.nim` (no VC/BN split) or `validator_clients.nim` (with Validator Client/Beacon Node split) + +For slashing protection (in contrast to slashing detection) we only care about our own validators. We also assume that before signing a block or an attestation, the node is "synced" to the chain, i.e. it knows last finalized epoch of the whole blockchain. + +The `isSynced` function has a naive heuristic for now and will be changed in the future to properly handle weak subjectivity period. + +[https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L87-L111](https://github.com/status-im/nim-beacon-chain/blob/nbc-audit-2020-1/beacon_chain/validator_duties.nim#L87-L111)