Skip to content

security: fix inverted fundraiser deadline logic in token-fundraiser - #674

Open
NikkiAung wants to merge 1 commit into
solana-foundation:mainfrom
NikkiAung:fix/fundraiser-deadline-inversion
Open

security: fix inverted fundraiser deadline logic in token-fundraiser#674
NikkiAung wants to merge 1 commit into
solana-foundation:mainfrom
NikkiAung:fix/fundraiser-deadline-inversion

Conversation

@NikkiAung

@NikkiAung NikkiAung commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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.rs required duration <= elapsed_days to 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 with FundraiserEnded — the opposite of what that error name implies.

refund.rs required duration >= elapsed_days to pass — refunds only succeeded before the deadline. Once the deadline genuinely passes (exactly when contribute() starts working per the bug above), refund() stops working.

Net effect: with any realistic nonzero duration, contribute() and refund() 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 themcheck_contributions requires the target met, refund requires "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.MD prose 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 actual refund.rs source 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 >= 0 is 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 <= elapsedduration > elapsed (window still open).
  • refund.rs: duration >= elapsedduration <= elapsed (window has closed).
  • readme.MD: same flip for the one inverted snippet (contribute section); the refund section was already correct, left untouched.
  • Fixed the misleading comments above both checks ("Check if the fundraising duration has been reached" described neither direction correctly).

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 = 0 means "expired at creation" (was "never expires" pre-fix) — no validation exists on duration in initialize, 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 confirms contribute() 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 confirming refund() 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, Contributor account closed). Tightened the "robustness" tests to assert specific error codes instead of swallowing errors.
  • tests/fundraiser.ts (real solana-test-validator, no way to fast-forward its clock — confirmed there's no RPC for it and --warp-slot is 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 cascading AccountNotInitialized (no Contributor account ever got created) rather than a direct "refund wrongly succeeds" repro — that specific symptom only reproduced under the original tests' degenerate duration=0 setup. 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 the contribute() half in isolation.

Verification

Ran the full pre-fix/post-fix × old-tests/new-tests matrix locally (both suites, matching what anchor test --validator legacy runs in CI):

  • Pre-fix + original tests (duration=0): all green — reproduces exactly how the masking happened.
  • Pre-fix + new tests (duration=1): multiple independent failures, including the boundary test directly showing contribute() succeeding when it should have rejected.
  • Post-fix + new tests: all 16 tests pass across both suites.

Also ran cargo check, pnpm exec tsc --noEmit, and prettier --check . (root, which is what CI's Prettier job actually runs — the subproject's own pnpm lint script pins an unrelated, older prettier version and isn't used by CI). No IDL/client regeneration needed — only require! conditions changed, not account structs or instruction signatures.

Explicitly out of scope (flagged as follow-ups, not fixed here)

  • Lossy cast on a possibly-negative elapsed value: as u16 on a negative i64 (clock set backwards) wraps to a large u16, which would flip both checks' effective behavior. A hardened version would widen to i64 with saturating_sub instead of narrowing. Left out to keep this a minimal, obviously-correct diff.
  • refund.rs/checker.rs gate on vault.amount (the live ATA balance) instead of fundraiser.current_amount (the tracked value). Since the vault is a plain ATA, anyone can transfer tokens into it directly to push vault.amount >= amount_to_raise, blocking refunds or enabling check_contributions without genuine contributions. Real, separate finding.
  • check_contributions closes the Fundraiser PDA while Contributor PDAs may still exist, after which refund can never run for those contributors, stranding their rent. Also separate.

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.
@NikkiAung
NikkiAung requested a review from dev-jodee as a code owner August 6, 2026 09:49
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR corrects the fundraiser’s inverted deadline checks and strengthens both validator and LiteSVM regression coverage.

  • Contributions are accepted only before the configured duration elapses.
  • Refunds become available at the exact deadline boundary.
  • Tests now use a nonzero duration, assert exact Anchor errors, and verify post-refund account state.
  • The README contribution example is aligned with the corrected program logic.

Confidence Score: 4/5

The 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

Filename Overview
tokens/token-fundraiser/anchor/programs/fundraiser/src/instructions/contribute.rs Correctly closes contributions when elapsed whole days reach the configured duration.
tokens/token-fundraiser/anchor/programs/fundraiser/src/instructions/refund.rs Correctly enables refunds at and after the fundraiser deadline.
tokens/token-fundraiser/anchor/readme.MD Updates the contribution snippet and comment to match the corrected deadline behavior.
tokens/token-fundraiser/anchor/tests/fundraiser.ts Uses a meaningful duration and replaces swallowed failures with exact active-window error assertions.
tokens/token-fundraiser/anchor/tests/litesvm.test.ts Adds deterministic exact-boundary coverage and verifies successful refund balances and account closure.

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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Unsigned commit blocks merge

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 dev-jodee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please try to reduce the size of the comments, LLM are pretty verbose but this just makes the code really hard to read

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.

2 participants