Audit adherence to spec feedbacks

[terencechain]

Opening this issue to track the adherence to spec feedbacks from the Quantstamp audit

• 1. In beacon-chain/blockchain/process_attestation.go, the function onAttestation(), many parts of the specification for on_attestatation() delay consideration, rather than avoiding consideration. However, the function onAttestation() does not seem to ever delay - only returns (possibly with errors). Ensure that the intent of the specification is preserved.
• 2. In beacon-chain/blockchain/process_attestation_helpers.go#157, it may not be entirely clear why the seed impact on attestation verification is omitted from the specification.
• 3. Typo in beacon-chain/blockchain/process_block_helpers.go#45, “feature” -> “future”.
• 4. Typo in beacon-chain/blockchain/process_block_helpers.go#444, “couldn’t received” -> “couldn’t be received”.
• 5. In beacon-chain/blockchain/process_block_helpers.go#225, based on the definition of params.BeaconConfig().SafeSlotsToUpdateJustified, should return a false here instead of a true. We recommend clarifying the definition and/or this specific case.
• 6. In beacon-chain/core/validators/validator.go, although the implementation of SlashValidator() conforms to the specification, it is unclear whether BeaconState.slashings should represent per-epoch sums of slashed effective balances (i.e., balances before slashing) or only deltas (i.e., the actual slashed balances).
• 7. In beacon-chain/core/helpers/rewards_penalties.go#30, we check if total == 0, which relates to the max() function in the spec pseudocode. Although it should be semantically equivalent in practice, it may be best to strictly match the spec by changing the line to total < params.BeaconConfig().EffectiveBalanceIncrement.
• 8. In beacon-chain/core/helpers/committee.go, it is unclear how the domain of certain calls to Seed() are determined. For example, in UpdateProposerIndicesInCache() on L344 DomainBeaconAttester is used, whereas in precomputeProposerIndices() on L371 DomainBeaconProposer is used.
• 9. In beacon-chain/p2p/encoder/ssz.go, according to specification Encoding-dependent header: Req/Resp protocols using the ssz or ssz_snappy encoding strategies MUST encode the length of the raw SSZ bytes, encoded as an unsigned protobuf varint. On L34, the encoder contains a function Encode() that can violate the spec (as opposed to EncodeWithLength()). The function seems to be used in tests only. We suggest moving it to testing scope only and removing it from the encoder.
• 10. Specification pseudocode does not always exactly match the ETH2 specification, including, but not limited to the following examples:

1. In beacon-chain/core/helpers/validators.go in the pseudocode on L116, MIN_SEED_LOOKAHEAD is used instead of MAX_SEED_LOOKAHEAD.
2. In beacon-chain/core/blocks/block_operations.go in the pseudocode for process_block_header, the function call signing_root(state.latest_block_header) is used instead of hash_tree_root(state.latest_block_header).
3. In beacon-chain/core/helpers/committee.go, in the pseudocode for get_beacon_committee, we have index=epoch_offset, instead of index=(slot % SLOTS_PER_EPOCH) * committees_per_slot + index.
4. In beacon-chain/core/state/transition.go in the pseudocode for state_transition, there are several missing snippets such as if validate_result: assert verify_block_signature(state, signed_block).
5. In beacon-chain/core/validators/validator.go, after L113, the if-block is missing. There should be a line for whistleblower_index = proposer_index.
6. The type Hash is often used instead of Bytes32.
7. In beacon-chain/core/block_operations.go#626, it is unclear where the assertion assert data.index < get_committee_count_at_slot(state, data.slot) is enforced.

• 11. In beacon-chain/core/block_operations.go#1038, it seems the depth parameter noted in the spec pseudocode is not used. On the other hand, the depth variable is write-only in the specification. We recommend aligning the specification and code with each other, perhaps, mixing in depth into the root. beacon chain/core/blocks/block_operations.go, the comments before ProcessRandao(), VerifyIndexedAttestation(),
• 12. In beacon-chain/core/helpers/attestation.go, the comment before SlotSignature() does not match the specification for get_slot_signature().
• 13. In beacon-chain/core/helpers/committee.go, the comment before BeaconCommitteeFromState() does not match the specification.
• 14. In beacon-chain/core/helpers/rewards_penalties.go, the specification for get_total_balances() has a max() statement which does not appear to be faithfully implemented. In particular, if 0 < total < EffectiveBalanceIncrement, then the wrong value will be returned.
• 15. Typo in beacon-chain/core/helpers/validators.go#116, “MIN” -> “MAX”.

[terencechain]

On this today

[terencechain]

• Addressed 1. with code comment to clarify
• Addressed 2. with a commit on removing old code
• Addressed 3. and 4. with a commit on fix
• For 5. The implementation is correct. We implemented according to spec. https://github.com/ethereum/eth2.0-specs/blob/dev/specs/phase0/fork-choice.md#should_update_justified_checkpoint
• The feedback for 6 is confusing. Addressed 6 with code comment to clarify
• Addressed 7 with a commit on code fix
• Addressed 8 with a commit on code comment to clarify
• Addressed 9 with a commit on code fix
• Addressed 10 with a commit on code comment to clarify
• Addressed 11 with a commit on code comment to clarify
• Addressed 12 with a commit on code comment to clarify
• Addressed 13 with a commit on code comment to clarify
• 14 Same as 7
• 15 is no longer relevant. The comment has been gone