rsam_eth - BribeRewarder uses wrong address to check owner of tokenId

[sherlock-admin2]

rsam_eth

High

BribeRewarder uses wrong address to check owner of tokenId

Summary

Vulnerability Detail

after casting a vote for a pid, the voted amount will be deposited into a BribeRewarder:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L211

_notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount);

the BribeRewarder::deposit function is called here from Voter:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L225

    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)) {
                //@audit call BribeRewarder::deposit
                rewarders[i].deposit(periodId, tokenId, deltaAmount);
                _userBribesPerPeriod[periodId][tokenId].push(rewarders[i]);
            }
        }
    }

we can see that only voter contract is able to call BribeRewarder::deposit:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L143-L147

    function deposit(
        uint256 periodId,
        uint256 tokenId,
        uint256 deltaAmount
    ) public onlyVoter { //@audit only voter can call deposit, otherwise things will be messed up
        _modify(periodId, tokenId, deltaAmount.toInt256(), false);

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

However, on the next line, _modify function is passing wrong address (msg.sender which is address of voter) as account parameter in Voter::ownerOf function:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L264-L266

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

Voter::ownerOf function expects second parameter (account) to be owner of tokenId, but since voter doesn't own that nft, deposit function will revert, which in result reverts Vote::vote function rendering voting process impossible:
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L402-L404

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

Impact

any call to Voter::vote function will be reverted if there exist at least one BribeRewarder for current epoch, the only fix will be to either ignore all bribe rewarders by moving into next epoch which does not have any rewarders, however, since rewarders can be created an added into an epoch by anyone, this approach might not be feasable, making the impact of this issue higher.

Code Snippet

Tool used

Manual Review

Recommendation

to resolve this issue, pass msg.sender (the address who invoked Voter::vote) into BribeRewarder::deposit function, this requires BribeRewarder::deposit and BribeRewarder::_modify to be changed slightly taking one more parameter (caster):

BribeRewarder.sol:

    function deposit(
        uint256 periodId,
        uint256 tokenId, 
        uint256 deltaAmount 
        address caster //address casting votes
    ) public onlyVoter {
        _modify(periodId, tokenId, deltaAmount.toInt256(), caster, false);
        emit Deposited(periodId, tokenId, _pool(), deltaAmount);
    }

    function _modify(
        uint256 periodId,
        uint256 tokenId,
        int256 deltaAmount,
       address caster,
        bool isPayOutReward
    ) private returns (uint256 rewardAmount) {
        //pass vote caster as owner of this position
        if (!IVoter(_caller).ownerOf(tokenId, caster)) {
            revert BribeRewarder__NotOwner();
        }
        //...
   }

Voter.sol:

    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)) {
                //@audit-fix pass msg.sender as voter
                rewarders[i].deposit(periodId, tokenId, deltaAmount, msg.sender);
                _userBribesPerPeriod[periodId][tokenId].push(rewarders[i]);
            }
        }
    }

Duplicate of #39