Hunter - in BribeRewarder::_modify() the check of the ownership of the tokenId with msg.sender as the passed parameter will make voter::vote() always revert

[sherlock-admin3]

Hunter

Medium

in BribeRewarder::_modify() the check of the ownership of the tokenId with msg.sender as the passed parameter will make voter::vote() always revert

Summary

in BribeRewarder::_modify() the check of the ownership of the tokenId with msg.sender as the passed parameter will make voter::vote() always revert

Vulnerability Detail

in function vote()

File: Voter.sol
153:     function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
///////////////.............. skip unnecessary code
210: 
211:             _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount); // msg.sender, deltaAmount);
///////////////.............. skip unnecessary code
219:     }

we call _notifyBribes in Line #211

File: Voter.sol
221:     function _notifyBribes(uint256 periodId, address pool, uint256 tokenId, uint256 deltaAmount) private {
222:         IBribeRewarder[] storage rewarders = _bribesPerPriod[periodId][pool];
223:         for (uint256 i = 0; i < rewarders.length; ++i) {
224:             if (address(rewarders[i]) != address(0)) {
225:                 rewarders[i].deposit(periodId, tokenId, deltaAmount);
226:                 _userBribesPerPeriod[periodId][tokenId].push(rewarders[i]);
227:             }
228:         }
229:     }

this function calls rewarders contract (BribeRewarder) so that it notifies them that the user voted for their Pool, so that the user becomes eleigble for claiming rewards

in Line #225 we call deposit() in (BribeRewarder) contract

File: BribeRewarder.sol
143:     function deposit(uint256 periodId, uint256 tokenId, uint256 deltaAmount) public onlyVoter {
144:         _modify(periodId, tokenId, deltaAmount.toInt256(), false);
145: 
146:         emit Deposited(periodId, tokenId, _pool(), deltaAmount);
147:     }

the function deposit() has onlyVoter modifier so that only the voter contract is able to call this function which is a good thing, but we need to keep in mind that in the current instance of of BribeRewarder the msg.sender is the voter contract

Now we call _modify in Line #144

File: BribeRewarder.sol
260:     function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward)
261:         private
262:         returns (uint256 rewardAmount)
263:     {
264:         if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
265:             revert BribeRewarder__NotOwner();
266:         }
267: 
///////////////............... Skip unnecessary code
298:     }

we see in line #264 we check if msg.sender is the owner of this tokenId passed by calling the voter contract

the problem here is that since msg.sender is the voter contract, this check will always fail (since voter contract is not the owner of the tokenId)

the confusion arised from the fact that _modify() function is called inside claim() which is called by the actuall user not the voter contract and this check will pass then on claim() by users

but this check is wrong during deposit() as described above

Impact

the vote() in voter contract is corrupted and will always fail and revert for any Pool having BribeRewarder which will almost always be the case.

MediumL breaking core contract functionality

Code Snippet

vote()

https://github.com/sherlock-audit/2024-06-magicsea/blob/7fd1a65b76d50f1bf2555c699ef06cde2b646674/magicsea-staking/src/Voter.sol#L153-L219

File: Voter.sol
153:     function vote(uint256 tokenId, address[] calldata pools, uint256[] calldata deltaAmounts) external {
154:         if (pools.length != deltaAmounts.length) revert IVoter__InvalidLength();
155: 
156:         // check voting started
157:         if (!_votingStarted()) revert IVoter_VotingPeriodNotStarted();
158:         if (_votingEnded()) revert IVoter_VotingPeriodEnded();
159: 
160:         // check ownership of tokenId
161:         if (_mlumStaking.ownerOf(tokenId) != msg.sender) {
162:             revert IVoter__NotOwner();
163:         }
164: 
165:         uint256 currentPeriodId = _currentVotingPeriodId;
166:         // check if alreay voted
167:         if (_hasVotedInPeriod[currentPeriodId][tokenId]) {
168:             revert IVoter__AlreadyVoted();
169:         }
170: 
171:         // check if _minimumLockTime >= initialLockDuration and it is locked
172:         if (_mlumStaking.getStakingPosition(tokenId).initialLockDuration < _minimumLockTime) {
173:             revert IVoter__InsufficientLockTime();
174:         }
175:         if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
176:             revert IVoter__InsufficientLockTime();
177:         }
178: 
179:         uint256 votingPower = _mlumStaking.getStakingPosition(tokenId).amountWithMultiplier;
180: 
181:         // check if deltaAmounts > votingPower
182:         uint256 totalUserVotes;
183:         for (uint256 i = 0; i < pools.length; ++i) {
184:             totalUserVotes += deltaAmounts[i];
185:         }
186: 
187:         if (totalUserVotes > votingPower) {
188:             revert IVoter__InsufficientVotingPower();
189:         }
190: 
191:         IVoterPoolValidator validator = _poolValidator;
192: 
193:         for (uint256 i = 0; i < pools.length; ++i) {
194:             address pool = pools[i];
195: 
196:             if (address(validator) != address(0) && !validator.isValid(pool)) {
197:                 revert Voter__PoolNotVotable();
198:             }
199: 
200:             uint256 deltaAmount = deltaAmounts[i];
201: 
202:             _userVotes[tokenId][pool] += deltaAmount;
203:             _poolVotesPerPeriod[currentPeriodId][pool] += deltaAmount;
204: 
205:             if (_votes.contains(pool)) {
206:                 _votes.set(pool, _votes.get(pool) + deltaAmount);
207:             } else {
208:                 _votes.set(pool, deltaAmount);
209:             }
210: 
211:             _notifyBribes(_currentVotingPeriodId, pool, tokenId, deltaAmount); // msg.sender, deltaAmount);
212:         }
213: 
214:         _totalVotes += totalUserVotes;
215: 
216:         _hasVotedInPeriod[currentPeriodId][tokenId] = true;
217: 
218:         emit Voted(tokenId, currentPeriodId, pools, deltaAmounts);
219:     }

_notifyBribes()

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

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

deposit()

https://github.com/sherlock-audit/2024-06-magicsea/blob/7fd1a65b76d50f1bf2555c699ef06cde2b646674/magicsea-staking/src/rewarders/BribeRewarder.sol#L143-L147

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

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

_modify()

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

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

Tool used

Manual Review

Recommendation

an easy solution would be changing the check _modify() to tx.origin here

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

with taking into considerations the tradeOff using tx.origin (if users of the protocol are phished, this will give the ability to attacker to claim the rewards in their behalf)

but i recommend it cause it is simpler and would require less logic changes

Duplicate of #39