search

code-423n4 / 2023-03-neotokyo-findings

4 stars 0 forks source link

Wrong accounting of share leading to incorrect amount of BYTES be minted per second #347

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1388

Vulnerability details

Impact

In NeoTokyoStaker, staker is a competitive system where stakers compete for a fixed emission rate in each of the S1 Citizen, S2 Citizen, and LP token staking pools. For each staking pool, there are some reward windows. Each reward window has different emission rates configurable by admin. So when calculating the final reward amount for a user, it does a for loop through every reward window and sum them up.

However, it used the final points and totalPoints when staker claimed reward for every previous reward window, which is not correct since totalPoints is different every time other stakers deposit/withdraw.

The result is the reward is incorrectly calculated, it can be higher or lower than expected, and both cases are high risk issues.

Proof of Concept

Consider a scenario:

  1. Citizen S2 pools have 2 reward windows. The first starts at t1 = 100 and rate1 = 200, the second starts at t2 = 200 and rate2 = 300

  2. Alice and Bob both stakes at t0 = 100, Citizen S2 tokens have the same weight (100) so Alice and Bob should receive equal amounts of rewards at any moment.

  3. Alice withdraw first at t3 = 300, her share is equal to 100 and totalPoints = 200, she will receive

    totalReward = ((t2 - t1) * rate1 + (t3 - t2) * rate2)
= 100 * 200 + 100 * 300 = 50000
share = points * totalReward / totalPoints 
= 100 * 50000 / 200 = 25000
totalPoints = 100

  4. Bob withdraw at the same time, but his TX is executed after Alice, he will receive

    totalReward = ((t2 - t1) * rate1 + (t3 - t2) * rate2)
= 100 * 200 + 100 * 300 = 50000
share = points * totalReward / totalPoints 
= 100 * 50000 / 100 = 50000

As we can see, Alice and Bob both deposit and withdraw at the same time, but Alice only gets 25000 while Bob gets 50000 token reward. Totally, they receive 75000 while totalReward is only 50000, which is more than expected amount of BYTES should be minted.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing the formula to calculate rewards for users. For example, in Master Chef, we need to calculate the accumulated reward rate per share of the pool every time a user interacts with the pool and only need to update the position of each user when they interact.

c4-judge commented 1 year ago

hansfriese marked the issue as duplicate of #423

c4-judge commented 1 year ago

hansfriese marked the issue as satisfactory