Skip to content

Security: UInt64 overflow in LP residual weight permanently locks funds above ~$10k LP - #19

Merged
andsav merged 1 commit into
qmrkt:mainfrom
y4motion:fix/uint64-overflow-lp-residual-weight
May 1, 2026
Merged

Security: UInt64 overflow in LP residual weight permanently locks funds above ~$10k LP#19
andsav merged 1 commit into
qmrkt:mainfrom
y4motion:fix/uint64-overflow-lp-residual-weight

Conversation

@y4motion

@y4motion y4motion commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Bug Bounty Submission (Issue #13)

Two issues found during manual review of market_app/contract.py:

Finding 1: UInt64 Overflow in LP Residual Weight (Medium)

_entry_weighted_sum_checked (L587–590) computes shares × timestamp with a mulw overflow guard. When called from _total_residual_weight() with lp_shares_total × (settlement_timestamp − 1) > 2⁶⁴, the assert fires and permanently blocks claim_lp_residual() for all LPs.

Threshold (2026): $10,376 USDC total LP contribution
Impact: LP residual permanently locked — no LP can claim
Trend: Threshold decreases over time (
$9,742 by 2030, ~$8,350 by 2040)

Call chain:

claim_lp_residual → _claimable_residual → _total_residual_weight
  → _calculate_weight → _entry_weighted_sum_checked(lp_shares_total, settlement_ts-1)
  → op.mulw OVERFLOW → REVERT

Secondary: the total_lp_weighted_entry_sum addition (L1110–1112) uses native + which wraps silently on UInt64 overflow in AVM, potentially corrupting weight calculations when many small LP entries accumulate.

Fix: Use 128-bit intermediates via mulw/divmodw in _calculate_weight, consistent with how lmsr_mul_div_floor already handles wide arithmetic.

Finding 2: Missing Inventory Coverage Invariant (Low)

The reference model asserts q[i] >= total_user_shares[i] but the on-chain _assert_invariants() (L631–666) omits this check. Defense-in-depth gap.

PoC

5 passing tests in tests/test_poc_residual_overflow.py:

PYTHONPATH=. .venv/bin/python -m pytest tests/test_poc_residual_overflow.py -v
# 5 passed

Full details in VULNERABILITY_REPORT.md.

closes #13

…nvariant

Finding 1 (Medium): _entry_weighted_sum_checked reverts when
lp_shares_total * settlement_timestamp > 2^64, permanently blocking
claim_lp_residual() for all LPs. Threshold: ~$10,376 USDC (2026).
Decreases over time as Unix timestamps grow.

Finding 2 (Low): q[i] >= total_user_shares[i] is enforced in the
reference model but missing from _assert_invariants on-chain.

Includes 5-test PoC suite (tests/test_poc_residual_overflow.py).
@andsav

andsav commented May 1, 2026

Copy link
Copy Markdown
Collaborator

Hi @y4motion ,

Thank you for your report! I will review the claims and repro and get back to you.

@andsav

andsav commented May 1, 2026

Copy link
Copy Markdown
Collaborator

UInt64 Overflow in LP Residual Weight

The primary claim is valid and earns the bounty.

uses native + which wraps silently on UInt64 overflow in AVM, potentially corrupting weight calculations when many small LP entries accumulate

+ doesn't wrap around silently on AVM on overflow, it panics, the transaction doesn't go through

The reference model asserts q[i] >= total_user_shares[i] but the on-chain _assert_invariants() (L631–666) omits this check.

Low-priority, since no other bug surfaced this missing invariant. Could add it as defense-in-depth.

@andsav
andsav merged commit 1cd9fec into qmrkt:main May 1, 2026
1 check passed
@y4motion

y4motion commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

@andsav Thanks for the review and merge! Glad the report was helpful for the protocol's security. I will claim the bounty via the DevLoot platform.

@andsav

andsav commented May 1, 2026

Copy link
Copy Markdown
Collaborator

@y4motion looks like the issue didn't link to the devloot bounty because the "closes #xx" line was not in the PR description (cc: @p2arthur I think there should be a way to manually link or re-open an issue)

please send me your algo address, I can send you manually

@y4motion

y4motion commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

@andsav Thanks for sorting this out! Apologies for the missing Closes #XX — will include it going forward.

Here's my Algo address:

VBVECJA7Z6F4SF57L2FQGHPOWUL5XGYSLDLJ6TJDA3LDJWSUJLX5FCP25I

@andsav

andsav commented May 2, 2026

Copy link
Copy Markdown
Collaborator

@p2arthur

p2arthur commented May 2, 2026

Copy link
Copy Markdown
Contributor

Sorry guys, just saw it here. Were you guys able to set this up? @andsav were you able to refund the bounty from the platform?

I will think about a way for bounties to work on those kind of situations.

@andsav

andsav commented May 2, 2026

Copy link
Copy Markdown
Collaborator

@p2arthur yup, I withdrew and sent it to their wallet directly, nothing lost here!

I think allowing the bounty creator to link a PR directly would be the easiest, the PR -> Issue link might not always be there so this can be brittle.

@p2arthur

p2arthur commented May 2, 2026

Copy link
Copy Markdown
Contributor

Awesome, its a flows I wasn't able to test in production before, so glad it worked as intended when closing the issue without a PR.

Yeah, I can give the option of the bounty creator to link a PR and reward the creator instead of only enabling them to refund. Sounds like an easy way to fix that

@y4motion

y4motion commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for sorting this out @andsav, funds received! And thanks for checking in @p2arthur 🤝

The manual PR-link override idea sounds like a solid UX improvement for DevLoot — would definitely smooth out these edge cases. Happy to keep contributing to the ecosystem, just ping me whenever new bounties drop!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

question.market contracts bug bounty

3 participants