ChinmayF - Double voting is possible in Voter contract

[sherlock-admin3]

ChinmayF

High

Double voting is possible in Voter contract

Summary

Anyone who has staked tokens in the MLUMStaking contract gets a voting power equivalent to the amount of assets they deposited(* the multiplier in MLUMStaking). They are allowed to vote for their favorite farms(ie. pools) in order to direct more LUM emissions to that farm in Masterchefv2.

Voter.sol::vote() function checks that the tokenID(staking position) was locked for atleast minimumLockTime(ie. currently 90 days) in order to be eligible for voting. It also has a hasVotedInPeriod() check to allow an NFT to vote only once in a voting period.

But these checks are insufficient and double voting is still possible.

Vulnerability Detail

This is a part of the vote() function :

        // check if _minimumLockTime >= initialLockDuration and it is locked
        if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
            revert IVoter__InsufficientLockTime();
        }
        if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }

The first check is good in checking that when the position was initially locked, it was locked for at least 90 days. This works correctly.

The second check says that the lock duration of the position should be greater than the periodDuration (14 days right now) to be eligible for voting. This is done to ensure that the assets associated with that tokenID will remain locked for the entire duration of currentVotingPeriod => as is also seen in the comment "check if _minimumLockTime >= initialLockDuration and it is locked".

But this has been implemented incorrectly and will allow double voting using the same assets. Lets go through a step-by-step example :

Assume that a user opened a staking position of 90 days with amountWithMultiplier = 20000e18. So initialLockDuration = 90 days which will make the first check pass. Also, lockDuration = 90 days because he did not renew, extend or add to the position, so it will remain the same as initialLockDuration. So the second check will also pass and the tokenID is eligible for voting.

• Now suppose that the position lock is about to expire in 1 day. Now a voting period starts. So the user votes with this tokenID, and the vote data gets updated with his "amountWithMultiplier".
• After 1 day, his position lockDuration has ended. So he can now withdraw the NFT on MLUMStaking's end. He withdraws the NFT, collects all harvest rewards earned in MLUMStaking and basically he gets his "position.amount" assets back to his address.
• He immediately opens another staking position on the MLUMStaking with the same assets. He gets a new tokenID minted with initialLockDuration again = lockDuration = 90 days.
• Since the voting period is still running(only 1 day has passed), he comes to the Voter contract again and votes for his favorite pool with his new tokenID. This will go through normally because the tokenID is a new NFT with no entry in _hasVotedInPeriod mapping.
• And boom ! The attacker used the same assets to vote twice in the same voting period via different tokenIDs.

If the attacker has a large amount of stakedToken, they can use it to vote twice for their favorite farm, and this will make their votes count twice in the Voter.sol contract. This way, he was able to utilize the same X amount of assets he had to vote 2X in the same voting period.

This is very problematic because the whole LUM emissions mechanics in MasterChefV2.sol depends on the votes accrued for a pool. Attacker can direct much more LUM emissions to their favorite pool if they have many such positions open.

Note that this can be done with a position lock that is expiring anytime during the voting period. And this can also be done with position locks that have already expired before the voting period started (but the owner has not withdrawn anything).

Apart from the double voting, the user will also earn double rewards on all bribe rewarders registered for that period and pool. This way they can steal the rewards from other users. To do this, they have to withdraw all assets from the staking position and leave 1 wei to keep the NFT alive(they need to be the owner of the NFT to claim rewards from bribe rewarders after the voting period ends). The actual attack above also works in this case.

Impact

This can be used to manipulate the distribution of LUM rewards. It also corrupts the votes data and completely hijacks the Voting and bribing functionality if many attackers start doing this.

It should never be possible to vote twice using the same assets, thats the whole purpose of a locked NFT/ locked asset position used for voting.

Code Snippet

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

Tool used

Manual Review

Recommendation

The solution is simple : In vote() function, instead of checking that the lockDuration needs to be > periodDuration, check that the ending lock time of the staked position NFT is > periodDuration, which will ensure that the associated staked assets will remain locked until the current voting period ends.

        uint256 startLockTime = _mlumStaking.getStakingPosition(tokenId).startLockTime;
        uint256 lockDuration =  _mlumStaking.getStakingPosition(tokenId).lockDuration;
        uint256 endTime = startLockTime + lockDuration;

        if(endTime < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }

Duplicate of #166