ChinmayF - Voting and bribe rewards can be hijacked during emergency unlock by already existing positions

[sherlock-admin4]

ChinmayF

High

Voting and bribe rewards can be hijacked during emergency unlock by already existing positions

Summary

There are many ways of modifying an already existing staking position : but they exhibit different behavior when an emergency unlock is made active.
renewLock() and extendLock() are straight away disallowed.
withdrawFromPosition() and emergencyWithdraw() are allowed but it can only pull out assets and not do anything else.

addToPosition() is a different case, it has no restrictions even when the emergency unlock is active. This can be used by an attacker to modify pre-existing position to vote with a new amount in Voter.

This is a big problem because all other ways of starting, or renewing a staking position are blocked.

Vulnerability Detail

If a position already exists before the emergency unlock started, the owner of such positions can add amounts to them, and use this new amount for voting in the current voting period.

All other existing positions can also vote, but no new amount of assets can enter the voting circle because :

• calling createPosition will set lockDuration to 0 for this new position (which will revert when voting due to being < minimumLockTime) : see here and here
• calling renewLock and extendLock is straightaway reverted.

Now because no new assets can enter the voting amount circulation during this period, this creates a monopoly for already existing positions to control the voting outcome with lesser participants on board.

This can be particularly used by an attacker to hijack the voting process in the following steps :

• If the attacker already knows the peculiarty of addToPosition(), they can plan in advance and open many many 1 wei staking positions with long lock duration (such that they are eligible for voting based on minimumLockTime etc. in Voter.sol) and wait for the emergency unlock.
• If the emergency unlock happens they suddenly start adding large amounts to their 1 wei positions.
• This increases their amountWithMultiplier(sure multiplier will get limited to 1x but we are talking about the amount itself) and thus they have more voting power.
• emergencyUnlock is meant to be used in an emergency which can happen anytime. If a voting period is running during that time, there is no way to stop the current voting period.
• The attacker can use this new voting power for all their positions to hijack the voting numbers because other users cant open new positions or renew/extend, and honest users are not aware of this loophole in the system via addToPosition()

In this way, an attacker can gain monopoly over the votes during an emergencyUnlock, which ultimately influences the LUM emissions directed to certain pools.

Impact

Voting process can be manipulated during an emergencyUnlock using pre-existing positions. This also leads to a hijack of the bribe rewards if any bribe rewarders are set for those pools.

High severity because this can be used to unfairly influence the voting outcome and misdirect LUM rewards, which is the most critical property of the system. Also this is unfair to all other voters as they lose out on bribe rewards, and the attacker can get a very high share of it.

Code Snippet

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

Tool used

Manual Review

Recommendation

This can be solved by simply disallowing any activity apart from emregencyWithdraw during the time when emergencyUnlock is active. Apply the check used in _lockPosition() to addToPosition() too.

[0xSmartContract]

During emergency unlock, adding to existing positions can increase voting power and manipulate the voting process. This can unfairly affect voting outcomes and rewards.

[0xHans1]

PR: https://github.com/metropolis-exchange/magicsea-staking/pull/10

[0xSmartContract]

These issues all focus on how the system can be abused during emergency unlocking, which can have negative consequences for users and the contract. The main goal is to ensure that only safe and fair transactions are made during emergency unlocking, and to prevent malicious users from exploiting system vulnerabilities to gain unfair advantage.

[sherlock-admin2]

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

[chinmay-farkya]

Escalate

There are many duplication errors here, out of which most issues are invalid in reality. These are listed below:

• Reentrants - addToPosition() isn't disabled during emergency unlock #15 is not a duplicate because it simply says addToPosition should have a certain check without describing the issue created. The impact has not been included, neither does the submission describe a valid attack path. This is against sherlock’s duplication rules here
• bbl4de - createPosition() and addToPosition() are callable even in case of emergency, decreasing rewards for users #131 the core point is the decrease in user’s rewards which is invalid because the emergencyWithdraw() function has comments saying the rewards have been let go by the staker because its an emergency, in which case the rewards he lost will not even be redistributed as pointed out by issue iamnmt - Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking #460. So the new staker cannot gain the rewards left out by emergency withdrawers as per current logic. This is invalid and not a duplicate.
• tedox - Users can abuse MlumStaking::setEmergencyUnlock to steal tokens #134 is not a duplicate. Its invalid because frontrunning is not possible on Iota evm and the contest readme says the protocol is to be deployed on Iota only. See here : IOTA VM Docs
• iamnmt - During emergency unlock period, users still can increase a position's lockDuration by calling MlumStaking#addToPosition, which makes the position eligible for voting in Voter #184 has wrong recommendation because avgDuration needs to be actually increased generally, the impact mentioned there is also wrong. The ability to increase lockDuration is an expected behavior (thats how the design is) and becoming eligible for voting has nothing to do with emergencyunlock, it can also be done in normal operations and is not malicious. This doesn't mention the increase in voting power which is the primary issue, voting eligibility is of no concern. Hence invalid.
• radin200 - User is able to increase staking amount and lockDuration in MlumStaking no matter _emergencyUnlock's value #387 again lacks any valid attack path and has no description of any impact. According to sherlock rules, a valid duplicate should “Identify a valid attack path or vulnerability path”. Hence invalid
• PASCAL - User won't get accurate rewards under certain circumstances #567 is completely invalid. The description of the issue says ”So no matter how long the user staked before the current amountWithMultiplier will be amount multiplied with 1 instead of the right multiplier which will affect the reward” which is false because before the lockmultiplier gets updated, _harvestPosition() is called which uses the previous amountWithMultiplier to calculate the rewards earned uptil now and directly sends these rewards to the user. After that multiplier is updated for the position and that is expected behavior as the previous rewards have already been claimed and emergency unlock time period is meant for withdrawing and not to earn multiplied rewards for the period itself.
• Praise03 - During Emergency Unlock, User can call withdrawFromPosition to make premature withdrawals and get full rewards. #575 is invalid. The core point is that users can use withdrawFromPosition during emergency unlock. and claim rewards they had earned. But this is expected behavior. The rewards they earn were anyways earned by their positions, so its not a malicious action and does not lead to an attack or loss of funds for anyone.
• krot-0025 - MlumStaking::emergencyWithdraw leads to loss in case of funds and not allows user to withdraw stake during emergency. #660 is invalid due to the same reasoning as Praise03 - During Emergency Unlock, User can call withdrawFromPosition to make premature withdrawals and get full rewards. #575. Users using withdrawFromPosition during emergencyUnlock is completely normal as they only get their own rewards which they deserved.
• pashap9990 - _accRewardsPerShare is computed wrongly if unlockOperator calls emergencyWithdraw #368 is not a duplicate of this issue, but a duplicate of iamnmt - Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking #460.

Apart from these listed, other issues seem to be valid duplicates of #290.

[sherlock-admin3]

Escalate

There are many duplication errors here, out of which most issues are invalid in reality. These are listed below:

• Reentrants - addToPosition() isn't disabled during emergency unlock #15 is not a duplicate because it simply says addToPosition should have a certain check without describing the issue created. The impact has not been included, neither does the submission describe a valid attack path. This is against sherlock’s duplication rules here
• bbl4de - createPosition() and addToPosition() are callable even in case of emergency, decreasing rewards for users #131 the core point is the decrease in user’s rewards which is invalid because the emergencyWithdraw() function has comments saying the rewards have been let go by the staker because its an emergency, in which case the rewards he lost will not even be redistributed as pointed out by issue iamnmt - Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking #460. So the new staker cannot gain the rewards left out by emergency withdrawers as per current logic. This is invalid and not a duplicate.
• tedox - Users can abuse MlumStaking::setEmergencyUnlock to steal tokens #134 is not a duplicate. Its invalid because frontrunning is not possible on Iota evm and the contest readme says the protocol is to be deployed on Iota only. See here : IOTA VM Docs
• iamnmt - During emergency unlock period, users still can increase a position's lockDuration by calling MlumStaking#addToPosition, which makes the position eligible for voting in Voter #184 has wrong recommendation because avgDuration needs to be actually increased generally, the impact mentioned there is also wrong. The ability to increase lockDuration is an expected behavior (thats how the design is) and becoming eligible for voting has nothing to do with emergencyunlock, it can also be done in normal operations and is not malicious. This doesn't mention the increase in voting power which is the primary issue, voting eligibility is of no concern. Hence invalid.
• radin200 - User is able to increase staking amount and lockDuration in MlumStaking no matter _emergencyUnlock's value #387 again lacks any valid attack path and has no description of any impact. According to sherlock rules, a valid duplicate should “Identify a valid attack path or vulnerability path”. Hence invalid
• PASCAL - User won't get accurate rewards under certain circumstances #567 is completely invalid. The description of the issue says ”So no matter how long the user staked before the current amountWithMultiplier will be amount multiplied with 1 instead of the right multiplier which will affect the reward” which is false because before the lockmultiplier gets updated, _harvestPosition() is called which uses the previous amountWithMultiplier to calculate the rewards earned uptil now and directly sends these rewards to the user. After that multiplier is updated for the position and that is expected behavior as the previous rewards have already been claimed and emergency unlock time period is meant for withdrawing and not to earn multiplied rewards for the period itself.
• Praise03 - During Emergency Unlock, User can call withdrawFromPosition to make premature withdrawals and get full rewards. #575 is invalid. The core point is that users can use withdrawFromPosition during emergency unlock. and claim rewards they had earned. But this is expected behavior. The rewards they earn were anyways earned by their positions, so its not a malicious action and does not lead to an attack or loss of funds for anyone.
• krot-0025 - MlumStaking::emergencyWithdraw leads to loss in case of funds and not allows user to withdraw stake during emergency. #660 is invalid due to the same reasoning as Praise03 - During Emergency Unlock, User can call withdrawFromPosition to make premature withdrawals and get full rewards. #575. Users using withdrawFromPosition during emergencyUnlock is completely normal as they only get their own rewards which they deserved.
• pashap9990 - _accRewardsPerShare is computed wrongly if unlockOperator calls emergencyWithdraw #368 is not a duplicate of this issue, but a duplicate of iamnmt - Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking #460.

Apart from these listed, other issues seem to be valid duplicates of #290.

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.

[J4X-98]

Hey @chinmay-farkya @0xSmartContract ,

#15 clearly describes precisely the issue of the main submission. Being able to add liquidity on a paused contract is by itself already a medium impact. While the main issue described another medium impact (that leverages this), #15 should stay a valid dup.

[Oot2k]

I think this issue should be grouped with #138 or low.

Both have the same root cause of: addToPosition allowing to higher voting power.
Fixing 138 would fix this issue as well, because if addToPosition would update the times correctly it would not be possible to vote anymore.

I would also want to point out that this issue describes an extreme edge case Szenario:

• One bribe rewarder only rewards one Staking contract
• In case the admin decides to active emergency withdraw, all reward tokens are considered lost by definition (only way to withdraw is without claiming them)
• so even in case a user is abled to vote for reward distribution, the outcome of this vote does not matter

Hench I would consider the impact of this issue to be low.

[chinmay-farkya]

@J4X-98

#15 clearly describes precisely the issue of the main submission. Being able to add liquidity on a paused contract is by itself already a medium impact. While the main issue described another medium impact (that leverages this), #15 should stay a valid dup.

Your issue 15 does not identify any valid attack path. Simply stating that liquidity can be added while paused cannot be considered as a medium impact unless it can be shown to have second order effects or a loss/manipulation of some kind.

This issue 290 describes such an impact of misuse of the voting power while 15 does not identify it.

[Pascal4me]

@0xSmartContract
My issue #567 even though it's not a duplicate doesn't mean it is invalid.
It addresses the ability of a user to alter their position when contract is unlocked due to lack of access control on 'addToPosition()', yes the user will harvest current rewards with correct multiplier then lock multiplier will be set to zero because pool is unlocked so if the user comes back in let's say 6 months time to harvest again they'll get rewards calculates with zero lock multiplier.
But they all kinda have same root cause so I don't know
@WangSecurity I clearly started the issue for validity here

[chinmay-farkya]

I think this issue should be grouped with #138 or low.

Both have the same root cause of: addToPosition allowing to higher voting power. Fixing 138 would fix this issue as well, because if addToPosition would update the times correctly it would not be possible to vote anymore.

I would also want to point out that this issue describes an extreme edge case Szenario:

• One bribe rewarder only rewards one Staking contract
• In case the admin decides to active emergency withdraw, all reward tokens are considered lost by definition (only way to withdraw is without claiming them)
• so even in case a user is abled to vote for reward distribution, the outcome of this vote does not matter

Hench I would consider the impact of this issue to be low.

The assertion that this issue is same as #138 is incorrect because #138 is talking about how you can keep the position active with the same multiplier(and thus gain unfair share of rewards) under "normal operations of the system" due to the use of initialLockDuration, while this issue 290 talks about how during "only an emergencyUnlock" (and not under normal operations like #138) the positions can be gamed to immediately gain extra voting power and manipulate the voting data.

Both have separate root causes : core issue of 138 is that initialLockDuration is used for assigning multiplier "under normal operation"
while core issue of 290 is : during emergency unlock, addToPosition is allowed to add amount and gain greater voting power

Both have separate fixes :
fix for 138 is : to use lockDuration while assigning multiplier under all "normal operations"
fix for 290 is : restrict calling addToPosition while emergency unlock is active

These are clearly two separate issues.

[bbl4de]

@J4X-98

#15 clearly describes precisely the issue of the main submission. Being able to add liquidity on a paused contract is by itself already a medium impact. While the main issue described another medium impact (that leverages this), #15 should stay a valid dup.

Your issue 15 does not identify any valid attack path. Simply stating that liquidity can be added while paused cannot be considered as a medium impact unless it can be shown to have second order effects or a loss/manipulation of some kind.

This issue 290 describes such an impact of misuse of the voting power while 15 does not identify it.

I agree with @J4X-98 regarding the validity of issues based simply on the possibility to add liquidity to a paused contract. Also, issue #131 describes this impact which can be mitigated in the same way as the main issue. Your report - #290 is indeed presenting a valid attack vector that is possible based on the impact described by #131. I simply did not state what the malicious could do with the additional voting power. It is not a valid argument for invalidating an issue, rather a justification for the main issue to be chosen for the report.

[chinmay-farkya]

It is not a valid argument for invalidating an issue, rather a justification for the main issue to be chosen for the report.

Well we are governed by sherlock's rules for valid duplications. Its not a justification for an issue getting chosen for the report, but for what counts as a valid duplicate and what does not.

The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate.
Identify the root cause
Identify at least a Medium impact
Identify a valid attack path or vulnerability path
Only when the "potential duplicate" meets all four requirements will the "potential duplicate" be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates.

[bbl4de]

It is not a valid argument for invalidating an issue, rather a justification for the main issue to be chosen for the report.

Well we are governed by sherlock's rules for valid duplications. Its not a justification for an issue getting chosen for the report, but for what counts as a valid duplicate and what does not.

The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate.
Identify the root cause
Identify at least a Medium impact
Identify a valid attack path or vulnerability path
Only when the "potential duplicate" meets all four requirements will the "potential duplicate" be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates.

You're right. I should reiterate my reply. Adding liquidity to a paused contract is an attack vector - it's very straight forward because the bug is quite simple. What you have presented is additional impact of the attack vector. With this in mind, we may go through the requirements for a valid duplicate from the rules you have cited for issue #131:
1.Root cause - insufficient validation included
2.Medium impact - adding liquidity to a paused contract, manipulating reward distribution included
3.Valid attack path - simply calling addToPosition() at the right time is the attack vector in itself included

My #131 finding is therefore a valid duplicate according to the rules. What you provided is additional impact based on the root cause and primary impact, as @J4X-98 commented.

Note that I'm referring to my issue #131, I did not review other escalated findings.

[J4X-98]

@J4X-98

#15 clearly describes precisely the issue of the main submission. Being able to add liquidity on a paused contract is by itself already a medium impact. While the main issue described another medium impact (that leverages this), #15 should stay a valid dup.

Your issue 15 does not identify any valid attack path. Simply stating that liquidity can be added while paused cannot be considered as a medium impact unless it can be shown to have second order effects or a loss/manipulation of some kind.

This issue 290 describes such an impact of misuse of the voting power while 15 does not identify it.

Pausing is a security feature (like for example blacklisting) which is clearly bypassed and that's also described in our issue. This is medium on any platform.

Just because you found another medium impact it doesn't mean that all other medium impacts resulting out of the same root cause are invalid.

[yash-0025]

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

[chinmay-farkya]

Well @J4X-98 now I'm curious to know when did "adding liquidity while paused" become a medium impact in itself.

I disagree because if the liquidity they are adding during a paused state can be withdrawn later, and as long as it does not affect anything else in the system (like rewards etc.), it is not a medium impact but a low one.

[J4X-98]

Pausing means the protocol is paused e.g. no one can add or withdraw liquidity. The same way it is in any other DEX, vault etc. that's the whole reasoning behind it. This is broken by the implementation.

I don't think discussing with you makes any sense if this is not clearly a medium to you. I've already added my info for the judge and will wait for his judging.

[0xSmartContract]

This issue can be grouped with #138

[chinmay-farkya]

@0xSmartContract

This and #138 are completely different issues

#138 :

• It is about how the lockDuration calculation can be skewed and misused
• Bug manifests in normal operations
• Issue is that a certain multiplier effect can be maintained for longer than deserved
• Fix is to use new lockDuration when calculating multiplier

#290 :

• It is not related to the lockDuration, it is about how a position's vote amount can be increased
• Bug will only manifest in emergency unlock situation, and not under normal operations
• In case of emergency unlock, the staker actually loses the multiplier instead of maintaining the previous one, the new multiplier is zero
• Fix is to not allow addToPosition when emergencyUnlock is active.

[0xSmartContract]

This issue can be grouped with #138

They have similar root causes: Both issues are related to the position update logic in the MlumStaking contract. Issue 138 is related to incorrect multiplier calculation when adding to positions, while issue 290 is related to adding to positions during emergency locks.

They have interrelated effects: Both issues can affect users’ voting power and rewards. 138 allows users to get higher multipliers, while 290 allows voting manipulation during emergency locks.

Overlapping of solutions: To solve both issues, it is necessary to change the behavior of the addToPosition() function. These changes can be considered together to provide a more comprehensive solution.

Both issues are related to the position management and locking logic in the MlumStaking contract.

For these reasons, combining issues 138 and 290 can provide a more holistic and efficient solution, and it is a nice duplicate of these two to improve the overall code quality.

We are hearing from only @chinmay-farkya about grouped with 138 , this is not healthy, I would like to get comments from many valid Watsons on this issue

[J4X-98]

I think grouping these issues is not the right way ahead. Our issue #15 describes that pausing does not correctly work which is in no way related to #138. Grouping should be done based on the root cause not based on which issues affect a similar part of the code. Just because there are separate issues in the same function, they can not just be grouped into one.

[0xSmartContract]

1- At the moment we need to agree on now is: (This issue can be grouped with #138)
2-Then we look at the status of duplicates (incl. #15)

[J4X-98]

As stated before this group of issues and #138 are completely different issues. They just occur in a similar part of the code but have different root causes, different scenarios and different mitigations.

[chinmay-farkya]

@0xSmartContract I don't know why you are saying that this is similar to #138 . These are two completely different issues with different fixes, root causes, and affecting different mechanisms.

One is about lockDuration and multipliers, the other (this #290) is about manipulating the vote amount via addPOsition instead. It is not even remotely related to that as pointed in my comment here. Bugs manifest in completely different situations (the multiplier issue #138 will not even happen in case of emergency Unlocks but the lock amount s can only be manipulated in case of emergencyUnlock).

Overlapping of solutions: To solve both issues, it is necessary to change the behavior of the addToPosition() function

While the protocol team may need to comprehensively look at all bugs together, that doesn't mean that the issues are duplicates. While fixing any issue, the team will always need to look at the fix's effects on the whole system, does that make all the issues of a contract under one duplicate ?

[robertodf99]

Here I agree with @chinmay-farkya regarding the differences between both issues. While both stem from a malfunction in the same function, they can be considered independent of each other.

Issue #138 involves incorrect staking logic, whereas issue #290 pertains to unrestricted access control. The solution for the former involves changing the multiplier calculation logic, while the latter requires implementing a proper access control mechanism.

The consequences of the malfunction and the solutions for each issue are not interconnected. Unless both solutions can be addressed with a single approach, I wouldn’t consider them related.

[WangSecurity]

About issues mentioned in the escalation comment:

1. Reentrants - addToPosition() isn't disabled during emergency unlock #15 -- I agree it's a different issue, moreover, it looks as a design recommendation, just being able to deposit when you shouldn't I believe is not an impact but rather the root cause. Plus, I don't see any information that there should be such a check, hence, it may be intended.
2. bbl4de - createPosition() and addToPosition() are callable even in case of emergency, decreasing rewards for users #131 -- also not a duplicate and looks like describing the intended functionality.
3. tedox - Users can abuse MlumStaking::setEmergencyUnlock to steal tokens #134 -- also not a duplicate. But I would argue that currently, front-running is still possible on IOTA. It's unreliable, but you still can see the mempool (until the new IOTA EVM is launched with no mempool at the end of 2024), so you can send lots of transactions increasing the chance of successful front-running. But, this also requires the reward token to be minted right before the emergency unlock is set to true, moreover, looks to be intended. Hence, I agree with it being invalid.
4. iamnmt - During emergency unlock period, users still can increase a position's lockDuration by calling MlumStaking#addToPosition, which makes the position eligible for voting in Voter #184 -- as I understand, the escalation comment is precise here, agree it's invalid.
5. radin200 - User is able to increase staking amount and lockDuration in MlumStaking no matter _emergencyUnlock's value #387 -- don't see any medium severity impact.
6. PASCAL - User won't get accurate rewards under certain circumstances #567 -- again, the escalation comment is precise here, will invalidate it.
7. Praise03 - During Emergency Unlock, User can call withdrawFromPosition to make premature withdrawals and get full rewards. #575 -- don't see a medium impact and seems to be intended.
8. krot-0025 - MlumStaking::emergencyWithdraw leads to loss in case of funds and not allows user to withdraw stake during emergency. #660 -- same as above
9. pashap9990 - _accRewardsPerShare is computed wrongly if unlockOperator calls emergencyWithdraw #368 -- agree it's a duplicate of iamnmt - Unclaimed rewards when emergency withdrawing are not redistributed in MasterChef and MlumStaking #460

I agree it's not a duplicate of #138 based on this, this and this comments. Let me know if you want a deeper analysis of these 2 issues, but I don't see a point in me repeating the same arguments another time.

Planning to accept the escalation, invalidate 8 issues above and keep this a separate high severity family.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: accepted

[chinmay-farkya]

Hey @WangSecurity did you intend to keep this as high severity ?

Your previous comment says this :

keep this a separate high severity family

[WangSecurity]

Yes, you’re correct, excuse me for the typo, I meant to keep this as medium as it was initially. But if you have arguments disputing the severity, please share them.

[chinmay-farkya]

I think it is medium, so we'll settle it down.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.