code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1077 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1098 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1155

Vulnerability details

Impact

Due to integer division, users will lose points when staking a small amount of BYTES into Citizen. The value of points affects user rewards. Users should be treated equally no matter how much they stake.

Proof of Concept

Let's take a look at the code snippet of the _stakeBytes function:

//
    function _stakeBytes (
        uint256
    ) private {
        ...
        ...
            PoolData storage pool = _pools[AssetType.S1_CITIZEN];
            unchecked {
                uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);    //_BYTES_PER_POINT = 200 * 1e18
                citizenStatus.stakedBytes += amount;
                citizenStatus.points += bonusPoints;
                pool.totalPoints += bonusPoints;
            }
        ...
        ...
            PoolData storage pool = _pools[AssetType.S2_CITIZEN];
            unchecked {
                uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);    //_BYTES_PER_POINT = 200 * 1e18
                citizenStatus.stakedBytes += amount;
                citizenStatus.points += bonusPoints;
                pool.totalPoints += bonusPoints;
            }
        ...
    }

It can be clearly seen from the code that if the amount is less than 2e18, bonusPoints is equal to 0, then citizenStatus.points will not increased. citizenStatus.points is the most critical parameter when calculating rewards. We can see its effect in getPoolReward.

Through the comments and codes of _BYTES_PER_POINT, we can conclude that one point requires 2e18 BYTES. But the _stakeBytes function does not check the amount parameter, which leads to user losses.

Tools Used

Manual Review