fix: reject limit == i128::MAX in TokenTransferPolicy - #120
Open
alberto-crossmint wants to merge 1 commit into
Open
fix: reject limit == i128::MAX in TokenTransferPolicy#120alberto-crossmint wants to merge 1 commit into
alberto-crossmint wants to merge 1 commit into
Conversation
extract_transfer_total and check_spending_limit both use checked_add(...).unwrap_or(i128::MAX) — saturating on overflow. That is safe as long as the configured limit is strictly less than i128::MAX, because the subsequent `new_total > limit` check rejects overflowed values correctly. But `limit == i128::MAX` degenerates into unlimited spending: `i128::MAX > i128::MAX` is false, so every spend is approved and the tracker saturates and stays saturated. validate_policy already rejects `limit <= 0`; extend it to also reject `limit == i128::MAX`. Same error code (InvalidPolicy) since the surface to integrators is unchanged: pass a sensible positive bound.
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
1 participant
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.
Summary
TokenTransferPolicyarithmetic inextract_transfer_totalandcheck_spending_limituses saturating adds (checked_add(...).unwrap_or(i128::MAX)). That design is safe for anylimit < i128::MAX, because the subsequentnew_total > limitcomparison correctly rejects saturated values.It is not safe for
limit == i128::MAX:new_total = tracker.spent.checked_add(total_amount).unwrap_or(i128::MAX)if new_total > limit→if i128::MAX > i128::MAX→falsei128::MAX, and it stays there.So
limit = Some(i128::MAX)silently degenerates into unlimited spending.validate_policyalready rejectslimit <= 0. Extend the guard to also rejectlimit == i128::MAX, using the sameInvalidPolicyerror code (the surface to integrators is unchanged: pass a sensible positive bound).Test plan
test_on_add_rejects_i128_max_limit— registering a signer withlimit = Some(i128::MAX)panics withInvalidPolicy (#80).test_on_add_rejects_zero_limit/test_on_add_rejects_negative_limitstill pass — same code path, same error.token_transfer_policy_testcases pass (39 existing + 1 new).Risk
Trivial. Anyone who had configured
i128::MAXon purpose was misconfigured (unlimited spending dressed up as a tracked cap). If that's the intent, the correct configuration islimit: None, which bypasses the tracker entirely.