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

[nksazonov]

Description

A user submits an EscrowDeposit finalize request to the off-chain API, the backend validates it with the state model, signs it with the node key, and stores it immediately as the latest signed state before any on-chain execution is confirmed.
The problem is that the Go validator models EscrowDeposit differently from the smart contract. Off-chain, ApplyEscrowDepositTransition increases the user’s home balance and also increases home NodeNetFlow.

    state.HomeLedger.UserBalance = state.HomeLedger.UserBalance.Add(newTransition.Amount)
    state.HomeLedger.NodeNetFlow = state.HomeLedger.NodeNetFlow.Add(newTransition.Amount)

However, on-chain, home-chain FINALIZE_ESCROW_DEPOSIT requires the node allocation to be cleared and both home-side net-flow deltas to remain unchanged.

        require(candidate.homeLedger.nodeAllocation == 0, IncorrectNodeAllocation());
        // nothing changes from initiate escrow deposit state
        require(userNfDelta == 0, IncorrectUserNetFlowDelta());
        require(nodeNfDelta == 0, IncorrectNodeNetFlowDelta());

So the backend can sign and store an escrow-finalize state that the contract itself would reject.
That mismatch is exploitable because the attacker does not need to execute the invalid escrow-finalize state on-chain. They only need the backend to accept it as the latest signed state. Once that happens, the inflated off-chain home balance can be used as the basis for a later normal HomeWithdrawal request. That later state can still satisfy the contract’s withdrawal checks when compared against the real on-chain channel state, allowing the attacker to withdraw node liquidity that was never actually credited on-chain.

Summary by CodeRabbit

• Bug Fixes

  • Corrected escrow deposit transition logic to properly clear node balance during ledger updates instead of modifying net flow.

• Tests

  • Added comprehensive test coverage for escrow deposit transition validation, including scenarios for successful deposits, invalid balance states, mismatched amounts, and incorrect transition sequencing.

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ 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.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds comprehensive test coverage for escrow deposit transition validation in StateAdvancerV1 and updates the ApplyEscrowDepositTransition logic to set the node balance to zero instead of incrementing net flow. Existing tests are refactored to validate both simple and realistic state scenarios.

Changes

Cohort / File(s)	Summary
Escrow Deposit Validation Tests pkg/core/state_advancer_test.go	Introduces comprehensive test suite with helper newMutualLockState and five validation sub-cases covering successful transitions, home ledger mismatches, amount discrepancies, and improper transition sequencing.
Escrow Deposit Transition Logic pkg/core/types.go	Updates ApplyEscrowDepositTransition to set HomeLedger.NodeBalance to zero instead of incrementing HomeLedger.NodeNetFlow during escrow deposits.
Escrow Deposit Transition Test Refactoring pkg/core/types_test.go	Splits TestState_ApplyEscrowDepositTransition into two subtests: basic user balance adjustment and a comprehensive scenario validating node balance clearing while preserving net flows.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• YNU-679: apply and validate transitions #491: Introduced ValidateAdvancement and the original ApplyEscrowDepositTransition semantics that this PR directly extends with updated logic and validation tests.

Suggested reviewers

• philanton
• dimast-x
• ihsraham

Poem

🐰 A ledger so balanced, a bunny's delight,
Where escrow deposits set node balances right,
Zero and steady, the flow stays the same,
Tests that run parallel, in victory's name! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title correctly identifies the main change: fixing the EscrowDeposit transition to clear node balance instead of incrementing node net flow, aligning backend validation with on-chain contract requirements.

_{✏️ 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-m-c02

---

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)

225-239: Use a reachable escrow-deposit fixture in this subtest.

ValidateAdvancement only accepts escrow deposits whose amount matches the preceding mutual lock, but this case seeds EscrowLedger.UserBalance = 100 and applies amount = 10. That exercises a partial finalize path the validator will never accept. Prefer constructing the fixture via ApplyMutualLockTransition (or matching the escrow balances/flows to amount) so the regression stays aligned with the real validator path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/core/types_test.go` around lines 225 - 239, The subtest seeds
EscrowLedger.UserBalance=100 and then calls ApplyEscrowDepositTransition with
amount=10 which creates an escrow-deposit that ValidateAdvancement will reject
because it must match the preceding mutual lock; fix by making the test
construct a compatible prior mutual lock (use ApplyMutualLockTransition to
create the mutual lock with the same amount) or else set
EscrowLedger.UserBalance and the transition amount to match the mutual lock
semantics so that ApplyEscrowDepositTransition produces a reachable
escrow-deposit that ValidateAdvancement would accept; update the test to call
ApplyMutualLockTransition (or adjust EscrowLedger.UserBalance to equal amount)
before calling ApplyEscrowDepositTransition so the fixture mirrors real
validator flow.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/core/types_test.go`:
- Around line 225-239: The subtest seeds EscrowLedger.UserBalance=100 and then
calls ApplyEscrowDepositTransition with amount=10 which creates an
escrow-deposit that ValidateAdvancement will reject because it must match the
preceding mutual lock; fix by making the test construct a compatible prior
mutual lock (use ApplyMutualLockTransition to create the mutual lock with the
same amount) or else set EscrowLedger.UserBalance and the transition amount to
match the mutual lock semantics so that ApplyEscrowDepositTransition produces a
reachable escrow-deposit that ValidateAdvancement would accept; update the test
to call ApplyMutualLockTransition (or adjust EscrowLedger.UserBalance to equal
amount) before calling ApplyEscrowDepositTransition so the fixture mirrors real
validator flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 79687cbf-aa81-463b-a41a-74db69d334b9

📥 Commits

Reviewing files that changed from the base of the PR and between b9f0a18 and 03e4cd6.

📒 Files selected for processing (3)

• pkg/core/state_advancer_test.go
• pkg/core/types.go
• pkg/core/types_test.go