search

code-423n4 / 2021-12-pooltogether-findings

0 stars 0 forks source link

Unsafe uint64 casting may overflow #123

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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 commented 2 years ago

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

dmvt commented 2 years ago

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.