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: 10/25
Findings: 4
Award: $799.95
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: johnnycash
Also found by: WatchPug, csanuragjain, gpersoon, gzeon, harleythedog, kemmio, kenzo, leastwood, pauliax
pauliax
Anyone can createPromotion with any arbitrary _ticket supposed it follows the proposed interface. Thus, it is possible to create a promotion with a malicious ticket that returns arbitrary values for getAverageBalanceBetween and getAverageTotalSuppliesBetween. When claiming the rewards, a malicious user can drain the contract by claiming more tokens than were transferred initially on createPromotion.
Depending on the intentions, you can add auth checks on createPromotion, or add balance assertions that more tokens than entitled are not claimed from the promotion.
#0 - PierrickGT
2021-12-13T22:17:17Z
🌟 Selected for report: pmerkleplant
Also found by: GiveMeTestEther, WatchPug, defsec, pauliax
pauliax
Deflationary (fee on transfer) / rebasing tokens are not supported. Because anyone can createPromotion with an arbitrary token, such tokens may be lost forever.
Consider checking the actual amounts transferred (balance before/after) if you want to include such tokens in promotions.
#0 - PierrickGT
2021-12-13T22:20:23Z
🌟 Selected for report: gpersoon
Also found by: 0xabcc, WatchPug, cmichel, danb, gzeon, pauliax, pmerkleplant, sirhashalot
pauliax
Under normal circumstances, block.timestamp is always > 0, so the first condition is not really necessary:
require( _promotionEndTimestamp > 0 && _promotionEndTimestamp >= block.timestamp, "TwabRewards/promotion-not-active" );
#0 - PierrickGT
2021-12-13T22:26:55Z
🌟 Selected for report: pauliax
99.8892 USDC - $99.89
pauliax
Functions that are only used within this contract and are invoked only once can be refactored to eliminate extra gas costs of calling a function. Such functions are _updateClaimedEpoch and _isClaimedEpoch. This may reduce the readability so consider if you want to sacrifice it for a bit cheaper execution.
#0 - PierrickGT
2021-12-13T22:25:25Z
As mention by the author of the issue, we prefer to keep code clarity rather than save a little bit on gas for these tow cases. That's why I've acknowledged the issue.
pauliax
There are some validations that could be enforced on createPromotion:
epochDuration should be > 0 to prevent division by zero error in _getCurrentEpochId. 0 duration would make it impossible to cancel the promotion.
_tokensPerEpoch * _numberOfEpochs should be > 0 to prevent spam of 0 token promotions.
_numberOfEpochs should be < 256. Also, in extendPromotion, _extendedNumberOfEpochs should be < 256.
Consider enforcing proposed validations to make the codebase more robust.
#0 - PierrickGT
2021-12-13T22:19:34Z