fix: remediate all Round 3 security audit findings#69
Merged
Conversation
Address 43+ findings across 13 audit issues (#56-#68) spanning all layers: Critical: - CRIT-01: MergeForkState now merges all modified accounts, not just contract address High: - HIGH-01: BlockBuilder receives pre-constructed TransactionExecutor - HIGH-02: Sandboxed orphaned Task awaited before scope disposal Medium: - MED-01/03: VoterPublicKey binding enforced in BasaltBft and PipelinedConsensus - MED-01 (Execution): ChargeGasAndIncrementNonce uses actual gas for tip credit - MED-02: InsufficientBalance paths report tx.GasLimit as GasUsed - MED-03: UInt256.TryAdd overflow check in TransactionValidator - NEW-M01: TcpTransport per-IP counter leak on failed connections - MED-01 (Compliance): SanctionsList post-add eviction Low/Informational: - UInt256 hex truncation guard, KeystoreManager key zeroing - PipelinedConsensus sequential proposal rejection - SlashingEngine atomic percent slash, ValidatorSet lock - StateDbRef GetModifiedStorageKeys/GetModifiedAccounts delegation - REST multi-read consistency via state fork - PeerInfo dead code removal, ReputationScorer thread safety - TransportEncryption/EpisubService concurrency docs and fixes - ComplianceEngine defensive copy, BST-1155/3525 checked arithmetic - ViewingKey/ConfidentialityModule ephemeral key zeroing - BridgeState recipient validation - GraphQL Mutation logger injection, case-insensitive 0x handling - Explorer AOT JSON context, faucet key zeroing, DataDir symlink resolution - Rate limiter global policy, CORS hardening - Docker no-new-privileges and cap_drop ALL - Codec size comment, BLS PoP documentation, nullifier reset docs 44 files changed, 2334 tests passing, 0 warnings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses 43+ findings across all 13 Round 3 audit issues, spanning every layer of the stack (core, consensus, storage, execution, network, API, SDK, bridge, compliance, confidentiality, explorer, generators, node).
44 files changed — 427 insertions, 152 deletions. 2,334 tests passing, 0 warnings.
Critical (1)
MergeForkStatediscarded non-contract account mutations (balance/nonce changes lost after contract calls). AddedGetModifiedAccounts()toIStateDatabaseand rewrote merge to iterate all dirty accounts.High (2)
BlockBuilderused parameterlessTransactionExecutorconstructor (no chain params, no validator). ReorderedSetupBlockProduction()to pass pre-constructed executor.Taskwas orphaned on timeout, risking cross-contract state corruption.dispatchTasknow awaited before scope disposal.Medium (6)
VoterPublicKeybinding enforced inBasaltBftandPipelinedConsensusvote/view-change handlers — reject messages where SenderId doesn't match registered public key.ChargeGasAndIncrementNoncenow computes actual gas equivalent for proposer tip credit.tx.GasLimitasGasUsed(not 0).UInt256.TryAddoverflow check inTransactionValidatorforvalue + gasCost.TcpTransportper-IP counter leaked on failed connections — moved increment after successful connect.SanctionsListpost-add eviction when at capacity.Low (~25)
GetByAddress, StateDbRef delegation forGetModifiedStorageKeys/GetModifiedAccounts0xhandlingInformational (3)
no-new-privileges+cap_drop: ALLon all validatorsDesign Decisions (unchanged)
UInt256implicitintconversion — runtimeOverflowExceptionon negative values is intentional for ergonomics.WeightedLeaderSelector.StakeToWeightprecision — ~0.02% deviation acceptable for leader selection.Test plan
dotnet build -c Release— 0 errors, 0 warningsdotnet test -c Release— 2,334 tests passing across 16 test projectsCloses #56, closes #57, closes #58, closes #59, closes #60, closes #61, closes #62, closes #63, closes #64, closes #65, closes #66, closes #67, closes #68