radin200 - Users can't vote due to permanent BribeRewarder__NotOwner DoS

[sherlock-admin2]

radin200

High

Users can't vote due to permanent BribeRewarder__NotOwner DoS

Summary

Users can't vote due to permanent BribeRewarder__NotOwner DoS

Vulnerability Detail

In scenario where user has mlum staking position and wants to vote in current period, he executes Voter::vote. Trough vote execution Voter::_notifyBribes is called to deposit the deltaAmounts into BribeRewarder.

 function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external { 
    .....
  
     for (uint256 i = 0; i < pools.length; ++i) { 

         .....
  
         _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount); // msg.sender, deltaAmount); 
     } 
  
     .....
 } 
  
 function _notifyBribes(uint256 periodId, address pool, uint256 tokenId, uint256 deltaAmount) private { 
     .....
     for (uint256 i = 0; i < rewarders.length; ++i) { 
         .....
             rewarders[i].deposit(periodId, tokenId, deltaAmount); 
             .....
     } 
 } 

It is intended that onlyVoter is able(be the msg.sender) to execute BribeRewarder::deposit, but after that _modify is executed and checks if the current msg.sender is owner of the tokenId. There is no way for Voter to be the owner of the token so the deposit function will revert every time it is called.

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

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

modifier onlyVoter() {
        _checkVoter();
        _;
    }

function _checkVoter() internal view virtual {
        if (msg.sender != address(_caller)) {
            revert BribeRewarder__OnlyVoter();
        }
    }

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

        .....

    }

Impact

Permanent voting DoS, due to bad validation

Code Snippet

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

Tool used

Manual Review

Recommendation

1. Remove the owner of tokenId check, Voter validates it before the execution of BribeRewarder::deposit anyway.
2. Validate the caller in BribeRewarder::claim before calling BribeRewarder::_modify

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

        .....

    }

function claim(uint256 tokenId) external override {
        uint256 endPeriod = IVoter(_caller).getLatestFinishedPeriod();

        uint256 totalAmount;

        // calc emission per period cause every period can every other durations
        for (uint256 i = _startVotingPeriod; i <= endPeriod; ++i) {
+++     if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
+++       revert BribeRewarder__NotOwner();
+++     }
            totalAmount += _modify(i, tokenId, 0, true);
        }

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

Duplicate of #39