Skip to content

Harden Operator↔Sponsorship interactions - #1093

Merged
hpihkala merged 2 commits into
mainfrom
other-fixes
Oct 10, 2026
Merged

hpihkala merged 2 commits into
mainfrom
other-fixes

Conversation

@hpihkala

Copy link
Copy Markdown
Contributor

Harden Operator↔Sponsorship interactions: triggerAnotherOperatorWithdraw double-minting and staking on behalf of others

Follow-up to the withdrawEarningsFromSponsorships fix (#1092). While auditing all token flows between Operator and Sponsorship contracts we found two further issues. Neither is as severe as the original drain, but both let an outsider tamper with another party's accounting, so we fix them in the same release.

1. Operator.triggerAnotherOperatorWithdraw accepts any address as the "other Operator"

Problem

QueueModule._triggerAnotherOperatorWithdraw(otherOperator, sponsorships) measures the DATA balance delta around the call otherOperator.withdrawEarningsFromSponsorships(sponsorships) and mints the delta to the owner as a fisherman reward (via _delegate). It did not check what otherOperator actually is:

  • Self-call. Passing the Operator's own address makes it withdraw its own earnings. The earnings are split correctly inside the inner call (profit to the pool, protocol fee, operator's cut), but the outer balance delta then re-mints the whole amount again as a "fisherman reward" to the owner — at the delegators' expense. In our test scenario the owner ends up with 1380 DATA worth of stake instead of 1038, while the delegator drops from 9342 to 9000.
  • Arbitrary contract. The address need not be an Operator at all. A malicious contract can, inside its withdrawEarningsFromSponsorships, re-enter the Operator and e.g. delegate DATA to an address of its choosing. That DATA is minted once as a delegation and then minted again to the owner as a fisherman reward, as it also shows up in the outer balance delta. With a 10000 DATA attacker deposit the owner's position inflates from 1380 to 10702 DATA, the attacker's 10000 is worth 5093, and the delegator falls from 9000 to 4584.

Only the Operator's owner / nodes can call triggerAnotherOperatorWithdraw, so this is an insider attack against the pool's delegators rather than an external drain — but it breaks the invariant that owner and delegators share the pool pro rata.

Fix

QueueModule._triggerAnotherOperatorWithdraw now rejects:

  • otherOperatorAddress == address(this) → CannotTriggerOwnWithdraw()
  • any address that was not deployed by the OperatorFactory (deploymentTimestamp(addr) == 0) → AccessDeniedStreamrOperatorOnly()

Real Operators run the already-fixed withdrawEarningsFromSponsorships (balance-delta based, NotMyStakedSponsorship check), so a genuine Operator cannot be used to smuggle extra DATA in either.

2. Anyone can stake on behalf of any address in Sponsorship

Problem

Both staking entry points accepted a beneficiary that was different from the payer:

  • Sponsorship.stake(address operator, uint amount) pulled DATA from msg.sender but credited operator.
  • onTokenTransfer (ERC677 transferAndCall) decoded a 20- or 32-byte address from data and credited that address.

Hence any third party could, with their own DATA:

  • Force an Operator into a Sponsorship it does not service. The Operator is joined on the Sponsorship side but its own stakedInto/indexOfSponsorships bookkeeping knows nothing about it. It will not run the stream, so it can be flagged and slashed there, and withdrawEarningsFromSponsorships rejects the Sponsorship as NotMyStakedSponsorship.
  • Inflate an existing stake behind the Operator's back, increasing its slashing exposure beyond what the Operator ever committed. Operator.onTokenTransfer treats DATA coming back from a staked Sponsorship as earnings, so the surprise stake also distorts the pool's value accounting when it's eventually unstaked.

(Note the Sponsorship side does not even get a chance to notice: OperatorContractOnlyJoinPolicy only checks that the beneficiary is an Operator, not who paid.)

Fix

Sponsorship now requires the staker to be the beneficiary:

  • stake(operator, amount) reverts with CannotStakeForOthers(operator, staker) unless operator == _msgSender().
  • onTokenTransfer reverts with the same error if the decoded address differs from the ERC677 sender, in both the 20-byte and 32-byte branches.

Operators are unaffected: StakeModule._stake already calls sponsorship.stake(address(this), amount).

Tests

  • Operator.test.ts: "triggerAnotherOperatorWithdraw does NOT accept the Operator itself or a non-Operator as the other Operator" — self-call reverts with CannotTriggerOwnWithdraw, a fake re-entering contract (TestFakeOperator) reverts with AccessDeniedStreamrOperatorOnly, and the honest path still yields the expected pro-rata split (owner 1038 / delegator 9342).
  • Sponsorship.test.ts: "will NOT let stake on behalf of someone else" — covers approve + stake(), 20-byte and 32-byte transferAndCall data, both for a fresh join and for topping up an existing stake. Verified to fail on the previous Sponsorship.sol.
  • Existing tests that relied on the admin staking for other wallets now have those wallets stake for themselves (Sponsorship.test.ts, MaxOperatorsJoinPolicy.test.ts, OperatorContractOnlyJoinPolicy.test.ts). The "Multiple stakings and sponsorings" scenario additionally had its un-awaited advanceToTimestamp calls (which raced against the following tx, hence the old "withdraw goes to block +1" comments) replaced by an awaited evm_setNextBlockTimestamp, so intermediate earnings expectations are now exact round numbers; final payouts are unchanged.
  • New test helper contract: testcontracts/TestFakeOperator.sol.

Full test/hardhat/OperatorTokenomics suite: 289 passing. solhint and eslint clean.

Deployment

Both Operator and Sponsorship are EIP-1167 clones with no upgrade path, so existing contracts cannot be patched; only newly created ones pick up the fixes.

Component Action
Operator template + NodeModule, QueueModule, StakeModule Redeploy all four and call OperatorFactory.updateTemplates(...) (scripts/upgradeOperatorTemplate.ts). Only QueueModule changed in this PR, but the modules inherit Operator for storage layout and are always rolled together. Operator.version() stays at 4 (same release as the withdrawEarnings fix; the version-history comment lists this fix under 4).
Sponsorship template Redeploy and call SponsorshipFactory.updateTemplate(newTemplate).
OperatorFactory, SponsorshipFactory, StreamrConfig, policies No changes.

@hpihkala
hpihkala requested a review from jtakalai October 10, 2026 09:05

@jtakalai jtakalai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks to me like it does what it says on the tin. Preventing fishermaning own Operator is definitely good, preventing staking for others makes sense given the meaning of staking in the network.

@hpihkala
hpihkala added this pull request to stack #1095 October 10, 2026 10:09
@hpihkala
hpihkala force-pushed the other-fixes branch 2 times, most recently from 9455ee7 to 7f987ad Compare October 10, 2026 10:40
Base automatically changed from fix-stakemodule-vulnerability to main October 10, 2026 16:15
@hpihkala
hpihkala merged commit 235f058 into main Oct 10, 2026
1 check passed
@hpihkala
hpihkala deleted the other-fixes branch October 10, 2026 16:17
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