KupiaSec - Improper msg.sender check in the BribeRewarder._modify() function

[sherlock-admin3]

KupiaSec

High

Improper msg.sender check in the BribeRewarder._modify() function

Summary

When the _modify() function is called within the deposit() function, it will be reverted because of an incorrect check on msg.sender.

Vulnerability Detail

The deposit() function includes the onlyVoter modifier, indicating that msg.sender is the Voter contract. Consequently, when calling the _modify() function, it will encounter a revert at L264 due to the requirement for msg.sender to be the owner of tokenId, not the Voter contract. This renders voting impossible, as the Voter.vote() function triggers the deposit() function via the Voter._notifyBribes() function.

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

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

----------------------

    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();
        }

        ...
    }

Impact

Voting is impossible, resulting in the breakdown of the core functionality of the protocol.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

The msg.sender check should not be applied when the _modify() function is called within the deposit() function.

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

        ...
    }

Duplicate of #39