Hunter - unchecked (finished lockDuration of mlumStaking positions) during vote() opening up the ability to double vote with same funds in the same period.

[sherlock-admin2]

Hunter

High

unchecked (finished lockDuration of mlumStaking positions) during vote() opening up the ability to double vote with same funds in the same period.

Summary

unchecked (finished lockDuration of mlumStaking positions) during vote() in votercontract opening up the ability to double vote with same funds in the same period.

Vulnerability Detail

in vote()

File: Voter.sol
153:     function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
154:         if (pools.length != deltaAmounts.length) revert IVoter__InvalidLength();
155: 
///////////////...............skip
165:         uint256 currentPeriodId = _currentVotingPeriodId;
166:         // check if alreay voted
167:         if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
168:             revert IVoter__AlreadyVoted();
169:         }
170: 
171:         // check if _minimumLockTime >= initialLockDuration and it is locked
172:         if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
173:             revert IVoter__InsufficientLockTime();
174:         }
175:         if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
176:             revert IVoter__InsufficientLockTime();
177:         }
///////////////...............skip
179:         uint256 votingPower = _mlumStaking.getStakingPosition(tokenId).amountWithMultiplier;
///////////////...............skip
216:         _hasVotedInPeriod[currentPeriodId][tokenId] = true;
217: 
218:         emit Voted(tokenId, currentPeriodId, pools, deltaAmounts);
219:     }

we see that in Line #167 we revert if the user has voted before in this Period

in Lines #172,#175 we check that lockDuration and initialLockDuration are larger than _periodDuration(14 days) and _minimumLockTime respectively

To give more context about lockDuration from _mlumStaking

• those are duration that the user chose to lock his mlum and is capped at 365 Days ( to prevent very high multipliers)
• those multipliers are used to get amountWithMultiplier that is used for rewards calculations - and voting power retreivals as seen in Line tedox - Users can double their voting power inside Voter.sol #179

The problem here comes from the fact that there is no check for finished lockDuration of mlumStaking positions and i will describe with an example why

• Alice stakes for 365 Days and gets max multiplier for his money
• Alice lock duration ends but he didn't withdraw his mlumStaking position nor renewed it
• Alice sees a Pool that needs votes and wants to vote for it (taking into considerations any intentions for doing so)
• Alice Votes for it
• Alice withdraws his position fully and destroy his position (burning the tokenId that he voted with before)
• In the same time Alice Deposit his full balance again and gets minted a new tokenId that is not registerd in _hasVotedInPeriod mapping
• Alice votes again for the same Pool

Impact

now this has alot of impacts

• breaking invariant of being able to vote two times in the same period with the same funds
• stealing extra rewards from BribeRewarder contract connected to that Pool
• funds loss to other Pools in the voting period that should have had more weight than this Pool and would have gotten larger mlum emmissions
• voting manipulation and unfair results over all

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/7fd1a65b76d50f1bf2555c699ef06cde2b646674/magicsea-staking/src/Voter.sol#L153-L219

Tool used

Manual Review

Recommendation

check if position.startLockTime + position.lockDuration) <= _currentBlockTimestamp() revert the vote() with error renew your lock

or you need to make sure that position.startLockTime + position.lockDuration) will be less than _currentBlockTimestamp() and will be less than the end time of voting period

Duplicate of #166