Neo Tokyo contest - kenzo's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1155 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#L1627 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1515 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1580

Vulnerability details

When a user stakes or unstakes LP tokens, the following calculation is used. Note the early division by 1e16:

uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

Due to this division, amounts smaller than 1e16 are not properly considered. This creates various inconsistencies when staking and unstaking.

The problem occurs also when calculating staking points for BYTES.

Impact

At the worst case, a user wouldn't be able to withdraw all his staked funds from the contract. He would lose 0.01 LP tokens or 2 BYTES. While not a huge amount, it's not dust either. These are still funds lost for no reason, and obviously not expected behavior from a staking contract.

Additionally, the user will have to manipulate his transaction to withdraw most of his funds - in some scenarios, a simple withdrawal of his whole staked amount will simply revert.

Additionally, a user may get undue rewards by withdrawing 0.01 LP tokens or 2 BYTES at a time, since that will not change his amount of staking points.

These fundamental problems is why I consider this a high severity issue, although understandably it might also be considered medium.

Proof of Concept

Let's look at how staking points are calculated according to the amount staked. This calculation returns the same staking points for different amounts staked, and has very low granularity (decimals/precision level).

uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

Due to the division by 1e16, amounts smaller than 1e16 will not contribute to the calculation. I will show the several problems this causes.

Important: please note that the contract saves these points in the user's staking struct, and later tries to deduct the result of this calculation when unstaking.

User can't withdraw all his funds + needs 2 withdrawal transactions

The following POC shows a scenario where the user can't withdraw all his funds.

1. User deposits 100e18-1 LPs. According to the abovementioned calculation, he gets 19998 points.

2. User deposits 100e18-1 LPs again, gets 19998 points.

3. Total points of the user in the contract - 39996.

4. User withdraws 100e18-1 + 1e16+1 LPs. 20002 points are deducted from his balance.

5. User has 19994 points left in the contract.

6. If the user tries to redeem his remaining stake - 100e18-1 - (1e16+1) - due to the inaccurate calculation, the contract would try to deduct 19996 points from the user's point balance

7. But the user only has 19994 points. So the linked deduction line will revert due to underflow and the user can not withdraw his full balance. You can use these Solidity functions to see the calculation of the points from these inputs. This shows how in certain scenarios, a user can not withdraw all his stake. The user can ask to withdraw 100e18-1 - (2e16) and succeed. But still the two problems are there:

8. User has 1e16 tokens stuck in the contract

9. User needs 2 transactions to unstake his stake

Can not withdraw full stake in one transaction

There can be other simpler scenarios for this problem to occur. For example, depositing 100e18 - 1 twice (getting 19998 points twice) and then withdrawing the whole amount at once (trying to deduct 39998 points) - the withdrawal will revert for same reason - trying to deduct more points than the user has. So user can't simply withdrawal all his funds in one transaction.

User can get undue rewards

For the same reason - the low granularity - user can get undue rewards. If for example user deposits 1e18 tokens, then withdraws 1e16-1, the points calculated would be 0, but the user will still be able to withdraw 1e16-1 tokens. By doing this repeatedly, the user can withdraw his stake, while still keeping the same amount of staking points in the contract. He can also get more rewards by redepositing. The viability of this attack will depend on gas costs and rewards value.

Same in BYTES

Similar problem occurs when calculating points for BYTES staking:

uint256 bonusPoints = (amount * 100 / (200 * 1e18));

Here the granularity is 2e18, so amounts smaller than that would be lost in calculation. Although the points are deducted only from the total pool and not from each staker's individual position, the lack of granularity is still there, therefore stakers can get undue rewards etc'.

Note: division before multiplication

There's an additional small loss of value due to division before multiplication. This manifests mostly as 1 staking point lost. This is a different issue than the one described above. (If this early division is considered a valid issue, please split this issue into that issue as well.) To fix this lacking precision, change the calculation so the division happens only after the multiplication.

amount * timelockMultiplier * 100 / 1e18  / divisor;

Recommended Mitigation Steps

Honestly I am not sure and unfortunately do not have enough time to properly consider this, but the options I see are:

1. Use bigger precision for the staking points, so that 1 staking point is 1e18 instead of 1. Then you would have to multiply the points calculation by 1e18 initially.
2. Use a more Masterchef-like approach, where you don't recalculate the amount of points to deduct upon staking, but rather remove points propotionally to the amount unstaked. So if a user has 101 tokens deposited in timelock "100", and he asks to withdraw 50 tokens, remove 50/101 staking points. You can also use tokens to track the staked amount but that is a relatively big change.