0uts1der - vote Function Broken Due to Incorrect Ownership Check in BribeRewarder Contract

[sherlock-admin4]

0uts1der

High

vote Function Broken Due to Incorrect Ownership Check in BribeRewarder Contract

Summary

The _modify function in the BribeRewarder contract has a logical error. This flaw causes an ownership check error during the voting process, preventing users with voting rights from completing their votes. Consequently, this renders the protocol's core functionality broken and may lead to economic losses.

Vulnerability Detail

Users can stake stakedToken in the MlumStaking contract by calling the createPosition function, creating a position with a specified lockDuration. This allows them to receive staking rewards, which are periodically distributed as rewardToken by the MlumStaking contract, and grants them voting rights for various pools, with the voting logic handled by the vote function in the voter contract.

Additionally, the protocol has a bribery mechanism where stakeholders can create a BribeRewarder for a specific pool through the RewarderFactory contract, setting the bribeToken to incentivize users with voting rights to vote for a particular pool in exchange for bribery tokens.

However, a logical error in the _modify function of the BribeRewarder contract makes the vote function perpetually unavailable (revert), meaning users with a position (i.e., voting rights) cannot call the vote function to complete their vote, rendering the protocol's core functionality inoperative and potentially causing economic losses.

Specifically, when a user with a staked position (i.e., lsNFT) calls the vote function to vote for a particular pool and receive bribery tokens for those pools, the transaction reverts. The vote function is as follows:

function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
    if (pools.length != deltaAmounts.length) revert IVoter__InvalidLength();

    // check voting started
    if (!_votingStarted()) revert IVoter_VotingPeriodNotStarted();
    if (_votingEnded()) revert IVoter_VotingPeriodEnded();

    // check ownership of tokenId
    if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
        revert IVoter__NotOwner();
    }

    uint256 currentPeriodId = _currentVotingPeriodId;
    // check if already voted
    if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
        revert IVoter__AlreadyVoted();
    }

    // check if _minimumLockTime >= initialLockDuration and it is locked
    if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
        revert IVoter__InsufficientLockTime();
    }
    if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
        revert IVoter__InsufficientLockTime();
    }

    uint256 votingPower = _mlumStaking.getStakingPosition(tokenId).amountWithMultiplier;

    // check if deltaAmounts > votingPower
    uint256 totalUserVotes;
    for (uint256 i = 0; i < pools.length; ++i) {
        totalUserVotes += deltaAmounts[i];
    }

    if (totalUserVotes > votingPower) {
        revert IVoter__InsufficientVotingPower();
    }

    IVoterPoolValidator validator = _poolValidator;

    for (uint256 i = 0; i < pools.length; ++i) {
        address pool = pools[i];

        if (address(validator) != address(0) && !validator.isValid(pool)) {
            revert Voter__PoolNotVotable();
        }

        uint256 deltaAmount = deltaAmounts[i];

        _userVotes[tokenId][pool] += deltaAmount;
        _poolVotesPerPeriod[currentPeriodId][pool] += deltaAmount;

        if (_votes.contains(pool)) {
            _votes.set(pool, _votes.get(pool) + deltaAmount);
        } else {
            _votes.set(pool, deltaAmount);
        }

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

    _totalVotes += totalUserVotes;

    _hasVotedInPeriod[currentPeriodId][tokenId] = true;

    emit Voted(tokenId, currentPeriodId, pools, deltaAmounts);
}

The vote function then calls the _notifyBribes function to notify the rewarder which set for the target pool of the user's vote, recording the user's vote for subsequent bribery token distribution:

function _notifyBribes(uint256 periodId, address pool, uint256 tokenId, uint256 deltaAmount) private {
    IBribeRewarder[] storage rewarders = _bribesPerPeriod[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]);
        }
    }
}

The deposit function in the BribeRewarder contract is then called to record the votes:

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

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

function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
    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;
    }

    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;
    }
}

Here, note that the _modify function first checks the ownership of the position:

// BribeRewarder contract
if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
    revert BribeRewarder__NotOwner();
}
// Voter contract
function ownerOf(uint256 tokenId, address account) external view returns (bool) {
    return _mlumStaking.ownerOf(tokenId) == account;
}

However, there is a logical error in this check, leading to a series of calls that result in a revert.

Let's review the call chain: A user (Bob) with a staked position calls the vote function of the Voter contract, which in turn calls the deposit function of the BribeRewarder contract, and then checks the ownership in the Voter contract's ownerOf function, as follows:

Bob -> Voter.vote(_notifyBribes) -> BribeRewarder.deposit(_modify) -> Voter.ownerOf

Therefore, when _modify calls Voter.ownerOf, the msg.sender parameter passed is actually the address of the Voter contract, not the user Bob's address. Consequently, the ownership check fails, causing the entire vote function to revert.

A full proof of concept (POC) demonstrating this issue is as follows:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import "forge-std/Test.sol";

import "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol";
import "../src/transparent/TransparentUpgradeableProxy2Step.sol";

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

contract VoterBribeTest is Test {
    address payable immutable DEV = payable(makeAddr("dev"));
    address payable immutable ALICE = payable(makeAddr("alice"));
    address payable immutable BOB = payable(makeAddr("bob"));
    address pool = makeAddr("pool");

    Voter private _voter;
    MlumStaking private _stakingPool;
    RewarderFactory private factory;

    ERC20Mock private _stakingToken;
    ERC20Mock private _rewardToken;
    ERC20Mock private _bribeToken;

    function setUp() public {
        vm.startPrank(DEV);
        _stakingToken = new ERC20Mock("MagicLum", "MLUM", 18);
        vm.label(address(_stakingToken), "stakingToken");

        _rewardToken = new ERC20Mock("USDT", "USDT", 6);
        vm.label(address(_rewardToken), "rewardToken");

        _bribeToken = new ERC20Mock("Bribe", "bribe", 18);
        vm.label(address(_bribeToken), "bribeToken");

        address stakingPoolImpl = address(new MlumStaking(_stakingToken, _rewardToken));
        vm.label(stakingPoolImpl, "stakingPoolImpl");

        _stakingPool = MlumStaking(
            address(
                new TransparentUpgradeableProxy2Step(
                    stakingPoolImpl, ProxyAdmin2Step(address(1)), abi.encodeWithSelector(MlumStaking.initialize.selector, DEV)
                )
            )
        );
        vm.label(address(_stakingPool), "stakingPool");

        MasterChefMock mock = new MasterChefMock();
        vm.label(address(mock), "MasterChefMock");

        address factoryImpl = address(new RewarderFactory());
        vm.label(factoryImpl, "factoryImpl");

        factory = RewarderFactory(
            address(
                new TransparentUpgradeableProxy2Step(
                    factoryImpl,
                    ProxyAdmin2Step(address(1)),
                    abi.encodeWithSelector(
                        RewarderFactory.initialize.selector, DEV, new uint8[](0), new address[](0)
                    )
                )
            )
        );
        vm.label(address(factory), "RewarderFactory");

        address voterImpl = address(new Voter(mock, _stakingPool, factory));
        vm.label(voterImpl, "voterImpl");

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

        _voter.updateMinimumLockTime(2 weeks);

        factory.setRewarderImplementation(
            IRewarderFactory.RewarderType.BribeRewarder, IRewarder(address(new BribeRewarder(address(_voter))))
        );

    }

    function _createPosition(address user) internal {
        _stakingToken.mint(user, 2 ether);

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

    function testTokenOwner() public {
        vm.startPrank(ALICE);
        BribeRewarder rewarder = BribeRewarder(payable(address(factory.createBribeRewarder(_bribeToken, pool))));

        _bribeToken.mint(address(ALICE), 20e18);
        ERC20Mock(address(_bribeToken)).approve(address(rewarder), 20e18);

        rewarder.fundAndBribe(1, 2, 10e18);
        vm.stopPrank();

        vm.startPrank(DEV);
        _voter.startNewVotingPeriod();
        vm.stopPrank();

        _createPosition(BOB);
        
        vm.startPrank(BOB);
        address[] memory pools = new address[](1);
        pools[0] = pool;
        uint256[] memory deltaAmounts = new uint256[](1);
        deltaAmounts[0] = 1e18;
        _voter.vote(1, pools , deltaAmounts);
        vm.stopPrank();
    }
}

1. In the setUp function, the project developer (DEV) deploys relevant contracts, including the staking contract MlumStaking, the staking token _stakingToken, the reward token _rewardToken, and the bribery token _bribeToken. It also deploys the MasterChefMock contract, the RewarderFactory contract, and the Voter contract, setting the minimum lock time for voting to two weeks and configuring the BribeRewarder in the factory.

2. Next, Alice, a stakeholder, wants users to vote for a specific pool. She calls the factory's createBribeRewarder function to create a BribeRewarder with _bribeToken as the bribery token and then calls the fundAndBribe function to bribe for periods 1 and 2 with 10e18 _bribeToken for each period:

function fundAndBribe(uint256 startId, uint256 lastId, uint256 amountPerPeriod) external payable onlyOwner {
    IERC20 token = _token();
    uint256 totalAmount = _calcTotalAmount(startId, lastId, amountPerPeriod);

    if (address(token) == address(0)) {
        if (msg.value < totalAmount) {
            revert BribeRewarder__InsufficientFunds();
        }
    } else {
        token.safeTransferFrom(msg.sender, address(this), totalAmount);
    }

    _bribe(startId, lastId, amountPerPeriod);
}

Thus, Alice transfers 20e18 _bribeToken to the created BribeRewarder contract for subsequent periods 1 and 2.

3. The project developer calls the startNewVotingPeriod function, starting a new period (period 1).

4. Another user, Bob, stakes 2 ether of _stakingToken in the MlumStaking contract for two weeks, meeting the minimum lock time required for voting.

5. Bob attempts to vote for the pool to receive bribery tokens. However, the vote function call reverts, preventing Bob from exercising his voting rights.

The error log shows:

3663] 0xB5FC6d2158D659163022717Dc1bf8F69E829e83D::deposit(1, 1, 1000000000000000000 [1e18])
    │   │   │   ├─ [3458] BribeRewarder::deposit(1, 1, 1000000000000000000 [1e18]) [delegatecall]
    │   │   │   │   ├─ [2529] voter::ownerOf(1, voter: [0x8e54203dae61b811f3f7059d9bD1e413f65CE677]) [staticcall]
    │   │   │   │   │   ├─ [2078] voterImpl::ownerOf(1, voter: [0x8e54203dae61b811f3f7059d9bD1e413f65CE677]) [delegatecall]
    │   │   │   │   │   │   ├─ [1113] stakingPool::ownerOf(1) [staticcall]
    │   │   │   │   │   │   │   ├─ [665] stakingPoolImpl::ownerOf(1) [delegatecall]
    │   │   │   │   │   │   │   │   └─ ← [Return] bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]
    │   │   │   │   │   │   │   └─ ← [Return] bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]
    │   │   │   │   │   │   └─ ← [Return] false
    │   │   │   │   │   └─ ← [Return] false
    │   │   │   │   └─ ← [Revert] BribeRewarder__NotOwner()
    │   │   │   └─ ← [Revert] BribeRewarder__NotOwner()
    │   │   └─ ← [Revert] BribeRewarder__NotOwner()
    │   └─ ← [Revert] BribeRewarder__NotOwner()
    └─ ← [Revert] BribeRewarder__NotOwner()

This shows that the voter::ownerOf(1, voter: [0x8e54203dae61b811f3f7059d9bD1e413f65CE677]) check fails because the address passed for checking ownership is the voter contract's address, not Bob's address, leading to the BribeRewarder__NotOwner error.

Clearly, this logical flaw renders the protocol's core voting and bribery mechanisms inoperative, causing economic losses. In this POC scenario, Alice's 20e18 _bribeToken transferred to the BribeRewarder contract for bribery cannot be distributed since no user can successfully vote, resulting in permanent economic loss.

To summarize, the ownership check in the _modify function needs to properly handle the msg.sender to accurately reflect the actual user initiating the vote call. This correction is crucial for enabling the intended functionality of voting and bribery in the protocol.

Impact

The logical error in the _modify function of the BribeRewarder contract disables the protocol's voting mechanism. Users cannot vote, causing the vote function to revert, which prevents the distribution of bribery tokens. This flaw disrupts the protocol's core functionality and leads to economic losses.

Code Snippet

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

Tool used

Manual Review

Recommendation

In fact, it can be observed that the vote function already includes an ownership check for the position.

function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
    if (pools.length != deltaAmounts.length) revert IVoter__InvalidLength();

    // check voting started
    if (!_votingStarted()) revert IVoter_VotingPeriodNotStarted();
    if (_votingEnded()) revert IVoter_VotingPeriodEnded();

    // check ownership of tokenId
    if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
        revert IVoter__NotOwner();
    }
    // skip

Therefore, the check in the _modify function is actually intended for verifying ownership when users directly call the claim function of the BribeRewarder contract to withdraw bribery tokens.

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

    uint256 totalAmount;

    // calculate emission per period because every period can have different durations
    for (uint256 i = _startVotingPeriod; i <= endPeriod; ++i) {
        totalAmount += _modify(i, tokenId, 0, true);
    }

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

Clearly, the claim function requires an ownership check for the position, and this check is implemented in the subsequent _modify function call.

Thus, a simple and feasible solution is to move the ownership check logic from the _modify function to the claim function, as shown below.

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

    uint256 totalAmount;

    // calculate emission per period because every period can have different 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);
}

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

    // skip
}

Duplicate of #39