code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Contract would revert in getPoolReward () when it tries to get the applicable time period to calculate the reward.

Since it is breaking one of the core functionality which would result in halt, setting this as High.

Proof of Concept

claimReward() functio calls the getPoolReward() function to get the reward and tax. When getPoolReward tries to calculate the reward for the _recipient based on their points total. It tries to Iterate through the entire array of pool reward windows to find the applicable time period.

If the last reward time is less than the starting time of this window, then the reward was accrued in the previous window. For this, the logic is

        for (uint256 i; i < windowCount; ) {
            RewardWindow memory window = pool.rewardWindows[i];

            /*
                If the last reward time is less than the starting time of this 
                window, then the reward was accrued in the previous window.
            */
            if (lastPoolRewardTime < window.startTime) {
                uint256 currentRewardRate = pool.rewardWindows[i - 1].reward;

in the for loop iteration, when i =0, current reward rate is calculated as uint256 currentRewardRate = pool.rewardWindows[i - 1].reward;, it is clear, solidity revert when it flow, i.e 0-1 will revert.

Tools Used

Manual review

Recommended Mitigation Steps

Use unchecked as shown below,

if (lastPoolRewardTime < window.startTime) { unchecked { uint256 currentRewardRate = pool.rewardWindows[i - 1].reward; } . . }