Smart Contract Security Architecture: Three-Phase Plan (DevNet → Testnet → Mainnet) #88
robertocarlous
started this conversation in
General
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Background and motivation
DIN is now in the final stretch of P3 — eight reviewed and approved PRs are queued against
develop, P3-6.3b (audit preparation: NatSpec, invariant tests, firm outreach) is scheduled for Aug 17–30, and the roadmap identifies a smart contract audit as a P3 long-term deliverable. This is the right moment to align on what security work is required at each phase before we hand anything to an external auditor or open the network to external validators.An internal security review was completed in July 2026 by @Abidoyesimze, documented in
Documentation/technical/audits/foundry-src-security-review.md(pinned at commitd136ff3). That review found 2 Critical, 2 High, 4 Medium, and 8 Low/Informational findings. This discussion uses those findings as the baseline and proposes what remediation is needed at each of the three deployment phases.Contract structure context
DIN's contracts split into two tiers with fundamentally different security properties:
Platform contracts (
DinToken,DinCoordinator,DinValidatorStake,DINModelRegistry,DinFeeRouter,DinTreasury) — upgradeable via Transparent Proxy, deployed once per network. If a bug is found here, it can be fixed via an upgrade without redeploying the whole network. ProxyAdmin is currently held by the DIN-Representative EOA.Task contracts (
DINTaskCoordinator,DINTaskAuditor) — non-upgradeable, deployed per model by the model owner. There is no upgrade path. If a critical bug exists in a live task contract, the model owner's only recovery is to abandon the GI in progress and redeploy new contracts from scratch — losing all in-flight state from honest participants. This is the headline constraint that makes the Critical findings below more severe than they would be in an upgradeable system.Current known risk inventory — verified against
develop(Aug 2026)The table below was verified by reading
developdirectly, not just the pinned review commit.slashAuditors,finalizeEvaluation,finalizeT1/T2Aggregation,slashAggregators. A single attacker can register hundreds of Sybil addresses (~0.00001 ETH each at default exchange rate), causing every one of these functions to exceed the Optimism block gas limit permanently. GI is bricked with no recovery path.develop.registerDINaggregatorandregisterDINAuditorhave no registration cap.MAX_LM_SUBMISSIONSonly applies to client model submissions, not validators. Realforgegas measurements in the review put spec-scalefinalizeEvaluationat ~223M gas — 7.5× the ~30M Optimism block limit.bytes32(0)is used as both a "no submission" sentinel and a valid submission value insubmitT1/T2Aggregation. If one aggregator submitsbytes32(0)and is the only submitter in their batch before the window closes (a routine scenario, not a targeted attack),finalizeT1/T2Aggregationreverts permanently withTC_NoSubmissions— the GI cannot advance, and the contracts are not upgradeable.develop. NeithersubmitT1AggregationnorsubmitT2Aggregationvalidate_aggregationCID != bytes32(0). The bug is one missing input check away from being fixed.finalizeT1/T2Aggregationaccepts a "winning" CID. If only 1-of-3 assigned aggregators submits before the model owner closes the window, that single submission becomes the consensus result with no cross-validation. The entire point of multi-aggregator batches (fault tolerance) is defeated.develop.finalizeT1/T2Aggregationaccepts any plurality winner as final with nominSubmissionscheck. Compare toDINTaskAuditor._tryFinalizeEligibilitywhich correctly enforcesminEligibilityQuorum.createAuditorsBatchesandautoCreateTier1AndTier2usesblockhash(block.number - 1)(address shuffle) andblock.timestamp(model-index shuffle). Both are public and known before the transaction is submitted. The model owner — who calls these functions and benefits from favorable batch assignments — can grind block selection to cluster their Sybil-controlled validators into the same batch as target honest validators.develop. Both_shuffleAddressArrayfunctions useblockhash(block.number - 1)andblock.timestamp. PR #63 (auditor commit-reveal) is in-flight but not merged; the aggregation shuffle (autoCreateTier1AndTier2) is unaddressed and tracked in BL-11.submitT1/T2Aggregation) is unaddressed.DINModelRegistry.disableModel()kill-switch has no effect on a liveDINTaskCoordinator/DINTaskAuditor. A model the DIN-Representative disables (e.g. due to malicious behavior or a slashing bug) can continue running GIs and slashing honest validators.develop—disableModelis not present in the develop-branchDINModelRegistryyet. Becomes live once those PRs merge, and the disconnect will be present from day one unless explicitly wired.DINTaskAuditor.slashAuditors()has no internal GI-state precondition of its own. Every other function in the file independently re-checks the current GI state;slashAuditorsrelies entirely onDINTaskCoordinator.slashAuditors()as its single gate. Defense-in-depth is absent for the one function that triggers actual slashing.ValidatorStatus.JailedandjailedUntilwere declared but nothing ever wrote to them — the entire jailing mechanism was unreachable dead code.jailValidatoris fully implemented in the post-PR-#65 contracts. Closes once #65 merges.DinValidatorStake.stake()callssafeTransferFrom(external call) before updatingactiveStake— violates checks-effects-interactions. Mitigated today bynonReentrantandDIN_TOKENbeing a protocol-deployed trusted contract.develop.depositAndMint()has no minimum-deposit check. A small enoughmsg.valuecan roundmintAmountto zero via integer division while ETH is retained.requestModelRegistrationsilently keeps any overpayment above the required fee.rejectModel()never refunds the fee paid inrequestModelRegistration. Confirm this is intentional anti-spam design.withdrawFees(address payable to)has no zero-address check — calling withaddress(0)silently burns the entire fee balance (owner-only).dinvalidatorStakeContract_addressanddintaskcoordinator_contract_addresswith no zero-address check. Deployer-controlled risk only.totalDepositedRewardsandRewardDepositedevent declared but never written/emitted; contract has noreceive()/fallback(). Dead code / incomplete feature.updateDinPerEth()andupdateValidatorStakeContract()take effect immediately with no timelock — front-run/back-run window around rate changes.Additionally, two key-management facts shape the risk profile regardless of contract code:
onlyOwnerparameter setters (setMinStake,setUnbondingPeriod,updateDinPerEth) take effect in the same block with no timelock — a compromised key can change validator economics instantly.Phase 1 — DevNet (P3, now through testnet readiness)
Goal: resolve all Critical and High findings, and complete audit-prep deliverables (P3-6.3b), before opening the network to external validators or moving to testnet.
The non-upgradeability of task contracts makes it significantly cheaper to fix C-1 and C-2 now, while there are few live task contract deployments. Every task contract deployed before the fix carries the vulnerability for its lifetime.
Proposed remediation tasks (to become Issues after team alignment here)
C-2 fix — reject
bytes32(0)at submission timeAdd
if (_aggregationCID == bytes32(0)) revert TC_ZeroCID();to bothsubmitT1AggregationandsubmitT2Aggregation. This is a one-line input validation change with no architectural impact. It moves the failure from "whole-GI permanent brick at finalize time" to "individual submitter rejected immediately." Cheapest fix in this entire list.C-1 fix — registration cap + paginated finalize/slash
Two-part:
MAX_REGISTERED_AGGREGATORSandMAX_REGISTERED_AUDITORSconstants to capregisterDINaggregator/registerDINAuditorat a network-appropriate ceiling (mirrors howMAX_LM_SUBMISSIONSalready caps client submissions).finalizeEvaluation,slashAuditors,finalizeT1/T2Aggregation,slashAggregators— e.g.slashAuditors(uint _GI, uint startBatch, uint endBatch)— tracking alastProcessedBatchcursor per GI so large rounds can be advanced in multiple transactions rather than one unbounded call. See C-1 in the audit report for the gas extrapolation; at spec scale (50 batches, 500 auditors),finalizeEvaluationexceeds the block limit by 7.5×.H-1 fix — minimum submission quorum in finalize functions
Add
require(submissionCount >= T1_AGGREGATORS_PER_BATCH / 2 + 1)(or a configurableminT1Submissions) beforefinalizeT1Aggregation/finalizeT2Aggregationaccepts a winning CID. Mirror the existingminEligibilityQuorumpattern already correct inDINTaskAuditor._tryFinalizeEligibility. Batches below quorum should either revert (forcing a retry window) or be marked "unresolved" for out-of-band handling — team to decide.H-2 / M-1 — aggregation-side commit-reveal or VRF
PR #63 covers the auditor batch shuffle and scoring commit-reveal. The outstanding gap is
autoCreateTier1AndTier2's aggregator batch shuffle and the aggregation submission commit-reveal — currently tracked in BL-11 pending PR #68. This needs to be a concrete follow-on PR once #63 and #68 merge, not just a backlog note.M-2 — document or wire the kill-switch
Decision needed: (a) wire
DINTaskCoordinator/DINTaskAuditorto checkDINModelRegistry.modelDisabled(modelId)on every state-changing call — requires addingmodelIdand registry address to task contract constructors; or (b) update NatSpec and operator docs to state explicitly thatdisableModelis registry-metadata-only and does not stop a live GI. Option (b) is a one-day doc change. Option (a) is the correct fix but a larger change. Team to decide scope before testnet.M-3 — add GI-state guard inside
DINTaskAuditor.slashAuditors()Add an explicit
GIstatecheck consistent with every other function in the file. One-line defensive check, no architectural change.L-1 — CEI fix in
DinValidatorStake.stake()Move
validator.activeStake += amountand_syncValidatorStatus(validator)before thesafeTransferFromcall. Correct behavior is identical (mitigated bynonReentrant), but the fix is trivial and removes the CEI violation before an auditor flags it as a "won't fix" in the report.P3-6.3b deliverables (already scheduled)
NatSpec on all P3 public/external functions, protocol invariants document, fuzz/invariant tests for slashing, storage layout doc, audit package assembly, firm outreach. Recommendation: C-1, C-2, H-1 fixes should land before the audit package is sent — an auditor seeing unpatched Criticals in a "pre-audit" submission is a credibility hit and adds unnecessary re-review scope.
Phase 2 — Testnet
Goal: independent external validation of the fixed codebase, tighter key management, and a circuit breaker before real external validators stake on the protocol.
External audit
Scope: all platform contracts + task contract templates, post-P3-fixes. Minimum one firm; ideally two independent reviews of the task contracts specifically, given they are non-upgradeable. Timing: after P3-6.3b deliverables are complete and C-1/C-2/H-1/H-2 fixes are merged, so the audit is on patched code. The P3 roadmap targets smart contract audits as a P3 long-term deliverable (week 16, Aug 2026). If fixes slip past Aug 30, audit timing should track the fixes, not the calendar.
ProxyAdmin multisig
Migrate ProxyAdmin custody from a single EOA to a Gnosis Safe (3-of-5 or similar, team key holders). A single compromised key should not be able to push a malicious upgrade with no delay or second opinion. This is a deployment/key-management change — no contract code modification needed.
Timelock on critical parameter changes
Wrap
setMinStake,setUnbondingPeriod,updateDinPerEth, andupdateValidatorStakeContractbehind aTimelockControllerwith a 24–48h delay. Gives validators time to see an adverse parameter change and exit before it takes effect. OZTimelockControlleris a standard fit; the PROPOSER and EXECUTOR roles map to the multisig above.Emergency pause / circuit breaker
Add a
Pausablemechanism to the platform contracts (at minimumDinValidatorStakeandDINTaskCoordinator/DINTaskAuditorstate-changing calls), callable by the DIN-Representative or the multisig. Not needed for the current trusted DevNet environment, but essential before external validators are at risk. ThedisableModelM-2 fix above covers a model-level stop; this covers a network-level stop in a genuine emergency.Private bug bounty
Invite a small set of whitehat researchers before public testnet launch. Small fixed bounty pool, scoped to smart contracts and CLI. Immunefi supports private programs and the setup cost is low.
Hardware wallets for all testnet-equivalent signing keys
Ledger/Trezor for ProxyAdmin signers and any key with
onlyOwneraccess on deployed contracts. Document key custody and rotation policy.Phase 3 — Mainnet
Goal: every control from testnet must be hardened, not relaxed, before real economic value is at stake.
Second independent external audit (different firm)
One audit gives one perspective; two audits at different firms gives meaningful coverage, especially given the complexity of the GI lifecycle state machine and the economic security properties of the slashing/reward system. The second audit should explicitly scope: (1) the tokenomics paper (P3-DOC5) cross-checked against contract behavior; (2) the slashing math and reward distribution; (3) the two-tier aggregation protocol correctness. These are areas where a missed edge case has direct financial impact and where a protocol-domain expert adds more value than a generic contract auditor.
Formal verification of critical invariants
Use Certora Prover or Halmos to certify:
The fuzz/invariant tests from P3-6.3b (already partially satisfied by
SlashingInvariants.t.solin PR #66) are a good starting point but not a substitute — fuzz tests find bugs probabilistically, formal verification proves absence.Extended timelock
48h minimum for parameter changes, 72h for contract upgrades, at mainnet TVL. Validator operators need time to react to changes that affect their economics.
Multisig for treasury and slash proceeds
The
slashTreasuryaddress should be a multisig at mainnet — not a single EOA — so that slashed funds cannot be unilaterally redirected by a single key holder.Incident response runbook
A documented decision tree before mainnet launch:
This is the "operator-safe recovery steps per failure mode" document from P3-4.3 that is currently listed as not started. It should be written before mainnet, not after the first incident.
Backdoor attack scope statement
The current threat model explicitly excludes backdoored models (models that maintain clean-data accuracy and evade the marginal-gain gate — tracked as RES-2). Before mainnet, a one-page scope boundary statement should document this exclusion explicitly so external validators and model owners understand what the protocol does and does not guarantee, and so external ML security researchers know what is in scope for the bug bounty.
Progressive decentralization path
Consistent with Abraham's August 2026 governance decision, on-chain governance (security council, token voting) is post-mainnet. The path from current EOA
onlyOwner→ multisig (testnet) → DAO (post-mainnet) should be documented before mainnet so validators understand the governance trajectory they are staking into and what protections they have today.Open questions
C-1 design choice: Do we prefer (a) cap-only (limit registration counts, fix the root cause), (b) pagination-only (allow large counts, handle them in chunks), or (c) both? The audit review recommends both. Is there a gas budget constraint on L2 that makes the pagination approach complex to implement cleanly?
H-1 unresolved batch handling: When
finalizeT1/T2Aggregationfinds a batch that hasn't met the minimum submission quorum, should it (a) revert the whole call (model owner retries after waiting for more submissions), (b) mark the batch as "unresolved" and skip it (some batches finalize, others don't), or (c) something else? Each option has different recovery implications.M-2 kill-switch scope: Wiring task contracts to check
DINModelRegistry.modelDisabled(modelId)is the correct fix but requires addingmodelIdand registry address to the task contract constructors. Given that task contracts are already deployed per-model, this could be done at deploy time with no migration cost for future deployments. Is there a reason to keep the task contracts registry-unaware, or do we want to wire this properly now?External audit timing: P3-6.3b is scheduled Aug 17–30. If C-1/C-2/H-1 fixes land in the same window, we could have an audit-ready package by Aug 30. Is that a realistic target, or should audit outreach be a P4 milestone that follows the P3 merge backlog clearing?
Testnet timelock scope: Should the timelock wrap all
onlyOwnersetters (includingblacklistValidator,setSlashTreasury)? A full timelock on admin functions complicates rapid incident response — for example, if a validator needs emergency blacklisting, a 24h delay is harmful. Preferred approach: timelock only the economic parameter setters, keep the emergency-response functions on the multisig with no delay.Mainnet audit budget: Has any budget been allocated for two external audits + Immunefi bounty pool + formal verification tooling? These are non-trivial costs and should be in the mainnet launch plan. If the team has contacts at audit firms already (from P3-6.3b outreach), sharing those here would help scope the timeline.
Summary — what needs to be decided before we open implementation tasks
Cc: @umeradl
All reactions