Skip to content
This repository has been archived by the owner on Jan 12, 2025. It is now read-only.

DPS - _notifyBribes() will revert everytime because of wrong check in BribeRewarder::_modify() #557

Closed
sherlock-admin3 opened this issue Jul 15, 2024 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A High severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Jul 15, 2024

DPS

High

_notifyBribes() will revert everytime because of wrong check in BribeRewarder::_modify()

Summary

The deposit() which is called in _notifyBribes() will revert because when the deposit() in BribeRewarder.sol calls the _modify() it will check if the msg.sender is owner of the nft position. This check will not pass because the msg.sender is the Voter contract, not the owner of the nft

Vulnerability Detail

This is where the deposit() from BribeRewarder.sol 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]);
            }
        }
    }

When we call it like this the msg.sender in it will be the Voter contract, then inside this deposit() function we call the _modify() that has a check if the owner of this tokenId is calling the function

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

    }

This will revert because the Voter contract is not owner of the nft

Impact

High because _notifyBribes will revert everytime

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L221

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

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

Tool used

Manual Review

Recommendation

In the _modify() function BribeRewarder.sol check if the msg.sender is the Voter contract, if it is just proceed with the transaction

Duplicate of #39

@github-actions github-actions bot added duplicate High A High severity issue. labels Jul 21, 2024
@sherlock-admin2 sherlock-admin2 added the Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label label Jul 22, 2024
@sherlock-admin4 sherlock-admin4 changed the title Big Violet Bee - _notifyBribes() will revert everytime because of wrong check in BribeRewarder::_modify() DPS - _notifyBribes() will revert everytime because of wrong check in BribeRewarder::_modify() Jul 29, 2024
@sherlock-admin4 sherlock-admin4 added the Reward A payout will be made for this issue label Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A High severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

3 participants