comment: review & commit some issue.

src/core/BucketRewardPool.sol

@@ -19,11 +19,13 @@ contract BucketRewardPool is IBucketRewardPool, OwnableUpgradeable {
         __Ownable_init_unchained();
     }
 
+    // todo. whether check newCommitter like in setVoter in StrategyManager.sol ?
     function setCommitter(address newCommitter) external onlyOwner {
         committer = newCommitter;
         emit SetCommitter(newCommitter);
     }
 
+    // todo. whether check newApprover like in setVoter in StrategyManager.sol ?
     function setApprover(address newApprover) external onlyOwner {
         approver = newApprover;
         emit SetApprover(newApprover);
@@ -39,6 +41,7 @@ contract BucketRewardPool is IBucketRewardPool, OwnableUpgradeable {
 
     function commitRoot(uint256 batch, bytes32 root) external override {
         require(committer == msg.sender, "not committer");
+        // todo. bug fix, should be ==> approvedBatch[batch]
         require(!approvedBatch[batch], "already approved");
 
         batchRoot[batch] = root;
@@ -53,6 +56,7 @@ contract BucketRewardPool is IBucketRewardPool, OwnableUpgradeable {
         emit ApproveRoot(msg.sender, batch);
     }
 
+    // todo. not need authority ?
     receive() external payable {
         uint256 batch = ++currentbatch;
         emit BatchReward(msg.sender, batch, msg.value);

src/core/StrategyManager.sol

@@ -99,6 +99,7 @@ contract StrategyManager is IStrategyManager, OwnableUpgradeable {
             address strategy = _strategies[i];
             uint256 ratio = strategyRatio[strategy];
 
+            // todo. use strategy variable other than _strategies[i]
             result += IStrategy(_strategies[i]).totalAmount() * ratio / RATIO_FACTOR;
         }
 
@@ -150,6 +151,7 @@ contract StrategyManager is IStrategyManager, OwnableUpgradeable {
         return _rewardTokenSet.values();
     }
 
+    // todo. return value deleted ?
     function distributeRewards(address token, uint256 amount) external payable override returns (bool) {
         require(_rewardTokenSet.contains(token), "not distributable");
         if (token == IOTX_REWARD_TOKEN) {

src/strategies/BaseStrategy.sol

@@ -57,6 +57,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
             IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
         }
 
+        // todo. if totalAmount == 0, the rewardValue will not be claim
         if (totalAmount > 0) {
             accTokenPerAmount[_token] = accTokenPerAmount[_token] + (_amount * PRECISION_FACTOR) / totalAmount;
         }
@@ -86,6 +87,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
 
     function pendingReward(address _token, address _staker, uint256 _amount) internal view returns (uint256) {
         uint256 _accTokenPerAmount = accTokenPerAmount[_token];
+        // todo. maybe only check _accTokenPerAmount==0?
         if (_amount == 0 && _accTokenPerAmount == 0) {
             return 0;
         }
@@ -95,6 +97,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
     function claimReward(address token) external nonReentrant {
         uint256 reward = pendingReward(token, msg.sender);
         if (reward > 0) {
+            // todo. whether rewardDebt[token][msg.sender] += reward?
             rewardDebt[token][msg.sender] = amount[msg.sender] * accTokenPerAmount[token] / PRECISION_FACTOR;
             if (token == IOTX_REWARD_TOKEN) {
                 payable(msg.sender).sendValue(reward);
@@ -109,6 +112,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
     function claimReward() external nonReentrant {
         uint256 reward = pendingReward(IOTX_REWARD_TOKEN, msg.sender);
         if (reward > 0) {
+            // todo. whether rewardDebt[token][msg.sender] += reward?
             rewardDebt[IOTX_REWARD_TOKEN][msg.sender] =
                 amount[msg.sender] * accTokenPerAmount[IOTX_REWARD_TOKEN] / PRECISION_FACTOR;
             payable(msg.sender).sendValue(reward);
@@ -122,6 +126,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
                 address token = rewardTokens[i];
                 reward = pendingReward(token, msg.sender);
                 if (reward > 0) {
+                    // todo. whether rewardDebt[token][msg.sender] += reward?
                     rewardDebt[token][msg.sender] = amount[msg.sender] * accTokenPerAmount[token] / PRECISION_FACTOR;
                     IERC20(token).safeTransfer(msg.sender, reward);
 
@@ -138,6 +143,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
 
             emit ClaimReward(IOTX_REWARD_TOKEN, staker, reward);
         }
+        // todo. maybe move forward follow patten: Checks-Effects-Interactions
         uint256 _accTokenPerAmount = accTokenPerAmount[IOTX_REWARD_TOKEN];
         if (_accTokenPerAmount > 0) {
             rewardDebt[IOTX_REWARD_TOKEN][staker] = newAmount * _accTokenPerAmount / PRECISION_FACTOR;
@@ -155,6 +161,7 @@ abstract contract BaseStrategy is IStrategy, OwnableUpgradeable, ReentrancyGuard
                 }
                 _accTokenPerAmount = accTokenPerAmount[token];
                 if (_accTokenPerAmount > 0) {
+                    // todo. maybe move forward follow patten: Checks-Effects-Interactions
                     rewardDebt[token][staker] = newAmount * _accTokenPerAmount / PRECISION_FACTOR;
                 }
             }

src/strategies/BucketStrategy.sol

@@ -12,6 +12,7 @@ import "./BaseStrategy.sol";
 contract BucketStrategy is IBucketStrategy, BaseStrategy, ERC721Holder {
     using EnumerableSet for EnumerableSet.UintSet;
 
+    // todo. maybe move the constant to BaseStrategy?
     uint256 constant WEEK = 7 days;
     uint256 public constant UINT256_MAX = type(uint256).max;
 
@@ -49,13 +50,15 @@ contract BucketStrategy is IBucketStrategy, BaseStrategy, ERC721Holder {
 
     function _stake(address staker, uint256 bucketId) internal {
         IBucket bucketContract = IBucket(underlyingToken);
+        // todo. don't check owner but approve, otherwise the deposit by proxy will not be handled.
         require(bucketContract.ownerOf(bucketId) == staker, "not owner");
 
         Bucket memory bucket = bucketContract.bucketOf(bucketId);
         require(bucketInLocking(bucket.unlockedAt), "not locking bucket");
 
         bucketContract.transferFrom(staker, address(this), bucketId);
 
+        // todo. Define a meaningful constant ?
         stakeStatus[bucketId] = 1;
         bucketStaker[bucketId] = staker;
         uint256 stakingAmount = calculateBucketRestakeAmount(bucket.duration, bucket.amount);
@@ -79,6 +82,7 @@ contract BucketStrategy is IBucketStrategy, BaseStrategy, ERC721Holder {
         IBucket bucketContract = IBucket(underlyingToken);
         bucketContract.deposit{value: msg.value}(bucketId);
 
+        // todo. maybe extract a common func with _stake() to reduce redundancy logic.
         Bucket memory bucket = bucketContract.bucketOf(bucketId);
         uint256 stakingAmount = calculateBucketRestakeAmount(bucket.duration, msg.value);
         bucketAmount[bucketId] = bucket.amount;
@@ -101,6 +105,7 @@ contract BucketStrategy is IBucketStrategy, BaseStrategy, ERC721Holder {
             require(stakeStatus[bucketId] == 1, "not staking bucket");
             require(bucketStaker[bucketId] == msg.sender, "not staker");
 
+            // todo. Define a meaningful constant ?
             stakeStatus[bucketId] = 2;
             Bucket memory bucket = IBucket(underlyingToken).bucketOf(bucketId);
 
@@ -129,11 +134,13 @@ contract BucketStrategy is IBucketStrategy, BaseStrategy, ERC721Holder {
             require(bucketStaker[bucketId] == msg.sender, "not staker");
             require(unstakeTime[bucketId] + WEEK <= block.timestamp, "withdraw freeze");
 
+            // todo. Define a meaningful constant ?
             stakeStatus[bucketId] = 0;
             bucketStaker[bucketId] = address(0);
             unstakeTime[bucketId] = 0;
             bucketAmount[bucketId] = 0;
 
+            // todo. the amount of IOTX in bucket don't need transfer to recipient?
             IBucket(underlyingToken).transferFrom(address(this), recipient, bucketId);
 
             emit Withdraw(msg.sender, bucketId);
@@ -145,6 +152,7 @@ contract BucketStrategy is IBucketStrategy, BaseStrategy, ERC721Holder {
         require(stakeStatus[bucketId] == 1, "not staking bucket");
         Bucket memory bucket = IBucket(underlyingToken).bucketOf(bucketId);
         uint256 oldStakingAmount = bucketAmount[bucketId];
+        // todo. only handle bigger ?
         require(bucket.amount > oldStakingAmount, "invalid bucket amount");
 
         uint256 stakingAmount = calculateBucketRestakeAmount(bucket.duration, bucket.amount - oldStakingAmount);

src/strategies/LSTStrategy.sol

@@ -34,6 +34,8 @@ contract LSTStrategy is ILSTStrategy, BaseStrategy {
     function _stake(address _staker, uint256 _amount) internal {
         require(_amount > 0, "zero amount");
 
+        // todo. maybe use msg.sender not _staker.
+        // Otherwise, the business by the proxy will not be processed
         IERC20(underlyingToken).safeTransferFrom(_staker, address(this), _amount);
 
         uint256 originAmount = amount[_staker];