Neo Tokyo contest - Dug's results

Lines of code

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

Vulnerability details

Impact

In the getPoolReward function of NeoTokyoStaker.sol, a user's reward is calculated by first determining totalReward, which accounts for the total reward emissions since the user's lastPoolRewardTime.

Then, the user's allocation of the totalReward is determined by multiplying it by the users stake of pool.totalPoints.

    // Return final shares.
    unchecked {
        uint256 share = ((points * _PRECISION) / pool.totalPoints) * totalReward;
        uint256 daoShare = (share * pool.daoTax) / (100 * _DIVISOR);
        share /= _PRECISION;
        daoShare /= _PRECISION;
        return ((share - daoShare), daoShare);
    }

This would be appropriate if the total number of points in the pool were constant from the time the user staked until the time getPoolReward is called. However, changes in the pool.totalPoints over times creates unfair distributions of points.

Proof of Concept

To illustrate this consider a simplified use case where there is a single reward window with an emissions rate of 10 per second and a single timelock option of 1000 seconds. Alice and Bob are two participants, each staking an S2 citizen with 10 points.

• Alice begins staking at timestamp 0, and withdraws at timestamp 1000.
• Bob stakes at timestamp 999 and withdraws at timestamp 1999.

For both of these staking periods 10_000 totalRewards are emitted and user share is calculated with...

    share = ((points * _PRECISION) / pool.totalPoints) * totalReward;
    share /= _PRECISION;

When Alice's rewards are calculated, pool.totalPoints is 20, because of the one overlapping second with Bob's staking period, resulting in her share being 5_000, even though her stake represented the entire pool for 99.9% of her staking period.

When Bob's rewards are calculated, his share is 10_000.

So, over the course of the entire scenario, 19_990 rewards we're to be emitted, but only 15_000 were actually rewarded. Additionally, what was rewarded was unfairly distributed between identical stakes.

Tools Used

Manual review

Recommended Mitigation Steps

Consider a different method of tracking emissions and rewards such as how MasterChef's accSushiPerShare, lastRewardBlock, and staker rewardDebt variables allow for a more accurate calculation of rewards.

Custom error not returned on invalid asset type

In both the stake and withdraw functions of NeoTokyoStaker.sol, the following check exists...

      if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {
          revert InvalidAssetType(uint256(_assetType));
      }

This implies that a uint8(_assetType) value of 4 is valid. However the AssetType struct is defined as follows...

    enum AssetType {
        S1_CITIZEN,
        S2_CITIZEN,
        BYTES,
        LP
    }

This results in the InvalidAssetType(uint256 assetType) error not being returned in all cases where an invalid asset type is provided.

State changes should emit events

In BYTES2.sol, the changeStakingContractAddress and changeTreasuryContractAddress functions update storage variables without emitting events.

Staker contract address should be immutable/lockable

Considering the heavy interdependence between the NeoTokyoStaker.sol and BYTES2.sol contracts, it would be prudent to make the STAKER storage variable immutable or lockable.