PUSH0 - Users can artificially create a voting ballot with 2 weeks lockDuration, effectively bypassing the 3-month limit

[sherlock-admin4]

PUSH0

Medium

Users can artificially create a voting ballot with 2 weeks lockDuration, effectively bypassing the 3-month limit

Summary

The requirements to be eligible for voting (as per the code comments) is that the user must hold a staking duration of at least 3 months, and the position is locked. The contract exposes several functions for the user to increase their lock positions.

We show the method to create a lock position with initialLockDuration to be 3 months, and lockDuration to be 2 weeks. The position can then effectively be re-locked for 2 weeks, and then vote as necessary for each period.

Vulnerability Detail

The contract MLumStaking allows a user to stake their MLUM for voting power. The contract exposes two functions for extending their locking duration:

• renewLockPosition(), re-locks a position for exactly _stakingPositions[tokenId].lockDuration
• extendLockPosition(), extends a lock position by a user's choice of duration. The new duration must at least be as long as the position's initial lock duration.

The contract also exposes a function addToPosition(), which allows a user to add MLUM to a position. The position's lock duration is modified to the following as per the docs:

New Lock Time = (Remaining Lock Time * Staked MLUM Amount + MLUM Amount to add * Initial Lock Duration) / (MLUM Staked Amount + MLUM Amount to add)

uint256 avgDuration = (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration)
    / (position.amount + amountToAdd);

position.startLockTime = _currentBlockTimestamp();
position.lockDuration = avgDuration;

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L410-L411

A user can use this mechanism to their advantage to craft a position with an initial lock duration of 3 months but a lockDuration of 2 weeks using the following method:

• First, the user locks their desired amount of MLUM for 3 months. Let's assume it's 50 MLUM, the minimum amount required for voting
• Then, when the lock is 2 weeks away from expiring, the user uses addToPosition() and adds one wei of MLUM to their lock position.
  • From the formula, the new lockDuration will be (2 weeks * 50e18 + 1 * 3 months)/(50e18 + 1). This is a weighted sum that is heavily skewed towards 2 weeks, so the sum evaluates to 2 weeks.
• The position now has an initial lock duration of 3 months, but a lock duration of only 2 weeks

The user has created a lock position with initialLockDuration of 3 months, but a lock duration with only 2 weeks. This condition passes all voting checks (code comments and the code itself). The user can now continuously use renewLockPosition to "renew" the position, but merely extending it by 2 weeks while still gaining the full voting power (multiplier and amount) as if the position was locked for 3 months.

Impact

Users can create a voting ballot with a duration of two weeks, which allows perpetually renewing of their voting power, effectively bypassing the three-month restriction for voting eligibility.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L410-L411

Tool used

Manual Review

Recommendation

addToPosition()'s duration modification should be reworked to disable the effect of lowering the lock duration on the votes.

Duplicate of #138

[0xSmartContract]

Users initially lock their staking positions for 3 months and recalculate the lock period by adding a small amount of tokens 2 weeks before the lock period expires, effectively making the lock period 2 weeks. Thus, users maintain their voting authority by renewing the lock period every 2 weeks.

[0xHans1]

PR: #138

We will use the initalLockDuration on renewLockPosition

[sherlock-admin2]

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

[chinmay-farkya]

Escalate

This issue is not a separate bug but a duplicate of #138

Scenario 2 of #138 also describes the exact same attack path : use of addToPosition by adding 1 wei of new deposit to skew the avgDuration calculation.

[sherlock-admin3]

Escalate

This issue is not a separate bug but a duplicate of #138

Scenario 2 of #138 also describes the exact same attack path : use of addToPosition by adding 1 wei of new deposit to skew the avgDuration calculation.

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.

[Honour-d-dev]

this is a dup of #138 , same root cause

[0xSmartContract]

Both issues state that the locking period calculation formula in the addToPosition() function can be manipulated.

The solution suggestions are similar - it is stated that the locking period reduction effect of the addToPosition() function should be prevented.

As a result, these two issues seem to explain the same problem in different ways.

On the other hand, if we go into detail;
Issue number 366 deals with users manipulating the voting period and reducing the 3-month locking period to 2 weeks, while issue number 138 deals with users increasing their voting power by artificially extending the locking periods. Although both issues are related to the manipulation of locking periods, they examine different mechanisms and effects.

I am undecided on this issue, but different issues make more sense to me

[chinmay-farkya]

@0xSmartContract

Issue number 366 deals with users manipulating the voting period and reducing the 3-month locking period to 2 weeks, while issue number 138 deals with users increasing their voting power by artificially extending the locking periods.

Issue 366 is a very specific subset of what issue 138 describes. The core problem is the use of initialLockDuration for assigning the multiplier. 138 is a broader decsription of how the avgDuration calculation can be manipulated to maintain a certain multiplier, while 366 describes how the lockDuration can be maintained above 2 weeks to remain eligible for voting.

Issue 138 cannot be manifested without the conditions described in 366 because the avgDuration calculation needs to remain above 2 weeks to be eligible for voting and have some effect of the manipulated multiplier. That is why I say that both conditions are required for the bug to manifest, and 366 is a subset of 138 with a specific value of 2 weeks lockDuration. Same root cause and same offered recommendations.

You can also see that the sponsor fixed both the issues with a single fix.

[0xSmartContract]

After reviewing both issues (#366 and #138) and the comments provided, I agree that there are significant overlaps between them. However, I would like to emphasize a few key points that could help clarify their distinctiveness and the need for separate considerations:

Specificity of Issue #366: While it is true that Issue #366 can be seen as a subset of Issue #138, it addresses a very specific scenario where the lockDuration is manipulated to bypass the 3-month voting eligibility requirement by reducing it to 2 weeks. This specific attack vector is not explicitly highlighted in Issue #138, which makes Issue #366 a valuable addition for those focusing on voting mechanics and governance security.

Broader Context of Issue #138: Issue #138 provides a broader context of how the avgDuration calculation can be manipulated, focusing more on the overall multiplier mechanism. It describes various scenarios where users can exploit the addToPosition function to maintain or even enhance their voting power, beyond just the voting eligibility period.

Distinct Mechanisms and Effects: Although both issues stem from the same root cause—manipulation of the lock duration calculation—they target different outcomes. Issue #366 is concerned with maintaining voting eligibility with a reduced lock period, while Issue #138 addresses the potential increase in voting power by leveraging the multiplier calculation. These different outcomes highlight the need to address both issues independently to fully understand the impact and required fixes.

Therefore, while recognizing the overlap, I maintain that both issues warrant separate consideration due to their distinct focus areas and impact on the staking mechanism. Ensuring a comprehensive fix requires acknowledging the specific attack vector described in Issue #366 alongside the broader manipulation described in Issue #138.

I look to further discussions to reach the best possible resolution for both issues

[chinmay-farkya]

I believe that dups are decided based on same root causes, same fixes and same attack paths, which are all same for these issues.

[Reentrants]

Take a step back and think about this issue for a moment.

Is there a difference between additionally locking up 1 wei vs doing nothing when the lock is 2 weeks away from expiring? Both remain the same: initialLockDuration is at 3 months, (remaining) lock duration of 2 weeks. There is no benefit in locking up 1 more wei (or a few).

---

Let's now consider the broader issue, and why we didn't submit the issue. TLDR: it's a short term gain and becomes economically infeasible over time.

What's the minimum amount required to add to a position to have avgDuration be equal to _periodDuration = 14 days?

Working out the math,

minAdditionalAmount = (_periodDuration - timeLeft) * initialAmount / (initialLockDuration - _periodDuration);

Hence, if the initial lock duration is 1 year, u'd need to add ~4% of initial amount, but that's to hit the bare minimum of 14 days, and you will have to keep on increasing by 4% of the total stake every 14 days. Of course, the longer the initial lock up duration, the smaller the increment required. For instance, 5 years lockup = 0.077% increment, but at such timeframes, one may argue that the user should be entitled to being allowed to perform this "hack".

Would like to point out this isn't well covered by the primary issue.

[0xSmartContract]

@WangSecurity, I am in the grey area in this issue. I have stated my comments here, Watson's comments are above. For your information about the final decision.

#366 (comment)

[WangSecurity]

Firstly, I've hidden one comment that was subjective, sarcastic and pointless. Let's refrain from such comments and keep the discussion objective.

I agree that this issue is indeed a duplicate of #138, while the scenarios are different, the root cause I believe is the same. Hence, planning to accept the escalation and duplicate this issue with #138.

[WangSecurity]

Result:
Medium
Duplicate of #138

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.