PoolTogether TwabRewards contest - csanuragjain's results

Handle

csanuragjain

Vulnerability details

Impact

This can lead to loss of funds as there is no recovery function of funds stuck like this

Proof of Concept

1. User A creates a new promotion using createPromotion function. By mistake he provides 1 year ago value for _startTimestamp with promotion duration as 6 months

2. Since there is no check to see that _startTimestamp > block.timestamp so this promotion gets created

3. User cannot claim this promotion if they were not having promotion tokens in the 1 year old promotion period. This means promotion amount remains with contract

4. Even promotion creator cannot claim back his tokens since promotion end date has already passed so cancelPromotion will fail

5. As there is no recovery token function in contract so even contract cant transfer this token and the tokens will remain in this contract with no one able to claim those

Recommended Mitigation Steps

Add below check in the createPromotion function

function createPromotion(
        address _ticket,
        IERC20 _token,
        uint216 _tokensPerEpoch,
        uint32 _startTimestamp,
        uint32 _epochDuration,
        uint8 _numberOfEpochs
    ) external override returns (uint256) {
require(_startTimestamp>block.timestamp,"should be after current time");
}

Handle

csanuragjain

Vulnerability details

Impact

There is no check to see if promotion has already ended. User can take the advantage and steal contract funds

Proof of Concept

1. Attacker calls the claimRewards function with epochid greater than promotion endtimestamp
2. Since there is no check to verify if epochid has already bypassed promotion endtimestamp, contract will check if user maintained necessary balance and then will transfer reward even though promotion already ended

Recommended Mitigation Steps

No reward should be given for promotions which has ended

Handle

csanuragjain

Vulnerability details

Impact

You can steal rewards from the contract causing huge financial loss

Proof of Concept

1. Navigate to the contract at https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol

2. Assume User A has created a promotion with Token X

3. Now Attacker creates a new Promotion using createPromotion function with _numberOfEpochs as 0 and a very high value for _tokensPerEpoch and very low value for _epochDuration and token as X

    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;
    }

4. Since _numberOfEpochs is 0, so user does not have to pay anything and promotion gets created

5. After some time, Attacker or his colleague claims the reward for his own promotion using claimRewards function

    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;
    }

6. This internally calls the _calculateRewardAmount function which calculates the reward for Attacker. Since attacker has managed average balance for the token X so his reward amount is calculated

7. Finally, Contract transfer the reward to Attacker for promotion created by Attacker even though Attacker has never given any funds for this promotion (_numberOfEpochs was zero). Reward transfer will suceed as contract will have token X balance from other promotions

8. This means contract will lose funds and users wont be able to claim from genuine promotions

Recommended Mitigation Steps

Add below checks:

function _calculateRewardAmount(
        address _user,
        Promotion memory _promotion,
        uint256 _epochId
    ) internal view returns (uint256) {
require(_numberOfEpochs>=epochid, "reward exhausted for this promotion");
}