PoolTogether TwabRewards contest - kenzo's results

Handle

kenzo

Vulnerability details

There is no check that the user supplied epochIds are within the amount of epochs the promotion defines.

Impact

Loss of user funds. After promotion has concluded, user can claim illegitimate rewards for later epochs which are not part of the promotion.

Proof of Concept

In claimRewards, The only validation done on the user supplied epoch is that it has not been claimed. (Code Ref)

In _calculateRewardAmount, there is no validation either - only checking that the supplied epoch has finished: (Code ref) require(block.timestamp > _epochEndTimestamp, "TwabRewards/epoch-not-over"); This means that if the promotion was only configured for 1 epoch, if user has supplied the invalid epoch #2 (after it "ended"), it will pass all the checks and contract will send to the user funds according to his TWAB - although these funds belong to earlier valid epochs or to other promotions.

Recommended Mitigation Steps

In claimRewards, add inside the loop: require(_epochId < _promotion.numberOfEpochs,"TwabRewards/invalid-epochId");

This solution will also prevent funkiness around creating a promotion with numberOfEpochs=0. I think I won't submit the creation of a promotion with numberOfEpochs=0 as a seperate issue because (as I see at the moment) by itself it's not harmful, it's harm comes from the issue I outlined here.

Handle

kenzo

Vulnerability details

cancelPromotion deletes the promotion from the promotions data. This is premature, and means that users wouldn't be able to claim rewards for epochs that have already been finished and not yet claimed.

Impact

Loss of funds. The users wouldn't be able to claim their rewards, and the rewards will remain locked in the contract.

Proof of Concept

cancelPromotion deletes the promotion from the promotion list: (Code ref) delete _promotions[_promotionId];

If a user will then rightfully try to claim a reward for an epoch that has finished from the promotion, claimRewards will grab the promotion's data from _promotions . (Code ref #1, #2)

But as we've seen, that data has been emptied. So when _calculateRewardAmount will try to calculate the reward for the finished epoch, all promotion data (for example including tokensPerEpoch) will be 0. I believe that anyway TwabLib will return 0 for anything as _epochStartTimestamp == _epochEndTimestamp and the function will return 0. (Code ref #1, #2)

Recommended Mitigation Steps

Do not delete the promotion data. You can set a boolean that marks whether the promotion was deleted or not.

Handle

kenzo

Vulnerability details

An attacker can supply createPromotion with his own malicious Ticket contract. TWAB values will then be read from this malicious contract.

Impact

Total drainage of TwabRewards. As _calculateRewardAmount is calculating the reward to send using the TWAB values from the ticket, the malicious ticket could return values that will send all the TwabRewards balance to the attacker, including rewards from other promotions.

Proof of Concept

When creating a promotion, the promotion creator supplies the ticket, and the only validation is _requireTicket(_ticket). (Code ref)

_requireTicket can be passed easily by a malicious contract - the contract would just need to have a controller function which returns an address which is not address 0. (Code ref)

Then when calculating the reward amount in _calculateRewardAmount, the contract calls the user supplied ticket and calculates the amount to send using the results from the ticket: (Code ref)

ITicket _ticket = ITicket(_promotion.ticket);
uint256 _averageBalance = _ticket.getAverageBalanceBetween(_user,...)
uint256[] memory _averageTotalSupplies = _ticket.getAverageTotalSuppliesBetween(...)
return (_promotion.tokensPerEpoch * _averageBalance) / _averageTotalSupplies[0];

Therefore, the malicious ticket could engineer his malicious ticket to return values that will send the whole of TwabRewards' token balance to the attacker.

Recommended Mitigation Steps

If only 1 ticket is valid, save that ticket in the contract. Otherwise, save a registry of valid tickets.

Handle

kenzo

Vulnerability details

The contract only supports 255 epochs per promotion. However, this is only briefly mentioned in one comment and not enforced during creation/extension of promotions.

Impact

Loss of user funds. If a promotion creator (legitimate or malicious) will create or extend a promotion to have more than 255 epochs, the contract won't keep track of which later epochs have been claimed, and a user could repeatedly claim rewards for an epoch and drain the funds.

Proof of Concept

The only mention I see for the 255 epochs limit of the contract is in this line: [(Code ref])(https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L34)

/// @dev We pack epochs claimed by a user into a uint256. So we can't store more than 255 epochs.

This is not highly visible and there is no mention in createPromotion or extendPromotion that _numberOfEpochs should be less than 256.

If a promotion creator does create/extend a promotion with more than 255 epochs, when a user will claim an epochId > 255, the contract couldn't record that the epoch has been claimed, because that info is saved in uint256 which has only 256 bits. [(Code ref])(https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L355)

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

As this claim wouldn't get saved, the user could repeatedly claim the same epoch, thereby taking other users and promotions' funds.

Recommended Mitigation Steps

Add a check to createPromotion and extendPromotion that the new promotion's number of epochs is < 256.

Handle

kenzo

Vulnerability details

If a promotion creator has created a promotion for the future, and decides to cancel it, he can not do so due to an underflow in a calculation.

Impact

Promotion creator funds will be locked until promotion begins.

Proof of Concept

Promotion's startTimestamp is supplied in createPromotion, and can be set to the future as there is no validation on it. [(Code ref])(https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L92)

cancelPromotion will call _getRemainingRewards to calculate the awards to refund, and _getRemainingRewards will call _getCurrentEpochId to calculate how many epochs are remaining: [(Code ref])(https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L333:#L335)

(_promotion.numberOfEpochs - _getCurrentEpochId(_promotion))

_getCurrentEpochId runs the following code which will underflow if the promotion's startTimestamp is in the future. [(Code ref])(https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L278)

return (block.timestamp - _promotion.startTimestamp) / _promotion.epochDuration;

Recommended Mitigation Steps

In _getRemainingRewards, if the promotion has not started, return all the rewards.