Reentrants - BribeRewarder_modify() uses incorrect parameter to check tokenId owner

[sherlock-admin2]

Reentrants

High

BribeRewarder_modify() uses incorrect parameter to check tokenId owner

Summary

The BribeRewarder._modify() check uses the incorrect parameter of msg.sender to check the tokenId owner.

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

Vulnerability Detail

The flow is Voter.vote() -> Voter._notifyBribes() -> rewarders[i].deposit() -> rewarders[i].modify(), hence msg.sender is the _caller contract, not the actual voter. Can verify this because of the onlyVoter() check as well.

POC

The setup can be found in the gist.
https://gist.github.com/Reentrants/50898e49155a13b9eddbe69ea52c1a19#file-e2etest-t-sol-L175-L191

For brevity, the case is linked below.

function test_incorrect_modify_check() public {
        vm.startPrank(alice);
        staking.createPosition(1e18, 180 days);
        bribeRewarder = BribeRewarder(payable(address(rewarderFactory.createBribeRewarder(IERC20(address(0)), address(tokenA)))));
        bribeRewarder.fundAndBribe{value: 100 wei}(1, 100, 1);
        vm.startPrank(owner);
        voter.startNewVotingPeriod();
        vm.startPrank(alice);
        address[] memory pools = new address[](1);
        pools[0] = address(tokenA);
        uint256[] memory deltaAmounts = new uint256[](1);
        deltaAmounts[0] = 100;

        // voting reverts from incorrect staking NFT owner check
        vm.expectRevert(IBribeRewarder.BribeRewarder__NotOwner.selector);
        voter.vote(1, pools, deltaAmounts);
}

Impact

Users will not be able to vote.

Code Snippet

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

Tool used

Manual Review

Recommendation

_modify() needs to take in the caller of vote() for verification.

Duplicate of #39