KupiaSec - Pending rewards need to be processed when BribeRewarder.Deposit() is called

[sherlock-admin2]

KupiaSec

High

Pending rewards need to be processed when BribeRewarder.Deposit() is called

Summary

The issue is that when the BribeRewarder.Deposit() function is called, the pending bribe rewards are not being processed. This can lead to an incorrect calculation of the accDebtPerShare value. As a result, the overall distribution of bribe rewards may be incorrect.

Vulnerability Detail

In the BribeRewarder.Deposit() function, _modify() is called and _lastUpdateTimestamp is updated to block.timestamp.

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L143-L147

143:     function deposit(uint256 periodId, uint256 tokenId, uint256 deltaAmount) public onlyVoter {
144:         _modify(periodId, tokenId, deltaAmount.toInt256(), false);
145: 
146:         emit Deposited(periodId, tokenId, _pool(), deltaAmount);
147:     }

However, the rewards accrued in the previous epochs is not processed.
Assume that BribeRewarder.Deposit() is called with Voter.startNewVotingPeriod() in the same transaction. Then, _lastUpdateTimestamp is updated to startTime of the new epoch. It means that all the total pending rewards is not calculated.

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L260-L298

260:     function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
261:         private
262:         returns (uint256 rewardAmount)
263:     {
264:         if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
265:             revert BribeRewarder__NotOwner();
266:         }
267: 
268:         // extra check so we dont calc rewards before starttime
269:         (uint256 startTime,) = IVoter(_caller).getPeriodStartEndtime(periodId);
270:         if (block.timestamp <= startTime) {
271:             _lastUpdateTimestamp = startTime;
272:         }
273: 
274:         RewardPerPeriod storage reward = _rewards[_indexByPeriodId(periodId)];
275:         Amounts.Parameter storage amounts = reward.userVotes;
276:         Rewarder2.Parameter storage rewarder = reward.rewarder;
277: 
278:         (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = amounts.update(tokenId, deltaAmount);
279: 
280:         uint256 totalRewards = _calculateRewards(periodId);
281: 
282:         rewardAmount = rewarder.update(bytes32(tokenId), oldBalance, newBalance, oldTotalSupply, totalRewards);
283: 
284:         if (block.timestamp > _lastUpdateTimestamp) {
285:             _lastUpdateTimestamp = block.timestamp;
286:         }
287: 
288:         if (isPayOutReward) {
289:             rewardAmount = rewardAmount + unclaimedRewards[periodId][tokenId];
290:             unclaimedRewards[periodId][tokenId] = 0;
291:             if (rewardAmount > 0) {
292:                 IERC20 token = _token();
293:                 _safeTransferTo(token, msg.sender, rewardAmount);
294:             }
295:         } else {
296:             unclaimedRewards[periodId][tokenId] += rewardAmount;
297:         }
298:     }

Impact

Bribe rewards can be distributed incorrectly.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L143-L147

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L260-L298

Tool used

Manual Review

Recommendation

Pending rewards need to be processed when BribeRewarder.Deposit() is called.

    function deposit(uint256 periodId, uint256 tokenId, uint256 deltaAmount) public onlyVoter {
+       (uint256 startTime,) = IVoter(_caller).getPeriodStartEndtime(periodId);        
+       if (_lastUpdateTimestamp < startTime) {
+           for (uint256 i = _startVotingPeriod; i < periodId; ++i) {
+               _modify(i, tokenId, 0, false);
+           }
+       }

        _modify(periodId, tokenId, deltaAmount.toInt256(), false);

        emit Deposited(periodId, tokenId, _pool(), deltaAmount);
    }

Duplicate of #52

[KupiaSecAdmin]

Escalate

This issue is not a duplicate of #39; rather, it pertains to #52.

[sherlock-admin3]

Escalate

This issue is not a duplicate of #39; rather, it pertains to #52.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xSmartContract]

This is true. This issue is dup of #52

[WangSecurity]

Agree with the escalation, planning to accept it and duplicate with #52

[WangSecurity]

Result:
High
Duplicate of #52

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: accepted