Platform: Code4rena
Start Date: 09/12/2021
Pot Size: $25,000 USDC
Total HM: 12
Participants: 25
Period: 4 days
Judge: LSDan
Total Solo HM: 4
Id: 64
League: ETH
Rank: 12/25
Findings: 2
Award: $561.88
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: pmerkleplant
Also found by: GiveMeTestEther, WatchPug, defsec, pauliax
GiveMeTestEther
If the promotion token applies transfer fees, the total amount to claim will be less than "_tokensPerEpoch * _numberOfEpochs" ( bcs a part of this amount is the fee => (funds + fee), but only the "funds" can be withdrawn) but the calculation in "_calculateRewardAmount" is based on "_promotion.tokensPerEpoch" . This implies that if the rewards of a user sum up to the "remaining funds +fee" of the promotions tokens that the TwabRewards contract holds (the user is the last one claiming the rewards), the user won't be able to withdraw rewards for at least one epoch, bcs the safeTranfser() is called with an amount (includes the fee) higher than the contract holds.
This case happens most likely after the promotion has ended.
There is no way to withdraw those funds and they are locked forever in this contract (loss of funds).
Assumptions for a simple example:
In the "createPromotion()" the TwabRewards contract will receive "_tokensPerEpoch - fee" and not "_tokensPerEpoch". If the user wants to claim the rewards after _epochEndTimestamp has passed the "_calculateRewardAmount()" will return _promotion.tokensPerEpoch. Therefore the "_rewardsAmount" will be equal to "_promotion.tokensPerEpoch.".
The "claimRewards()" will try to "_promotion.token.safeTransfer(_user, _rewardsAmount);" but the contracts has only "_tokensPerEpoch - fee" of the promotion tokens and the safeTransfer will fail. User won't be ever able to claim the rewards.
https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L162 https://github.com/pooltogether/v4-periphery/blob/b520faea26bcf60371012f6cb246aa149abd3c7d/contracts/TwabRewards.sol#L289
Manual Analysis
#0 - PierrickGT
2021-12-13T22:21:14Z
🌟 Selected for report: GiveMeTestEther
Also found by: WatchPug
44.9501 USDC - $44.95
GiveMeTestEther
cancelPromotion() and its modifier both call _getPromotion() to get the Promotion struct. We can save one such call by removing the modifier and do the check of the modifier at the beginning of the cancelPromotion() block to save storage reads.
remove the modifier onlyPromotionCreator
do the require statement at the beginning of cancelPromotion()
function cancelPromotion(uint256 _promotionId, address _to) external override returns (bool) { Promotion memory _promotion = _getPromotion(_promotionId);
// do here the modifiers check require( msg.sender == _promotion .creator, "TwabRewards/only-promotion-creator" ); _requirePromotionActive(_promotion); require(_to != address(0), "TwabRewards/recipient-not-zero-address"); uint256 _remainingRewards = _getRemainingRewards(_promotion); delete _promotions[_promotionId]; _promotion.token.safeTransfer(_to, _remainingRewards); emit PromotionCancelled(_promotionId, _remainingRewards); return true;
}
🌟 Selected for report: robee
Also found by: GiveMeTestEther, Jujic, Meta0xNull, WatchPug, defsec, sirhashalot, ye0lde
GiveMeTestEther
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
2021-12-pooltogether\TwabRewards.sol: require(_to != address(0), "TwabRewards/recipient-not-zero-address"); length: 38 2021-12-pooltogether\TwabRewards.sol: require(_ticket != address(0), "TwabRewards/ticket-not-zero-address"); length: 35
Manual Analysis
#0 - PierrickGT
2021-12-13T17:25:24Z