iamnmt - Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking

[sherlock-admin2]

iamnmt

Medium

Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking

Summary

Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking.

Vulnerability Detail

Users can call MasterChef#emergencyWithdraw, MlumStaking#emergencyWithdraw to withdraw tokens without claiming the rewards.

But the unclaimed rewards are not redistributed in emergencyWithdraw call, which will left the rewards stuck in the contracts and lost forever. Other users can not claim the rewards and the protocol can not redistribute the rewards.

Impact

Unclaimed rewards when emergency withdrawing in MasterChef and MlumStaking will stuck in the contracts and lost forever.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L326

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L536

Tool used

Manual Review

Recommendation

Redistribute the unclaimed rewards in emergencyWithdraw

MasterchefV2.sol

    function emergencyWithdraw(uint256 pid) external override {
        Farm storage farm = _farms[pid];

        uint256 balance = farm.amounts.getAmountOf(msg.sender);
        int256 deltaAmount = -balance.toInt256();

-       farm.amounts.update(msg.sender, deltaAmount);

+       (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = farm.amounts.update(msg.sender, deltaAmount);

+       uint256 totalLumRewardForPid = _getRewardForPid(farm.rewarder, pid, oldTotalSupply);
+       uint256 lumRewardForPid = _mintLum(totalLumRewardForPid);

+       uint256 lumReward = farm.rewarder.update(msg.sender, oldBalance, newBalance, oldTotalSupply, lumRewardForPid);
+       lumReward = lumReward + unclaimedRewards[pid][msg.sender];
+       unclaimedRewards[pid][msg.sender] = 0;

+       farm.rewarder.updateAccDebtPerShare(oldTotalSupply, lumReward);

        farm.token.safeTransfer(msg.sender, balance);

        emit PositionModified(pid, msg.sender, deltaAmount, 0);
    }

MlumStaking.sol

function emergencyWithdraw(uint256 tokenId) external override nonReentrant {
        _requireOnlyOwnerOf(tokenId);

        StakingPosition storage position = _stakingPositions[tokenId];

        // position should be unlocked
        require(
            _unlockOperators.contains(msg.sender)
                || (position.startLockTime + position.lockDuration) <= _currentBlockTimestamp() || isUnlocked(),
            "locked"
        );
        // emergencyWithdraw: locked

+       _updatePool();

+       uint256 pending = position.amountWithMultiplier * _accRewardsPerShare / PRECISION_FACTOR - position.rewardDebt;

+       _lastRewardBalance = _lastRewardBalance - pending;

        uint256 amount = position.amount;

        // update total lp supply
        _stakedSupply = _stakedSupply - amount;
        _stakedSupplyWithMultiplier = _stakedSupplyWithMultiplier - position.amountWithMultiplier;

        // destroy position (ignore boost points)
        _destroyPosition(tokenId);

        emit EmergencyWithdraw(tokenId, amount);
        stakedToken.safeTransfer(msg.sender, amount);
    }

[0xSmartContract]

This report was chosen as the main report because it states the findings MasterChef#emergencyWithdraw and MlumStaking#emergencyWithdraw in a single report.

[0xSmartContract]

When emergency withdrawing are made, Unclaimed rewards in MasterChef and MlumStaking contracts remain stuck in the contracts and are lost forever

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/metropolis-exchange/magicsea-staking/pull/34

[chinmay-farkya]

Escalate

Some issues have been incorrectly included in the duplicates here

#536 says “Loss of reward tokens during emergency withdrawing” but thats the whole point of emergency Withdraw and this has been clearly mentioned in the comments. The user is willingly letting go off rewards during an emergency acc to expected design. This is not a bug but a feature.
#671 is not a duplicate and is invalid due to the same reason above. Both these do not talk about redistribution or stuck rewards, but only about the withdrawer himself losing out on rewards, which is expected behaviour as per comments.

Apart from these, the remaining issues are actually two sets of issues clubbed together incorrectly.

Set 1 is about “non-redistribution of rewards of stakers who use emergencyWithdraw for exit”. The root cause is that the accRewardPerShare or lastRewardBalance are not properly adjusted so the rewards that were let go off by the staker in MasterChef and MLUMStaking are left stuck. And the fix is to adjust accRewardPerShare and lastRewardBalance accordingly in emergencyWithdraw logic or add a sweep function. Here, briber cant get back the rewards that will get stuck.

Set 1 : #460, #589, #428. note that this also has another duplicate not mentioned here ie. #368

Set 2 is about the rewards lost in extraRewarder attached to a farm, if the extraRewarder is unlinked from the farm while it still stores unclaimed rewards of various stakers of that farm. Root cause is the non-existence of a direct claiming method in MasterChefRewarder contract and the fix is to add such a method. Here, the stakers will lose out on rewards they had “already earned” in the form of extra rewards. This is unrelated to the funds stuck because masterchefrewarder inherits from baserewarder which has a sweep function, but the problem here is about the deserved users unable to claim their earned rewards after unlinking.

We can clearly see that these two sets are completely different according to root cause, fix ,and the code segment & actor affected.

Set 2 : #342, #468, #559, #611, #649, #40

#559 is invalid because the issue description is wrong. It says “funds will be stuck forever”. But the truth is the sweep function will be used to retrieve the funds as _getTotalSupply will return 0 after becoming unlinked. So the funds are not stuck, the non-existence of a direct claim method is the problem as described above. In short, the description and impact of this submission is completely wrong, hence this is invalid.

#611 is invalid because the issue description is completely wrong. As we can see in the code, if getTotalSupply returns zero, then the whole of contract’s balance will be sweeped out. And the getTotalsupply returns zero after becoming unlinked. So the rewards are not permanently stuck and can be sweeped out by owner. Instead, the problem is something different not identified by this issue. Hence, this is invalid due to wrong description and impact.

#40 is invalid as it talks about something else around the rewards calculations using timestamp which is not even relevant to the context of this discussion. The Masterchef’s lastUpdateTimestamp is not relevant to masterchefRewarder contract’s distribution of "extra rewards".

So Set 2 has only 3 valid issues : #342, #468, #649

Both these sets are two separate valid Medium findings

[sherlock-admin3]

Escalate

Some issues have been incorrectly included in the duplicates here

#536 says “Loss of reward tokens during emergency withdrawing” but thats the whole point of emergency Withdraw and this has been clearly mentioned in the comments. The user is willingly letting go off rewards during an emergency acc to expected design. This is not a bug but a feature.
#671 is not a duplicate and is invalid due to the same reason above. Both these do not talk about redistribution or stuck rewards, but only about the withdrawer himself losing out on rewards, which is expected behaviour as per comments.

Apart from these, the remaining issues are actually two sets of issues clubbed together incorrectly.

Set 1 is about “non-redistribution of rewards of stakers who use emergencyWithdraw for exit”. The root cause is that the accRewardPerShare or lastRewardBalance are not properly adjusted so the rewards that were let go off by the staker in MasterChef and MLUMStaking are left stuck. And the fix is to adjust accRewardPerShare and lastRewardBalance accordingly in emergencyWithdraw logic or add a sweep function. Here, briber cant get back the rewards that will get stuck.

Set 1 : #460, #589, #428. note that this also has another duplicate not mentioned here ie. #368

Set 2 is about the rewards lost in extraRewarder attached to a farm, if the extraRewarder is unlinked from the farm while it still stores unclaimed rewards of various stakers of that farm. Root cause is the non-existence of a direct claiming method in MasterChefRewarder contract and the fix is to add such a method. Here, the stakers will lose out on rewards they had “already earned” in the form of extra rewards. This is unrelated to the funds stuck because masterchefrewarder inherits from baserewarder which has a sweep function, but the problem here is about the deserved users unable to claim their earned rewards after unlinking.

We can clearly see that these two sets are completely different according to root cause, fix ,and the code segment & actor affected.

Set 2 : #342, #468, #559, #611, #649, #40

#559 is invalid because the issue description is wrong. It says “funds will be stuck forever”. But the truth is the sweep function will be used to retrieve the funds as _getTotalSupply will return 0 after becoming unlinked. So the funds are not stuck, the non-existence of a direct claim method is the problem as described above. In short, the description and impact of this submission is completely wrong, hence this is invalid.

#611 is invalid because the issue description is completely wrong. As we can see in the code, if getTotalSupply returns zero, then the whole of contract’s balance will be sweeped out. And the getTotalsupply returns zero after becoming unlinked. So the rewards are not permanently stuck and can be sweeped out by owner. Instead, the problem is something different not identified by this issue. Hence, this is invalid due to wrong description and impact.

#40 is invalid as it talks about something else around the rewards calculations using timestamp which is not even relevant to the context of this discussion. The Masterchef’s lastUpdateTimestamp is not relevant to masterchefRewarder contract’s distribution of "extra rewards".

So Set 2 has only 3 valid issues : #342, #468, #649

Both these sets are two separate valid Medium findings

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.

[blckhv]

This should be low, code comments are the main source of truth when readme contains no information:

* @dev Emergency withdraws tokens from a farm, without claiming any rewards.

Additionally, this is an opportunity loss, since user who has called emergencyWithdraw can after that call claim and this will set the needed params:
https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L551-L554

function _modify(uint256 pid, address account, int256 deltaAmount, bool isPayOutReward) private {
        Farm storage farm = _farms[pid];
        Rewarder.Parameter storage rewarder = farm.rewarder;
        IMasterChefRewarder extraRewarder = farm.extraRewarder;

        (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = farm.amounts.update(account, deltaAmount);

        uint256 totalLumRewardForPid = _getRewardForPid(rewarder, pid, oldTotalSupply);
        uint256 lumRewardForPid = _mintLum(totalLumRewardForPid);

        uint256 lumReward = rewarder.update(account, oldBalance, newBalance, oldTotalSupply, lumRewardForPid);

        if (isPayOutReward) {
            lumReward = lumReward + unclaimedRewards[pid][account];
            unclaimedRewards[pid][account] = 0;
            if (lumReward > 0) _lum.safeTransfer(account, lumReward);
        } 

So no locked funds.

[iamnmt]

@blckhv

since user who has called emergencyWithdraw can after that call claim and this will set the needed params
So no locked funds.

I believe this claim is not correct. In the emergencyWithdraw function, the unclaimed rewards are not added to unclaimedRewards[pid][account]. Because of that, funds from the last time a user, who is emergency withdrawing, called _modify to the time the user called emergencyWithdraw are still locked.

[MatinR1]

Regarding the comment on issue #536, I would like to provide further clarification. While it is true that the emergency withdraw function is designed to allow users to withdraw their staked tokens immediately, forfeiting any accrued rewards, my report highlights a different concern. The issue is not about the intentional forfeiture of rewards during an emergency withdrawal, but rather the loss of unclaimed rewards due to the contract's logic.

I explicitly emphasized the unclaimed rewards in the proof of concept (POC) part. Throughout the entire report, my focus was on the unclaimed rewards. I understand that forfeiting rewards during an emergency withdrawal is an intended feature, as stated in the function's NatSpec description. If my mitigation suggestion was off-target, it was because my primary concern was ensuring that unclaimed rewards were properly addressed.

Therefore, I maintain that this issue should be considered a bug, as it impacts the overall integrity and expected functionality of the rewards distribution process.

[0xSmartContract]

Escalate 1 : 536 incorrectly included in the duplicates here.
Answer. 1 : I disagree ❌

Escalate 1
Issue number 460:
In this issue, it is argued that the loss of accumulated rewards during the emergency withdrawal process is a problem and that these rewards should be redistributed to other users or the system.

Issue number 536:
The same problem is detected in this issue, but the solution proposal is slightly different. Here, it is suggested that the accumulated rewards during the emergency withdrawal process should be given to the user.

If we break the basic problem and navigate inside, we will probably go to very different places, do not break away from the main picture.

---

Escalate 2
Escalate 2 : 671 incorrectly included in the duplicates here.
Answer. 2 : I disagree ❌. (Same reasons as above)

Both issues involve losing rewards during the emergency withdrawal process. Issue 460 focuses on the failure to redistribute rewards, while issue 671 focuses on the user losing their rewards. However, in both cases, the fundamental problem is that the emergency withdrawal process does not handle rewards correctly.

Both issues affect the integrity of the system. Either the failure to redistribute rewards (460) or the user losing their rewards (671) prevents the system from operating fairly and efficiently. These are two aspects of the same problem.

A similar approach is required to solve both problems: correctly calculating and processing rewards during the emergency withdrawal process. This could be either giving the rewards to the user or redistributing them fairly throughout the system.

The code changes that would be made to solve both problems would likely be similar.
For example, calling the _modify function or updating the reward calculations.

---

Escalate 3
Escalate 3 : Set 1 and Set 2
Answer 3 : I disagree ❌.

All these issues (Set 1 and Set 2) are caused by deficiencies in the reward distribution mechanism. Whether it is in the emergency exit case or in the case of extraRewarder disconnection, the main problem is that users and the project cannot receive the rewards.

The solutions suggested for both sets (fixing accRewardPerShare and lastRewardBalance, adding a sweep function, adding a direct request method) actually serve the same basic purpose: providing access to the rewards.

Important Note: Should the project receive these rewards or should the users receive them? This part is an architectural decision and the project can decide on this, both can happen together (You probably want it to be separated into two parts because of this dilemma anyway)

The code changes to be made to solve these problems require a single holistic approach.

As a result, instead of evaluating these problems as separate sets, it would be better to consider them all as different aspects of the same basic problem.

---

[chinmay-farkya]

@0xSmartContract for "Escalate 3" consider that both set of problems are in different contracts altogether.

While the set 1 is about MLUMStaking and MasterChef, the set 2 is about masterchefrewarder.

Even if these two sets are decided to remain clubbed as one, I will provide additional arguments to prove that the other issues I listed as invalid are actually invalid.

Regarding "Escalate 1" : quoted from #536 : "The absence of the harvesting mechanism in emergencyWithdraw() can lead to significant loss of accumulated rewards for users who invoke this function." which clearly is implying to the loss caused to the withdrawer themselves, which is expected behavior. These two issues #536 and #671 are saying that the withdrawer of the position himself is losing rewards because of logic not calling _harvestPosition (see the recommendation mentioned in that issue)

Same goes for "Escalate 2"

These two and the other ones I mentioned as invalid in original escalation Set 2 are invalid and not part of either Set 1, nor of Set 2 and not a duplicate even if these two sets remain combined into a single issue (please go over the reasoning in original escalation for why #559, #611, #40 are invalid issues wrongly validated.)

[0xSmartContract]

If we break the basic issue and navigate inside, we will probably go to very different places (differently contracts , differently functions, differently recommendation, differently others), do not break away from the main picture.

[yash-0025]

My Issue #660 also shows that the rewards fund is stucked for the user during emergency withdraw with the POC screenshot in the 2nd scenario I provided and the same issue which depicts the same Issue is this #460 and it is considered valid. So isn't it a mistaken duped rather than an invalid ? And the issue #660 is escalated and the reason given is invalid.

[0xSmartContract]

@0xSmartContract for "Escalate 3" consider that both set of problems are in different contracts altogether.

While the set 1 is about MLUMStaking and MasterChef, the set 2 is about masterchefrewarder.

Even if these two sets are decided to remain clubbed as one, I will provide additional arguments to prove that the other issues I listed as invalid are actually invalid.

Regarding "Escalate 1" : quoted from #536 : "The absence of the harvesting mechanism in emergencyWithdraw() can lead to significant loss of accumulated rewards for users who invoke this function." which clearly is implying to the loss caused to the withdrawer themselves, which is expected behavior. These two issues #536 and #671 are saying that the withdrawer of the position himself is losing rewards because of logic not calling _harvestPosition (see the recommendation mentioned in that issue)

Same goes for "Escalate 2"

These two and the other ones I mentioned as invalid in original escalation Set 2 are invalid and not part of either Set 1, nor of Set 2 and not a duplicate even if these two sets remain combined into a single issue (please go over the reasoning in original escalation for why #559, #611, #40 are invalid issues wrongly validated.)

1 - If we agree that the two sets remain as one, we can move on to the invalid ones ✅

2- When I re-evaluate within the framework of your views, 611, 559 and 40 and duplicates can be considered invalid ✅
When getTotalSupply is zero and the contract is unlinked, the entire balance can be sweeped. This indicates that unclaimed rewards are not actually permanently frozen and can be sweeped by the owner. Therefore, the description and effect of issue #611 #559 and #40 are incorrect and invalid.

[chinmay-farkya]

All these issues (Set 1 and Set 2) are caused by deficiencies in the reward distribution mechanism. Whether it is in the emergency exit case or in the case of extraRewarder disconnection, the main problem is that users and the project cannot receive the rewards.
The solutions suggested for both sets (fixing accRewardPerShare and lastRewardBalance, adding a sweep function, adding a direct request method) actually serve the same basic purpose: providing access to the rewards.

That is not correct. When they can't be solved with a single fix at a single place in code, and do not stem from the same root cause, how can they be duped ?
From my understanding, dups are based on the same issues, and not on the "same contract" or "same code component/mechanism"

Moreover, the statement "The solutions suggested for both sets (fixing accRewardPerShare and lastRewardBalance, adding a sweep function, adding a direct request method) actually serve the same basic purpose: providing access to the rewards." is actually incorrect.

As i pointed out in my original escalation, set 1 is about missing re-distribution of rewards : stakers willingly forefeited their rewards by using emergencyWithdraw and these rewards now "do not belong to anyone" and thus they will get stuck because of lcak of redistribution.

Set 2 is about missing way to claim rewards : stakers had earned rewards which they are "not willing to forfeit" but when extrarewarder is unlinked, they are forcefully forefeited for everyone. These rewards actually belong to these stakers as they had already accrued them in storage. (note that I'm not talking about the rewards that had not accrued). Now this is not the same as funds get stuck, because these should not be sweeped by the protocol, "they belong to the stakers" and require a direct claim method.

I think that these are clearly not dups.

[0xSmartContract]

I think we have made progress on the issue
To summarize the #460 issue;

@chinmay-farkya's suggestion;
Set 1 : #460, #589, #428, #368(+368's duplicates)
Set 2 : #342, #468, #649,
Invalids : #536 , #671 ,#559, #611,#40

Lead Judge suggestion ;
Only one set: #460, #589, #428,#342, #468, #649 , #536, #671
Invalids : #559, #611,#40

As the Lead Judge, I appreciate your detailed analysis, but I believe there are strong reasons to consider these issues as part of a single set. Here's why:

Overarching Problem: While the specific mechanisms differ, all these issues fundamentally relate to the mishandling of rewards in exceptional circumstances (emergency withdrawals or unlinking of extra rewarders). The core problem in each case is that rewards are not properly accounted for or distributed.

System Impact: Both scenarios (emergency withdrawals and unlinking extra rewarders) result in a similar outcome - users potentially losing access to rewards they've accrued. This represents a systemic flaw in the reward distribution mechanism of the protocol.

Holistic Solution Approach: While the exact code fixes may differ, addressing these issues requires a comprehensive review and redesign of how the protocol handles rewards in edge cases. This suggests a unified approach to solving the problem, rather than treating them as entirely separate issues.

While I acknowledge that the specific mechanisms and code locations differ, I believe the similarities in the fundamental problem, impact, and required solution approach justify grouping these issues together. This holistic view allows for a more comprehensive understanding and resolution of the protocol's reward distribution challenges.

Regarding the invalid issues (#559, #611, #40), I agree with your assessment. These issues either mischaracterize the problem or focus on aspects that are not directly related to the core issue of reward mishandling in exceptional circumstances.

[yash-0025]

My Issue #660 also shows that the rewards fund is stucked for the user during emergency withdraw with the POC screenshot in the 2nd scenario I provided and the same issue which depicts the same Issue is this #460 and it is considered valid. So isn't it a mistaken duped rather than an invalid ? And the issue #660 is escalated and the reason given is invalid.

@0xSmartContract May I know about this issue!!!

[WangSecurity]

I'll go in the same order that the escalation comment has:

1. Matin - The emergencyWithdraw() lacks the harvest function, leading to loss of rewards #536 -- I agree, the issue basically says if Bob calls emergencyWithdraw they lose their rewards. It's invalid, it's intended.
2. neogranicen - Staker in the MasterchefV2 will lose all his rewards upon emergency withdrawal #671 -- the same, invalid and intended.
3. I agree that there are 2 sets of issues with different root causes. This report is about tokens being stuck and undistributed after someone uses emergencyWithdraw. ChinmayF - Loss of rewards when extraRewarder is reset in Masterchef.sol #342 (and its duplicates) are about losing rewards when the Rewarder is unlinked.

But, I need a clarification about the rewarder, is it set by the MagicSea team or anyone can set it as a Bribe rewarder for example?

#660 is the same as #536 and #671, it just explains how the user loses their rewards during the emergency withdraw which is the exact purpose of this function. It does mention the loss for the protocol in the impact, but no explanation of how the protocol loses here. Hence, remains invalid.

Also, excuse me for the delay here.

[chinmay-farkya]

But, I need a clarification about the rewarder, is it set by the MagicSea team or anyone can set it as a Bribe rewarder for example?

It is set by the magicsea team I guess. But once they do unlink it, the impact is that the way of claiming users' rewards is immediately cut off from the extrarewarder. The admin does not have a workaround to time this action of "unlinking". This admin action harms everybody.

[MatinR1]

dear @WangSecurity,
I would kindly like to request a re-evaluation of my bug report #536 . My primary concern was focused on the unclaimed rewards, which I clearly emphasized in my report. The issue revolves around how these unclaimed rewards are handled within the system. I believe there might be a misunderstanding regarding the context of my findings.

Thank you for your consideration.

[WangSecurity]

It is set by the magicsea team I guess. But once they do unlink it, the impact is that the way of claiming users' rewards is immediately cut off from the extrarewarder. The admin does not have a workaround to time this action of "unlinking". This admin action harms everybody

Thank you for this clarification, but I'm afraid the following rule should apply:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

In this case, the admin unlinking the rewarder leads to users losing these rewards completely. Hence, I believe the rule is applicable and the second set should be invalid.

I would kindly like to request a re-evaluation of my bug report #536 . My primary concern was focused on the unclaimed rewards, which I clearly emphasized in my report. The issue revolves around how these unclaimed rewards are handled within the system. I believe there might be a misunderstanding regarding the context of my findings.

I still stand by my words in the previous comment. #536 is only a recommendation to allow users who use emergencyWithdraw to allow them get the rewards later.

Hence, I decide to accept the escalation, de-dup #536, #671, #342, #468, #559, #611, #649, #40

[iamnmt]

@WangSecurity

To summarize the second set of issues:

Internal pre-conditions:

1. An extra rewarder is set in MasterchefV2 for a pool.
2. Any users have unclaimed extra rewards in the extra rewarder.

Vulnerability path: Admin calls to MasterchefV2#setExtraRewarder(pid, extraRewarder) with ANY extraRewarder, then the current extraRewarder will be unlinked. Another way to phrase this vulnerability path is the admin triggers the `MasterChefRewarder#unlink function.

I believe that it is intended for the admin calls to MasterchefV2#setExtraRewarder(pid, extraRewarder) even when the first internal pre-condition is met, in other words, the function MasterchefV2#setExtraRewarder is intended for changing the extra rewarder, because of these reasons:

1. In MasterchefV2#setExtraRewarder there is no check for the current extra rewarder to be address(0).
2. There is a unlink function implemented in the extra rewarder, meaning it is expected for the extra rewarder to be unlinked.

I believe the second set of issues is valid because the vulnerability path is fulfilled the note of the fifth item of the third section in the judging rules:

5. (External) Admin trust assumptions: ...
   ...
   Note: if the attack path is possible with any possible value, it will be a valid issue.

Also, may I ask what is the "certain assumptions about the functioning of the code" in the context of this issue?

[chinmay-farkya]

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

This statement is only true imo when the admin has a workaround alternate way of doing things : such that it would not lead to losses, and so they are trusted to instead go for this alternate way.

Here, in the set 2 of issues, there is no alternate way for the admin. Setting a new extrarewarder will definitely cause loss of rewards for "all users" every time the admin uses this function, all unclaimed rewards in the previous extrarewarder contract will not be able to be claimed by users who earned them.

[WangSecurity]

I believe the second set of issues is valid because the vulnerability path is fulfilled the note of the fifth item of the third section in the judging rules:

It's correct, but I believe this rule is not applicable here. What I'm applying is the fifth item in the section called "List of Issue categories that are not considered valid".

Also, may I ask what is the "certain assumptions about the functioning of the code" in the context of this issue?

In this case, the assumption is that the voters should be able to get their extra rewards even if the extraRewarder is unlinked. In fact, the admin unlinking extraRewarder leads to users losing their extra rewards.

This statement is only true imo when the admin has a workaround alternate way of doing things : such that it would not lead to losses, and so they are trusted to instead go for this alternate way

Fair point, but I believe the rule doesn't imply anything like this. The rule is about the admin's actions causing loss of funds, e.g. pausing collateral leads to unfair liquidations (users cannot add or repay collateral) or the users losing their extra rewards if the admin unlinks the contract with extra rewards.

Hence, the decision remains as expressed in this comment. The escalation will be accepted.

[chinmay-farkya]

So is the verdict that "admin actions that do not have an alternate workaround way and will most definitely lead to loss of funds for users : is allowed to happen and will not be treated as a bug" ? That sounds illogical imo.

I will leave the decision to you, and ask for changing this rule in the future.

[iamnmt]

@WangSecurity

Why is the rule "List of Issue categories that are not considered valid" applied but not this rule?

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.
If no values are provided, the (external) admin is trusted to use values that will not cause any issues.
Note: if the attack path is possible with any possible value, it will be a valid issue.
Exception: It will be a valid issue if the attack path is based on a value currently live on the network to which the protocol will be deployed.

Invalidating the second set of issues is assuming that the admin will never trigger the MasterChefRewarder#unlink function, even though the function is implemented in the codebase. Shouldn't this be "breaking core contract functionality"?

This is my final comment on this issue. Hoping that the contest's rules will be updated to reflect admin related issues better.

[WangSecurity]

Why is the rule "List of Issue categories that are not considered valid" applied but not this rule?

Because this issue is not connected to admin set variables. Here we are talking about the admin's actions, not about values that lead to an issue.

Invalidating the second set of issues is assuming that the admin will never trigger the MasterChefRewarder#unlink function, even though the function is implemented in the codebase. Shouldn't this be "breaking core contract functionality"?

I didn't say that I invalidate this issue because "the admin will never trigger the unlink function". I'm invalidating this issue because admin unlinking the extra rewarder leads to a loss of unclaimed rewards. I agree that the admin will do that and call this function, but it's still the admin action that leads to a loss of funds. Excuse me if my previous answer wasn't precise enough, if you still don't understand my reasoning, please ask questions.

The decision remains the same as expressed here.

[chinmay-farkya]

I agree that the admin will do that and call this function, but it's still the admin action that leads to a loss of funds

Then the rule has to be relooked imo to clarify such situations.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.