0xAsen - Users can't vote because of a wrong check in BribeRewarder::_modify()

[sherlock-admin4]

0xAsen

High

Users can't vote because of a wrong check in BribeRewarder::_modify()

Summary

The current implementation mistakenly checks if the Voter contract is an owner of the user's staking position leading to a revert and inability of the users to vote.

Vulnerability Detail

Let's check the user flow to cast a vote.
A user calls Voter.sol::vote():

function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
//unrelated functionality
 
       // check ownership of tokenId
        if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
            revert IVoter__NotOwner();
        }

//unrelated functionality

             //notify the rewarders about the casted votes
            _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount);
}

As you can see, the function checks if the user(msg.sender) is the owner of the tokenId of the staking position and later the _notifyBribes function is called:

    function _notifyBribes(uint256 periodId, address pool, uint256 tokenId, uint256 deltaAmount) private {

        IBribeRewarder[] storage rewarders = _bribesPerPriod[periodId][pool];

        for (uint256 i = 0; i < rewarders.length; ++i) {
            if (address(rewarders[i]) != address(0)) {
                rewarders[i].deposit(periodId, tokenId, deltaAmount);
                _userBribesPerPeriod[periodId][tokenId].push(rewarders[i]);
            }
        }
    }

And the function calls the deposit() function for each rewarder so that it notifies them of the casted votes and so the user is able to collect his rewards. BribeRewarder.sol::deposit():

    function deposit(uint256 periodId, uint256 tokenId, uint256 deltaAmount) public onlyVoter {
        _modify(periodId, tokenId, deltaAmount.toInt256(), false);

        emit Deposited(periodId, tokenId, _pool(), deltaAmount);
    }

At this point the msg.sender is the Voter.sol contract and this is additionally verified by the modifier onlyVoter that enables only the Voter contract to call this function. Then _modify() is called:

    function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
        private
        returns (uint256 rewardAmount)
    {

        if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
            revert BribeRewarder__NotOwner();
        }
//unrelated functionality
}

However, as you can see, the function checks if the msg.sender is the owner of the tokenId(staking position), and if it's not, the transaction reverts.

But the msg.sender here is the Voter contract, and the owner is the user, so the transaction will revert even though the user should be able to vote.

Impact

Users are unable to vote.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L264-L266

Tool used

Manual Review

Recommendation

The check is unnecessary in the vote -> deposit flow as we're verifying that the user is the owner of tokenId in the vote() function as shown.

The check is only needed in the claim() function so it should be moved there and deleted from _modify like that:

    function claim(uint256 tokenId) external override {
        //get the latest finished period
        uint256 endPeriod = IVoter(_caller).getLatestFinishedPeriod();
        uint256 totalAmount;

        if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) { <----
            revert BribeRewarder__NotOwner();
        }

        // calc emission per period cause every period can every other durations
        for (uint256 i = _startVotingPeriod; i <= endPeriod; ++i) {

            totalAmount += _modify(i, tokenId, 0, true);
        }

        emit Claimed(tokenId, _pool(), totalAmount);
    }

Duplicate of #39