TessKimy - Wrong execution flow of vote function causes users can't use their voting power

[sherlock-admin3]

TessKimy

Medium

Wrong execution flow of vote function causes users can't use their voting power

Summary

While voting period is active, user can't use it's own voting power in different transactions due to wrong validation checks.

Vulnerability Detail

Let say Alice created a position using 2 ether with 2 weeks and she wants to vote for pool X with her 50% voting power. Voting power is defined as following line:

uint256 votingPower = _pool.getStakingPosition(AliceTokenId).amountWithMultiplier;

After calling vote() for the first time, her tokenId is eliminated from all the other voting operations without checking the value of used voting power.

Impact

It's a core system functionality. Voting power always should be equal to her position with multiplier. But user can't use it in separate transactions due to wrong sequence diagram.

Code Snippet

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

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

Proof of Concept

Following test function can be used for testing:

    function test_voteInSeparateTransactions() public {
        address user = ALICE;

        vm.prank(DEV);
        _voter.startNewVotingPeriod();

        _createPosition(user);

        vm.startPrank(user);
        uint256 votingPower = _pool.getStakingPosition(1).amountWithMultiplier;
        console.log("Voting power of Alice before vote: %s", votingPower);
        uint256[] memory partialVote = new uint256[](1);
        partialVote[0] = votingPower / 2;
        _voter.vote(1, _getDummyPools(), partialVote);
        // Voting power should be reduced by 50%
        console.log("Voting power of Alice after vote: %s", votingPower - votingPower / 2);
        console.log("Voting the remainning power...");
        _voter.vote(1, _getDummyPools(), partialVote);
        vm.stopPrank();
    }

It logged following lines:

Ran 1 test for test/Voter.t.sol:VoterTest
[FAIL. Reason: IVoter__AlreadyVoted()] test_voteInSeparateTransactions() (gas: 806706)
Logs:
  Voting power of Alice before vote: 1076700000000000000
  Voting power of Alice after vote: 538350000000000000
  Voting the remainning power...

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.31ms (2.64ms CPU time)

Ran 1 test suite in 36.83ms (4.31ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/Voter.t.sol:VoterTest
[FAIL. Reason: IVoter__AlreadyVoted()] test_voteInSeparateTransactions() (gas: 806706)

Encountered a total of 1 failing tests, 0 tests succeeded

Tool used

Manual Review

Recommendation

Storing voted amount in storage will solve the problem ( variable name should be configured again ):

@@ -29,7 +29,7 @@ contract Voter is Ownable2StepUpgradeable, IVoter {
     uint256 private _currentVotingPeriodId;

     /// @dev votingPeriodId => tokenId => hasVoted
-    mapping(uint256 => mapping(uint256 => bool)) private _hasVotedInPeriod;
+    mapping(uint256 => mapping(uint256 => uint256)) private _votedAmountInPeriod;

@@ -164,7 +166,8 @@ contract Voter is Ownable2StepUpgradeable, IVoter {

         uint256 currentPeriodId = _currentVotingPeriodId;
         // check if alreay voted
-        if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
+        uint256 votingPower = _mlumStaking.getStakingPosition(tokenId).amountWithMultiplier;
+        if (!(_votedAmountInPeriod[currentPeriodId][tokenId] < votingPower)) {
             revert IVoter__AlreadyVoted();
         }

[0xSmartContract]

This is an architectural choice. A tokenId can only vote once in a period, making voting simple and manageable. This approach helps maintain the integrity and accuracy of voting processes

[DemoreXTess]

Escalate If it's a design choice, I can't say it should be solved but I believe this approach is not user friendly when we compare with my approach. It's my first contest on Sherlock, I don't know how those processes work in here but I really wonder the sponsor's idea about it if we're able to ask.

[sherlock-admin3]

Escalate If it's a design choice, I can't say it should be solved but I believe this approach is not user friendly when we compare with my approach. It's my first contest on Sherlock, I don't know how those processes work in here but I really wonder the sponsor's idea about it if we're able to ask.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.