Uploaded files for judging

sherlock-audit · Dec 22, 2024 · 3c9592f · 3c9592f
1 parent 230e356
commit 3c9592f
Show file tree

Hide file tree

Showing 98 changed files with 7,002 additions and 10 deletions.
diff --git a/.gitignore b/.gitignore
diff --git a/001.md b/001.md
@@ -0,0 +1,114 @@
+Hidden Crepe Cormorant
+
+Medium
+
+# Loss of Rewards After Withdrawing All of One's Funds
+
+### Summary
+After the depositor withdraws all of their funds and if the remaining rewards are less than the fees, those rewards cannot be claimed and are locked in this contract.
+
+### Root Cause
+https://github.com/sherlock-audit/2024-11-tally/blob/main/staker/src/GovernanceStaker.sol#L720
+
+### Internal pre-conditions
+N/A
+
+### External pre-conditions
+N/A
+
+### Attack Path
+N/A
+
+### Impact
+The depositor or protocol loses the remaining rewards, which are locked in this contract.
+It may less for one depositor, but it is not less for all depositors.
+
+### PoC
+```solidity
+GovernanceStaker.sol
+        function _claimReward(DepositIdentifier _depositId, Deposit storage deposit, address _claimer)
+            internal
+            virtual
+            returns (uint256)
+        {
+            _checkpointGlobalReward();
+            _checkpointReward(deposit);
+
+            uint256 _reward = deposit.scaledUnclaimedRewardCheckpoint / SCALE_FACTOR;
+            // Intentionally reverts due to overflow if unclaimed rewards are less than fee.
+720:        uint256 _payout = _reward - claimFeeParameters.feeAmount;
+            if (_payout == 0) return 0;
+
+            // retain sub-wei dust that would be left due to the precision loss
+            deposit.scaledUnclaimedRewardCheckpoint =
+            deposit.scaledUnclaimedRewardCheckpoint - (_reward * SCALE_FACTOR);
+            emit RewardClaimed(_depositId, _claimer, _payout);
+
+            uint256 _newEarningPower =
+            earningPowerCalculator.getEarningPower(deposit.balance, deposit.owner, deposit.delegatee);
+
+            totalEarningPower =
+            _calculateTotalEarningPower(deposit.earningPower, _newEarningPower, totalEarningPower);
+            depositorTotalEarningPower[deposit.owner] = _calculateTotalEarningPower(
+                deposit.earningPower, _newEarningPower, depositorTotalEarningPower[deposit.owner]
+            );
+            deposit.earningPower = _newEarningPower.toUint96();
+
+            SafeERC20.safeTransfer(REWARD_TOKEN, _claimer, _payout);
+            if (claimFeeParameters.feeAmount > 0) {
+                SafeERC20.safeTransfer(
+                    REWARD_TOKEN, claimFeeParameters.feeCollector, claimFeeParameters.feeAmount
+                );
+            }
+            return _payout;
+        }
+```
+After the depositer withdraws all of their funds and if the remaing rewards are less than the fees, the `_claimReward()` function will always revert. Thus, the remaining rewards cannot be claimed and are locked in this contract.
+FYI, because this depositer's earningpower is zero , the `bumpEarningPower()` also revert for this depositer.
+
+### Mitigation
+```diff
+        function _claimReward(DepositIdentifier _depositId, Deposit storage deposit, address _claimer)
+            internal
+            virtual
+            returns (uint256)
+        {
+            _checkpointGlobalReward();
+            _checkpointReward(deposit);
+
+            uint256 _reward = deposit.scaledUnclaimedRewardCheckpoint / SCALE_FACTOR;
+            // Intentionally reverts due to overflow if unclaimed rewards are less than fee.
++           uint256 _fee = claimFeeParameters.feeAmount;
++           if (_reward <= _fee) {
++               _fee = _reward / 2;
++           }
++720:       uint256 _payout = _reward - _fee;
+-720:       uint256 _payout = _reward - claimFeeParameters.feeAmount;
+            if (_payout == 0) return 0;
+
+            // retain sub-wei dust that would be left due to the precision loss
+            deposit.scaledUnclaimedRewardCheckpoint =
+            deposit.scaledUnclaimedRewardCheckpoint - (_reward * SCALE_FACTOR);
+            emit RewardClaimed(_depositId, _claimer, _payout);
+
+            uint256 _newEarningPower =
+            earningPowerCalculator.getEarningPower(deposit.balance, deposit.owner, deposit.delegatee);
+
+            totalEarningPower =
+            _calculateTotalEarningPower(deposit.earningPower, _newEarningPower, totalEarningPower);
+            depositorTotalEarningPower[deposit.owner] = _calculateTotalEarningPower(
+                deposit.earningPower, _newEarningPower, depositorTotalEarningPower[deposit.owner]
+            );
+            deposit.earningPower = _newEarningPower.toUint96();
+
+            SafeERC20.safeTransfer(REWARD_TOKEN, _claimer, _payout);
+-           if (claimFeeParameters.feeAmount > 0) {
++           if (_fee > 0) {
+                SafeERC20.safeTransfer(
+-                   REWARD_TOKEN, claimFeeParameters.feeCollector, claimFeeParameters.feeAmount
++                   REWARD_TOKEN, claimFeeParameters.feeCollector, _fee
+                );
+            }
+            return _payout;
+        }
+```
diff --git a/002.md b/002.md
@@ -0,0 +1,86 @@
+Hidden Crepe Cormorant
+
+Medium
+
+# When The `totalEarningPower` Is Zero, The Rewards Are Locked
+
+### Summary
+When the `totalEarningPower` is zero, `lastCheckpointTime` is updated current time, but there is no one to receive the reward.
+
+### Root Cause
+https://github.com/sherlock-audit/2024-11-tally/blob/main/staker/src/GovernanceStaker.sol#L750
+
+### Internal pre-conditions
+N/A
+
+### External pre-conditions
+N/A
+
+### Attack Path
+N/A
+
+### Impact
+Rewards may be locked in this contract indefinitely.
+
+### PoC
+```solidity
+GovernanceStaker.sol
+294:    function lastTimeRewardDistributed() public view virtual returns (uint256) {
+            if (rewardEndTime <= block.timestamp) return rewardEndTime;
+            else return block.timestamp;
+        }
+        function _checkpointGlobalReward() internal virtual {
+            rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated();
+750:        lastCheckpointTime = lastTimeRewardDistributed();
+        }
+430:    function notifyRewardAmount(uint256 _amount) external virtual {
+            if (!isRewardNotifier[msg.sender]) {
+                revert GovernanceStaker__Unauthorized("not notifier", msg.sender);
+            }
+
+            // We checkpoint the accumulator without updating the timestamp at which it was updated,
+            // because that second operation will be done after updating the reward rate.
+            rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated();
+
+            if (block.timestamp >= rewardEndTime) {
+440:            scaledRewardRate = (_amount * SCALE_FACTOR) / REWARD_DURATION;
+            } else {
+                uint256 _remainingReward = scaledRewardRate * (rewardEndTime - block.timestamp);
+443:            scaledRewardRate = (_remainingReward + _amount * SCALE_FACTOR) / REWARD_DURATION;
+            }
+
+            rewardEndTime = block.timestamp + REWARD_DURATION;
+            lastCheckpointTime = block.timestamp;
+
+            if ((scaledRewardRate / SCALE_FACTOR) == 0) revert GovernanceStaker__InvalidRewardRate();
+
+            // This check cannot _guarantee_ sufficient rewards have been transferred to the contract,
+            // because it cannot isolate the unclaimed rewards owed to stakers left in the balance. While
+            // this check is useful for preventing degenerate cases, it is not sufficient. Therefore, it is
+            // critical that only safe reward notifier contracts are approved to call this method by the
+            // admin.
+            if (
+                (scaledRewardRate * REWARD_DURATION) > (REWARD_TOKEN.balanceOf(address(this)) * SCALE_FACTOR)
+            ) revert GovernanceStaker__InsufficientRewardBalance();
+
+            emit RewardNotified(_amount, msg.sender);
+        }
+```
+This protocol provides rewards over time, with a set start and end time, and `scaledRewardRate`(L440,L443).
+After `totalEarningPower` goes to zero, the protocol still attempts to provide rewards, and the `lastCheckpointTime` is updated to the current time, but there is no one to receive these rewards.
+As a result, these rewards locked in this contract indefinitely.
+
+### Mitigation
+When `totalEarningPower` is zero, 
+`lastCheckpointTime` should not be updated to the current time, or alternatively, 
+`rewardEndTime` should be increased by the same value as the increment of `lastCheckpointTime`.
+
+```diff
+        function _checkpointGlobalReward() internal virtual {
+            rewardPerTokenAccumulatedCheckpoint = rewardPerTokenAccumulated();
++           uint256 _lastCheckpointTime = lastTimeRewardDistributed();
++           if (totalEarningPower == 0) rewardEndTime += (_lastCheckpointTime - lastCheckpointTime);
++           lastCheckpointTime = _lastCheckpointTime;
+-750:       lastCheckpointTime = lastTimeRewardDistributed();
+        }
+```