0xboriskataa - Incorrect msg.sender expected

[sherlock-admin2]

0xboriskataa

High

Incorrect msg.sender expected

Summary

In Solidity, when a function in one contract calls a function on another contract, the msg.sender in the second function call refers to the address of the contract that made the call, not the original external caller. More can be read here.
There are several instances in the protocol where the wrong address is expected to be msg.sender. The reasons is that it makes the incorrect assumption that msg.sender stays the same but in fact it changes to the contract that made the function call.

Vulnerability Detail

Here are the instances where an issues occurs with explanation:

1. Voter.sol and BribeRewarder.sol are coded so that they interact with each other. When a user votes using Voter.sol: vote() the following subsequent calls to functions happen:
   Voter.sol: vote() -> Voter.sol: _notifyBribes() -> BribeRewarder.sol: deposit() -> BribeRewarder.sol: _modify()
   Let's look at a part of the code for _modify():

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

It calls Voter.sol:ownerOf() to check if msg.sender is the owner of the tokenId. Here the wrong assumption is made that msg.sender still refers to the EOA that called vote(). That is not the case as msg.sender is now the Voter.sol contract itself.

2. Voter.sol: createFarms() is a function that can be called only by the owner. It calls MasterchefV2.sol: add():

function add(IERC20 token, IMasterChefRewarder extraRewarder) external override {
      if (msg.sender != address(_lbHooksManager)) _checkOwnerOrOperator();
      ...
}

Here we check that msg.sender is either _lbHooksManager, owner or operator. _lbHooksManager address will not be set to Voter.sol and we can see that from the constructor:

   _voter = voter;
   _rewarderFactory = rewarderFactory;
   _lbHooksManager = lbHooksManager;

Owner and operator on the other hand are both EOAs. The same issue that occurs in 1 happens here. Msg.sender is expected to be either owner, operator or hooks manager but instead it will be the address of the Voter.sol contract.

3. BaseRewarder.sol expects a _caller to be set in the constructor:

     // @param caller The address of the contract that will call the onModify function.
    constructor(address caller) {
        _caller = caller;

MasterChefRewarder.sol inherits from BaseRewarder.sol and sets this caller during its own deployment:

    //@param caller The address of the contract that will call the onModify function.
    constructor(address caller) BaseRewarder(caller) {}

As we can see from the comments this caller contract is expected to call MasterChefRewarder.sol: onModify(). Let's see what happens if it does:

function onModify(address account, uint256 pid, uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply){
       ...
       reward = BaseRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
}

We can see that MasterChefRewarder.sol: onModify() calls BaseRewarder.sol: onModify():

    //@dev Called by the caller contract to update the rewards for a given account.
    function onModify(address account, uint256 pid, uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply) 
        public virtual override returns (uint256) { 
        if (msg.sender != _caller) revert BaseRewarder__InvalidCaller();
        }

As you can see it expects that msg.sender is the caller contract that called MasterChefRewarder.sol: onModify(). However that is not the case as the msg.sender is now the MasterChefRewarder.sol contract itself.

Impact

In all instances of this bug the transaction will simply revert. Because of this a lot of the functionality of the protocol will not work.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

For 1: BribeRewarder.sol: _modify() is called on deposits and claims. Deposits happen when users call Voter.sol: vote(). The vote() function already checks if caller is the owner of the tokenId. However claim() does not. The simplest fix is to remove the check for the owner of the tokenId in _modify() and instead put it in claim():

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

    function claim(uint256 tokenId) external override {
+       if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
+           revert BribeRewarder__NotOwner();
+        }
...    

For 2: Expect the caller of MasterchefV2.sol: add() to be the voter contract:

function add(IERC20 token, IMasterChefRewarder extraRewarder) external override {
-      if (msg.sender != address(_lbHooksManager)) _checkOwnerOrOperator();
+      if (msg.sender != address(_lbHooksManager) || msg.sender != address(_voter)) _checkOwnerOrOperator();
      ...
}

For 3:
Perhaps you can change the constructor of MasterChefRewarder.sol like so:

-   constructor(address caller) BaseRewarder(caller) {}
+   constructor() BaseRewarder(address(this)) {}

Duplicate of #39