[UTXO-BUG] /rewards/epoch: unbounded fetchall enables OOM DoS and wallet enumeration#6563
Conversation
…nd wallet enumeration
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 7c8ef9d903c616c623e877ec8486a3197ada5d54.
Verdict: request changes.
The route change is moving in the right direction for the single-response OOM case: /rewards/epoch/<epoch> now parses limit/offset, caps limit at 1000, and uses SQL LIMIT ? OFFSET ? instead of fetching every reward row for an epoch.
There are still merge blockers in the added regression/proof file on this Windows checkout:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 6527d8b6d990eb7bfa8f32121748e1d355dd3016
python -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/test_rewards_epoch_unbounded_poc.py
# passed
python -m pytest -q node/test_rewards_epoch_unbounded_poc.py --tb=short
# FAILED: 2 failed, 4 passed
# UnicodeDecodeError: 'charmap' codec can't decode byte 0x90 ... while open(source_path).read()
python tools/bcos_spdx_check.py --base-ref origin/main
# FAILED: node/test_rewards_epoch_unbounded_poc.py is missing an SPDX header
The pytest failure is caused by _rewards_epoch_body() opening rustchain_v2_integrated_v2.2.1_rip200.py without an explicit encoding. On Windows that uses cp1252 here and fails before the source-scan assertions run. Please read with encoding="utf-8" (or avoid source scanning) and add the required SPDX header to the new test file.
One scope note: pagination bounds the one-shot response/OOM behavior, but it does not actually remove unauthenticated wallet enumeration because callers can iterate offsets. If the bounty/PR is meant to fully fix enumeration, it needs an auth/rate-limit/redaction change too; otherwise I would frame this as the OOM/single-response bound and leave enumeration as residual risk.
|
Thanks for the detailed feedback, @eliasx45. Both blockers are fixed in 9d0203f: 1. 2. SPDX header added to the test file All 6 pytest tests pass locally after both changes. On the scope note about wallet enumeration via offset iteration: agreed, pagination alone does not prevent a motivated caller from iterating offsets to enumerate the full set. This PR's stated goal is bounding the single-response OOM behavior. Enumeration via auth, rate limiting or field redaction is residual risk and can be tracked separately if the project wants to address it. |
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed current head 9d0203fa9e1192a59e91bbdc6843de6fd1a83cda after the test/SPDX follow-up. Verdict: approve.
The prior merge blockers are fixed: the new regression reads the integrated module with encoding=utf-8, and the new test file has the required SPDX header. The route still uses bounded limit plus SQL LIMIT ? OFFSET ?, so the one-shot epoch rewards response is capped instead of fetching every reward row into memory.
Validation on this head:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 66c13199561fd3c311cd9216a7e4c0032e1b96b5
python -m py_compile node/rustchain_v2_integrated_v2.2.1_rip200.py node/test_rewards_epoch_unbounded_poc.py
# passed
python -m pytest -q node/test_rewards_epoch_unbounded_poc.py --tb=short
# 6 passed
python tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK
Scope note remains: pagination bounds the single-response OOM behavior; unauthenticated full enumeration by iterating offsets is residual product/security scope if the project wants to address it separately.
|
Thank you for the thorough re-review and for running the full validation suite (py_compile, pytest 6 passed, SPDX check, merge-tree clean). Agreed on the scope note: the bounded limit/offset caps the single-response OOM but offset-walking enumeration is a separate product or security decision for the project to address if needed. |
|
Updated the test file to add Flask integration coverage. Added Section C: TestRewardsEpochFlaskRoute, a unittest.TestCase class with 7 tests that call GET /rewards/epoch/ through the real app.test_client(). Covers: response structure (epoch, limit, offset, rewards), default limit bound with 500 DB rows, explicit limit enforcement, limit=9999 clamped to 1000, offset pagination producing non-overlapping pages, empty epoch returning an empty list, non-integer limit returning 400. Sections A (source scan) and B (standalone SQL documentation) are unchanged. All 13 tests pass locally. On the wallet enumeration note from the prior review: pagination bounds the single-response OOM, but iterating offsets can still enumerate wallets. Treating that as residual risk outside this fix, as it requires a separate auth or rate-limit change. |
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed current head 6137c6ffba3638faeb06837e5058fa5865a0e438 after the Flask integration test follow-up.
Verdict: approve.
The new Section C coverage is a useful improvement: TestRewardsEpochFlaskRoute now exercises GET /rewards/epoch/<epoch> through the real integrated Flask app and covers response shape, default bound, explicit limit, max-limit clamping, offset pagination, empty epoch, and non-integer limit handling. Sections A/B still document the source-level bounded query and standalone SQL behavior.
Validation on this Windows checkout:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 8aec80850c94ba0439897cd9173514ea478c116f
..\Rustchain\.venv\Scripts\python.exe -m py_compile node\rustchain_v2_integrated_v2.2.1_rip200.py node\test_rewards_epoch_unbounded_poc.py
# passed
PYTHONPATH=node ..\Rustchain\.venv\Scripts\python.exe -m pytest -q node\test_rewards_epoch_unbounded_poc.py --tb=short
# 13 passed
..\Rustchain\.venv\Scripts\python.exe tools\bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK
Scope note remains unchanged: this bounds the single-response OOM path for /rewards/epoch/<epoch>; preventing full offset-walk enumeration would be a separate auth/rate-limit/redaction decision.
|
Thanks for the thorough re-review, @eliasx45. Glad the Section C Flask integration tests hold up on your Windows checkout and that TestRewardsEpochFlaskRoute covers the full response-shape, clamping, pagination and edge-case paths you were looking for. Noted on the scope note: full offset-walk enumeration protection is a separate auth/rate-limit concern outside this PR. |
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
crystal-tensor
left a comment
There was a problem hiding this comment.
LGTM! Code review approved by @cx95zz (QClaw automated review agent).
Reviewed for: correctness, security, test coverage, and code quality.
No issues found - APPROVED.
|
Merged + paid 25 RTC (Medium #2867 — /rewards/epoch OOM). Reserved at Calibration note: the PR body frames pagination as also fixing wallet enumeration. Codex authoritative review noted that pagination reduces memory blowup, but does not remove the ability to enumerate wallets from the endpoint. If wallet enumeration is a real concern, a separate fix (authentication required, or response filtering) is needed. Not blocking this pay — just flagging for severity calibration on future findings. |
Introduces three foundation pieces to eliminate the recurring UTXO-OOM bug class (4 [UTXO-BUG] fixes shipped this week — #6526, #6535, #6537, #6562, #6563, #6571 — all the same .fetchall() shape): 1. node/db_helpers.py (190 LOC): - fetch_page(conn, sql, params, *, limit, offset=0, max_limit=1000) - Always appends LIMIT/OFFSET before issuing SELECT - Rejects sql already containing LIMIT (case-insensitive) - Rejects limit > max_limit or negative limit/offset - fetch_one_or_none(conn, sql, params) - For queries that MUST return 0 or 1 row - Raises if >1 row materializes - count_estimate(conn, table, *, where=None, params=()) 2. tests/test_db_helpers.py (208 LOC, 23 tests): - Happy path, edge cases, limit enforcement - SQL-already-has-LIMIT rejection (upper/lower/mixed case) - offset behavior, semicolon handling, zero-limit, etc. - All 23 pass against in-memory sqlite 3. scripts/check_fetchall.sh (117 LOC): - CI guard greps node/ for .fetchall() outside tests/deprecated - For each hit: checks same-line or prior-line opt-in annotation # fetchall-ok: <reason> where reason in: bounded-by-schema, pragma-result, internal-test-helper, already-paginated - Currently informational (will be wired into GH Actions in Part B) What this PR does NOT do (left intentionally claimable for #6627 bounty): - Site conversion of the ~50 .fetchall() instances in node/rustchain_v2_integrated_v2.2.1_rip200.py - Annotation sweep on the ~175 legit sites across other modules - GH Actions wire-in (.github/workflows/check_fetchall.yml) - Part B (25 RTC): CI guard wire + annotation sweep - Part A2 (25 RTC, if claimed separately): main-file conversion Scott as author, NOT claiming the bounty — this is operator foundation work so contributors can claim the larger sweep against a stable helper. Closes: foundation portion of #6627 Refs: #6526, #6535, #6537, #6562, #6563, #6571 (already-merged instances of the class) Co-authored-by: Scott Boudreaux <scottbphone12@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug
GET /rewards/epoch/ calls fetchall() with no SQL LIMIT, then serialises every miner_id and reward amount in that epoch into a single JSON response. There is no authentication gate and no row cap:
Exploit scenarios
OOM DoS: A node with 50,000 enrolled miners accumulates one epoch_rewards row per miner per epoch. A single unauthenticated request fetches the entire table slice into RAM and serialises it to JSON in one shot. Repeated requests can exhaust server memory.
Wallet enumeration: Every active miner_id (wallet address) and its exact reward share is returned with no authentication. An attacker iterates epoch numbers starting from 0 to harvest all wallet addresses for targeted social-engineering or phishing.
Fix
Added limit and offset query parameters (matching the pattern used by /api/nodes). The SQL query now uses LIMIT/OFFSET. The limit is capped server-side at 1000 rows per request.
The response envelope also returns limit and offset fields so callers can paginate.
Test file
node/test_rewards_epoch_unbounded_poc.py
Section A verifies via source scan that the fixed function contains SQL LIMIT and OFFSET. Section B verifies via direct SQLite queries that rows are bounded, offset correctly skips rows, the 1000-row cap is enforced, and an empty epoch returns an empty list.
All 6 tests pass after the fix.
Wallet
RTC64aa3fc417e75224e1574acae906fea34d94d140