test(learn-token): add security tests for overflow/underflow protection - #379
Merged
DeFiVC merged 2 commits intoAug 30, 2026
Conversation
Covers mint (supply cap overflow), transfer/burn (balance underflow), and batch_claim_reward (supply overflow) in tests/unit/token_tests.rs, verifying each reverts with a clear message and leaves balance/supply state untouched. Also fixes batch_claim_reward's supply-cap check to use checked_add, matching the pattern already used by mint and claim_reward. Without it, a claim evaluated while supply is near i128::MAX raises a raw arithmetic-overflow panic that aborts the whole batch, instead of being skipped like any other over-cap claim.
|
@meshackyaro 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! 🚀 |
iduhtheman
added a commit
to iduhtheman/chainlearn-contracts
that referenced
this pull request
Aug 30, 2026
test_security_batch_claim_reward_supply_overflow_skips_without_panicking and test_governance_proposal_lifecycle were merged into the same file without a closing brace between them, leaving the first test's body open and swallowing the #[test] attribute meant for the second -- an unclosed-delimiter error blocking the entire test binary from compiling. Pre-existing on upstream/main since ChainLearnOfficial#379.
iduhtheman
added a commit
to iduhtheman/chainlearn-contracts
that referenced
this pull request
Aug 31, 2026
test_security_batch_claim_reward_supply_overflow_skips_without_panicking and test_governance_proposal_lifecycle were merged into the same file without a closing brace between them, leaving the first test's body open and swallowing the #[test] attribute meant for the second -- an unclosed-delimiter error blocking the entire test binary from compiling. Pre-existing on upstream/main since ChainLearnOfficial#379.
DeFiVC
pushed a commit
that referenced
this pull request
Aug 31, 2026
…d-blocking bug fixes (#378) * fix(progress-tracker): stop double-writing progress with a wrongly-doubled reference complete_module_in_place and submit_quiz_score_in_place both take progress: &mut ProgressInfo, but the single storage write at the end of each passed &progress -- taking a reference to the reference (&&mut ProgressInfo) instead of the reference itself, which does not satisfy the IntoVal bound Persistent::set requires and fails to compile. Each function then also recomputed overall_progress and eligible_for_credential a second time immediately after the write, overwriting nothing (the write already happened) and never being persisted -- dead code left over from a merge that inserted the version-tracking write in front of, instead of in place of, the original recompute-then-let-the-caller-write pattern. Fixed both call sites to pass the &mut ProgressInfo directly (no extra &) and removed the now-pointless post-write recompute in each -- the values already written to storage are the correct, final ones computed earlier in the same function, and nothing downstream reads the recomputed-and-discarded copies. * fix(credential-nft): CredentialVerification.display can't use Option<CredentialDisplay> CredentialVerification.display was Option<CredentialDisplay>, but soroban-sdk 21.7.7's #[contracttype] derive does not implement the ScVal (client/spec) conversion for Option<T> where T is a custom struct -- only for SDK built-ins like Symbol. This compiled under a bare cargo check (which only exercises the runtime Val path used inside the contract itself) but failed cargo test / the generated client with a concrete E0277 trait-bound error on TryFrom<&Option<CredentialDisplay>> for ScVal, confirmed directly against this SDK version -- a real, previously-undetected break in the already-merged code, and there was no existing test coverage exercising verify_credential_with_display at all to have caught it. Fixed by making display a Vec<CredentialDisplay> holding 0 or 1 elements instead, via new no_display/one_display helpers -- every field inside CredentialDisplay itself stays a true Option<Symbol>, which does work, so nothing about the type's actual optionality is weakened. Added two tests: no display data set (empty Vec, info unaffected) and display data set and returned correctly. * fix(tests): add missing version field to a Course test fixture Course gained a version: u32 field (#245); this test's manually- constructed Course literal, used to bypass create_course's own validation for a zero-module edge case, was never updated and failed to compile against the new struct shape. * fix(tests): correct CredentialInfo field name in a renewal test test_renew_credential_extends_expiry referenced .expiry, but CredentialInfo's actual field is expires_at -- a naming mismatch between this test and the struct it exercises that left the test suite unable to compile. * docs(credential-nft): note #227 is a duplicate of merged #242's Soulbound transfer rejection This branch originally set out to implement #227 ("Add credential transfer rejection with reason") by making `transfer` return Result<(), ContractError> with a Soulbound variant and requiring `from.require_auth()`. While rebasing onto current main, it turned out #227 is a content-duplicate of #242 (identical title and body), which was already implemented and merged via #374. Upstream's version returns the same typed Soulbound error but deliberately omits `require_auth()`, since the rejection is unconditional and reads/writes no storage -- there is nothing to authorize. Equivalent tests already exist in tests/unit/credential_tests.rs (test_transfer_always_returns_soulbound_error, test_transfer_rejects_even_without_auth_or_existing_credential, test_transfer_does_not_mutate_credential_state), including one that explicitly asserts the call succeeds without any mocked auth. Kept upstream's implementation and doc comment as-is (citing #242) and added a note cross-referencing the #227 duplicate; did not reintroduce the auth requirement or duplicate the existing test coverage. * fix(tests): restore missing closing brace between two adjacent test fns test_security_batch_claim_reward_supply_overflow_skips_without_panicking and test_governance_proposal_lifecycle were merged into the same file without a closing brace between them, leaving the first test's body open and swallowing the #[test] attribute meant for the second -- an unclosed-delimiter error blocking the entire test binary from compiling. Pre-existing on upstream/main since #379. * fix(progress-tracker): restore achievement variants misplaced by #374's merge, and fix two resulting compile errors #374's merge of storage-size tracking (#239) into this file botched the surrounding hunk: it closed `ProgressTrackerDataKey`'s enum body right after the new `StorageSize` variant, leaving the pre-existing `Achievements`/`AchievementEarned` variants stranded as dangling tokens after `write_entry`'s function body instead of inside the enum, and left `write_entry` itself unclosed. This broke `cargo check --workspace` on plain `main` with a parse error, confirmed present identically on `main` before this branch touched anything -- `types.rs` was otherwise byte-for-byte unchanged from `main`. Moved `Achievements`/`AchievementEarned` back into the enum body (right after `StorageSize`, where the diff put them originally) and closed `write_entry` where the parser actually needed it. Fixing the parse error surfaced two further pre-existing, unrelated compile errors in the same achievement-awarding code path, also present unchanged on `main`: - `complete_module_in_place` called a `get_learner_stats_internal` that was never defined (only the public `get_learner_stats(env: Env, ...)` exists) -- likely a rename that was never finished. Called the real function instead, cloning `env`/`learner` since this call site only has `&Env`/`&Address`. - `earn_achievement`'s `achievement_earned` event published `&AchievementType` by reference; soroban-sdk 21.7.7 doesn't implement the ScVal conversion for a reference to a custom `#[contracttype]` enum (only for owned values and SDK built-ins), so publishing failed to compile. `achievement_type` isn't used after this call, so passed it by value instead. `cargo check --workspace` and `cargo test -p progress-tracker --lib` (79 passed) now succeed.
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
2 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.
Closes #291
Summary
Adds security tests in
tests/unit/token_tests.rsthat verifylearn-token's arithmetic is protected against overflow and underflow:mint): fills supply toi128::MAX, then confirms one more token reverts with the clear"maximum supply cap exceeded"message (viachecked_add) rather than wrapping, and that a follow-up test confirms balance/supply are byte-for-byte unchanged after the reverted call.transfer,burn): confirms subtracting from a zero balance reverts with"insufficient balance"before any subtraction happens, plus state-unchanged checks for both paths.batch_claim_reward): this function is documented to skip over-cap claims rather than aborting the batch. With supply already ati128::MAX, its cap check (current_supply + reward_amount > max_supply) was a raw+, so the addition itself would overflow and panic with a raw, non-domain-specific message — aborting the entire batch instead of just skipping the one over-cap claim. Fixed to usechecked_add, matching the pattern already used inmintandclaim_reward, so it now gracefully skips instead of panicking.Acceptance criteria
"maximum supply cap exceeded","insufficient balance"— no raw arithmetic-trap messages reach a caller)catch_unwind)Test plan
cargo test— full suite passes (108 tests acrosstests/unitandtests/integration, including 7 new security tests)cargo build --release --target wasm32-unknown-unknown— succeedscargo fmt --all -- --check/cargo clippy— no new violations introduced by this change (pre-existing, unrelated findings elsewhere in the repo are untouched)Note:
cargo test --workspacecurrently fails to compile onmain(confirmed viagit stashagainst a clean checkout of upstreammain, and viagh run listshowing CI already red on recentmaincommits) due to ~35 pre-existing, unrelated type errors incontracts/learn-token/src/lib.rs's own inline#[cfg(test)] mod testsblock (e.g.client.mint(&voter1, &100)missing thecallerargument). That module is untouched by this PR and is out of scope for #291 — flagging it here for visibility.