sherlock-admin2 commented 1 month ago

araj

Medium

Voter receives the bribe reward for `all` voting period, regardless of how many votingPeriod he voted for

Summary

Voter receives the bribe reward for all voting period, regardless of how many votingPeriod he voted for

Vulnerability Detail

When a user votes for a pool, bribeRewarder of that pool stores the amounts of vote user voted in that votingPeriod using Amounts::update()

 function _notifyBribes(uint256 periodId, address pool, uint256 tokenId, uint256 deltaAmount) private {
...
          @>   rewarders[i].deposit(periodId, tokenId, deltaAmount);
                _userBribesPerPeriod[periodId][tokenId].push(rewarders[i]);
            }
    }

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

 function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward) {
....
    @>    (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = amounts.update(tokenId, deltaAmount);
....
    }

When a voter claim for his rewards, it loops over _modify() from _startVotingPeriod to lastEndedVotingPeriod & _modify() calculates the rewardAmount(based on deltaAmount or voteAmount) and transfers it to user.

But the problem is it assumes user has voted for all voting period because balances returned by Amount.update() is same for all votingPeriod as it only stores total votes but doesn't stores how many votes user voted in a particular votingPeriod. As result votes voted in a particular votingPeriod is used for all votingPeriod

  function claim(uint256 tokenId) external override {
        uint256 endPeriod = IVoter(_caller).getLatestFinishedPeriod();
...
        for (uint256 i = _startVotingPeriod; i <= endPeriod; ++i) {
        @>    totalAmount += _modify(i, tokenId, 0, true);
        }
    }

function _modify(uint256 periodId, uint256 tokenId, int256 deltaAmount, bool isPayOutReward){
...
   @>     (uint256 oldBalance, uint256 newBalance, uint256 oldTotalSupply,) = amounts.update(tokenId, deltaAmount);

        uint256 totalRewards = _calculateRewards(periodId);

     @>   rewardAmount = rewarder.update(bytes32(tokenId), oldBalance, newBalance, oldTotalSupply, totalRewards);
...
        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;
        }
    }

//How this works(very simple example & step-by-step)

Suppose a bribeRewarder is created for poolA from 1st votingPeriod to 5th votingPeriod ie startVotingPeriod = 1 & lastVotingPeriod = 5

A user voted in 1st votingPeriod for poolA for voteAmount = 2e18, which stores the voteAmount in bribeRewarder using Amount::update() (oldAmount = 0, newAmount = 2e18, oldTotalAmount = 0, newTotalAmount = 2e18)

function update(Parameter storage amounts, bytes32 key, int256 deltaAmount)
    internal
    returns (uint256 oldAmount, uint256 newAmount, uint256 oldTotalAmount, uint256 newTotalAmount)
{
    oldAmount = amounts.amounts[key];
    oldTotalAmount = amounts.totalAmount;

    if (deltaAmount == 0) {
        newAmount = oldAmount;
        newTotalAmount = oldTotalAmount;
    } else {
        newAmount = oldAmount.addDelta(deltaAmount);
        newTotalAmount = oldTotalAmount.addDelta(deltaAmount);

        amounts.amounts[key] = newAmount;
        amounts.totalAmount = newTotalAmount;
    }
}

After 5th votingPeriod, user claims his bribe rewards which run the loop on _modify() from 1(startId) to 5(lastId) (See above claim() )
For 1st votingPeriod, amounts.update() will return all balances(oldAmount = 2e18, newAmount = 2e18, oldTotalAmount = 2e18, newTotalAmount = 2e18) & _calculateRewards() will return the totalRewards and all those will be used in rewarder.update() to calculate rewardAmount that will be sent to user
```
rewardAmount = rewarder.update(bytes32(tokenId), oldBalance, newBalance, oldTotalSupply, totalRewards);
```
Again for 2nd votingPeriod, amounts.update() will return all balances(oldAmount = 2e18, newAmount = 2e18, oldTotalAmount = 2e18, newTotalAmount = 2e18) and same thing will happen as above for all votingPeriod

All this is happening because bribeRewarder is only storing totalVotes of user but not storing how many votes user voted in a particular votingPeriod. As result rewarder assumes user has voted for all votingPeriod

Impact

User will receive rewards for all votingPeriod even though he voted for it or not

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L153C4-L164C6 https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L260C3-L298C6 https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L300C4-L313C6

Tool used

Manual Review

Recommendation

Use mapping to store users vote as per votingPeriod and use while claiming

0xSmartContract commented 1 month ago

Even though the user voted for only one voting period, the total reward he receives is calculated based on the reward rate for all voting periods.

In this case, the user also receives rewards for the periods in which he did not vote.

sherlock-admin2 commented 1 month ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/metropolis-exchange/magicsea-staking/pull/20

chinmay-farkya commented 1 month ago

Escalate

This issue and its whole family is completely invalid because of the following reason :

User votes data ie. amounts and rewards accrued is actually stored per voting period, unlike what this issue claims.

This is the relevant line : https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L274

As we can see that the rewards data is stored in _rewards array according to a corresponding index derived from the periodID.

Lets have a look at the _rewards array.

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

    struct RewardPerPeriod {
        Amounts.Parameter userVotes;
        Rewarder2.Parameter rewarder;
    }

    RewardPerPeriod[] internal _rewards;

userVotes is the amount deposited and rewarder is the data for rewards accrued.

Now lets look at _modify:


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

The _modify function is called with a "periodID" which is used to fetch the corresponding element from _rewards array and then amounts and rewarder struct are further extracted from this.

So everytime _modify is called for a periodID, it fetches the exact storage for that particular period and updtes it with amount of votes and/or rewards earned. This means that any votes data in _rewards[0] is not automatically carried over to _rewards[1].

Hence, what this issue claims as "rewards can be claimed for all periods if user voted for one" is false as the corresponding amounts will be zero if not voted and rewards are calculated using this amount so it will be zero for non-voted periods. The Amouts and rewarder structs are actually separate for every periodID.

sherlock-admin3 commented 1 month ago

Escalate

This issue and its whole family is completely invalid because of the following reason :

User votes data ie. amounts and rewards accrued is actually stored per voting period, unlike what this issue claims.

This is the relevant line : https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L274

As we can see that the rewards data is stored in _rewards array according to a corresponding index derived from the periodID.

Lets have a look at the _rewards array.

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/rewarders/BribeRewarder.sol#L55-L60
    struct RewardPerPeriod {
        Amounts.Parameter userVotes;
        Rewarder2.Parameter rewarder;
    }

    RewardPerPeriod[] internal _rewards;
userVotes is the amount deposited and rewarder is the data for rewards accrued.

Now lets look at _modify:
     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);
The _modify function is called with a "periodID" which is used to fetch the corresponding element from _rewards array and then amounts and rewarder struct are further extracted from this.

So everytime _modify is called for a periodID, it fetches the exact storage for that particular period and updtes it with amount of votes and/or rewards earned. This means that any votes data in _rewards[0] is not automatically carried over to _rewards[1].

Hence, what this issue claims as "rewards can be claimed for all periods if user voted for one" is false as the corresponding amounts will be zero if not voted and rewards are calculated using this amount so it will be zero for non-voted periods. The Amouts and rewarder structs are actually separate for every periodID.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Honour-d-dev commented 1 month ago

This issue is invalid!

0xSmartContract commented 1 month ago

I'd like to see a Mat POC on this issue (those that claim to be valid or those that claim to be invalid.)

chinmay-farkya commented 1 month ago

@0xSmartContract I think there is no need for a POC. Its enough to understand that the votes and rewards data is stored per periodID in the BribeRewarder, and then used according to the periodID.

And that is very clear from the _rewards array.

    struct RewardPerPeriod {
        Amounts.Parameter userVotes;
        Rewarder2.Parameter rewarder;
    }

    RewardPerPeriod[] internal _rewards;

anonjob commented 1 month ago

@0xSmartContract My issue #577 is wrongly attached to this as a duplicate. My issue talks about all the votes adding up which is not reset to 0 at the start of a new epoch (nor is the keeper capable of resetting them). This is completely different from the above main issue. Kindly look into it.

0xSmartContract commented 1 month ago

@0xSmartContract I think there is no need for a POC. Its enough to understand that the votes and rewards data is stored per periodID in the BribeRewarder, and then used according to the periodID.

And that is very clear from the _rewards array.
    struct RewardPerPeriod {
        Amounts.Parameter userVotes;
        Rewarder2.Parameter rewarder;
    }

    RewardPerPeriod[] internal _rewards;

My call here is not only to you, but to everyone who has contributed to this issue, to everyone who wants this title to be valid or invalid, seeing the POC in the foundry where the codes are running will solve this problem, it is not right to proceed with only a part of the code.

Reentrants commented 1 month ago

The burden of proof should be on those who think the issue is valid, but for YOUR sake @0xSmartContract.

Setup: https://gist.github.com/Reentrants/50898e49155a13b9eddbe69ea52c1a19

function test_reward_distribution_only_voted_periods() public {
  deal(alice, 10 ether);
  vm.startPrank(alice);
  staking.createPosition(1e18, 180 days);
  bribeRewarder = BribeRewarder(payable(address(rewarderFactory.createBribeRewarder(IERC20(address(0)), address(tokenA)))));
  // fund 5 periods, 1 ether per period
  bribeRewarder.fundAndBribe{value: 5 ether}(1, 5, 1 ether);
  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] = 1e18;

  // note: need BribeRewarder to fix the NFT ownership check
  // comment out the lines
  // if (!IVoter(_caller).ownerOf(tokenId, msg.sender)) {
  //     revert BribeRewarder__NotOwner();
  // } 
  voter.vote(1, pools, deltaAmounts);

  // now increment periods
  vm.startPrank(owner);
  for (uint i; i < 5; i++) {
      vm.warp(block.timestamp + 14 days);
      voter.startNewVotingPeriod();
  }

  // check that Alice only receives rewards for 1 period
  vm.startPrank(alice);
  uint256 balanceBefore = alice.balance;
  bribeRewarder.claim(1);
  uint256 balanceAfter = alice.balance;
  // ~= 1 ETH, rounding errors
  assertEq(balanceAfter - balanceBefore, 999999999999302399);
}

0xAraj commented 1 month ago

Issue #355 is a duplicate of this issue and has a working PoC https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/355

Reentrants commented 1 month ago

355 PoC is invalid, because it uses `VoterMock`. Its `setStartAndEndTime()` isn't period-dependent. Calling its `setStartAndEndTime()` affects all periods. Hence, the reward calculation isn't right, you'll find that the rewards accrued goes to the first period because its endTime has been extended into the 2nd.

Logs:
  period id:  1
  endTime:  1209601
  totalRewards:  3571428571428240000
  rewardAmount:  0
  period id:  1
  endTime:  1209601
  totalRewards:  6428571428570832000
  rewardAmount:  6428571428570831999
  period id:  1
  endTime:  2937601 // << this is incorrect
  totalRewards:  9999999999999072000
  rewardAmount:  9999999999999072000
  period id:  2
  endTime:  2937601
  totalRewards:  0
  rewardAmount:  0

anonjob commented 1 month ago

@0xSmartContract Issue #577 has been incorrectly grouped with this one. That issue is regarding the votes for all the voting periods add up and how it affects the pool reward calculation. As they are different issues, please look into deduplicating them and judging them separately.

chinmay-farkya commented 1 month ago

@0xSmartContract Issue #577 has been incorrectly grouped with this one. That issue is regarding the votes for all the voting periods add up and how it affects the pool reward calculation. As they are different issues, please look into deduplicating them and judging them separately.

That is expected working. The top pools ids weights are decided by the keeper script based on the votes accumulated by all farms over their entire lifetime and that is what directs the LUM emissions. The votes for all periods need to be added up. See sync-farm.js and FarmLens.sol. #577 is not an issue.

0xSmartContract commented 1 month ago

Thanks a lot @Reentrants @chinmay-farkya for your feedback and POC

POC ;

Alice only voted for one period and received about 1 ETH after 5 periods, which shows that the system is working properly
The amount received (999999999999302399 wei) is about 1 ETH. The small difference is probably due to gas fees or rounding.
The test shows that the user only receives a reward for the period they voted, contrary to what the original problem claims.
This POC shows that the original problem does not exist. The system only rewards users for the periods they voted for, not for all periods. The POC seems correct and reliable because it correctly tests the expected behavior and the results make sense.

So my first decision was wrong and i think Escalade is true and issue and dups is invalid

WangSecurity commented 1 month ago

I agree with the escalation and @chinmay-farkya and @Reentrants arguments. Planning to accept the escalation and invalidate the issue.

P.S. If you want a more detailed answer from me, I can give it, but I don't see a point in repeating the same arguments as above. Also, if you believe it's still valid, please submit a correct and a working POC.

WangSecurity commented 4 weeks ago

Result: Invalid Has duplicates

sherlock-admin4 commented 4 weeks ago

Escalations have been resolved successfully!

Escalation status:

chinmay-farkya: accepted

sherlock-admin2 commented 2 weeks ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-06-magicsea-judging

araj - Voter receives the bribe reward for `all` voting period, regardless of how many votingPeriod he voted for #114

Voter receives the bribe reward for `all` voting period, regardless of how many votingPeriod he voted for

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-06-magicsea-judging

araj - Voter receives the bribe reward for `all` voting period, regardless of how many votingPeriod he voted for #114

Voter receives the bribe reward for all voting period, regardless of how many votingPeriod he voted for

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Voter receives the bribe reward for `all` voting period, regardless of how many votingPeriod he voted for