PoolTogether TwabRewards contest - sirhashalot's results

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

Handle

sirhashalot

Vulnerability details

Impact

The _calculateRewardAmount function casts epoch timestamps from uint256 to uint64 and these may overflow. The epochStartTimestamp value is a function of the user-supplied _epochId value, which could be extremely large (up to 2**255 – 1). While Solidity 0.8.x checks for overflows on arithmetic operations, it does not do so for casting – the OpenZeppelin SafeCast library offers this. The overflow condition could cause _epochStartTimestamp > _epochEndTimestamp, which the Ticket.sol getAverageBalanceBetween may not be expected to handle. The _epochStartTimestamp could overflow to have a value before the actual start of the promotion, also impacting the rewards calculation.

Proof of Concept

There are 4 uint64 casting operations in the _calculateRewardAmount function of TwabRewards.sol: https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L304-L312

Tools Used

Manual analysis

Recommended Mitigation Steps

While requiring _epochId <= 255 may help, it does not remove the issue entirely, because a very large _epochDuration value can still cause an overflow in the product (_epochDuration * _epochId) used in _epochStartTimestamp. However, other options exist:

1. Making these uint256 variables of type uint64 and therefore removing the casting of uint256 to the small uint64 would remove this risk and probably be the most gas-efficient solution.
2. Add require(_epochEndTimestamp > _epochStartTimestamp); to line 299, next to the existing require statement and before the uint64 casting operations
3. Use the OpenZeppelin SafeCast library to prevent unexpected overflows.