Yashar - The voting mechanism for the periods that have a bribe will be DoSed

[sherlock-admin3]

Yashar

Medium

The voting mechanism for the periods that have a bribe will be DoSed

Summary

The deposit function in BribeRewarder.sol incorrectly checks token ownership, leading to a DoS of the voting mechanism when there's a bribe for a period.

Vulnerability Detail

Users who want to vote should call the vote function.
The vote function will call _notifyBribes:

            _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount); // msg.sender, deltaAmount);
        }

_notifyBribes checks if there's a bribe rewarder for the given period and pool. If there is, it will call the deposit function of the BribeRewarder.sol:

            if (address(rewarders[i]) != address(0)) {
                rewarders[i].deposit(periodId, tokenId, deltaAmount);
                _userBribesPerPeriod[periodId][tokenId].push(rewarders[i]);

The deposit function in BribeRewarder.sol will call _modify to deposit votes for the given period and token ID:

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

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

The problem is that there's a check in _modify to see if the msg.sender is the owner of the token or not:

        if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
            revert BribeRewarder__NotOwner();
        }

In the context of the BribeRewarder.sol, the msg.sender is the Voter.sol and not the actual owner of the token, so the transaction will revert with the BribeRewarder__NotOwner error.

Coded PoC

Please make a file named VoteDoS.t.sol in this path: /test/ and paste the following test code in it:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import "forge-std/Test.sol";

import "../src/transparent/TransparentUpgradeableProxy2Step.sol";

import {ERC20Mock} from "./mocks/ERC20.sol";
import {MasterChefMock} from "./mocks/MasterChefMock.sol";
import {MlumStaking} from "../src/MlumStaking.sol";
import "../src/Voter.sol";
import {IVoter} from "../src/interfaces/IVoter.sol";
import "../src/interfaces/IBribeRewarder.sol";
import "../src/rewarders/BribeRewarder.sol";
import "../src/rewarders/RewarderFactory.sol";

contract VoteDoS is Test {
    address payable immutable DEV = payable(makeAddr("dev"));
    address payable immutable ALICE = payable(makeAddr("alice"));

    address pool = makeAddr("pool");

    Voter private _voter;
    MlumStaking private _pool;

    ERC20Mock private _stakingToken;
    ERC20Mock private _rewardToken;
    ERC20Mock private _bribeRewardToken;

    BribeRewarder rewarder;
    RewarderFactory factory;

    function setUp() public {
        _bribeRewardToken = new ERC20Mock("Reward Token", "RT", 6);

        vm.prank(DEV);
        _stakingToken = new ERC20Mock("MagicLum", "MLUM", 18);

        vm.prank(DEV);
        _rewardToken = new ERC20Mock("USDT", "USDT", 6);

        vm.prank(DEV);
        address poolImpl = address(new MlumStaking(_stakingToken, _rewardToken));

        _pool = MlumStaking(
            address(
                new TransparentUpgradeableProxy2Step(
                    poolImpl, ProxyAdmin2Step(address(1)), abi.encodeWithSelector(MlumStaking.initialize.selector, DEV)
                )
            )
        );

        vm.prank(DEV);
        MasterChefMock mock = new MasterChefMock();

        address factoryImpl = address(new RewarderFactory());
        factory = RewarderFactory(
            address(
                new TransparentUpgradeableProxy2Step(
                    factoryImpl,
                    ProxyAdmin2Step(address(1)),
                    abi.encodeWithSelector(
                        RewarderFactory.initialize.selector, address(this), new uint8[](0), new address[](0)
                    )
                )
            )
        );
        address voterImpl = address(new Voter(mock, _pool, IRewarderFactory(address(factory))));

        _voter = Voter(
            address(
                new TransparentUpgradeableProxy2Step(
                    voterImpl, ProxyAdmin2Step(address(1)), abi.encodeWithSelector(Voter.initialize.selector, DEV)
                )
            )
        );

        factory.setRewarderImplementation(
            IRewarderFactory.RewarderType.BribeRewarder, IRewarder(address(new BribeRewarder(address(_voter))))
        );
        rewarder = BribeRewarder(payable(address(factory.createBribeRewarder(_bribeRewardToken, pool))));

        vm.prank(DEV);
        _voter.updateMinimumLockTime(2 weeks);
    }

    function test_VoteDoS() public {
        ERC20Mock(address(_bribeRewardToken)).mint(address(this), 20e18);
        ERC20Mock(address(_bribeRewardToken)).approve(address(rewarder), 20e18);

        rewarder.fundAndBribe(1, 10, 1e18);

        vm.prank(DEV);
        _voter.startNewVotingPeriod();
        assertEq(1, _voter.getCurrentVotingPeriod());

        (uint256 startTime, uint256 endTime) = _voter.getPeriodStartEndtime(1);
        assertGt(endTime, startTime);
    
        _createPosition(ALICE);

        // Alice owns the token 1
        assertEq(_pool.ownerOf(1), ALICE);

        vm.prank(ALICE);
        vm.expectRevert(
            abi.encodeWithSelector(
                IBribeRewarder.BribeRewarder__NotOwner.selector
            )
        );
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());
    }

    /*
    ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
    ||||||||||||||||||||||||| INTERNAL FUNCTIONS |||||||||||||||||||||||||||||
    ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
    */
    function _createPosition(address user) internal {
        _stakingToken.mint(user, 10 ether);

        vm.startPrank(user);
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 2 weeks);
        vm.stopPrank();
    }

    function _getDeltaAmounts() internal pure returns (uint256[] memory deltaAmounts) {
        deltaAmounts = new uint256[](1);
        deltaAmounts[0] = 1e18;
    }

    function _getDummyPools() internal view returns (address[] memory pools) {
        pools = new address[](1);
        pools[0] = pool;
    }
}

Run the test:

forge test --mt test_VoteDoS

Impact

The voting mechanism for the periods that have a bribe will be completely DoSed, users won't get their rewards and the funds will be locked up in BribeRewarder.sol.

Code Snippet

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

Tool used

Manual Review

Recommendation

Given that the _modify function is also used in claim and works in that context as expected, you can't remove that check from _modify. Instead, you have to implement another function that doesn't include that check and call that function from deposit:

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

        emit Deposited(periodId, tokenId, _pool(), deltaAmount);
    }
+
+    function _deposit(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
+        private
+        returns (uint256 rewardAmount)
+    {
+        // 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;
+        }
+    }

Duplicate of #39