dimulski - Voting for a pool will always revert

[sherlock-admin4]

dimulski

High

Voting for a pool will always revert

Summary

The Voter.sol contract allows users who have staked tokens in the MlumStaking.sol contract to vote for certain pools, and potentially receive bribes for that. When a user decides to vote for a certain pool he has to call the vote() function, there are some check to see if a user owns the NFT, and how many voting power does he have. The vote() function calls the _notifyBribes() function which in turn calls the deposti() function on each BribeRewarder instance for that specific pool and voting period. However in the BribeRewarder.sol the deposti() function calls the _modify() function, which will always revert when called form the Voter.sol contract as Voter.sol is not the owner of the NFT, due to the following check:

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

Vulnerability Detail

Gist
After following the steps in the above mentioned gist, add the following test to the AuditorVotingTests.t.sol file:

    function test_VotingWillRevert() public {
        vm.startPrank(bob);
        BribeRewarder rewarder;
        rewarder = BribeRewarder(payable(address(rewarderFactory.createBribeRewarder(bribeRewardToken, pool))));
        bribeRewardToken.mint(bob, 10_000e6);
        bribeRewardToken.approve(address(rewarder), type(uint256).max);
        rewarder.fundAndBribe(1, 1, 10_000e6);
        vm.stopPrank();

        vm.startPrank(owner);
        voter.startNewVotingPeriod();
        vm.stopPrank();

        uint256 lockDuration = 365 days;
        mintMlumPosition(alice, 1e18, lockDuration);

        vm.startPrank(alice);
        IMlumStaking.StakingPosition memory mlumPosition;
        mlumPosition = mlumStaking.getStakingPosition(1);
        address[] memory pools = new address[](1);
        uint256[] memory deltaAmounts = new uint256[](1);
        pools[0] = pool;
        deltaAmounts[0] = mlumPosition.amountWithMultiplier;
        vm.expectRevert(abi.encodeWithSelector(IBribeRewarder.BribeRewarder__NotOwner.selector));
        voter.vote(1, pools, deltaAmounts);
        vm.stopPrank();
    }

To run the test use: forge test -vvv --mt test_VotingWillRevert

Impact

Users are not able to vote, making the whole Voter.sol contract obsolete, and bricking one of the most important functionalities of the protocol. Given the fact that the whole project in scope for the audit is focused on users voting, and receiving rewards or bribes in exchange this vulnerability is of high severity.

Code Snippet

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

Tool used

Manual Review & Foundry

Recommendation

In the BribeRewarder.sol contract add the following changes

• In deposit()

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

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

• In claim()

    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) {
-            totalAmount += _modify(i, tokenId, 0, true);
+           totalAmount += _modify(i, tokenId, 0, true, true);
        }

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

• In _modify()

-    function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
+   function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward, bool checkOwner)
        private
        returns (uint256 rewardAmount)
    {
-        if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
-            revert BribeRewarder__NotOwner();
-        }
-
-       // extra check so we dont calc rewards before starttime
-       (uint256 startTime,) = IVoter(_caller).getPeriodStartEndtime(periodId);
-       if (block.timestamp <= startTime) {
-           _lastUpdateTimestamp = startTime;
-       }

+       {
+           if (checkOwner && !IVoter(_caller).ownerOf(tokenId, msg.sender)) {
+               revert BribeRewarder__NotOwner();
+           }       
+           // extra check so we dont calc rewards before starttime
+           (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;
        }
    }

Considering the deposit() function has the onlyVoter modifier only the Voter.sol contract can call this function when the vote() function is called, where the owner of the nft is sufficiently checked. Other fixes are also possible, however this seems the easiest.

Duplicate of #39