PoolTogether TwabRewards contest - cmichel's results

Handle

cmichel

Vulnerability details

The TwabRewards contract has an implicit restriction of 256 epochs per promotion as it uses a bitmask in a uint256 to mark claimed epochs 0-255, see _isClaimedEpoch.

"/// @dev We pack epochs claimed by a user into a uint256. So we can't store more than 255 epochs."

First, this comment should be "cannot store more than 256 epochs" as there are 256 bits that can be set in a uint256, but the epoch index cannot be more than 255 (epochs are indexed at 0).

Second, this restriction is never enforced and the 257th epoch can be claimed over and over again, which allows an attacker to steal the entire contract balance.

POC

• attacker creates a promotion using a _token that is already used in a different promotion (for example, WETH) with 257 epochs: createPromotion(_ticket, _token=WETH, _tokensPerEpoch=1e18, _startTimestamp=now, _epochDuration=0, _numberOfEpochs=257). They need to transfer 257 * 1e18 tokens. (The ticket contract does not matter, they can even create their own fake ticket contract where they own 100% of the total supply)
• They can repeatedly claim the 257th epoch 10,000 times by calling claimRewards([256, 256, 256, ...]). The _updateClaimedEpoch(_userClaimedEpochs, _epochId=256) call that is supposed to mark the 257th epoch as claimed does not do anything as the 1 << 256 does not fit into a uint256 and is 0. Then _userClaimedEpochs | (uint256(1) << _epochId) = 0 | (1 << 256) = 0 | 0 = 0 returns 0, the initial value, which allows reclaiming it.
• The attacker steals the 1e18 tokens 10,000 times but only paid 1e18 257 times.

I tested the relevant code in this test file.

Impact

All tokens in the contract (from other promotions) can be stolen.

Recommended Mitigation Steps

Check _numberOfEpochs <= 256 in createPromotion and _promotion.numberOfEpochs + _numberOfEpochs <= 256 in extendPromotion