fix: audit findings round final 3

[philanton]

Summary

Three audit-finding fixes from the MF3 batch, intended to squash-merge.

Commits

• MF3-L01 (1827cfbf) — pkg/core: validate aggregate net-flow / allocation ranges. Individually-valid scaled balances can still overflow the contract's int256 net-flow sum or uint256 allocation sum, producing states the contract will always reject. Adds aggregate bounds checks in both Ledger.Validate (domain) and contractLedger.Validate (ABI/packing) as defense in depth.

• MF3-H01 (064512de) — nitronode/event_handlers: issue challenge_rescue on Challenged → Closed unconditionally. Drops the HasSignedFinalize skip that under-credited the user when a Path-1 timeout close landed at version Y < F despite a node-signed Finalize existing locally. Sources rescue prev via GetLastUserState across both channel-attached and detached entries; NewChallengeRescueState now branches on prev.HomeChannelID (detached → append at (prev.Epoch, prev.Version+1); in-channel → wrap to (prev.Epoch+1, 0)), eliminating the state-ID collision the old skip worked around. Also drops the redundant epoch filter from SumNetTransitionAmountAfterVersion.

• MF3-I01 (c6f945af) — docs only: promotes the implicit "events processed in strict (block, log_index) order with idempotent resume" property of pkg/blockchain/evm/listener.go into a named, citeable invariant. Cross-references it at every consumer that relies on it (receiver-credit issuance in channel_v1 / app_session_v1, close handler in event_handlers), so future changes that weaken any of the four guarantees have to update the named consumers. No behavior change — auditor reframed I01 as Informational defense-in-depth.

Test plan

• go build ./... clean
• go vet ./... clean
• go test ./... all packages pass
• New tests on each commit (see individual commit messages)

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Enhanced validation to prevent overflow conditions in state allocations and net flows.

• Documentation

  • Expanded documentation on event ordering guarantees and channel closure recovery scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors the challenge-rescue flow and state-query API to support cross-channel user-scoped lookups and simplified net-transition calculations. It adds explicit channel closure tracking to rescue-state construction, introduces per-user state queries, simplifies filtering logic, enforces aggregate overflow validation on ledgers, and documents critical MF3-I01 event-ordering guarantees.

Changes

Challenge Rescue & State Query Refactoring

Layer / File(s)	Summary
Core Interface & Contract Updates pkg/core/interface.go, pkg/blockchain/evm/channel_hub_reactor.go	Adds GetLastUserState(wallet, asset, signed) to fetch latest user state across channels/detached chain entries; removes epoch parameter from SumNetTransitionAmountAfterVersion(channelID, minVersion) across all store and blockchain-reactor interfaces.
Ledger Aggregate Overflow Validation pkg/core/state_packer.go, pkg/core/state_packer_test.go, pkg/core/types.go, pkg/core/types_test.go	contractLedger.Validate and Ledger.Validate now check that allocation sums (UserAllocation + NodeAllocation) fit uint256 and net-flow sums (UserNetFlow + NodeNetFlow) fit int256, with new test cases validating overflow detection and boundary conditions.
Challenge Rescue State Construction pkg/core/types.go, pkg/core/types_test.go	NewChallengeRescueState now accepts explicit closedChannelID and implements dual placement: fresh epoch at version 0 for in-channel previous states (clean ledger seeded by amount), or appended at prev.Version+1 for detached-tip states (inheriting prev ledger and adding amount).
Net-Transition Sum Query Simplification nitronode/store/database/interface.go, nitronode/store/database/state.go, nitronode/event_handlers/testing.go, nitronode/store/database/db_store_test.go, pkg/blockchain/evm/channel_hub_reactor_test.go	Removes epoch parameter from SumNetTransitionAmountAfterVersion across all implementations, mocks, and test call-sites; SQL logic now uses only home_channel_id and version > minVersion predicates, excluding post-finalize rows.
Unconditional Challenge Rescue & Cross-Channel Query nitronode/event_handlers/service.go	HandleHomeChannelClosed now unconditionally calls issueChallengeRescue when wasChallenged is true (prior conditional on node-signed Finalize removed); issueChallengeRescue uses GetLastUserState(wallet, asset, false) instead of channel-specific lookup and removes invariant check on prev.Version <= closureVersion.
Challenge Rescue Test Coverage Updates nitronode/event_handlers/service_test.go	Updates existing rescue scenario tests to use new GetLastUserState and simplified SumNetTransitionAmountAfterVersion calls; introduces TestHandleHomeChannelClosed_TimeoutAfterFinalize_AppendsRescue verifying rescue placement after detached-tip states; adjusts coop-close and collision-regression tests with detached-tip fixtures and zero-amount off-channel rescue assertions.
Event Ordering & Idempotency Documentation nitronode/api/app_session_v1/handler.go, nitronode/api/channel_v1/handler.go, pkg/blockchain/evm/listener.go	Expanded block comments documenting MF3-I01 release-path and challenge-rescue ordering guarantees, event-handler delivery invariants, and the prerequisite that HandleHomeChannelChallenged executes before HandleHomeChannelClosed for wedge prevention.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• layer-3/nitrolite#759: Modifies the same HandleHomeChannelClosed/issueChallengeRescue behavior and core.NewChallengeRescueState construction plumbing as this PR's refactoring.
• layer-3/nitrolite#745: Updates Prometheus gauge labeling from Origin to application_id with associated exporter and interface changes.

Suggested reviewers

• dimast-x
• nksazonov
• ihsraham

Poem

🐰 The rescue states now leap across the channels wide,
With closedChannelID to guide them on their ride,
And cross-user queries spanning all the chain,
Aggregate bounds keep ledgers sane—no overflow pain!
MF3-I01's promises are plainly documented here,
Event ordering guaranteed, the pathway crystal clear.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'fix: audit findings round final 3' is vague and generic, using non-descriptive phrasing that does not convey the specific nature of the changes (aggregate validation bounds, challenge-rescue emission logic, or documentation updates).	Revise the title to be more specific about the primary changes, e.g., 'fix: add aggregate bounds validation and always emit challenge rescue on Challenged→Closed' or focus on the most impactful fix.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/audit-findings-finalx3

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

pkg/core/types_test.go (1)

248-277: ⚡ Quick win

Assert full ledger inheritance in the detached-path rescue test.

This case is named as “inheriting ledger,” but it currently only checks balances/net-flows. Add assertions for TokenAddress and BlockchainID so the intended invariant is pinned.

Suggested test tightening

 		prev := State{
 			Asset:         "USDC",
 			UserWallet:    "0xUser",
 			Epoch:         4,
 			Version:       2,
 			HomeChannelID: nil,
 			HomeLedger: Ledger{
+				TokenAddress: "0xToken",
+				BlockchainID: 1,
 				UserBalance: decimal.NewFromInt(10),
 				UserNetFlow: decimal.Zero,
 				NodeBalance: decimal.Zero,
 				NodeNetFlow: decimal.NewFromInt(10),
 			},
 		}
@@
 		assert.Equal(t, prev.Epoch, rescue.Epoch)
 		assert.Equal(t, prev.Version+1, rescue.Version)
 		assert.Nil(t, rescue.HomeChannelID)
+		assert.Equal(t, prev.HomeLedger.TokenAddress, rescue.HomeLedger.TokenAddress)
+		assert.Equal(t, prev.HomeLedger.BlockchainID, rescue.HomeLedger.BlockchainID)
 		assert.True(t, decimal.NewFromInt(52).Equal(rescue.HomeLedger.UserBalance), "got %s", rescue.HomeLedger.UserBalance.String())
 		assert.True(t, decimal.NewFromInt(52).Equal(rescue.HomeLedger.NodeNetFlow), "got %s", rescue.HomeLedger.NodeNetFlow.String())

🤖 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 `@pkg/core/types_test.go` around lines 248 - 277, The test "Success - detached
prev appends at next version inheriting ledger" currently only asserts
balances/net-flows but claims full ledger inheritance; update the assertions
after NewChallengeRescueState to also assert that prev.HomeLedger.TokenAddress
and prev.HomeLedger.BlockchainID are preserved on rescue.HomeLedger (e.g.,
require.Equal(t, prev.HomeLedger.TokenAddress, rescue.HomeLedger.TokenAddress)
and require.Equal(t, prev.HomeLedger.BlockchainID,
rescue.HomeLedger.BlockchainID)); locate these fields on the State.HomeLedger
and the NewChallengeRescueState result to add the two checks.

🤖 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 `@pkg/core/types_test.go`:
- Around line 248-277: The test "Success - detached prev appends at next version
inheriting ledger" currently only asserts balances/net-flows but claims full
ledger inheritance; update the assertions after NewChallengeRescueState to also
assert that prev.HomeLedger.TokenAddress and prev.HomeLedger.BlockchainID are
preserved on rescue.HomeLedger (e.g., require.Equal(t,
prev.HomeLedger.TokenAddress, rescue.HomeLedger.TokenAddress) and
require.Equal(t, prev.HomeLedger.BlockchainID, rescue.HomeLedger.BlockchainID));
locate these fields on the State.HomeLedger and the NewChallengeRescueState
result to add the two checks.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1ec74c55-61c9-491d-b0f1-acc0da649e37

📥 Commits

Reviewing files that changed from the base of the PR and between 5ce6458 and 0e638a1.

⛔ Files ignored due to path filters (1)

• sdk/mcp/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (16)

• nitronode/api/app_session_v1/handler.go
• nitronode/api/channel_v1/handler.go
• nitronode/event_handlers/service.go
• nitronode/event_handlers/service_test.go
• nitronode/event_handlers/testing.go
• nitronode/store/database/db_store_test.go
• nitronode/store/database/interface.go
• nitronode/store/database/state.go
• pkg/blockchain/evm/channel_hub_reactor.go
• pkg/blockchain/evm/channel_hub_reactor_test.go
• pkg/blockchain/evm/listener.go
• pkg/core/interface.go
• pkg/core/state_packer.go
• pkg/core/state_packer_test.go
• pkg/core/types.go
• pkg/core/types_test.go