M2-I02: fix(clearnode/api): normalize participant addresses consistently across endpoints#712
Conversation
…tly across endpoints Audit finding M2-I02 noted that NormalizeHexAddress was applied in some RPC paths (RequestCreation, the channel/app session-key state endpoints, and issueTransferReceiverState) but not others, so equivalent representations of the same Ethereum address — mixed case, missing 0x prefix, or EIP-55 checksum vs lowercased — bypassed duplicate checks and reached the store and signature verification in non-canonical form. This change funnels every wallet/owner/participant address that crosses an RPC boundary through pkg/core.NormalizeHexAddress before it is used for duplicate detection, packing, signing, or store queries: Handlers: - channel_v1.SubmitState: normalize incoming user_wallet before toCoreState so the lock, channel check, store lookup, and signature verify all see the canonical form. - app_session_v1.SubmitDepositState: same for the user_state.user_wallet. - app_session_v1.CreateAppSession: replace strings.ToLower with NormalizeHexAddress in the participant duplicate-check loop and propagate the normalized address into appDef.Participants so packing, app session ID derivation, and the persisted participant list all use the canonical form. This is the audit's central concern — case-only duplicates are now rejected. - apps_v1.SubmitAppVersion: validate and normalize req.App.OwnerWallet early; use the normalized form for AllowAppRegistration, persistence, and signature verification (replacing the previous bare strings.ToLower at storage time). Getters: - channel_v1.GetChannels, GetLatestState, GetHomeChannel - app_session_v1.GetAppSessions (participant filter) - user_v1.GetBalances, GetTransactions - apps_v1.GetApps (owner_wallet filter) All now reject malformed addresses up-front and pass the canonical lowercase form to the store layer. Tests: one focused test per touched endpoint exercises the normalization contract — getters assert that a mixed-case input reaches the store as canonical lowercase; submit handlers assert that an invalid hex input is rejected before any store interaction; CreateAppSession asserts that two participant addresses that differ only in case are detected as duplicates; SubmitAppVersion asserts that a mixed-case owner_wallet is persisted in canonical lowercase form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
philanton
requested review from
dimast-x,
ihsraham and
nksazonov
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/api/app_session_v1/submit_deposit_state.go (1)
186-244:⚠️ Potential issue | 🟠 MajorNormalize
alloc.Participantaddresses before use in participant lookups and ledger entries.The participant addresses from
appStateUpd.Allocations(from client RPC requests) are used directly without normalization, whileparticipantWeightskeys are built from normalized addresses usingstrings.ToLower(). This creates a case-sensitivity mismatch: a client sending mixed-case addresses like0xAbCd...will fail to match lowercase keys like0xabcd...in the participantWeights map, causing valid participants to be rejected at line 220 and creating address inconsistencies in ledger entries at line 234.Apply
core.NormalizeHexAddress()toalloc.Participantat the start of the allocation processing loop (line 187) and use the normalized value throughout, consistent with how participant addresses are handled at session creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/submit_deposit_state.go` around lines 186 - 244, Incoming participant addresses from appStateUpd.Allocations are not normalized before lookups and ledger writes, causing mismatches with participantWeights (which uses lowercase keys) and inconsistent ledger entries; fix by calling core.NormalizeHexAddress on alloc.Participant at the start of the loop that processes appStateUpd.Allocations and then use that normalizedParticipant variable for all subsequent operations (participantWeights lookup, comparisons, tx.RecordLedgerEntry, incomingAllocations map keys, and any error messages) so address casing is consistent across ValidateDecimalPrecision/asset checks and ledger recording.
🧹 Nitpick comments (2)
clearnode/api/app_session_v1/create_app_session.go (1)
73-95: Normalization flow looks correct.The normalization is applied before deduplication, weight tally, and propagation back into
appDef.Participants[i].WalletAddress, which ensures packing, app session ID derivation, and the persisted participant list all see the canonical form. This is the right place to do it.One small consistency note: the duplicate-error message on Line 89 still uses the raw
participant.WalletAddress, while the key isparticipantWallet. Consider logging the canonical form (or both) so operators can correlate the duplicate against what actually entered the dedup map:♻️ Optional tweak
- // Check for duplicate participant addresses - if _, exists := participantWeights[participantWallet]; exists { - c.Fail(rpc.Errorf("duplicate participant address: %s", participant.WalletAddress), "") - return - } + // Check for duplicate participant addresses + if _, exists := participantWeights[participantWallet]; exists { + c.Fail(rpc.Errorf("duplicate participant address: %s", participantWallet), "") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/create_app_session.go` around lines 73 - 95, Update the duplicate-participant error to report the canonical normalized address (participantWallet) instead of the raw input so operators can correlate the message with the deduplication key; locate the duplicate check that uses participantWeights and c.Fail (within the loop that sets participantWallet, totalWeights, participantWeights and appDef.Participants[i].WalletAddress) and change the error argument from participant.WalletAddress to participantWallet (or include both original and normalized forms) so the log matches the map key used for deduplication.clearnode/api/apps_v1/get_apps_test.go (1)
19-19: Inconsistent fixture address length.Line 18 was updated to a canonical 42-char address but Line 19 still uses
"0x2222". Since this value is in the mock's return data (not the handler's input), it doesn't trigger normalization and won't fail the test — but it makes the fixtures look inconsistent next to the surrounding cleanup. Consider expanding it to a canonical address for symmetry:♻️ Optional consistency tweak
- {App: app.AppV1{ID: "app-2", OwnerWallet: "0x2222", Metadata: "0x00", Version: 1}}, + {App: app.AppV1{ID: "app-2", OwnerWallet: "0x2222222222222222222222222222222222222222", Metadata: "0x00", Version: 1}},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/apps_v1/get_apps_test.go` at line 19, The test fixture uses an inconsistent short wallet address in the mock return (AppV1.OwnerWallet set to "0x2222"); update the mock in get_apps_test.go so the AppV1 entry for ID "app-2" uses a canonical 42-character hex address (e.g. pad to 40 hex chars after 0x) to match surrounding fixtures and maintain consistency in the AppV1 mock data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@clearnode/api/app_session_v1/submit_deposit_state.go`:
- Around line 186-244: Incoming participant addresses from
appStateUpd.Allocations are not normalized before lookups and ledger writes,
causing mismatches with participantWeights (which uses lowercase keys) and
inconsistent ledger entries; fix by calling core.NormalizeHexAddress on
alloc.Participant at the start of the loop that processes
appStateUpd.Allocations and then use that normalizedParticipant variable for all
subsequent operations (participantWeights lookup, comparisons,
tx.RecordLedgerEntry, incomingAllocations map keys, and any error messages) so
address casing is consistent across ValidateDecimalPrecision/asset checks and
ledger recording.
---
Nitpick comments:
In `@clearnode/api/app_session_v1/create_app_session.go`:
- Around line 73-95: Update the duplicate-participant error to report the
canonical normalized address (participantWallet) instead of the raw input so
operators can correlate the message with the deduplication key; locate the
duplicate check that uses participantWeights and c.Fail (within the loop that
sets participantWallet, totalWeights, participantWeights and
appDef.Participants[i].WalletAddress) and change the error argument from
participant.WalletAddress to participantWallet (or include both original and
normalized forms) so the log matches the map key used for deduplication.
In `@clearnode/api/apps_v1/get_apps_test.go`:
- Line 19: The test fixture uses an inconsistent short wallet address in the
mock return (AppV1.OwnerWallet set to "0x2222"); update the mock in
get_apps_test.go so the AppV1 entry for ID "app-2" uses a canonical 42-character
hex address (e.g. pad to 40 hex chars after 0x) to match surrounding fixtures
and maintain consistency in the AppV1 mock data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55da5d54-dd4c-4daa-99e0-656843dbbac8
📒 Files selected for processing (22)
clearnode/api/app_session_v1/create_app_session.goclearnode/api/app_session_v1/create_app_session_test.goclearnode/api/app_session_v1/get_app_sessions.goclearnode/api/app_session_v1/get_app_sessions_test.goclearnode/api/app_session_v1/submit_deposit_state.goclearnode/api/app_session_v1/submit_deposit_state_test.goclearnode/api/apps_v1/get_apps.goclearnode/api/apps_v1/get_apps_test.goclearnode/api/apps_v1/submit_app_version.goclearnode/api/apps_v1/submit_app_version_test.goclearnode/api/channel_v1/get_channels.goclearnode/api/channel_v1/get_channels_test.goclearnode/api/channel_v1/get_home_channel.goclearnode/api/channel_v1/get_home_channel_test.goclearnode/api/channel_v1/get_latest_state.goclearnode/api/channel_v1/get_latest_state_test.goclearnode/api/channel_v1/submit_state.goclearnode/api/channel_v1/submit_state_test.goclearnode/api/user_v1/get_balances.goclearnode/api/user_v1/get_balances_test.goclearnode/api/user_v1/get_transactions.goclearnode/api/user_v1/get_transactions_test.go
M2-I02: fix(clearnode/api): normalize participant addresses consistently across endpoints (#712) M2-L01: docs(protocol): clarify session-key authorization scope and metadata extensibility (#711) M2-M01: fix(clearnode): alert on home channel challenge instead of auto-checkpoint (#710) M2-H01: fix(contracts/ChannelHub): add finalize escrow home chain intent check (#704)
2 participants
Summary
Audit finding M2-I02 flagged that
NormalizeHexAddresswas applied in some RPC paths (RequestCreation, the channel/app session-key state endpoints,issueTransferReceiverState) but not others, so equivalent representations of the same Ethereum address — mixed case, missing `0x` prefix, or EIP-55 checksum vs lowercased — bypassed duplicate checks and reached the store / signature verification in non-canonical form.This PR funnels every wallet/owner/participant address that crosses an RPC boundary through
pkg/core.NormalizeHexAddressbefore it is used for duplicate detection, packing, signing, or store queries.Handlers
channel_v1.SubmitStateapp_session_v1.SubmitDepositStateapp_session_v1.CreateAppSessionapps_v1.SubmitAppVersionGetters
channel_v1.GetChannels,GetLatestState,GetHomeChannel;app_session_v1.GetAppSessions(participant filter);user_v1.GetBalances,GetTransactions;apps_v1.GetApps(owner_wallet filter) — all now reject malformed addresses up-front and pass the canonical lowercase form to the store layer.Tests
One focused test per touched endpoint exercises the normalization contract:
CreateAppSessionasserts two participant addresses that differ only in case are detected as duplicates.SubmitAppVersionasserts a mixed-case `owner_wallet` is persisted in canonical lowercase form.The store layer's existing internal `strings.ToLower` calls are left untouched — they remain a defence-in-depth safety net for legacy data, but normalization now happens at the API boundary so the canonical form flows through packing, signing, and duplicate detection.
Test plan
Summary by CodeRabbit
Release Notes
Enhancements
Tests