Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createPromotion() Lack of input validation for _epochDuration can potentially freeze promotion creator's funds #106

Open
code423n4 opened this issue Dec 12, 2021 · 2 comments
Assignees
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working

Comments

@code423n4
Copy link
Contributor

Handle

WatchPug

Vulnerability details

https://github.com/pooltogether/v4-periphery/blob/0e94c54774a6fce29daf9cb23353208f80de63eb/contracts/TwabRewards.sol#L88-L116

function createPromotion(
    address _ticket,
    IERC20 _token,
    uint216 _tokensPerEpoch,
    uint32 _startTimestamp,
    uint32 _epochDuration,
    uint8 _numberOfEpochs
) external override returns (uint256) {
    _requireTicket(_ticket);

    uint256 _nextPromotionId = _latestPromotionId + 1;
    _latestPromotionId = _nextPromotionId;

    _promotions[_nextPromotionId] = Promotion(
        msg.sender,
        _ticket,
        _token,
        _tokensPerEpoch,
        _startTimestamp,
        _epochDuration,
        _numberOfEpochs
    );

    _token.safeTransferFrom(msg.sender, address(this), _tokensPerEpoch * _numberOfEpochs);

    emit PromotionCreated(_nextPromotionId);

    return _nextPromotionId;
}

In the current implementation of createPromotion(), _epochDuration is allowed to be 0.

However, when _epochDuration = 0, it will be impossible for users to claim the rewards, and the promotion creator won't be able to cancel it.

PoC

  1. Alice called createPromotion() to create a promotion with the following parameters:
    • _token: USDC
    • _tokensPerEpoch: 10,000
    • _epochDuration: 0
    • _numberOfEpochs: 10
  2. 100,000 USDC was transferred from Alice to the TwabRewards contract;
  3. Users tries to claimRewards() but the transaction always revert at _ticket.getAverageTotalSuppliesBetween() -> TwabLib.getAverageBalanceBetween() due to div by 0.
  4. Alice tries to cancelPromotion() to retrieve the funds, but it always reverts at _requirePromotionActive() since the promotion already ended.

As a result, Alice's 100,000 USDC is frozen in the contract.

Recommendation

Consider adding require(_epochDuration > 0) in createPromotion().

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 12, 2021
code423n4 added a commit that referenced this issue Dec 12, 2021
@PierrickGT
Copy link
Member

Duplicate of #29

@PierrickGT PierrickGT marked this as a duplicate of #29 Dec 14, 2021
@PierrickGT PierrickGT self-assigned this Dec 14, 2021
@PierrickGT PierrickGT added the duplicate This issue or pull request already exists label Dec 14, 2021
@dmvt dmvt removed the duplicate This issue or pull request already exists label Jan 9, 2022
@dmvt
Copy link
Collaborator

dmvt commented Jan 9, 2022

I do not consider this to be a duplicate of #29 because the warden in #29 does not mention this specific failure case. This is indeed an easy to encounter bug that can be triggered as the result of a user error or a frontend bug. Loss of all funds for the promotion would be the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants