Unsafe uint64 casting may overflow

[code423n4]

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.

[PierrickGT]

Duplicate of #58

[dmvt]

I do not consider this to be a duplicate of 58 because it describes an actual impact that, while extremely unlikely, could result in loss of funds. This also makes it a medium severity issue.

2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.