Skip to content

fix: subscribe() was sending tokens out instead of collecting them#13

Open
PragyaTripathi990 wants to merge 1 commit into
seetadev:mainfrom
PragyaTripathi990:fix/subscribe-transferfrom-bug
Open

fix: subscribe() was sending tokens out instead of collecting them#13
PragyaTripathi990 wants to merge 1 commit into
seetadev:mainfrom
PragyaTripathi990:fix/subscribe-transferfrom-bug

Conversation

@PragyaTripathi990

Copy link
Copy Markdown

Summary

Critical logic bug: subscribe() called pptToken.transfer(msg.sender, SUBSCRIPTION_AMOUNT) which sent 10 PPT tokens to the subscriber for free instead of charging them. Anyone could call subscribe() and drain the contract's token balance.

Fixed by changing to pptToken.transferFrom(msg.sender, address(this), SUBSCRIPTION_AMOUNT) — the correct pull-payment pattern where the subscriber pays the fee.

Also added:

  • An explicit allowance check with a human-readable error message so callers understand they need to approve() the contract before subscribing.
  • A TokensWithdrawn event on withdrawTokens() for on-chain auditability.

Before / After

// BEFORE (buggy — sends tokens TO subscriber)
require(pptToken.transfer(msg.sender, SUBSCRIPTION_AMOUNT), "Token transfer failed");

// AFTER (correct — pulls tokens FROM subscriber)
require(
    pptToken.allowance(msg.sender, address(this)) >= SUBSCRIPTION_AMOUNT,
    "Insufficient token allowance. Approve the contract first."
);
require(
    pptToken.transferFrom(msg.sender, address(this), SUBSCRIPTION_AMOUNT),
    "Token transfer failed"
);

Test plan

  • Deploy PPTToken + MedInvoiceContract locally
  • Calling subscribe() without approving → reverts with allowance error
  • Calling subscribe() after approve(contract, 10e18) → succeeds, deducts 10 PPT from caller
  • Calling subscribe() twice → reverts with "Already subscribed"

Closes #1

🤖 Generated with Claude Code

The original subscribe() called pptToken.transfer(msg.sender, SUBSCRIPTION_AMOUNT)
which sent tokens TO the subscriber for free instead of charging them.
Changed to pptToken.transferFrom(msg.sender, address(this), SUBSCRIPTION_AMOUNT)
so the contract correctly pulls the subscription fee from the caller.

Added an explicit allowance check with a clear error message so callers know
to approve the contract before subscribing.

Added TokensWithdrawn event to withdrawTokens() for on-chain auditability.

Closes seetadev#1
@PragyaTripathi990 PragyaTripathi990 force-pushed the fix/subscribe-transferfrom-bug branch from 0ea0c17 to 0a4752c Compare May 10, 2026 11:56
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.

Code Audit – Security, Maintainability, Performance

1 participant