Bug bounty Submission-Unaccounted ERC20 Transfers to Colony Contract Deflate Reward Snapshot and Distort Payouts

[zitx0x]

Summary:

Production code at ColonyRewards.sol snapshots totalTokens from a live balanceOf(address(this)) on the colony token. A permissionless transfer to the colony before startNextRewardPayout lowers that snapshot and inflates every claimant’s reward for that cycle.
Simply to say
The payout snapshot uses a raw ERC20 balance of the colony contract as part of the reward denominator. That balance can be changed by anyone sending the colony token directly to the colony contract, so the reward snapshot can be manipulated before payout starts.

Finding Description

The broken invariant is:
The reward denominator must be derived from protocol controlled accounting, not from an externally mutable ERC20 balance held by the colony contract.
In the current flow, startNextRewardPayout() computes totalTokens as:

uint256 totalTokens = ERC20Extended(token).totalSupply() - ERC20Extended(token).balanceOf(address(this));

This mixes two different sources of truth:

1. TokenLocking tracks who actually locked tokens and is used for user eligibility.
2. ERC20 balanceOf(address(this)) is a live, externally mutable balance that anyone can increase by transferring the colony token directly to the colony contract.

A malicious actor can transfer the configured colony token to the colony contract before startNextRewardPayout() is called. That increases balanceOf(address(this)), which lowers totalTokens, which inflates every claimant’s share in that payout cycle. The result is reward distortion and possible payout exhaustion / revert for later claimants.

Step by step state trace

Control (no attack)
Colony token supply fixed; colony holds 300 CLNY (operational).
Users lock CLNY in TokenLocking (not on colony balance).
startNextRewardPayout: totalTokens = supply - 300 → e.g. 2550 (PoC numbers).
rewardPayoutCycles[id].totalTokens = 2550 (immutable for cycle).
Each claimRewardPayout: reward ∝ 1 / f(totalTokens) with frozen 2550.

Attack
Attacker: colonyToken.transfer(colony, 50 ether), no colony function required.
balanceOf(colony) = 350; supply unchanged.
startNextRewardPayout: totalTokens = supply - 350 → 2500.
Same users/rep/locks → strictly higher rewards
If Σ rewards > amountRemaining, line 116 amountRemaining -= reward reverts (Solidity 0.8).
Late claimants denied; finalizeRewardPayout later only adjusts pendingRewardPayments, not unpaid users .

This is not a generic worthless token issue no, rather It only applies to the colony token set during initialization. The problem is that the payout math trusts a balance that is not controlled by the protocol.

Impact Explanation
The impact is reward manipulation and payout DoS.
A manipulated snapshot can cause:
inflated reward shares for the current cycle,
oversubscription of the payout pot,
later claims reverting when accounting underflows or the pot is exhausted.

This is not silent corruption per se, Solidity 0.8 will revert on underflow. The harm is that the payout cycle becomes unreliable and can be deliberately distorted.

Likelihood Explanation(Medium severity)
This is exploitable because:
ERC20 transfers to the colony contract are permissionless,
no colony function is required to alter balanceOf(address(this)),
the snapshot is taken only once in startNextRewardPayout(),
the attack only needs to happen before that snapshot.

The attacker must spend real colony tokens, so this is not free which is what makes it a medium bug.

poc:

function test_directTransferManipulatesSnapshotAndRewards() public {
    uint256 supply = clny.totalSupply();
    uint256 colonyBalBefore = clny.balanceOf(address(colony));
    uint256 uncorruptedTotalTokens = supply - colonyBalBefore;

    console.log("BEFORE ATTACK");
    console.log("colony CLNY balance", colonyBalBefore / 1e18);
    console.log("uncorrupted totalTokens", uncorruptedTotalTokens / 1e18);

    // Step 3: permissionless transfer of colony token to colony (only offensive action)
    vm.prank(alice);
    clny.transfer(address(colony), 50 ether);

    uint256 colonyBalAfter = clny.balanceOf(address(colony));
    console.log("AFTER 50 CLNY transfer");
    console.log("colony CLNY balance", colonyBalAfter / 1e18);

    // Step 4-5: snapshot uses live balanceOf(colony) — ColonyRewards.sol:56-58
    uint256 payoutId = colony.startNextRewardPayout(address(usdc));
    (, uint256 snapshottedTotalTokens,,,,,) = colony.cycles(payoutId);

    console.log("SNAPSHOT");
    console.log("snapshotted totalTokens", snapshottedTotalTokens / 1e18);
    console.log("live formula supply - balanceOf(colony)", (supply - colonyBalAfter) / 1e18);

    assertEq(snapshottedTotalTokens, supply - colonyBalAfter);
    assertLt(snapshottedTotalTokens, uncorruptedTotalTokens);

    uint256 honestAliceReward = _computeReward(
        100 ether, 500 ether, colony.colonyWideReputation(), uncorruptedTotalTokens, 3_000 ether
    );

    vm.prank(alice);
    colony.claimRewardPayout(payoutId);
    uint256 alicePayout = usdc.balanceOf(alice);

    console.log("ALICE CLAIM");
    console.log("USDC with honest totalTokens", honestAliceReward / 1e18);
    console.log("USDC with tainted snapshot", alicePayout / 1e18);

    assertGt(alicePayout, honestAliceReward);
}

Recommendation

Do not use balanceOf(address(this)) as part of the reward denominator.
Use protocol owned accounting instead, for instance:
a tracked internal balance variable,
or a snapshot mechanism based only on locked / accounted tokens.