Reconcile redemption reserve, add emergency timelock, storage-exhaustion stress test - #1134
Merged
joelpeace48-cell merged 3 commits intoAug 27, 2026
Conversation
Closes FinesseStudioLab#834 redeem() checked the requested payout against a mirrored REDEMPTION_RESERVE u64 counter that is updated in lock-step with fund_reserve/withdraw_reserve but never reconciled against the SAC token balance the contract actually holds. Any external transfer, rounding, or partial failure elsewhere could desync the two, letting a redemption over-pay past what the contract can actually cover. redeem() now bounds every payout by min(mirrored counter, real SAC balance via token::Client::balance), computed and compared entirely in i128 instead of casting the (potentially larger-range) asset amount down to u64 for the comparison and the reserve update. withdraw_reserve/fund_reserve are unchanged since their own SAC transfer already reverts safely on insufficient real balance. Adds two tests: one asserting the mirrored counter and the SAC balance stay in lockstep after a normal redemption (the conservation invariant the issue asks for), and one that manually desyncs the mirrored counter above the real balance and confirms redeem() still rejects the over-payout rather than trusting the stale counter.
Closes FinesseStudioLab#838, closes FinesseStudioLab#835 Adds queue_timelock/execute_timelock/cancel_timelock to the rewards contract (in the same lib.rs edited by the prior commit): an admin-only, delay-gated primitive alongside the existing quorum-based governance timelock (issue FinesseStudioLab#735) for sensitive single-admin actions (upgrade, redemption-rate change, reserve withdrawal) that don't need multi-voter approval, just a mandatory delay so a compromised or mistaken admin key can't act instantly. queue_timelock(op_hash) records eta_ledger = now + configured delay; execute_timelock reverts with TimeLockActive before eta and clears the entry after; cancel_timelock vetoes at any time. Same "record here, caller dispatches the actual effect after this returns" pattern as the existing execute_privileged_op. Wiring this as a hard gate on upgrade/set_redemption_rate/withdraw_reserve themselves is left as a follow-up — doing so now would change those functions' error surface and risk breaking existing callers/tests that call them directly with just admin+nonce. Adds contracts/campaign/src/test.rs::test_high_volume_registration_stress, registering 3,000 distinct participants in one run and asserting every registration succeeds and is recorded — the high-N class of test issue FinesseStudioLab#835 asks for, since tiny-N unit tests can't catch a regression that reintroduces an unbounded per-instance-entry collection. Documents the per-entry size budget and which storage keys are safe-by-construction (persistent, per-key) vs. the bug class being guarded against (instance-storage collections that grow one entry without bound) in the new docs/STORAGE_MODEL.md. Wires a named CI step for this test into contracts-ci.yml so a regression is called out on its own rather than buried in `cargo test --workspace` output. Note: contracts/campaign's existing test suite (both src/test.rs and src/fuzz_test.rs) currently fails to compile on main for reasons unrelated to this change — several `assert_eq!` sites expect the pre-SDK-bump `Result<Result<T, Error>, _>` shape from `try_*` client calls, and set_merkle_root is called with a stale extra argument. This pre-dates this PR (verified by stashing this commit's test.rs changes and reproducing the same failures against main). I could not get a full green `cargo test` run for the campaign crate as a result, though `cargo check --lib` passes and the new test's use of the plain (non-try) `register()` client call follows the same pattern as already-passing neighboring tests in the same file. I'll file a follow-up issue documenting this separately.
|
@miraclesonly 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 was referenced Aug 27, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let user = Address::generate(&env); | ||
|
|
||
| // 100 bps = 0.01 asset per point. | ||
| client.set_redemption_rate(&admin, &0, &asset, &100); |
| let (client, admin, asset, contract_id) = setup_redemption(&env); | ||
| let user = Address::generate(&env); | ||
|
|
||
| client.set_redemption_rate(&admin, &0, &asset, &10_000); // 1:1 |
| let (client, admin) = setup_timelock(&env); | ||
|
|
||
| assert_eq!(client.timelock_delay(), DEFAULT_TIMELOCK_DELAY); | ||
| client.set_timelock_delay(&admin, &0, &500); |
| fn test_timelock_early_execute_rejected() { | ||
| let env = Env::default(); | ||
| let (client, admin) = setup_timelock(&env); | ||
| client.set_timelock_delay(&admin, &0, &1_000); |
| fn test_timelock_queue_then_execute() { | ||
| let env = Env::default(); | ||
| let (client, admin) = setup_timelock(&env); | ||
| client.set_timelock_delay(&admin, &0, &100); |
joelpeace48-cell
merged commit Aug 27, 2026
016d695
into
FinesseStudioLab:main
4 of 17 checks passed
6 tasks
joelpeace48-cell
pushed a commit
that referenced
this pull request
Aug 28, 2026
Closes #851 Fixes a build-blocking bug on main first: two independently-merged PRs each added a new Error variant at discriminant 46 (TimelockNotFound, from #1134, and BelowMinClaim, from a separately-merged issues-850-860- 861-848 batch) — colliding once merged together, so `cargo check -p trivela-rewards-contract` currently fails with E0081 "discriminant value 46 assigned more than once" on main. Renumbers BelowMinClaim to 48 (the next free slot); safe since nothing could have successfully deployed relying on that specific discriminant value while the crate didn't compile. Adds contracts/rewards/fuzz/fuzz_targets/fuzz_multisig.rs: adversarial fuzzing of verify_multisig (used by set_paused) covering duplicate signers, unknown (unregistered) signers, corrupted signatures, and nonce replay — extending the existing fuzz_balance.rs coverage to the co-admin ed25519 multisig path. Asserts try_set_paused never panics for any signature-set shape, and that success implies a sufficiently large set of distinct, valid, registered signatures plus an unconsumed nonce; replaying the exact same call after a success must then be rejected. Wires both fuzz_balance and the new fuzz_multisig into contract-fuzzing.yml as libFuzzer (cargo-fuzz) runs, time-boxed to the workflow's existing FUZZ_DURATION, alongside the pre-existing proptest- based fuzz_* unit tests that workflow already ran (which weren't actually running the cargo-fuzz targets in contracts/rewards/fuzz/ at all before this).
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
3 participants
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
Closes #834
Closes #838
Closes #835
Closes #837
#834 — redemption reserve reconciliation.
redeem()checked payouts against a mirroredREDEMPTION_RESERVEu64counter kept in lock-step byfund_reserve/withdraw_reserve, but neverreconciled against the actual SAC token balance the contract holds. An external transfer, rounding,
or a partial failure elsewhere could desync the two, letting a redemption over-pay past what the
contract can actually cover.
redeem()now bounds every payout bymin(mirrored counter, token::Client::balance(contract)), computed and compared entirely ini128instead of casting the asset amount down to
u64for the comparison/update. Two new tests: oneasserting the mirrored counter and the real SAC balance stay in lockstep after a normal redemption,
one that manually desyncs the counter above the real balance and confirms
redeem()still rejectsthe over-payout.
#838 — emergency timelock. Adds
queue_timelock/execute_timelock/cancel_timelock: anadmin-only, delay-gated primitive for sensitive single-admin actions (upgrade, redemption-rate
change, reserve withdrawal) that don't need the multi-voter approval the existing quorum-based
governance timelock (issue #735) requires — just a mandatory delay so a compromised or mistaken
admin key can't act instantly.
queue_timelock(op_hash)recordseta_ledger = now + configured delay;execute_timelockreverts withTimeLockActivebeforeeta_ledgerand clears the entryafter;
cancel_timelockvetoes at any time. Follows the same "record here, caller dispatches theactual effect after this returns" pattern as the existing
execute_privileged_op. Note: wiringthis as a hard gate on
upgrade/set_redemption_rate/withdraw_reservethemselves is left as afollow-up — doing so now would change those functions' error surface and risk breaking existing
callers/tests that call them directly with just admin+nonce today. Five new tests cover default
delay, configuring a custom delay, early-execution rejection, queue→execute, cancel, and
double-queue rejection.
#835 — storage-exhaustion stress test. Adds
contracts/campaign/src/test.rs::test_high_volume_registration_stress, registering 3,000 distinctparticipants in one run and asserting every registration succeeds and is recorded — tiny-N unit
tests can't catch a regression that reintroduces an unbounded per-instance-entry collection, since
the collection would still be small at N=2 or N=3. New
docs/STORAGE_MODEL.mddocuments theper-entry size budget and which storage keys are safe-by-construction (persistent, per-key, like
PARTICIPANTand the newTIMELOCK_ENTRY) vs. the bug class this guards against (an instance-storagecollection that grows one entry without bound). Wires a named CI step into
contracts-ci.ymlso aregression here is called out on its own rather than buried in
cargo test --workspaceoutput.#837 — external audit. Not addressed in this PR: commissioning and publishing an independent
smart-contract audit isn't something a PR can close — it requires actually engaging and paying an
external firm. I've left a comment on that issue instead of claiming it here.
Pre-existing issue found (not fixed here)
contracts/campaign's existing test suite (bothsrc/test.rsandsrc/fuzz_test.rs) currentlyfails to compile on
main, unrelated to this PR — severalassert_eq!sites expect the pre-SDK-bumpResult<Result<T, Error>, _>shape fromtry_*client calls, and oneset_merkle_rootcall sitepasses a stale extra argument. I verified this predates this PR by stashing this PR's
test.rschanges and reproducing the same failures against
maindirectly. I couldn't get a full greencargo testrun for the campaign crate as a result, thoughcargo check --libpasses and the newstress test's use of the plain (non-
try)register()client call follows the same pattern asalready-passing neighboring tests in the same file. Filing a follow-up issue for this separately.
Test plan
cargo test --libincontracts/rewards— all new tests pass (redeem conservation/desync,5 timelock tests)
cargo check --libincontracts/campaignpassescargo test --libincontracts/campaign— blocked by the pre-existing, unrelated compilefailure noted above