sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

dhank - Voter.sol::getVotesPerPeriod() is actually returning the votes of the period in a particular pool instead of the total votes in that period. #627

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

dhank

Medium

Voter.sol::getVotesPerPeriod() is actually returning the votes of the period in a particular pool instead of the total votes in that period.

Summary

getVotesPerPeriod() is actually returning the votes of the period in a particular pool instead of the total votes in that period. This is because we are not updating _totalVotesInPeriod when the user marks his vote in that period.

Vulnerability Detail

    /// @dev period => total amount of votes
    mapping(uint256 => uint256) private _totalVotesInPeriod; 

1) _totalVotesInPeriod is not updated whenever a user marks his vote using vote(). https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/Voter.sol#L153-L219

2) No function to get _totalVotesInPeriod

3) Also getVotesPerPeriod() is returning wrong value than expected.

    function getVotesPerPeriod(uint256 periodId, address pool) external view override returns (uint256) {
        //..@audit _totalVotesInPeriod is not updated anywhere and _totalVotesInPeriod is supposed to be returned
        return _poolVotesPerPeriod[periodId][pool];
    }

Returning votes in a particular pool instead of total votes

Impact

getVotesPerPeriod() is actually returning the votes of the period in a particular pool instead of the total votes in that period.

Code Snippet

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

Tool used

Manual Review

Recommendation

Update _totalVotesInPeriod when user votes and add a getter to get this value.

0xSmartContract commented 4 months ago

getVotesPerPeriod does not affect anything, so there is no loss of funds, so it is informational