The current design may result in users claiming a large number of rewards

[code423n4]

Lines of code

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

Vulnerability details

Impact

configurePools is used to set the startTime and reward for each window in the pool,

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

but since in getPoolReward, if pool.rewardWindows[0].startTime != 0 will cause an overflow,

            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; // @auidt: If rewardWindows[0].startTime > 0, it will overflow here due to 0-1

so pool.rewardWindows[0].startTime must be 0 to not cause getPoolReward to fail. This introduces the new issue that when pool.rewardWindows[0].startTime == 0, if pool.rewardWindows[0].reward is not equal to 0, the user will be able to claim a large amount of rewards. Consider configurePools is called, set pool.rewardWindows[0].startTime = 0, pool.rewardWindows[0].reward ! = 0, then in the getPoolReward function, the user can claim the accumulated rewards since January 1, 1970

                if (lastPoolRewardTime < window.startTime) { // @auidt: lastPoolRewardTime == 0
                                        ...
                } else if (i == windowCount - 1) {
                    unchecked {
                        uint256 timeSinceReward = block.timestamp - lastPoolRewardTime; // @audit: claim the accumulated rewards since January 1, 1970
                        totalReward = window.reward * timeSinceReward;
                    }
                    break;
                }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider setting the startTime and reward of _pools.rewardWindows[0] to 0 by default in configurePools.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as primary issue

[TimTinkers]

To the warden: I would have appreciated a full test case proof of concept for this attack; I will have to prepare one myself tomorrow to verify. @hansfriese if I can verify this issue it is 100% a high risk and not a medium risk.

[hansfriese]

@TimTinkers OK. I will.

288 seems to contain a POC. Will mark #288 as primary after you confirm.

[TimTinkers]

Thanks @hansfriese, I responded to #288.

[hansfriese]

The user's reward won't be manipulated. Please check #288 for details. Close as invalid due to the wrong impact.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Insufficient proof