fix: mark_paid payout hardening, add_volume attribution, execute_proposal quorum manipulation (#619-622) - #625
Merged
Levi-Ojukwu merged 10 commits intoJul 29, 2026
Conversation
|
@mhikel66 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
Fixes for issues #619, Fixes #620, Fixes #621, Fixes #622. Note up front: #619 and #621 were both already resolved on
mainbefore this PR — details below, with the actual commits identified. #620 and #622 were genuinely still open and are fixed here.#619 —
mark_paidLP payout underflow — already fixed, hardened furtherThe exact line the issue cites (
lp_earned = primary_lp_payout - primary_lp_funded) was a bare subtraction in an earlier commit, butmainalready uses.saturating_sub(...)today (introduced before this issue was filed — see the commit history oncontracts/invoice_liquidity/src/lib.rs). The panic/wraparound impact described in the issue is not reachable on currentmain; it's also independently mitigated by this repo's[profile.release] overflow-checks = true.Since the acceptance criteria specifically asks for
checked_sub(notsaturating_sub) plus explicit error handling around the adjacent overflow risk, this PR still makes that change for auditability and defense-in-depth:primary_lp_payout'schecked_mul(...).unwrap_or(0)now returnsContractError::ArithmeticOverflowinstead of silently collapsing to a corrupting zero payout on genuine overflow.lp_earnednow useschecked_sub(...).unwrap_or(0)explicitly (same runtime behavior as the existingsaturating_sub, just auditable at the call site).distribute_amountslightly less than / equal to / significantly less thaninvoice.amount), asserting settlement succeeds and pays the correct non-negative amount in each case.Recommend closing #619 referencing the commit that already fixed it, once maintainers confirm.
#620 —
add_volumedouble-counting / hardcoded EURC index — fixedConfirmed live and exactly as described: a redundant XLM check block after an early return, and
token_list.get(2)hardcoded for EURC. Rather than looping the token list by a different index scheme,add_volumenow matches by the token's actual configured SAC address (is_xlm_token/is_usdc_token/is_eurc_token, which already existed for normalization elsewhere in the file) via anif/else ifchain — each token increments at most one counter, with no dependency onTokenListorder at all. Added 2 tests: one confirming no cross-contamination across all three tokens, one confirming correct EURC attribution after reorderingTokenListviaremove_token/add_token.#621 — unbounded reputation decay loop — already fixed
Also already resolved on
main:get_payer_scorealready capsperiods_passedagainstMAX_REPUTATION_DECAY_PERIODS(1000) and short-circuits to a score of 0 beyond that, with both acceptance-criteria tests already present (test_reputation_decay_bounded_for_extremely_long_inactivity,test_reputation_decay_bounded_when_decay_period_is_one_ledger). No changes made — re-implementing an identical fix would just be a no-op diff. Recommend closing with reference to the existing implementation/tests.#622 — caller-supplied
total_supplyinexecute_proposal— fixed, with one adjustmentConfirmed live:
execute_proposal(proposal_id, total_supply)let any caller pick the quorum denominator. The issue's acceptance criteria asks to querytotal_supplyfrom the governance token on-chain — that's not possible with this repo's pinnedsoroban-sdk = "21.4.0": neitherTokenInterfacenorStellarAssetInterfacein that version expose atotal_supply()method (verified directly against the vendored SDK source). Instead:total_supplyis now seeded atinitialize()time (new requiredgov_token_total_supply: i128param) and stored as contract state.set_gov_token_total_supply/get_gov_token_total_supply, gated by the sameiln_contract.require_auth()boundary already used forset_min_quorum_bps/set_min_proposal_balance.execute_proposal(proposal_id)drops the parameter entirely and reads the stored value.executeProposal— which, independently of this issue, was already calling the contract with completely mismatched arguments (an address whereproposal_idwas expected, and nototal_supplyat all) — to just passproposal_id, fixing that latent bug as a side effect.initialize/execute_proposalcall site acrosscontracts/iln_governance/src/test.rs(~24 sites) andtests_benchmarks.rs. One test (test_execute_quorum_not_reached_rejected) needed a real behavioral adjustment — with the new seeded default it would've hit quorum instead of missing it — fixed via an explicitset_gov_token_total_supplyoverride to restore its original intent.Pre-existing, unrelated issues found (not fixed here, out of scope)
contracts/tests/governance_lifecycle_test.rsandcontracts/tests/governance_main_integration_test.rsboth calliln_governance'sinitialize(...)with the wrong argument count independent of this PR (missingdistribution_contract) — already broken onmaintoday.governance_lifecycle_test.rsadditionally isn't wired into any[[test]]target in anyCargo.toml, so it isn't compiled at all currently. Updated both files'execute_proposal/initializecall sites for consistency with the new signature anyway, but did not fix the pre-existing arg-count mismatch — that's unrelated to any of these 4 issues.contracts/fuzz/src/lib.rscalls bothinvoice_liquidity::initializeandiln_governance::initializewith argument counts that don't match either contract's current signature — already broken onmain, unrelated to this PR.contracts/invoice_liquidity/src/tests_new_features.rsandtests_mev_mitigation.rsfail to compile on currentmain(missingMockAuth/MockAuthInvoke/testutils::Eventsimports) — confirmed via A/B against the pristine files, unrelated to this PR.test::test_upgrade_does_not_affect_existing_invoices,test::test_upgrade_emits_correct_event,test::test_upgrade_snapshot_before_after, andtests_min_invoice_amount::test_admin_adds_token_with_different_decimalsfail on currentmainindependent of this PR (confirmed via A/B).iln_governance'stest::test_vote_receipt_available_within_ttlfails on currentmainindependent of this PR (confirmed via A/B) — appears to be a ledger/TTL-archival interaction unrelated to total_supply.docs/contract-spec.json(generated viascripts/gen-spec.ts) is now stale for both changed contract interfaces — flagging for a maintainer to regenerate, since doing so requires the full Soroban CLI build toolchain.Verification
Built a scratch Cargo workspace (
invoice_liquidity+iln_governance+insurance_pool, matching the real workspace members and the repo's actualCargo.lock) and ran, natively (no local git clone/push involved — everything above was pushed via the GitHub API):cargo test -p invoice_liquidity --lib: 118 passed, 4 failed — all 4 confirmed pre-existing via A/B against the pristine files (see above).cargo test -p iln_governance --lib: 97 passed, 1 failed — confirmed pre-existing via A/B.cargo clippyon both crates: no new warnings beyond what's already onmain.cargo fmt --check: clean on every line this PR touches (pre-existing unrelated formatting nits elsewhere untouched).Test plan
cargo test -p invoice_liquidity --lib— 5 new tests pass (3 for Unchecked Arithmetic Underflow inmark_paidLP Payout Calculation #619, 2 for Double-Counting and Fragile Token Indexing inadd_volume#620)cargo test -p iln_governance --lib— all existing governance tests pass under the new signaturecargo clippy/cargo fmt --checkclean on all changed linesdocs/contract-spec.jsonmark_paidLP Payout Calculation #619 / Bound Reputation Decay Loop to Prevent Unbounded Iteration and DoS #621 as already-resolved, or point out if I'm missing context