Repository navigation
Conversation
Adds a CI grep check (audit follow-up, Sujith finding #4 on PR #1715) that fails any PR introducing direct .safeTransfer( / .safeTransferFrom( calls in src/ outside LibAsset and an explicit allowlist of pre-existing offenders. Native-ETH safeTransferETH calls remain allowed. EXSC-300 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 16 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a new GitHub Actions workflow that enforces ERC20 transfer routing through ChangesERC20 Transfer Routing Enforcement
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review feedback: the regex only caught SafeTransferLib-style calls, so raw IERC20.transfer/.transferFrom (the original Tron USDT bug class) passed silently - including LidoWrapper.sol, the contract explicitly named in audit finding #4. Widen the pattern and allowlist LidoWrapper (grandfathered) and Executor (false positive: transferFrom on our own ERC20Proxy). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/enforceLibAssetRouting.yml (1)
28-30: ⚡ Quick winHarden checkout by disabling credential persistence
At Line 29,
actions/checkoutshould setpersist-credentials: falsesince this job only performs local grep scans and does not need authenticated git operations after checkout.Suggested patch
- name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: falseBased on learnings from static analysis, this maps to the reported
artipackedcredential-persistence warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/enforceLibAssetRouting.yml around lines 28 - 30, The checkout step using actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 should be hardened by disabling credential persistence; update the checkout step (the actions/checkout invocation) to include persist-credentials: false so credentials are not retained in the runner after checkout for this local grep scan job.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/enforceLibAssetRouting.yml:
- Around line 28-30: The checkout step using
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 should be hardened by
disabling credential persistence; update the checkout step (the actions/checkout
invocation) to include persist-credentials: false so credentials are not
retained in the runner after checkout for this local grep scan job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff43e899-0d32-49d5-a462-d03e0343eb99
📒 Files selected for processing (1)
.github/workflows/enforceLibAssetRouting.yml
The job only runs local grep scans and needs no authenticated git operations after checkout (CodeRabbit finding; matches house style in other workflows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai The |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Which Linear task belongs to this PR?
EXSC-300 — Add solhint rule (or CI grep) enforcing LibAsset routing for ERC20 transfers
Audit follow-up to PR #1715 (Sujith, informational finding #4).
Why did I implement it this way?
The ticket offered two options: a custom solhint rule or a CI grep step. I went with the CI grep step (a new workflow,
enforceLibAssetRouting.yml):solhint-plugin-*), which conflicts with the repo's TypeScript/Bash-only scripting rule and adds dependency surface for a single rule.jsonChecker.yml,protectSecurityRelevantCode.yml).Behavior:
.solfile insrc/outside the allowlist contains.safeTransfer(or.safeTransferFrom(.safeTransferETH(is not matched and remains allowed everywhere (the regex requires(directly aftersafeTransfer/safeTransferFrom).HopFacetPacked,AcrossFacetPacked,CBridgeFacetPacked,AcrossFacetPackedV3) are explicitly allowlisted with a comment, per the AC.main(TokenWrapper,LiFiDEXAggregator,ReceiverStargateV2,ReceiverAcrossV3,ReceiverChainflip,WithdrawablePeriphery— consistent with the audit finding "Out-of-scope facets and LidoWrapper bypass LibAsset"). They are grandfathered in a separately-commented allowlist section so the check is green onmainfrom day one while still rejecting any NEW file or any new call site in a currently-clean file.Verification evidence (run locally against this branch):
bash -non the extracted check script: passsrc/): exit 0src/EvilFacet.solwithIERC20(t).safeTransfer(...): exit 1, offending file + line printedsrc/EthOnlyFacet.solwith onlysafeTransferETHcalls (both method-style and library-style): exit 0bunx prettier --checkon the workflow file: passDRAFT status / pr-ready note
/pr-ready(local CodeRabbit) could not be run to a clean state: the CLI returned "Review limit reached" (org usage credits exhausted), twice. Per the pre-PR workflow this PR is therefore opened as a draft; cloud CodeRabbit will review on this PR as the safety net. Aself-review-passsweep (mechanical/semantic/executable) was completed with no findings.Checklist before requesting a review
/pr-ready(local CodeRabbit) on this branch and resolved (or explicitly documented) all findings — see.agents/commands/pr-ready.md(attempted; rate-limited — documented above)Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)
🤖 Generated with Claude Code