dany.armstrong90 - Voting is always reverted when it has bribes to notify.

[sherlock-admin2]

dany.armstrong90

High

Voting is always reverted when it has bribes to notify.

Summary

BribeRewarder.sol#_modify() checks that msg.sender is owner of tokenId.
So when it is called from Voter, this call is always reverted.

Vulnerability Detail

BribeRewarder.sol#deposit() function which is called from Voter contract is as follows.

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

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

Here, BribeRewarder.sol#_modify() function is as follows.

    function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
        private
        returns (uint256 rewardAmount)
    {
264     if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
            revert BribeRewarder__NotOwner();
        }

        ...
    }

On L264, it checks that msg.sender(voter contract) is owner of tokenId.
Voter.sol#ownerOf() which is called here is as follows.

    function ownerOf(uint256 tokenId, address account) external view returns (bool) {
        return _mlumStaking.ownerOf(tokenId) == account;
    }

But the owner of tokenId is not Voter contract.
So BribeRewarder.sol#deposit() is always reverted.
We can see this fact from Voter.sol#vote() function.

    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
161@>   if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
            revert IVoter__NotOwner();
        }

        ...

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

            ...

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

        _totalVotes += totalUserVotes;

        _hasVotedInPeriod[currentPeriodId][tokenId] = true;

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

We can see on L161, owner of tokenId is caller of Voter.sol#vote() function.

Impact

Voting is always reverted when it has bribes to notify.

Code Snippet

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

Tool used

Manual Review

Recommendation

BribeRewarder.sol#_modify() function has to be modified as follows.

    function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
        private
        returns (uint256 rewardAmount)
    {
-       if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
+       if (msg.sender != _caller && !IVoter(_caller).ownerOf(tokenId, msg.sender)) {
            revert BribeRewarder__NotOwner();
        }

        ...
    }

Duplicate of #39