pwning_dev - Overlapping Voting Periods

[sherlock-admin4]

pwning_dev

High

Overlapping Voting Periods

Summary

@pwning_dev

Vulnerability Detail

The function startNewVotingPeriod increments _currentVotingPeriodId and starts a new voting period without checking if the previous voting period has ended. This could lead to overlapping voting periods, where a new voting period starts before the previous one has concluded.

function startNewVotingPeriod() public onlyOwner {
    _currentVotingPeriodId++;

    VotingPeriod storage period = _startTimes[_currentVotingPeriodId];
    period.startTime = block.timestamp;
    period.endTime = block.timestamp + _periodDuration;

    emit VotingPeriodStarted();
}

Impact

this bug can lead to manipulation of voting results, allowing a single tokenId to cast more votes than intended. This can significantly distort the voting outcomes, potentially leading to unfair distribution of rewards, compromised integrity of the voting process, and financial loss for honest participants.

Code Snippet

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

Tool used

Manual Review

Recommendation

1. Add a check to ensure the previous voting period has ended before starting a new one.

2. Immediately mark the tokenId as having voted before proceeding with any other state changes or external calls.

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 already voted
    if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
        revert IVoter__AlreadyVoted();
    }

    // Immediately mark as voted to prevent reentrancy or double voting
    _hasVotedInPeriod[currentPeriodId][tokenId] = true;

    // 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();
    }

    uint256 votingPower = _mlumStaking.getStakingPosition(tokenId).amountWithMultiplier;

    // check if deltaAmounts > votingPower
    uint256 totalUserVotes;
    for (uint256 i = 0; i < pools.length; ++i) {
        totalUserVotes += deltaAmounts[i];
    }

    if (totalUserVotes > votingPower) {
        revert IVoter__InsufficientVotingPower();
    }

    IVoterPoolValidator validator = _poolValidator;

    for (uint256 i = 0; i < pools.length; ++i) {
        address pool = pools[i];

        if (address(validator) != address(0) && !validator.isValid(pool)) {
            revert Voter__PoolNotVotable();
        }

        uint256 deltaAmount = deltaAmounts[i];

        _userVotes[tokenId][pool] += deltaAmount;
        _poolVotesPerPeriod[currentPeriodId][pool] += deltaAmount;

        if (_votes.contains(pool)) {
            _votes.set(pool, _votes.get(pool) + deltaAmount);
        } else {
            _votes.set(pool, deltaAmount);
        }

        _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount); // msg.sender, deltaAmount);
    }

    _totalVotes += totalUserVotes;

    emit Voted(tokenId, currentPeriodId, pools, deltaAmounts);
}

[WangSecurity]

Invalid based on the comment #166 (comment).