fix(ICRC_Rosetta): DEFI-2944: prevent overflow before ingress-window validation in ICRC1 construction payloads#10807
Open
mbjorkqvist wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the ICRC1 Rosetta construction_payloads endpoint against a caller-triggered integer overflow when defaulting ingress_end from caller-controlled ingress_start, ensuring malformed metadata is rejected via existing ingress-window validation instead of panicking.
Changes:
- Compute default
ingress_endusingsaturating_addto avoid overflow beforevalidate_ingress_windowruns. - Add a regression test asserting a near-
u64::MAXingress_startis rejected without panic and returns the expected error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…loads The default `ingress_end` is computed as `ingress_start + ingress_interval` before `validate_ingress_window` runs. Since `ingress_start` comes from optional, caller-controlled metadata, a near-`u64::MAX` `ingress_start` with `ingress_end` omitted overflows this addition and panics the request thread, leaving the ICRC1 Rosetta construction endpoint with a request-triggered DoS despite the ingress-window guard. This test documents the current (panicking) behaviour via `#[should_panic]`; the follow-up commit fixes it and updates the test to assert the request is rejected with an error instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ICRC1 construction payloads Compute the default `ingress_end` with `saturating_add` instead of `+`. A near-`u64::MAX` `ingress_start` from caller metadata now saturates to `u64::MAX` rather than overflowing and panicking, and the resulting out-of-range window is rejected by the existing `validate_ingress_window` check, returning a clean error to the client. The regression test added in the previous commit is updated to assert the request is rejected with an error rather than panicking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mbjorkqvist
force-pushed
the
mathias/DEFI-2944-overflow-ingress-window-validation
branch
from
July 17, 2026 11:12
d0c6b5f to
a6d8ed3
Compare
mbjorkqvist
changed the title
|
✅ No security or compliance issues detected. Reviewed everything up to a6d8ed3. Security Overview
Detected Code Changes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ICRC1 Rosetta
construction_payloadsendpoint computed the defaultingress_endbefore the ingress-window validation ran. Becauseingress_startcomes from optional, caller-controlled metadata, a near-u64::MAXingress_startwithingress_endomitted overflowed that computation and panicked the request thread — a request-triggered DoS despite the existing ingress-window guard.The default is now computed with a saturating add, so an out-of-range window is rejected with a clean error by the existing validation instead of panicking.
The change is split into two commits: the first adds a test that exposes the panic, the second applies the fix and updates the test to assert the request is rejected with the expected error.