Aymen0909 - BribeRewarder.deposit Will Always Revert, Blocking the Voting Mechanism

[sherlock-admin3]

Aymen0909

High

BribeRewarder.deposit Will Always Revert, Blocking the Voting Mechanism

Summary

The BribeRewarder.deposit function will always revert when called by the Voter contract due to an incorrect token ownership check. This will lead to a Denial of Service (DOS) of the voting mechanism and subsequently block any rewards distribution.

Vulnerability Detail

Once a user stakes MLUM tokens, they receive an ERC721 token representing their staking position. This token allows them to vote by calling the Voter.vote function and providing the list of pools they want to vote on and their associated vote amounts.

The Voter.vote function, upon recording the user's vote, calls _notifyBribes to inform the BribeRewarder that the user has indeed voted on that specific pool. This makes the user eligible for additional reward distribution.

The issue arises at this step because _notifyBribes will invoke the BribeRewarder.deposit function to record the user's participation:

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

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

As seen, this function calls BribeRewarder._modify, which contains a check to ensure that its caller (msg.sender) is always the ERC721 token owner, as highlighted below:

function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
    private
    returns (uint256 rewardAmount)
{   
    //@audit will revert on deposit as msg.sender == Voter 
    if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
        revert BribeRewarder__NotOwner();
    }

    ...
}

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

This will cause the Voter's call to BribeRewarder.deposit to always revert because the function caller (msg.sender == address(Voter)) is not the real token owner. This will effectively DOS the voting mechanism for any pool that has implemented external bribes with BribeRewarder.

This issue will subsequently block all rewards distribution associated with pools that use external bribes (BribeRewarder).

Additionally, because the BribeRewarder.deposit function is only callable by the Voter contract (presence of the onlyVoter modifier), which is not the real token owner of the ERC721 tokens used during voting, the function will always revert and can never be called.

Impact

Complete DOS of the voting mechanism for pools that use BribeRewarder to include bribe rewards.

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#L260-L267

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

Tool used

Manual Review

Recommendation

To address this issue, update the BribeRewarder._modify function to check that the original voter account (which called Voter.vote in the first place) is the real token owner. This can be done by checking the tx.origin address as follows (note that this change does not impact the claiming process):

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

    // extra check so we don't calculate rewards before start time
    (uint256 startTime,) = IVoter(_caller).getPeriodStartEndtime(periodId);
    if (block.timestamp <= startTime) {
        _lastUpdateTimestamp = startTime;
    }

    RewardPerPeriod storage reward = _rewards[_indexByPeriodId(periodId)];
    Amounts.Parameter storage amounts = reward.userVotes;
    Rewarder2.Parameter storage rewarder = reward.rewarder;

    (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = amounts.update(tokenId, deltaAmount);

    uint256 totalRewards = _calculateRewards(periodId);

    rewardAmount = rewarder.update(bytes32(tokenId), oldBalance, newBalance, oldTotalSupply, totalRewards);

    if (block.timestamp > _lastUpdateTimestamp) {
        _lastUpdateTimestamp = block.timestamp;
    }

    if (isPayOutReward) {
        rewardAmount = rewardAmount + unclaimedRewards[periodId][tokenId];
        unclaimedRewards[periodId][tokenId] = 0;
        if (rewardAmount > 0) {
            IERC20 token = _token();
            _safeTransferTo(token, msg.sender, rewardAmount);
        }
    } else {
        unclaimedRewards[periodId][tokenId] += rewardAmount;
    }
}

Duplicate of #39