SIG-750: gate forge fmt, solc warnings, and slither in CI#77
Conversation
There was a problem hiding this comment.
Summary
Hygiene-only PR that gates forge fmt --check, a solc-warning grep, and slither --fail-medium in CI before audit kickoff. Solidity formatter swaps from prettier-plugin-solidity to forge fmt (single source of truth, configured in [fmt] of foundry.toml). 10 production contracts receive targeted, justified Slither suppressions; SignalsCoreStorage gets one block-level uninitialized-state suppression with a four-category lifecycle comment. The only non-format production source change is the owner -> positionOwner local rename in SignalsPosition.burn to avoid shadowing inherited Ownable.owner() (event signature PositionBurned(uint256 indexed positionId, address indexed owner) is unchanged). DeployV1.s.sol anonymizes two unused parameters to clear unused-parameter warnings; both are confirmed unused inside their bodies and call sites pass by position.
Cross-PR Context
Sibling PRs (SIG-750)
- Only this PR. Public ABI, event signatures, and storage layout are unchanged, so no companion PRs in
v1-subgraph,v1-sdk,v1-server, orsignals-appare required. Confirmed by reading the diff (no event/struct/function-signature edits) andreferences/impact-map.md.
Issues
No issues found.
Verification performed:
- Position rename safety:
SignalsPosition.PositionBurnedevent definition isevent PositionBurned(uint256 indexed positionId, address indexed owner)and is unchanged. Only the local stack variable was renamed fromownertopositionOwner; the indexed/non-indexed shape and emitted value (ownerOf(positionId)) are preserved.v1-subgraphonly references this event inabis/SignalsCore.json(not in handler code), and the ABI is generated from the event signature, not the local variable. - DeployV1 unused parameters:
_resolvePaymentToken's body uses onlydeployerKey,envName,vm.envOr, andIERC20Metadata(verified by reading the function body);deployerwas dead._writeDeployOutput's body never references theoperatorsarray (operatorAllowlist flows throughparams.operatorAllowlisttodeployAllDeterministicinstead). Both anonymizations are mechanical compile-warning fixes, not behavior changes. - Slither suppressions audit: Each annotation matches detector semantics and surrounding code.
controlled-delegatecallon_delegateView/_riskGateis justified (modules are owner-set, dispatch design).incorrect-equalityontodayBatchId == 0,openPositionCount == 0, andreceivedAmount == 0are all sentinel comparisons against natural zero values, not float-style fuzziness.divide-before-multiply+write-after-writeon_toSettlementTick(both copies) is a clamp-then-snap-to-grid pattern where the floor division is the intended alignment, and the second write reads the clamped tick throughoffsetbefore reassigning.uninitialized-localoncumulativeCostWad/cumulativeProceeds/totalPayout/theDeployConfigstruct are all accumulators or pre-assigned structs.unused-returnonapplyDepositdiscardsmintedShares(per-depositor accounting happens at claim time, not batch time) and oncomputeSeedStatsdiscardsrootSum/minFactor(onlydeltaEtis needed for prior admissibility).reentrancy-no-ethon_initializeProxiesis justified by the single-shotexecutedguard and trusted proxy targets. SignalsCoreStorageblock-level suppression: The block spans the entire storage container. Every storage variable currently inside is enumerated by category in the lifecycle comment (A: initialize-then-setter; B: module-owned operational state; C: default-zero sentinels; D: UUPS gap). The four categories cover all current fields. Risk that a future writer adds a variable inside the block silently inheriting suppression is real but documented.- CI gate design:
^Warning \([0-9]+\):is anchored to solc's current warning prefix. A solc upgrade that changes the prefix would silently disable the gate. Mitigation: solc is pinned to 0.8.28 infoundry.toml, foundry pinned tostable, slither pinned to0.11.5. Storage layout, ABIs, gas snapshot, andyarn lintcontinue to gate other regressions. - Format commit isolation: Commit
41ca17bis purelyforge fmtoutput. Verifiedgit diff 41ca17b 2d8e80fis config-only (.prettierrc,foundry.toml,package.json,scripts/types/environment.ts,scripts/utils/release.ts,yarn.lock) with no contract source edits hidden inside.
Suggestions
None.
Verdict
APPROVE: zero unresolved items. The hygiene gates are well-scoped, the suppressions are targeted with justifications matching detector semantics and surrounding code, the format commit is mechanical and isolated, and the only production source edit (owner -> positionOwner) is an audit-friendly rename that preserves the event ABI. Risks called out in the PR description (block-level uninitialized-state suppression, solc-prefix-anchored grep gate, format-commit size) are acknowledged with reasonable mitigations for a pre-audit baseline.
Context
SIG-750. Hygiene sweep before audit kickoff: gate
forge fmt --check, zero solc warnings, andslither --fail-mediumin CI so audit firms see a clean baseline. Closes the last cleanup task in epic SIG-742.Decisions
prettier-plugin-solidityremoved;forge fmtis now the single source of truth forcontracts/,script/, andtest/.[fmt]block infoundry.tomlpins line length 120, tab width 4, double quotes, no bracket spacing.package.jsonformatandformat:checkscripts updated;lint-stagedSolidity rule removed. Prettier still ownsscripts/**/*.ts.41ca17bso reviewers can skip it and focus on the five hygiene commits that follow.slither.config.json) withfilter_paths: node_modules/|contracts/testonly/. Production detectors stay enabled. False positives use targetedslither-disable-*annotations with reasons rather than detector-level disables.SignalsCoreStorageuses a single block suppression foruninitialized-statebecause the abstract storage container is structurally never the write site; the block comment enumerates the four lifecycle categories (initialize-then-setter, module-owned operational state, default-zero sentinels, UUPS gap).ModulesUpdatedunindexed-address Slither finding left in place (5 address parameters; Solidity caps indexed at 3). Documented, not changed, because indexing would be ABI-visible.^Warning \([0-9]+\):againstforge buildoutput to fail only on native solc warnings. Existing forge-lint findings remain out of scope by plan.stablein CI; Slither pinned to0.11.5.owner→positionOwnerlocal rename inSignalsPosition.burn; existing position/core suites and ABI/storage diff checks cover it.Impact
Public ABI, storage layout, and event signatures are unchanged. The only non-format production source edit is a local variable rename in
SignalsPosition.burn(owner→positionOwner); the emittedPositionBurned(positionId, positionOwner)event still uses the same indexed/non-indexed shape and value (ownerOf(positionId)).Downstream repos (
v1-subgraph,v1-sdk,v1-server,signals-app) consume ABI/events; this PR does not change either. No sibling PRs required.Risk Areas
41ca17b) touches 111 files (-3220 / +1599). It is mechanicalforge fmtoutput, but its size obscures any non-fmt edit hidden inside. Confirm by viewing only commits2d8e80f..HEADfor the actual hygiene changes, or by runningforge fmt --checkonorigin/mainrebased onto41ca17b^.SignalsCoreStorageSlither block suppression spans the entire storage block. Any new storage variable added inside that block silently inherits the suppression. The lifecycle comment is the contract for future writers; reviewer should confirm the four-category enumeration covers all current fields.SignalsCore,SignalsDeployer,ThetaTimeFeePolicy,ClmsrMath,LPVaultModule,MarketLifecycleModule,OracleModule,RiskModule,TradeModule,SignalsRouter). Each annotation has a justification comment; reviewer should verify the justification matches the detector and surrounding code.slither.config.jsonfilterscontracts/testonly/. Anything moved out oftestonly/later is automatically re-scanned, but anything moved intotestonly/becomes invisible to Slither.^Warning \([0-9]+\):) is anchored to solc's current warning prefix. A solc upgrade that changes the prefix would silently disable the gate._resolvePaymentTokenand the_serializeContractsoperators param inDeployV1.s.sollost their parameter names (replaced with anonymous types) to clear unused-parameter warnings. Call sites still pass the same arguments by position; verify deployment scripts that introspect parameter names (if any) are not affected.Out of Scope
--fail-medium; they are not fixed in this PR.