Skip to content

H-I02: docs(contracts): add a note that rebasing tokens are not supported#670

Merged
nksazonov merged 2 commits into
fix/audit-findingsfrom
fix/audit-h-i02
Apr 10, 2026
Merged

H-I02: docs(contracts): add a note that rebasing tokens are not supported#670
nksazonov merged 2 commits into
fix/audit-findingsfrom
fix/audit-h-i02

Conversation

@nksazonov
Copy link
Copy Markdown
Contributor

@nksazonov nksazonov commented Apr 9, 2026

Description

The ChannelHub contract tracks node balances using a static _nodeBalances mapping that is fundamentally incompatible with rebasing tokens (e.g., stETH, aTokens, rebase stablecoins). When rebasing tokens rebase, their balances change autonomously without any transfer occurring, creating a critical desynchronization between the accounting tracked in _nodeBalances and the actual token balance held by the contract.

When a rebasing token experiences a negative rebase (balance decrease), the actual contract balance decreases proportionally while _nodeBalances remains completely unchanged. This creates an insolvency scenario where the sum of all recorded node balances in _nodeBalances exceeds the actual tokens held by the contract.

The vulnerability manifests in a first-come-first-served pattern: early withdrawers successfully withdraw their full recorded amounts, while later withdrawers face silent transfer failures. The ChannelHub's transfer failure handling mechanism (_pushFunds) catches these failures and adds the amounts to a reclaim queue (_reclaims mapping), but critically, these are unfulfillable obligations - the tokens simply don't exist in the contract to satisfy these debts. When users attempt to claim their reclaimed funds via claimFunds(), the transaction reverts because the contract lacks sufficient tokens.

The issue affects all vault operations (depositToVault, withdrawFromVault, purgeEscrowDeposits) and channel operations that rely on _nodeBalances for accounting. No balance reconciliation mechanism exists to detect or handle this mismatch.

Summary by CodeRabbit

  • Documentation
    • Added clarification that rebasing tokens (e.g., stETH, aTokens) are not supported due to accounting incompatibility.
    • Provided guidance to use non-rebasing token alternatives (e.g., wstETH instead of stETH).

@nksazonov
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 9, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 47256e7c-5618-4de2-9f7d-a4dcf08e3ab1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Documentation updates across three files clarifying that rebasing tokens (stETH, aTokens, rebase stablecoins) are not supported by the protocol due to static accounting ledger constraints that cannot track autonomous balance changes.

Changes

Cohort / File(s) Summary
Token Compatibility Documentation
contracts/SECURITY.md, contracts/src/ChannelHub.sol, protocol-description.md
Added warnings and explanations across security, implementation, and protocol description documents specifying that rebasing tokens are incompatible with the static ledger-based accounting model (_nodeBalances, lockedFunds). Documentation includes rationale (autonomous balance changes cause permanent divergence) and guidance to use non-rebasing equivalents (e.g., wstETH instead of stETH).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • ihsraham
  • philanton
  • dimast-x

Poem

🐰 Hops with glee through docs so bright,
"No rebasing tokens!"—plain insight,
Static ledgers hold the line,
wstETH wrapped, the solution fine!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and accurately summarizes the main change: adding documentation about rebasing token incompatibility with the protocol.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/audit-h-i02

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 9, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@contracts/src/ChannelHub.sol`:
- Around line 299-300: The comment warns that rebasing tokens are unsupported
but not enforced; update the ChannelHub contract to enforce token compatibility
by adding an allowlist and checking it in depositToNode: introduce a
mapping(address=>bool) allowedToken (and an admin setter like
setAllowedToken/renounceAllowedToken in the ChannelHub) and
require(allowedToken[token], "token not allowed") at the start of depositToNode
to reject unsupported tokens and prevent _nodeBalances desync; ensure any
admin/setter functions are properly access-controlled (e.g., onlyOwner) and
include tests exercising depositToNode reverts for disallowed tokens and success
for allowed ones.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 734db7c2-21b7-41b9-b474-4a78edc63c9f

📥 Commits

Reviewing files that changed from the base of the PR and between c2310fe and 0ab6639.

📒 Files selected for processing (3)
  • contracts/SECURITY.md
  • contracts/src/ChannelHub.sol
  • protocol-description.md

Comment thread contracts/src/ChannelHub.sol
@nksazonov nksazonov changed the title H-I02: docs(contracts): add a not that rebasing tokens are not supported H-I02: docs(contracts): add a note that rebasing tokens are not supported Apr 10, 2026
@nksazonov nksazonov merged commit c4a9f3e into fix/audit-findings Apr 10, 2026
3 checks passed
@nksazonov nksazonov deleted the fix/audit-h-i02 branch April 10, 2026 08:59
nksazonov added a commit that referenced this pull request Apr 16, 2026
H-H01: fix(contracts): disallow challenge with CLOSE or FIN_MIG intents (#631)

H-M01: fix(contracts/ChannelHub): remove transferred amount check (#636)

fix(contracts/ChannelHub): allow unblocking escrow ops after migration (#637)

M-C01: feat(contracts): add token check between states (#639)

H-L03: fix(contracts/ChannelHub): skip disputed escrow during purge (#640)

M-H04: feat(contracts/ChannelHub): purge during escrow challenge finalization, count both skip and purge (#641)

M-M02: fix(contracts): require zero allocs on close (#643)

M-C02: fix(rpc/core): apply finalize escrow deposit correctly (#644)

H-L02: fix(contracts/ChannelHub): revert on withdrawFromVault failure (#651)

M-H03: fix(clearnode): log correct fields on failure (#647)

M-C03: fix(pkg/core): disallow negative amount transitions (#645)

H-L01: fix(contracts/ChannelHub): add CH address to prevent val addition replay (#650)

M-H06(clearnode): restrict max channel challenge duration (#654)

M-M03(clearnode): revert EscrowLock on insufficient home user balance (#655)

M-M04(clearnode): support default signer even if it is not approved (#656)

M-M05(clearnode): require correct intent on FinalizeEscrowWithdrawal (#657)

M-M09: reject asset decimals exceeding its token's decimals (#659)

M-L01: return correct errors during state advancement validation (#660)

M-L03: limit number of related ids per session key state (#662)

M-I02: normalize input hex addresses (#663)

M-H01: feat(contracts/ChannelHub): restrict to one node (#649)

M-H07: fix(contracts/ChannelHub): emit stored, not arbitrary candidate state (#664)

M-I01: feat(contracts/ChannelHub): remove updateLastState flag (#665)

H-L06: fix(contracts/ChannelHub): remove payable from methods, clarify in create (#666)

H-L09: feat(contracts/ChannelEngine): add non-home migration version check (#669)

YNU-839: fix blockchain listener lifecycle (#658)

M-H09: use channel signer for all channel state node sigs (#667)

H-L07: fix(contracts/ChannelHub): restrict createChannel to non-existing channels (#668)

H-I02: docs(contracts): add a note that rebasing tokens are not supported (#670)

H-I03: fix(contracts/ChannelHub): use CIE pattern in depositToHub (#671)

M-H11: revert empty signatures on quorum verification (#672)

M-H11: reject issuance of receiver state during escrow ops (#674)

H-I01: docs(contracts): note that fee-on-transfer tokens are not supported (#675)

M-L04: docs: mention liquidity monitoring (#677)

fix(contracts/ChannelHub): fix initialize escrow deposit dos (#679)

M-H08: enforce strict transition ordering after MutualLock and EscrowLock (#680)

fix: run forge fmt (#681)

M-L05: docs(contracts): document native token deposits (#685)

fix(clearnode): resolve a set of audit findings (#686)

M-H13: feat(contract): add validateChallengeSignature, revert in SK validator (#688)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants