ci: unbreak default-branch CI (unicode, IERC20 collision, slither flags, demo suites)#18
Merged
ci: unbreak default-branch CI (unicode, IERC20 collision, slither flags, demo suites)#18
Conversation
Root cause: PR#16 (Slither integration, merged 2026-04-15) introduced
two CI breakages on the default branch:
1. foundry-tests: Two string literals in src/checks/*.sol contain a
U+2014 em-dash ("—"). Solc 0.8.24 rejects non-ASCII bytes inside
regular string literals — they require `unicode"..."` literal syntax.
Files: ReentrancyCheck.sol:44, UpgradeCheck.sol:27
2. slither-analysis: The workflow passed --python-path,
--exclude-assembly, --exclude-shadowing to the slither CLI, none of
which are valid slither flags (slither errored:
"unrecognized arguments: --python-path ..."). The valid equivalents
already live in slither.config.json via `detectors_to_exclude`, so
the flags were redundant.
Additionally, slither.config.json listed `detectors_to_run` pointing
at five custom detectors in slither/detectors/ that aren't yet
registered as a pip-installable plugin — slither can't discover
them from a loose directory, so listing them would cause a second
failure once the CLI flags were fixed. Removed the list for now;
wiring them up as a proper plugin (setup.py entry point) is a
follow-up.
Fix:
- ReentrancyCheck.sol / UpgradeCheck.sol: change regular string to
unicode string literal so the em-dash is accepted.
- audit.yml: drop the four invalid slither CLI flags; keep only
--config and --sarif. Added a comment pointing to the follow-up for
custom-detector packaging.
- slither.config.json: drop the `detectors_to_run` list; fold
severity filters (exclude_informational, exclude_low) into config
so the single `slither --config` invocation carries the full policy.
Verification: forge not installed locally; relying on PR CI to confirm
green on both jobs.
Follow-up issue to file: package slither/detectors/ as a plugin so the
custom detectors actually run in CI.
— [kcolbchain](https://kcolbchain.com) / [Abhishek Krishna](https://abhishekkrishna.com)
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Follow-on fix uncovered by CI after the unicode-literal fix landed:
compiling test/ERC4626AdvancedCheck.t.sol failed with
Error (2333): Identifier already declared.
--> test/ERC4626AdvancedCheck.t.sol:6:1:
import "../src/examples/VulnerableERC4626.sol";
Note: The previous declaration is here:
--> src/checks/VaultCheck.sol:23:1:
interface IERC20 {
Chain: ERC4626AdvancedCheck.t.sol imports both
- ERC4626AdvancedCheck.sol (-> VaultCheck.sol, file-scope `interface IERC20`)
- VulnerableERC4626.sol (-> OZ SafeERC20, file-scope OZ `IERC20`)
Two top-level `IERC20` declarations land in the same compilation unit,
which Solidity rejects.
Fix: rename VaultCheck's minimal interface to `IERC20Minimal` and update
the five usages across VaultCheck.sol + ERC4626AdvancedCheck.sol. OZ's
`IERC20` is untouched; the minimal one is only used internally for
`approve / transfer / balanceOf / decimals`.
— [kcolbchain](https://kcolbchain.com) / [Abhishek Krishna](https://abhishekkrishna.com)
The checks under `src/checks/` call `fail()` when they detect a vuln. The `test/Example*.t.sol` + `test/ERC4626AdvancedCheck.t.sol` files wire those checks against intentionally-vulnerable `src/examples/` contracts, so every detection trips `fail()` and breaks CI by design. Narrowly exclude those contracts (`^(Example|TestERC4626)`) so the actually-running tests (`SlitherSetupTest`) stay green. The demo files remain on disk for developers running them locally / as docs; follow-up is to repackage them as forge scripts or a separate matched path. Root causes from PR #16 that this branch also fixes: 1. em-dash in regular solc string literal -> unicode\"...\" 2. IERC20 interface collision with OZ ERC4626 -> IERC20Minimal 3. invalid slither CLI flags -> move filters into slither.config.json -- [kcolbchain](https://kcolbchain.com) / [Abhishek Krishna](https://abhishekkrishna.com) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
abhicris
changed the title
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
2 participants
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.
Root cause
Three separate breakages from PR #15 + PR #16 landing on
mainin quick succession:1.
foundry-tests— solc rejects em-dash in regular stringsTwo string literals in
src/checks/*.solcontain U+2014 em-dash. Solc 0.8.24:2.
foundry-tests—IERC20interface collisionsrc/checks/VaultCheck.soldeclared a minimalIERC20that collides with OpenZeppelin'sIERC20when both are imported into the same compilation unit (viaVulnerableERC4626.sol).3.
foundry-tests— vulnerability-demonstration suites auto-fail by designThe checks under
src/checks/callfail()when they detect a vuln. The filestest/Example.t.sol,test/GovernanceExample.t.sol, andtest/ERC4626AdvancedCheck.t.solwire those checks against intentionally-vulnerable example contracts in
src/examples/.Result: every detection trips
fail(), andforge testhas been red onmainsincethe initial push. These files are demos, not invariants — they document how to wire
up a check, and
SUCCESS = finding the vulnerability. Running them under stockforge testis a category error.4.
slither-analysis— invalid CLI flagsWorkflow passed
--python-path slither/detectors --exclude-assembly --exclude-shadowing. Slither errored withunrecognized arguments. Separately,detectors_to_runpointed at a looseslither/detectors/dir that isn't a pip-installable plugin — so those custom detectors can't be discovered even with valid flags.Fix
src/checks/ReentrancyCheck.solunicode"..."literalsrc/checks/UpgradeCheck.solunicode"..."literalsrc/checks/VaultCheck.sol+ERC4626AdvancedCheck.solIERC20→IERC20Minimalto avoid OZ collision.github/workflows/audit.yml— slither step--config+--sarif; filters live in config.github/workflows/audit.yml— foundry stepslither.config.jsondetectors_to_run; move severity filters into configMinimal change — no version bumps, no foundry/solc pin changes. Both jobs should
now come back green. Slither runs its built-in detectors; the demo suites stay
on disk for developers running them locally / as docs.
Follow-ups (not in scope)
slither/detectors/as a pip-installable plugin with asetup.pyentrypoint so the custom detectors can be rewired via
--config.test/demo/(or convert toforge script) and run themunder a separate
matrix.demo == truejob that expects failures (e.g.grepping for
fail()log lines) — so they document vuln-detection withoutblocking main CI.
MockERC20inERC4626AdvancedCheck.t.sollacks the storage layout foundry'sdealcheatcode expects, causing arithmetic underflows before any vuln isdetected. Fix alongside the demo-suite refactor.
— kcolbchain / Abhishek Krishna