PoolTogether TwabRewards contest - leastwood's results

Handle

leastwood

Vulnerability details

Impact

The createPromotion function is called by a creator account (denoted as msg.sender) to fund a promotion with tokens allocated on a per epoch basis across a set epochs. However, the function does not perform the necessary checks on function inputs to ensure the promotion is able to function as intended.

In may be useful to ensure the inputs are within bounded ranges, i.e. _startTimestamp > block.timestamp and _epochDuration spans some minimum amount.

Proof of Concept

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

    function createPromotion(
        address _ticket,
        IERC20 _token,
        uint216 _tokensPerEpoch,
        uint32 _startTimestamp,
        uint32 _epochDuration,
        uint8 _numberOfEpochs
    ) external override returns (uint256) {
        _requireTicket(_ticket);

        uint256 _nextPromotionId = _latestPromotionId + 1;
        _latestPromotionId = _nextPromotionId;

        _promotions[_nextPromotionId] = Promotion(
            msg.sender,
            _ticket,
            _token,
            _tokensPerEpoch,
            _startTimestamp,
            _epochDuration,
            _numberOfEpochs
        );

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

        emit PromotionCreated(_nextPromotionId);

        return _nextPromotionId;
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider adding necessary input validation to the createPromotion function.

Handle

leastwood

Vulnerability details

Impact

claimRewards allows a user to collect their TWAB calculated rewards for a provided set of epochIds. The contract utilises a _claimedEpochs mapping which tracks claimed rewards per user. Each claimed epoch is represented by a single bit within a uint256 variable. When an epoch for a promotion has been claimed, the epoch's allotted bit is updated from 0 to 1. As the numberOfEpochs variable is restricted to a uint8 type, the maximum number of epochs a promotion can have is 255, which fits within the available bits of a uint256 type.

However, if block.timestamp exceeds a promotion's last epoch, users can continue to call claimRewards on an epochId which exceeds the last epoch. Hence, rewards will continue to be paid out even after a promotion has become inactive.

This severely impacts the correct distribution of rewards between users, potentially leading to a loss of funds for users who don't actively claim after a promotion has ended.

Proof of Concept

The POC consists of a series of steps an attacker must follow in order to exploit this contract:

• An honest promotion creator calls createPromotion using valid inputs.
• Lets assume _numberOfEpochs was set to 10 and _tokensPerEpoch was set to 10. Hence 100 tokens was transferred to the TwabRewards contract.
• The contract also has a balance of 100 tokens from another active promotion.
• The user holds all the tickets for the promotion just created, meaning when claimRewards is called on a given epoch, the user receives the entire allotted amount which is 10 tokens.
• 10 epochs pass and the user calls claimRewards on all the unclaimed epochs, receiving a total of 100 tokens in return.
• The promotion is now inactive and another epoch passes.
• The user calls claimRewards on epochId of 11.
• _isClaimedEpoch(_userClaimedEpochs, _epochId) returns false as the user has not claimed this epoch. _calculateRewardAmount proceeds to calculate that another 10 tokens is to be rewarded to the user.
• As a result, the user will continue to be rewarded with tokens as long as they continue to call claimRewards. This can be done up until epochId = 255.

As is outlined, it can be seen that any user could abuse this and continue to claim rewards even though the promotion in which they are a ticket holder of, has ended.

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider performing a check in _calculateRewardAmount to ensure _epochEndTimestamp does not exceed the final epoch's timestamp for a given promotion. This should ensure users cannot unfairly call claimRewards and either steal tokens from other promotions or steal tokens from other users who should be receiving their fair share of the promotion's reward tokens.

The following code in _calculateRewardAmount can be updated as per below to prevent the discussed issue:

        uint256 _epochDuration = _promotion.epochDuration;
        uint256 _epochStartTimestamp = _promotion.startTimestamp + (_epochDuration * _epochId);
        uint256 _epochEndTimestamp = _epochStartTimestamp + _epochDuration;

        uint256 _promotionEndTimestamp = _promotion.startTimestamp +
            (_promotion.epochDuration * _promotion.numberOfEpochs);

        require(
            _promotionEndTimestamp > 0 && _promotionEndTimestamp >= _epochEndTimestamp,
            "TwabRewards/promotion-not-active"
        );

        require(block.timestamp > _epochEndTimestamp, "TwabRewards/epoch-not-over");

Handle

leastwood

Vulnerability details

Impact

The createPromotion allows any user to create and fund promotions for a specific number of epochs. Ticket holders are entitled to a percentage of the rewards based on their TWAB. createPromotion references a _ticket address which can be controlled by the creator account. The requireTicket function is meant to validate the _ticket address, however, this can be bypassed by setting _ticket to a user controlled contract which returns valid values. Consequently, the malicious user can alter the result returned from _calculateRewardAmount and effectively drain the balance of ANY target token.

The affected function, claimRewards, does not check that the _rewardsAmount sent to a user is <= to to total rewards declared when creating a promotion. As a result, there is a risk that users can lose funds, hence this issue should be of high severity.

Proof of Concept

The POC consists of a series of steps an attacker must follow in order to exploit this contract and drain it of its token balance.

• The attacker creates a contract which satisfies the _requireTicket function. This contract must simply return a non-zero controllerAddress when a staticcall is made on the controller.selector.
• Valid input values to the createPromotion function are provided, including a non-zero amount _tokensPerEpoch and target token _token. The function will transfer some small non-zero amount from the attacker.
• One epoch passes and the creator of the promotion (who is also a ticket holder), calls claimRewards on the epochId which has just passed.
• The claimRewards function iterates through the single epochId and makes an internal call to _calculateRewardAmount
• The attacker controls the _ticket contract, which allows them to control the return values for the two functions _ticket.getAverageTotalSuppliesBetween and _ticket.getAverageBalanceBetween in _calculateRewardAmount. The following calculation dictates the returned reward amount.

return (_promotion.tokensPerEpoch * _averageBalance) / _averageTotalSupplies[0];

• As there is no inherent check to ensure that _rewardsAmount is <= to the number of tokens provided to the user's promotion, the attacker can control this value and set it to an amount larger than their provided token amount. Therefore, the attacker can effectively withdraw ANY amount of tokens from the TwabRewards contract, belonging to target token _token.
• This can be repeated with multiple promotions utilising various _token arguments to drain the contract balance for ALL tokens.

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider whitelisting _ticket contracts within TwabRewards. Alternatively, a more succinct solution could be to track the total rewards claimed for a given promotion. This should prevent more tokens from being sent out than provider by the promotion's creator.

Handle

leastwood

Vulnerability details

Impact

Users who have a small claim on rewards for various promotions, may not feasibly be able to claim these rewards as gas costs could outweigh the sum they receive in return. Hence, it is likely that a dust balance accrues overtime for tokens allocated for various promotions. Additionally, the _calculateRewardAmount calculation may result in truncated results, leading to further accrual of a dust balance. Therefore, it is useful that these funds do not go to waste.

Proof of Concept

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

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider allowing an admin account to skim a promotion's tokens if it has been inactive for a certain length of time. There are several potential implementations, in varying degrees of complexity. However, the solution should attempt to maximise simplicity while minimising the accrual of dust balances.

Handle

leastwood

Vulnerability details

Impact

The claimRewards function is called upon by ticket holders who parse a set of _epochIds they wish to claim rewards on. An internal call is made to _calculateRewardAmount to calculate the correct reward amount owed to the user. Subsequently, the _updateClaimedEpoch function will set the epoch bit of the tracked _claimedEpochs mapping, ensuring an epochId cannot be claimed twice for a given promotion.

However, there may be inaccuracies in the _calculateRewardAmount function, which results in more tokens being sent out than allocated by a promotion creator. This severely impacts the ability for users to claim their owed tokens on other promotions.

Proof of Concept

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

    function claimRewards(
        address _user,
        uint256 _promotionId,
        uint256[] calldata _epochIds
    ) external override returns (uint256) {
        Promotion memory _promotion = _getPromotion(_promotionId);

        uint256 _rewardsAmount;
        uint256 _userClaimedEpochs = _claimedEpochs[_promotionId][_user];

        for (uint256 index = 0; index < _epochIds.length; index++) {
            uint256 _epochId = _epochIds[index];

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

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

        _claimedEpochs[_promotionId][_user] = _userClaimedEpochs;

        _promotion.token.safeTransfer(_user, _rewardsAmount);

        emit RewardsClaimed(_promotionId, _epochIds, _user, _rewardsAmount);

        return _rewardsAmount;
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider checking that the total rewards claimed for a given promotion is strictly <= than the total allotted balance provided by the promotion creator. This should help prevent a single promotion from affecting the rewards claimable from other promotions.