GalloDaSballo
commented
2 years ago I see most functions don't have natspecs for parameters and I recommend to add for good understanding.
Valid Refactor
They must first be approved by zero and then the actual allowance must be approved.
Not valid, you won't need to approve(0) because the system always transferFrom the entire amount
It would be good to add a require() in setFeeInfo() of Booster.sol.
Valid Low, nice catch!
Add a require() in withdrawAndUnwrap() function of BaseRewardPool.sol.
Valid Refactor, Nice
Event should use indexed fields if there are three or more fields
Disagree on that specific event, no point in indexing those values
Neat report, some unique finds
1L, 2R
Summary
I see most functions don't have natspecs for parameters and I recommend to add for good understanding.
Low Risk Issues
Some tokens (USDT) do not work when changing the allowance from an existing non-zero allowance value. You need to approve zero amount first.
The initial values of "lockFeesIncentive" and "stakerLockFeesIncentive" are 10000 and 0 so the sum equals to FEE_DENOMINATOR. After you update these fees using setFeeInfo() function, if sum of two fees are greater than FEE_DENOMINATOR, the function earmarkFees() in L576-L595 will be failed because both fees will be calculated and transferred separately from feeToken balance of current contract. That's why I think it would be good to add a require() at L195.
require(_lockFeesIncentive + _stakerLockFeesIncentive <= FEE_DENOMINATOR, "invalid fee info");
Non-critical Issues
withdraw() function() in L214 has this require() and it would be good to add same require() for this function also.
require(amount > 0, "RewardPool : Cannot withdraw 0");