Large _epochId value impacts rewards calculation

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

The _epochId value is a uint256 that can be provided by the user in the _epochIds array in the important claimRewards and getRewardsAmount functions. The epochId value should be between 0 and 255, as evidenced by the bit shifting of a uint256 type in the functions _updateClaimedEpoch and _isClaimedEpoch. However, while the epochId value will not be negative because it is an unsigned integer, the edge case of epochId > 255 is not handled well. A user may submit an epochId of 300, or 300000, which can result in the following unexpected situations:

• In the _calculateRewardAmounts function, the _epochStartTimestamp may be greater than the _epochEndTimestamp (note that no casting overflow is required for this to occur, but a casting overflow in this same code is submitted as a separate finding)
• The current code relies on getAverageBalanceBetween() function in the out of scope Ticket.sol contract to properly handle _epochStartTimestamp > _epochEndTimestamp
• _isClaimedEpoch returns false for epochId values over 255
• _userClaimedEpochs array in the _updateClaimedEpochs function would remain unchanged for epochId values over 256

In summary, the case of epochId > 255 results in unexpected edge cases which allow a user to manipulate the rewards calculation in unexpected ways, especially if further code modifications do not examine this edge case. Edge case complexity would be substantially reduced if epochId is required to be <= 255 in the TwabRewards.sol file.

Proof of Concept

Two external functions of TwabRewards.sol allow a user to provide epochId values.

1. The claimRewards function: https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L162-L191
2. The getRewards function: https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L209-L222

However, the _epochId value has cascading impact due to external function calls and the impact can extend to other pooltogether contracts. For example, the call flow for the claimRewards function is: claimRewards() → _calculateRewardAmount() → Ticket.getAverageBalanceBetween (out of current scope)→ TwabLib.getAverageBalanceBetween (out of current scope)

Recommended Mitigation Steps

• In the two for loops that iterate over the user provided _epochIds array, or in the _calculateRewardAmount() function, add the following bounds check for the epochId value: require(epochIds[index] <= 255);
• Alternatively, a more gas-efficient solution is for the epochIds[] array to have type uint8, which inherently limits values to <= 255.
• Another alternative is to add require(_epochEndTimestamp > _epochStartTimestamp); to line 299, next to the existing require statement and before the uint64 casting operations, to prevent this type of manipulation

[PierrickGT]

This will be fixed in the following issue by casting _epochIds to uint8: https://github.com/code-423n4/2021-12-pooltogether-findings/issues/3

[PierrickGT]

Duplicate of https://github.com/code-423n4/2021-12-pooltogether-findings/issues/3