rsam_eth - Double voting using unlocked positions can occur every voting epoch

[sherlock-admin4]

rsam_eth

Medium

Double voting using unlocked positions can occur every voting epoch

Summary

If lock time of a position is ended during a voting epoch, its possible to vote, withdraw, create a new position and vote again using same tokens.
MlumStaking::_remainingLockTime(id) == 0 ===> voter::vote(id,,) ===> mlumStaking::withdrawFromPosition(id,) ===> receive position.amount => mlumStaking::createPosition(receive_tokens,) ===> new position ===> voter::vote(id+1,,)

Vulnerability Detail

A position must have been locked (initialLockDuration) for at least 3 months (_minimumLockTime) to be eligible for voting:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L172-L174

        if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
            revert IVoter__InsufficientLockTime();
        }

also lockDuration of this position must be greater than _periodDuration which is duration of a voting epoch:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L175-L177

        if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }

since position.lockDuration and position.initialLockDuration do not indicate how much time is remaining till the position is unlocked, a position can exist with sufficient lockDuration and initialLockDuration but with a position.startLockTime of 3 months ago (_minimumLockTime), which is considered unlocked and can be withdrawn:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L624-L628

    function _withdrawFromPosition(
        address nftOwner,
        uint256 tokenId,
        uint256 amountToWithdraw
    ) internal {
        require(amountToWithdraw > 0, "null");

        StakingPosition storage position = _stakingPositions[tokenId];
        require(
            _unlockOperators.contains(nftOwner) ||
                //@audit unlocked
                (position.startLockTime + position.lockDuration) <=
                _currentBlockTimestamp() ||
                isUnlocked(),
            "locked"
        );
        //...
        stakedToken.safeTransfer(nftOwner, amountToWithdraw);
   }

tokens received from this position can be used to create a new position with sufficient lockDuration and initialLockDuration:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L375-L382

and vote again using this new position, as we can see here (Voter:vote), only condition that we must pass is this:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L167-L169

        if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
            revert IVoter__AlreadyVoted();

and since we are using a new tokenId we can cast our votes again.

Impact

after _minimumLockTime (3 months) passed, every epoch, at least one double voting can occur by all the position owner who have locked their tokens at least _minimumLockTime ago.
example:

• block.timestamp == 1000
• new epoch started (epoch number = 0)
• _minimumLockTime = 300
• _periodDuration = 100
• Alice creates a position with 100 tokens and initialLockDuration == 300 (position.startLockTime == 1000)
• 100 seconds passed (block.timestamp == 1100)
• new epoch started (epoch number = 1)
• Bob creates a position with 100 tokens and initialLockDuration == 300 (position.startLockTime == 1100)
• 100 seconds passed (block.timestamp == 1200)
• new epoch started (epoch number = 2)
• John creates a position with 100 tokens and initialLockDuration == 300 (position.startLockTime == 1200)
• 100 seconds passed (block.timestamp == 1300)
• new epoch started (epoch number = 3)
• Alice double votes (200 votes, 2x bob and john votes) in epoch number 3 since her position is unlocked
• 100 seconds passed (block.timestamp == 1400)
• new epoch started (epoch number = 4)
• Bob double votes (200 votes, 2x alice and john votes) in epoch number 4 since his position is unlocked
• 100 seconds passed (block.timestamp == 1500)
• new epoch started (epoch number = 5)
• John double votes (200 votes, 2x bob and alice votes) in epoch number 5 since his position is unlocked
  the loop continues, since Alice created a new position _minimumLockTime seconds ago after double voting:
• 100 seconds passed (block.timestamp == 1600)
• new epoch started (epoch number = 6)
• Alice double votes (200 votes, 2x bob and john votes) in epoch number 6 since her position is unlocked
  and so on...

This coded PoC demonstrates this issue (add to Voter.t.sol):

    function testDoubleVote() public {
        //create a position with lock time of 14 days
        vm.prank(DEV);
        _createPosition(ALICE);

        //2 weeks passed
        skip(2 weeks);

        //start a new voting epoch
        vm.prank(DEV);
        _voter.startNewVotingPeriod();

        //epch started, no votes yet
        assertEq(_voter.getCurrentVotingPeriod(), 1);
        assertEq(_voter.getTotalVotes(), 0);

        vm.startPrank(ALICE);
        //1- first vote, 1 ether
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
        //2- withdraw
        _pool.withdrawFromPosition(1, 1 ether);
        //3- create a new position
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 2 weeks);
        //4- vote again, 1 ether
        _voter.vote(2, _getDummyPools(), _getDeltaAmounts());
        vm.stopPrank();

        //casted two times in same epoch
        assertEq(_voter.getCurrentVotingPeriod(), 1);
        assertEq(_voter.getTotalVotes(), 2 ether);
    }

Code Snippet

Tool used

Manual Review

Recommendation

instead of checking position.lockDuration, check remaining lock time of the position. to do so, we can add a view function into MlumStaking contract which gives us remaining lock time of a given position:

    function getPositionRemainingTime(uint id) public view returns (uint) {
        return _remainingLockTime(_stakingPositions[id]);
    }

and then use this function to check the remaining lock time of a position:

        if (_mlumStaking.getPositionRemainingTime(tokenId) < _periodDuration) {
            revert IVoter__InsufficientLockTime();
        }

Duplicate of #166