Skip to content

fix(distribution): align withdraw-address checks with bank send restrictions#3792

Open
codchen wants to merge 1 commit into
mainfrom
claude/validator-withdraw-crash-fix-c12f45
Open

fix(distribution): align withdraw-address checks with bank send restrictions#3792
codchen wants to merge 1 commit into
mainfrom
claude/validator-withdraw-crash-fix-c12f45

Conversation

@codchen

@codchen codchen commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

The distribution module decides whether a delegator's withdraw address may receive funds using its local blocked-address set together with CanSendTo. Those conditions do not fully match the restrictions the bank keeper enforces when it actually transfers from a module account, so an address accepted by the distribution checks can still be refused by the bank layer at send time.

This change consults the bank keeper's BlockedAddr in both places that gate a withdraw address, keeping the receivability decision in lockstep with the actual transfer:

  • SetWithdrawAddr — rejects such an address up front, consistent with the bank's transfer restrictions.
  • canReceiveWithdrawAddr — the internal receivability check used by GetDelegatorWithdrawAddr, so every reward/commission payout path resolves to an address the bank will accept.

Behavior is otherwise unchanged: when a withdraw address is not receivable, the payout falls back to the delegator, and any residual commission is routed to the community pool as before.

Testing

  • go test ./sei-cosmos/x/distribution/... — pass
  • Added regression tests for both entry points (the SetWithdrawAddr rejection and the AfterValidatorRemoved fallback), each verified to fail without the change.
  • gofmt / goimports / go vet clean; go build ./app/... clean.

🤖 Generated with Claude Code

…ictions

The distribution module gates whether a withdraw address may receive funds on
its local blocked-address set plus CanSendTo, which does not fully match the
restrictions the bank keeper applies to module-to-account transfers. As a
result an address accepted here could still be refused by the bank layer at
send time.

Consult the bank keeper's BlockedAddr in both SetWithdrawAddr and the internal
receivability check so the decision stays in lockstep with the actual transfer
gate. Existing behavior is preserved when an address is not receivable: the
payout falls back to the delegator and any residual commission is routed to the
community pool. Adds regression tests covering both entry points.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 23, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches reward/commission payout and validator-removal paths in EndBlock; behavior change is narrowly scoped to previously accepted-but-unpayable addresses, with tests for the coinbase-prefix gap.

Overview
Distribution withdraw-address validation now matches what the bank enforces on module-to-account sends by also calling bankKeeper.BlockedAddr, not only the local module blocklist and CanSendTo.

SetWithdrawAddr rejects blocked recipients up front (including reserved evm_coinbase-prefixed addresses that CanSendTo still allows). canReceiveWithdrawAddr uses the same rule so GetDelegatorWithdrawAddr and payout paths do not treat an address as receivable when SendCoinsFromModuleToAccount would fail—avoiding EndBlock panics in AfterValidatorRemoved force-withdraw for legacy bad withdraw addresses (fallback to the operator remains).

The distribution BankKeeper interface gains BlockedAddr. Regression tests cover rejection at set time and non-panicking fallback when a coinbase-prefixed withdraw addr was stored before the guard existed.

Reviewed by Cursor Bugbot for commit 3c07181. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJul 23, 2026, 3:54 AM

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A well-scoped, correct defensive fix that aligns the distribution module's withdraw-address receivability checks with what the bank keeper actually enforces at send time (BlockedAddr, which rejects evm_coinbase-prefixed addresses that CanSendTo misses). Both entry points are covered by regression tests; no blocking issues found.

Findings: 0 blocking | 2 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex reported no material findings, consistent with this review.
  • Minor redundancy: distribution's local k.blockedAddrs[withdrawAddr.String()] check overlaps with bank's BlockedAddr, which also consults the bank's own module blocklist. They are distinct maps so this is harmless belt-and-suspenders, not a defect — no change needed.

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.24%. Comparing base (96fa55d) to head (3c07181).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3792      +/-   ##
==========================================
- Coverage   60.20%   59.24%   -0.97%     
==========================================
  Files        2328     2236      -92     
  Lines      194502   184004   -10498     
==========================================
- Hits       117104   109013    -8091     
+ Misses      66849    65295    -1554     
+ Partials    10549     9696     -853     
Flag Coverage Δ
sei-chain-pr 64.88% <100.00%> (?)
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-cosmos/x/distribution/keeper/keeper.go 87.50% <100.00%> (+0.32%) ⬆️

... and 92 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — this correctly closes the gap between distribution's withdraw-address checks and what the bank keeper actually enforces on module-to-account sends.

Extended reasoning...

Overview

The PR adds a bankKeeper.BlockedAddr check to two gates in x/distribution/keeper/keeper.go: SetWithdrawAddr (reject up front) and canReceiveWithdrawAddr (used by GetDelegatorWithdrawAddr in payout paths). It adds BlockedAddr to the distribution module's BankKeeper interface, which is trivially satisfied by the existing bank keeper implementation. I verified against sei-cosmos/x/bank/keeper/send.go and keeper.go that SendCoinsFromModuleToAccount does gate on BlockedAddr (including the evm_coinbase-prefixed dynamically-derived addresses that CanSendTo does not catch), so the fix genuinely closes a real gap between the distribution module's local receivability decision and the bank's actual transfer-time enforcement.

Security risks

None introduced. This only tightens an existing check — it can only cause previously-accepted-but-unpayable addresses to be rejected or fall back to the delegator/community pool, never the reverse. No new attack surface, no auth/crypto logic changed.

Level of scrutiny

Low-to-medium. The change touches EndBlock-adjacent code (AfterValidatorRemoved force-withdraw), so correctness matters, but the change itself is a narrow, additive predicate tightening with clear fallback semantics that were already in place (unpayable withdraw address falls back to delegator; unpayable delegator routes commission to community pool). The interface change is a simple addition already implemented by the concrete bank keeper.

Other factors

Tests are strong: new regression tests cover both the SetWithdrawAddr rejection and the AfterValidatorRemoved non-panicking fallback for the coinbase-prefixed address case, each described as failing without the fix. No CODEOWNER-restricted or crypto-sensitive paths are touched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants