rbserver - MasterChefRewarder.unlink function does not execute _setRewardParameters(0, block.timestamp, 0) though it should

[sherlock-admin2]

rbserver

High

MasterChefRewarder.unlink function does not execute _setRewardParameters(0, block.timestamp, 0) though it should

Summary

Because the MasterChefRewarder.unlink function does not execute _setRewardParameters(0, block.timestamp, 0), _totalUnclaimedRewards and rewarder.accDebtPerShare become less than what they should be at the moment when the MasterChefRewarder contract is stopped, and _rewardsPerSecond is not updated to 0, which allows accounts to still earn and claim rewards after the MasterChefRewarder contract is stopped even though no new rewards should be given after such stop.

Vulnerability Detail

Since calling the MasterChefRewarder.stop function reverts, the MasterChefRewarder contract can only be stopped by calling the MasterChefRewarder.unlink function according to the MasterChefRewarder.stop function's comment. Unlike the BaseRewarder.stop function, the MasterChefRewarder.unlink function does not execute _setRewardParameters(0, block.timestamp, 0); thus, when the MasterChefRewarder.unlink function is called:

• _totalUnclaimedRewards is not incremented by the pending rewards, which is returned by the _rewarder.getTotalRewards function, at the moment when the MasterChefRewarder contract is stopped though it should be;
• since the _rewarder.updateAccDebtPerShare function is not called, rewarder.accDebtPerShare is not incremented by the debtPerShare corresponding to the total supply and pending rewards at the moment when the MasterChefRewarder contract is stopped though it should be;
• and _rewardsPerSecond is not updated to 0 though it should be when the MasterChefRewarder contract is stopped.

Impact

The accounting for _totalUnclaimedRewards and rewarder.accDebtPerShare become inaccurate in which these state variables become less than what they should be at the moment when the MasterChefRewarder contract is stopped. Moreover, because _rewardsPerSecond is not updated to 0, accounts can still earn and claim rewards after the MasterChefRewarder contract is stopped even though no new rewards should be given after such stop, which causes these accounts to receive such rewards that they are not entitled to.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/MasterChefRewarder.sol#L51-L56

    /**
     * @dev Reverts as the MasterChefRewarder contract should be stopped by the unlink function.
     */
    function stop() public pure override(IBaseRewarder, BaseRewarder) {
        revert MasterChefRewarder__UseUnlink();
    }

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/MasterChefRewarder.sol#L42-L49

    function unlink(uint256 pid) public override {
        if (msg.sender != _caller) revert BaseRewarder__InvalidCaller();
        if (pid != _pid()) revert BaseRewarder__InvalidPid(pid);
        if (_status != Status.Linked) revert MasterChefRewarder__NotLinked();

        _status = Status.Stopped;
        _isStopped = true;
    }

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BaseRewarder.sol#L184-L198

    function stop() public virtual override onlyOwner {
        if (_isStopped) revert BaseRewarder__AlreadyStopped();

        uint256 totalSupply = _getTotalSupply();
        uint256 totalPendingRewards = _rewarder.getTotalRewards(_rewardsPerSecond, _endTimestamp, totalSupply);

        _totalUnclaimedRewards += totalPendingRewards;
        _rewarder.updateAccDebtPerShare(totalSupply, totalPendingRewards);

        _setRewardParameters(0, block.timestamp, 0);

        _isStopped = true;
        ...
    }

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BaseRewarder.sol#L340-L376

    function _setRewardParameters(uint256 maxRewardPerSecond, uint256 startTimestamp, uint256 expectedDuration)
        internal
        virtual
        returns (uint256 rewardPerSecond)
    {
        if (startTimestamp < block.timestamp) revert BaseRewarder__InvalidStartTimestamp(startTimestamp);
        if (_isStopped) revert BaseRewarder__Stopped();
        if (expectedDuration == 0 && maxRewardPerSecond != 0) revert BaseRewarder__InvalidDuration();

        uint256 totalUnclaimedRewards = _totalUnclaimedRewards;
        uint256 totalSupply = _getTotalSupply();

        uint256 totalRewards = _rewarder.getTotalRewards(_rewardsPerSecond, _endTimestamp, totalSupply);

        totalUnclaimedRewards += totalRewards;

        uint256 remainingReward = _balanceOfThis(_token()) - totalUnclaimedRewards;
        uint256 maxExpectedReward = maxRewardPerSecond * expectedDuration;

        rewardPerSecond = maxExpectedReward > remainingReward ? remainingReward / expectedDuration : maxRewardPerSecond;
        uint256 expectedReward = rewardPerSecond * expectedDuration;

        if (expectedDuration != 0 && expectedReward == 0) revert BaseRewarder__ZeroReward();

        uint256 endTimestamp = startTimestamp + expectedDuration;

        _rewardsPerSecond = rewardPerSecond;
        _reserve = totalUnclaimedRewards + expectedReward;

        _endTimestamp = endTimestamp;
        _totalUnclaimedRewards = totalUnclaimedRewards;

        _rewarder.updateAccDebtPerShare(totalSupply, totalRewards);
        _rewarder.lastUpdateTimestamp = startTimestamp;
        ...
    }

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Rewarder.sol#L51-L63

    function getTotalRewards(
        Parameter storage rewarder,
        uint256 rewardPerSecond,
        uint256 endTimestamp,
        uint256 totalSupply
    ) internal view returns (uint256) {
        if (totalSupply == 0) return 0;

        uint256 lastUpdateTimestamp = rewarder.lastUpdateTimestamp;
        uint256 timestamp = block.timestamp > endTimestamp ? endTimestamp : block.timestamp;

        return timestamp > lastUpdateTimestamp ? (timestamp - lastUpdateTimestamp) * rewardPerSecond : 0;
    }

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Rewarder.sol#L153-L162

    function updateAccDebtPerShare(Parameter storage rewarder, uint256 totalSupply, uint256 totalRewards)
        internal
        returns (uint256)
    {
        uint256 debtPerShare = getDebtPerShare(totalSupply, totalRewards);

        if (block.timestamp > rewarder.lastUpdateTimestamp) rewarder.lastUpdateTimestamp = block.timestamp;

        return debtPerShare == 0 ? rewarder.accDebtPerShare : rewarder.accDebtPerShare += debtPerShare;
    }

Tool used

Manual Review

Recommendation

The MasterChefRewarder.unlink function can be updated to execute _setRewardParameters(0, block.timestamp, 0) like what the BaseRewarder.stop function does.

[0xSmartContract]

The functions in the MasterChefRewarder contract that carry the admin authority are specified using the onlyOwner modifier. These functions can be called by the admin and include admin actions.

Therefore, assumptions and operational changes caused by admin actions are not considered a valid security issue.

[rbserver]

Escalate
According to #304 (comment), this report is invalided due to admin actions; yet, reports like #342 also involve admin actions and the MasterChefRewarder.unlink function and are valid. The fix for the issue described by reports like #342 needs the MasterChefRewarder contract to allow stakers to claim rewards directly from it; however, as described by this report, the accounting for state variables like _totalUnclaimedRewards and rewarder.accDebtPerShare are inaccurate because the MasterChefRewarder.unlink function does not execute _setRewardParameters(0, block.timestamp, 0), which would cause the rewards to be claimed directly from the MasterChefRewarder contract to be inaccurate after the MasterChefRewarder.unlink function is called. Thus, can this report's validity be re-considered?

[sherlock-admin3]

Escalate
According to #304 (comment), this report is invalided due to admin actions; yet, reports like #342 also involve admin actions and the MasterChefRewarder.unlink function and are valid. The fix for the issue described by reports like #342 needs the MasterChefRewarder contract to allow stakers to claim rewards directly from it; however, as described by this report, the accounting for state variables like _totalUnclaimedRewards and rewarder.accDebtPerShare are inaccurate because the MasterChefRewarder.unlink function does not execute _setRewardParameters(0, block.timestamp, 0), which would cause the rewards to be claimed directly from the MasterChefRewarder contract to be inaccurate after the MasterChefRewarder.unlink function is called. Thus, can this report's validity be re-considered?

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]

The current functionality of the MasterChefRewarder.unlink function is compatible with the purpose of terminating the contract, and changes caused by actions carried out by the admin should not be considered a valid security issue. Also, the purpose of the unlink function is to terminate the contract, not to calculate the rewards. Therefore, the suggestion to add the _setRewardParameters function to the unlink function is not a valid solution.

Differences Between Issues 342 and 304

Issue 342 is related to users not being able to claim their rewards and losing them. This directly affects the user experience and causes a security vulnerability. Issue 304 is related to the unlink function not including the _setRewardParameters call, which is caused by admin actions.

The solution to issue 342 involves updating the unlink function to correctly calculate the rewards and allow users to claim their rewards. Issue number 304 cannot be handled in the same way because it is a situation that occurs due to admin actions.

My opinion is definitely clear

[WangSecurity]

Admin can call setRewardParameters right before or after calling unlink or even in bundle and it will not cause such an issue. About #342, it's duplicated to #460, which is also escalated, so the validity of #342 will be checked.

Planning to reject this escalation, since admins are trusted to call setRewardParameters separately to not cause any issues, and leave the issue as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• rbserver: rejected