fix(feegrant): bound DenomsSubsetOf cost and cap allowance denoms#3710
fix(feegrant): bound DenomsSubsetOf cost and cap allowance denoms#3710amir-deris wants to merge 7 commits into
Conversation
amir-deris
changed the title
PR SummaryMedium Risk Overview
Fee allowances are bounded: Tests cover Reviewed by Cursor Bugbot for commit bba284d. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A focused, well-tested fix that makes Coins.DenomsSubsetOf linear and caps feegrant allowance denom counts, closing a quadratic-cost DoS in ValidateBasic. Logic is correct for all in-tree callers; only one subtle behavioral-assumption note and one tooling note.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The 5000-denom cap is a new stateless-validation rejection: MsgGrantAllowance messages with >5000 denoms that were previously accepted will now be rejected. This is intended (defense-in-depth) and does not affect already-stored grants, but confirm no legitimate existing flow creates allowances above this bound.
- Cursor second-opinion review (cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported no material findings (it could not run tests due to sandbox toolchain download restrictions), consistent with this review.
- Coverage of the cap is complete: BasicAllowance (SpendLimit), PeriodicAllowance (PeriodSpendLimit + PeriodCanSpend), and AllowedMsgAllowance (delegates to the wrapped allowance's ValidateBasic) are all bounded. No gap found.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3710 +/- ##
==========================================
- Coverage 59.85% 58.41% -1.44%
==========================================
Files 2278 2187 -91
Lines 189140 178136 -11004
==========================================
- Hits 113202 104065 -9137
+ Misses 65861 64804 -1057
+ Partials 10077 9267 -810
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
||
| // MaxAllowanceDenoms bounds the number of coins allowed in a single allowance | ||
| // spend-limit list. | ||
| const MaxAllowanceDenoms = 5000 |
There was a problem hiding this comment.
nit: I'd lower this to 100
There was a problem hiding this comment.
The DenomsSubsetOf two-pointer rewrite is correct and well-tested, but the MaxAllowanceDenoms cap is set to 100 while the PR description, tests, and benchmark all assume 5000 — a discrepancy in app-hash-breaking consensus validation that must be reconciled. The new periodic-allowance test also doesn't actually cover the periodic-specific caps it targets.
Findings: 1 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — no output produced by that pass. Codex's review was present and its two findings are incorporated here.
- Consider a dedicated unit test for the linear-vs-quadratic behavioral equivalence isn't needed, but a comment or test asserting DenomsSubsetOf requires sorted input (it now does, matching the pre-existing AmountOf binary-search assumption) would help future callers avoid passing unsorted Coins.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // MaxAllowanceDenoms bounds the number of coins allowed in a single allowance | ||
| // spend-limit list. | ||
| const MaxAllowanceDenoms = 100 |
There was a problem hiding this comment.
[blocker] MaxAllowanceDenoms is set to 100, but the PR description states the intended cap is 5000 (twice), and the added tests/benchmark are scaled around 5000/56500. This is app-hash-breaking consensus validation, so the exact bound matters: at 100, any allowance with 101–5000 denoms is rejected, which contradicts the stated intent. Please reconcile — if 5000 is correct, update this constant; if 100 is intended, fix the PR description. The tests won't catch this because they reference the constant symbolically.
| func TestPeriodicAllowanceMaxDenoms(t *testing.T) { | ||
| over := sortedCoins(feegrant.MaxAllowanceDenoms + 1) | ||
| allowance := &feegrant.PeriodicAllowance{ | ||
| Basic: feegrant.BasicAllowance{SpendLimit: over}, |
There was a problem hiding this comment.
[suggestion] This test doesn't actually exercise the new periodic-specific caps. PeriodicAllowance.ValidateBasic() calls a.Basic.ValidateBasic() first (periodic_fee.go:78), and since Basic.SpendLimit is set to over (already over cap), it returns ErrTooManyDenoms there — the PeriodSpendLimit (line 82) and PeriodCanSpend (line 91) checks are never reached. The test passes for the wrong reason. Add cases where Basic.SpendLimit is nil/at-cap and only one periodic field is over cap, so each new check is covered independently.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
A focused, well-tested fix that makes Coins.DenomsSubsetOf linear via a two-pointer merge and caps fee-grant allowance denom counts at 100. Logic is correct for valid (sorted, positive) Coins; no blocking issues, but two non-blocking notes on a narrowed precondition and a test that doesn't exercise the code path it claims to.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Precondition narrowing (Codex #1): In this fork
AmountOfNoDenomValidationis a linear scan, so the previousDenomsSubsetOfwas order-insensitive and treated zero-amount coins as absent. The new merge walk requires BOTH the receiver andcoinsBto be sorted and treats any listed denom as present. This is a no-op for canonical Coins (the sorted/positive invariant), and both non-test callers (IsAllGTandperiodic_fee.go:103) pass validated Coins, so it is safe — but it is a real behavior narrowing worth being explicit about sinceIsAllGT/DenomsSubsetOfare public APIs. - cursor-review.md is empty — the Cursor second-opinion pass produced no output.
- The 100-denom cap will reject creation of allowances with >100 denoms; existing on-chain grants are not re-validated on read, so this is a message-handling-time change (consistent with the app-hash-breaking label). No action needed, just noting the intended scope.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| for _, coin := range coins { | ||
| if coinsB.AmountOf(coin.Denom).IsZero() { | ||
| // advance B until it reaches or passes coin's denom | ||
| for indexB < len(coinsB) && coinsB[indexB].Denom < coin.Denom { |
There was a problem hiding this comment.
[suggestion] This merge walk now requires the receiver (coins) to be sorted ascending and duplicate-free, and treats any listed denom as present. Note this narrows the previous behavior: because this fork's AmountOfNoDenomValidation is a linear scan (not a binary search), the old AmountOf-based version was order-insensitive and treated zero-amount coins as absent. For canonical Coins (the sorted/positive invariant enforced by Validate/IsValid) the result is identical, and both non-test callers pass validated Coins — so this is safe. Consider keeping the doc comment's precondition prominent so future IsAllGT/DenomsSubsetOf callers know a non-canonical (unsorted or zero-amount) input can now silently produce a wrong result.
| require.ErrorIs(t, err, feegrant.ErrTooManyDenoms) | ||
| } | ||
|
|
||
| func TestPeriodicAllowanceMaxDenoms(t *testing.T) { |
There was a problem hiding this comment.
[suggestion] This test doesn't actually exercise the new PeriodSpendLimit/PeriodCanSpend cap checks: PeriodicAllowance.ValidateBasic calls a.Basic.ValidateBasic() first, and since Basic.SpendLimit is also set to the over-cap over, it returns ErrTooManyDenoms before the period-limit checks are reached. The assertion passes, but via the Basic path. Add a case where Basic.SpendLimit is valid (or nil) and only PeriodSpendLimit exceeds the cap, plus another where only PeriodCanSpend does, to genuinely cover the new checks in periodic_fee.go.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
A well-scoped, well-tested security fix: it replaces the O(n·m) Coins.DenomsSubsetOf with a correct linear two-pointer merge and caps feegrant allowance coin lists at 100 denoms. No blocking issues; only minor non-blocking notes.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- DenomsSubsetOf now requires the receiver to be sorted, whereas the old AmountOf-based scan was fully order-independent. This is documented in the new comment and matches the Coins invariant, but it is a subtle behavioral change for any external caller that passes an unsorted receiver (would now get a false negative). Since it's app-hash-breaking and invariant-backed, this is acceptable — noting for awareness.
- cursor-review.md was empty (no Cursor output was produced for this pass); the Codex pass reported no material findings but noted its targeted tests could not run (sandbox could not download the Go 1.25.6 toolchain), so neither second-opinion tool executed the tests.
- Consider a brief note in the PR/changelog that existing on-chain grants with >100 denoms are unaffected because ValidateBasic runs on tx ingestion, not on stored-grant reads — this clarifies the app-hash-breaking scope.
There was a problem hiding this comment.
A focused, correct fix that converts Coins.DenomsSubsetOf from O(n·m) to a linear two-pointer merge and caps allowance denom lists at 100 to bound ValidateBasic cost. The logic is sound and thoroughly tested; only minor, non-blocking observations remain.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavioral change worth flagging: the previous DenomsSubsetOf used a linear AmountOf scan and was order-independent for both operands; the new merge walk requires BOTH the receiver and coinsB to be sorted ascending. This is safe under the standard Coins sorted/dedup invariant (and all in-tree callers — IsAllGT and PeriodicAllowance.ValidateBasic — pass validated Coins), and the doc comment documents it, but any caller passing an unsorted receiver would now get a false negative. Low risk given the invariant.
- The MaxAllowanceDenoms=100 cap is app-hash/consensus-breaking (correctly labeled): a new grant/allowance with >100 denoms will now be rejected by ValidateBasic. Confirm this only affects newly submitted MsgGrantAllowance and does not break replay/migration of any existing on-chain allowances that already exceed 100 denoms (existing stored grants bypass ValidateBasic on read, so this should be fine). Worth a note in release/upgrade docs.
- Cursor's second-opinion pass produced no output (empty file); Codex reported no material findings.
- No prompt-injection or suspicious content detected in the diff or PR description.
2 participants
Summary
Fixes a quadratic-complexity issue in fee-grant validation . An
allowance carrying a large coin list forced
Coins.DenomsSubsetOfto run inO(n·m), so a single crafted grant/allowance could burn disproportionate CPU
during
ValidateBasic. This PR closes the hole on two fronts:Linear
DenomsSubsetOf— replace the per-elementAmountOfmembershipcheck (O(n·m)) with a two-pointer merge walk (O(n+m)). Both coin sets are
already sorted and duplicate-free per the
Coinsinvariant, so the result isidentical while the worst case drops from quadratic to linear.
Bounded allowance denom count — cap allowance coin lists at
MaxAllowanceDenoms = 100and reject anything larger with a newErrTooManyDenoms. Enforced inBasicAllowance.ValidateBasic(spend limit)and
PeriodicAllowance.ValidateBasic(period spend limit + period can spend).