When pool.rewardWindows[0].startTime != 0, getPoolReward will overflow, resulting in failure to claim the reward

[code423n4]

Lines of code

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

Vulnerability details

Impact

In getPoolReward, all windows of the pool are iterated to calculate the reward. When lastPoolRewardTime < window.startTime, the reward rate of the previous window is got. When the user claims the reward for the first time, lastPoolRewardTime == 0. If pool.rewardWindows[0].startTime > 0, since lastPoolRewardTime < window.startTime, it will get pool.rewardWindows[0-1].reward, at this point an overflow will occur, resulting in the user not being able to claim the reward.

            uint256 totalReward;
            uint256 lastPoolRewardTime = lastRewardTime[_recipient][_assetType];
            uint256 windowCount = pool.rewardCount;
            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;

pool.rewardWindows.startTime is configured in configurePools, but configurePools does not require pool.rewardWindows[0].startTime == 0, if pool.rewardWindows[0].startTime > 0, getPoolReward will fail and the user will not be able to claim the reward.

    function configurePools (
        PoolConfigurationInput[] memory _inputs
    ) hasValidPermit(UNIVERSAL, CONFIGURE_POOLS) external {
        for (uint256 i; i < _inputs.length; ) {
            uint256 poolRewardWindowCount = _inputs[i].rewardWindows.length;
            _pools[_inputs[i].assetType].rewardCount = poolRewardWindowCount;
            _pools[_inputs[i].assetType].daoTax = _inputs[i].daoTax;

            // Set the pool reward window details by populating the mapping.
            uint256 lastTime;
            for (uint256 j; j < poolRewardWindowCount; ) {
                _pools[_inputs[i].assetType].rewardWindows[j] =
                    _inputs[i].rewardWindows[j];

                // Revert if an invalid pool configuration is supplied.
                if (j != 0 && _inputs[i].rewardWindows[j].startTime <= lastTime) {
                    revert RewardWindowTimesMustIncrease();
                }
                lastTime = _inputs[i].rewardWindows[j].startTime;
                unchecked { j++; }
            }
            unchecked { ++i; }
        }
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

            uint256 totalReward;
            uint256 lastPoolRewardTime = lastRewardTime[_recipient][_assetType];
            uint256 windowCount = pool.rewardCount;
            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) {
+               if (lastPoolRewardTime < window.startTime && i != 0) {
                    uint256 currentRewardRate = pool.rewardWindows[i - 1].reward;

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as primary issue

[TimTinkers]

This really ought to be out of scope as was clarified in the Discord thread for the competition; there is no intent to ever deploy with reward windows where the initial starting time is not 0. That being said, you took the time to include a fix which I appreciate and might as well include at this point.

[c4-sponsor]

TimTinkers marked the issue as sponsor acknowledged

[hansfriese]

Will consider as a valid medium because there is no documentation about initialStartTime = 0

[TimTinkers]

@hansfriese I disagree with the severity of medium.

• window.startTime is exclusively controlled via an administrative configurePools function which was flagged as out-of-scope
• every provided test case (demonstrating our intended deployment configuration) explicitly included an initial window.startTime of 0
• I clarified this early in the contest
• even if a misconfiguration slipped through all the way to production nobody would actually lose funds or accrued rewards, and admins could easily rectify this by just calling configurePools again

[c4-sponsor]

TimTinkers marked the issue as disagree with severity

[hansfriese]

Downgrade to QA as it was clarified in the FAQ already.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c