security: fix inverted fundraiser deadline logic in token-fundraiser - #674
security: fix inverted fundraiser deadline logic in token-fundraiser#674NikkiAung wants to merge 1 commit into
Conversation
contribute.rs and refund.rs both gated their time check on the wrong side of the comparison. contribute() required duration <= elapsed_days to pass - contributions only succeeded *after* the fundraiser's duration had already elapsed, and stayed open forever after that. refund() required duration >= elapsed_days - refunds only succeeded *before* the deadline. Net effect: with any realistic nonzero duration, contribute() and refund() are live in mutually-exclusive, backwards time windows. Nobody can contribute while the fundraiser claims to be running; once contributions start working (only possible post-deadline), refund is permanently blocked. If the target isn't met, funds sit in the vault with no instruction able to move them - check_contributions requires the target met, refund now requires "not yet past deadline" which is false by construction. A fund-lock, not a theft, but a complete break of the contract's core guarantee. Confirmed against the example's own README, whose prose states the correct intent while the code implemented the opposite - and whose refund code snippet already had the correct form, meaning refund.rs itself had drifted from the documented behavior, not the reverse. Fixed with a two-character flip per file: contribute.rs now requires duration > elapsed (window still open); refund.rs now requires duration <= elapsed (window has closed), matching the README exactly. Also fixed the one inverted README snippet (contribute section; the refund section was already correct) and the misleading comments above both checks. Both test suites previously called initialize(..., 0) - duration=0 made elapsed_days >= 0 trivially satisfy both inverted checks at once, completely masking the bug. Updated both to a realistic duration=1 and added adversarial coverage: tests/litesvm.test.ts (which can warp its own clock) now proves contribute() correctly rejects at the exact deadline boundary and refund() correctly succeeds once past it, with balance/account-closure assertions on the happy path. tests/fundraiser.ts (real validator, no way to fast-forward its clock) proves refund() is correctly rejected while still active. Verified the new tests actually fail against the pre-fix code (multiple independent failure signals, including a direct "succeeded when it should have rejected" repro at the boundary) and pass after. Out of scope, called out for follow-up: a possibly-negative elapsed value getting cast to u16 (wraps on a backwards clock - the fix widens nothing, kept as a 2-line diff instead of adding saturating_sub hardening); refund.rs/checker.rs gating on the vault's live token balance rather than the tracked current_amount, letting anyone grief the target check by transferring tokens into the vault directly; and check_contributions closing the Fundraiser PDA while Contributor PDAs may still be open, stranding their rent. All separate findings from this time-gate inversion.
Greptile SummaryThe PR corrects the fundraiser’s inverted deadline checks and strengthens both validator and LiteSVM regression coverage.
Confidence Score: 4/5The code changes appear behaviorally sound, but the unsigned pull request commit must be replaced with a signed, verifiable commit before merging. The corrected comparisons cleanly partition contribution and refund windows at the deadline, while the only blocking issue is the repository requirement that every commit be signed and verified. Important Files Changed
Reviews (1): Last reviewed commit: "security: fix inverted fundraiser deadli..." | Re-trigger Greptile |
| let current_time = Clock::get()?.unix_timestamp; | ||
| require!( | ||
| self.fundraiser.duration <= ((current_time - self.fundraiser.time_started) / SECONDS_TO_DAYS) as u16, | ||
| self.fundraiser.duration > ((current_time - self.fundraiser.time_started) / SECONDS_TO_DAYS) as u16, |
There was a problem hiding this comment.
Commit 4d003705c558c38e6e60b859c22ad32991c181f1 has no signature, so this pull request does not satisfy the repository requirement that commits be signed and verified.
Context Used: Request changes if the commits are not signed (ver... (source)
dev-jodee
left a comment
There was a problem hiding this comment.
two comments, across the file please reduce comment size it makes the code bloated
| // Asserts that `promise` rejects with the given Anchor custom error code | ||
| // (e.g. 'FundraiserNotEnded'), not just "something failed" - see the same | ||
| // helper in tests/fundraiser.ts for why this matters. | ||
| const expectAnchorError = async (promise: Promise<unknown>, code: string) => { |
There was a problem hiding this comment.
should be exported in a util function instead of reimplemented
|
|
||
| // Confirms refund() is correctly gated on FundraiserNotEnded while the | ||
| // fundraiser is genuinely still active. Note this doesn't reproduce the | ||
| // original bug in isolation: with a realistic nonzero duration, |
There was a problem hiding this comment.
please try to reduce the size of the comments, LLM are pretty verbose but this just makes the code really hard to read
Summary
token-fundraiser's time-based access control is inverted, breaking the program's core promise (contribute while active, refund if the goal isn't met by the deadline) and permanently locking contributor funds in the realistic case.contribute.rsrequiredduration <= elapsed_daysto pass — contributions only succeeded after the fundraiser's duration had already elapsed, staying open forever after that. While the fundraiser is genuinely "active," every contribution reverts withFundraiserEnded— the opposite of what that error name implies.refund.rsrequiredduration >= elapsed_daysto pass — refunds only succeeded before the deadline. Once the deadline genuinely passes (exactly whencontribute()starts working per the bug above),refund()stops working.Net effect: with any realistic nonzero duration,
contribute()andrefund()are live in mutually-exclusive, backwards windows. Nobody can contribute while the fundraiser claims to be running; once contributions start flowing in (only possible post-deadline), refund is permanently blocked. If the target isn't met, funds sit in the vault with no instruction able to move them —check_contributionsrequires the target met,refundrequires "not yet past deadline," which is now false by construction. A fund-lock, not a theft, but a complete break of the contract's core guarantee.Confirmed against intent, not just guessed: the example's own
readme.MDprose says contribute "checks that the fundraising duration has not elapsed" and refund is for "if the duration... has elapsed" — describing the correct behavior while the code implements the opposite. More directly: the README's refund code snippet already has the correct form — it's the actualrefund.rssource that drifted from the README, not the other way around. The README's contribute snippet has the same inverted form as the real bug and needed the identical fix.Why no test caught this: both test suites called
initialize(..., 0)— duration=0. With duration=0,elapsed_days >= 0is always true, which trivially (and accidentally) satisfies both inverted checks at once, completely masking the inversion. The "robustness test" cases also swallowed errors without asserting anything.Fix
Minimal, two-character operator flips (not expression rewrites), so the diff mirrors the README's already-correct refund form:
contribute.rs:duration <= elapsed→duration > elapsed(window still open).refund.rs:duration >= elapsed→duration <= elapsed(window has closed).readme.MD: same flip for the one inverted snippet (contribute section); the refund section was already correct, left untouched.Verified boundary semantics precisely: at
elapsed_days == duration, contribute must be closed and refund must be open — a deadline is an exclusive upper bound for contributing.Test changes
Both suites called
initialize(..., 0). Post-fix,duration = 0means "expired at creation" (was "never expires" pre-fix) — no validation exists ondurationininitialize, so this is a real semantic change worth flagging, not just a test-parameter tweak. Both suites needed a realistic nonzero duration to exercise anything meaningful.tests/litesvm.test.ts(can deterministically warp its own clock): added a test that warps to the exact deadline boundary and confirmscontribute()is rejected right at that instant (a direct, isolated repro of the "only works after the deadline" bug — this is the cleanest single proof that the fix is correct). Added a test confirmingrefund()is rejected while still active. Moved the existing happy-path refund test to run after the boundary warp, with stronger assertions (contributor ATA balance restored, vault drained to zero,Contributoraccount closed). Tightened the "robustness" tests to assert specific error codes instead of swallowing errors.tests/fundraiser.ts(realsolana-test-validator, no way to fast-forward its clock — confirmed there's no RPC for it and--warp-slotis a startup-only flag that doesn't help mid-suite): restructured the final refund test into an assertion that refund is correctly rejected while still active, with a vault-balance-unchanged check. Tightened the two "robustness" tests the same way.Honesty note on test design: with a realistic nonzero duration,
contribute()was broken from its very first call pre-fix (confirmed by running the new tests against the unpatched code), so the "refund rejected while active" tests actually fail pre-fix via a cascadingAccountNotInitialized(noContributoraccount ever got created) rather than a direct "refund wrongly succeeds" repro — that specific symptom only reproduced under the original tests' degenerateduration=0setup. Both are still valid regression tests (fail before the fix, pass after with the specific intended error), and the litesvm boundary test independently and cleanly proves thecontribute()half in isolation.Verification
Ran the full pre-fix/post-fix × old-tests/new-tests matrix locally (both suites, matching what
anchor test --validator legacyruns in CI):Also ran
cargo check,pnpm exec tsc --noEmit, andprettier --check .(root, which is what CI's Prettier job actually runs — the subproject's ownpnpm lintscript pins an unrelated, older prettier version and isn't used by CI). No IDL/client regeneration needed — onlyrequire!conditions changed, not account structs or instruction signatures.Explicitly out of scope (flagged as follow-ups, not fixed here)
as u16on a negativei64(clock set backwards) wraps to a large u16, which would flip both checks' effective behavior. A hardened version would widen toi64withsaturating_subinstead of narrowing. Left out to keep this a minimal, obviously-correct diff.refund.rs/checker.rsgate onvault.amount(the live ATA balance) instead offundraiser.current_amount(the tracked value). Since the vault is a plain ATA, anyone can transfer tokens into it directly to pushvault.amount >= amount_to_raise, blocking refunds or enablingcheck_contributionswithout genuine contributions. Real, separate finding.check_contributionscloses theFundraiserPDA whileContributorPDAs may still exist, after whichrefundcan never run for those contributors, stranding their rent. Also separate.