Neo Tokyo contest - Josiah's results

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

Vulnerability details

Impact

Precision issue leading to zero truncation due to numerator smaller than denominator in a ratio or a division happens readily in Solidity if extra cares have not been given to it. Arithmetic operations running into this incident are typically associated with division before multiplication and/or inadequate scaling of the numerator.

Proof of Concept

There are a total of 3 instances found in NeoTokyoStaker.sol.

1. Truncation to zero if amount of BYTES is less than 2e18 (2 tokens) in _stakeBytes ()

NeoTokyoStaker.sol#L203

	uint256 constant private _BYTES_PER_POINT = 200 * 1e18;

NeoTokyoStaker.sol#L1077-L1080

				uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
				citizenStatus.stakedBytes += amount;
				citizenStatus.points += bonusPoints;
				pool.totalPoints += bonusPoints;

Note: The decimal number of BYTES, as inherited from Openzeppelin ERC20.sol, is 18.

If amount were inputted as 2e18 - 1,

bonusPoints = (2e18 - 1) * 100 / (200 * 1e18) = (200e18 - 100) / 200e18 = 0

As a result,

citizenStatus.stakedBytes = citizenStatus.stakedBytes + (2e18 - 1) citizenStatus.points = citizenStatus.points + 0 pool.totalPoints = pool.totalPoints + 0

The staker ended up transferring (2e18 - 1) BYTES to the contract with zero point added to boost his/her reward emission in the S1 or S2 Citizen Pool.

2. Truncation to zero if amount of LP is less than 1e16 (0.01 tokens) in _stakeLP ()

NeoTokyoStaker.sol#L1155-L1164

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

			// Update the caller's LP token stake.
			stakerLPPosition[msg.sender].timelockEndTime =
				block.timestamp + timelockDuration;
			stakerLPPosition[msg.sender].amount += amount;
			stakerLPPosition[msg.sender].points += points;

			// Update the pool point weights for rewards.
			pool.totalPoints += points;

Assumption: The decimal number of LP is also 18, as inherited from Openzeppelin ERC20.sol.

If amount were inputted as 1e16 - 1,

points = (1e16 - 1) * 100 / 1e18 * timelockMultiplier / _DIVISOR = (1e18 - 100) / 1e18 * timelockMultiplier / _DIVISOR = 0 * * timelockMultiplier / _DIVISOR = 0

As a result,

stakerLPPosition[msg.sender].amount = stakerLPPosition[msg.sender].amount + (1e16 - 1) stakerLPPosition[msg.sender].points = stakerLPPosition[msg.sender].points + 0

The staker ended up transferring (1e16 - 1) LP to the contract with zero point added to boost his/her reward emission in the LP Pool.

3. Truncation to zero if amount of LP is less than 1e16 (0.01 tokens) in _withdrawLP ()

NeoTokyoStaker.sol#L1623-L1630

			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;

			// Update the caller's LP token stake.
			lpPosition.amount -= amount;
			lpPosition.points -= points;

			// Update the pool point weights for rewards.
			pool.totalPoints -= points;

Again, if amount were inputted as 1e16 - 1,

points = (1e16 - 1) * 100 / 1e18 * lpPosition.multiplier / _DIVISOR = (1e18 - 100) / 1e18 * lpPosition.multiplier / _DIVISOR = 0 * * lpPosition.multiplier / _DIVISOR = 0

As a result,

lpPosition.amount = lpPosition.amount - (1e16 - 1) lpPosition.points = lpPosition.points - 0

This vulnerability actually works in great favor to the staker. Specifically, the staker could repeatedly perform the exploit to cumulatively withdraw his/her LP tokens without deducting any points from his/her LP position.

Since stakerLPPosition never gets reset, it does not matter whether or not the staker's lpPosition.amount remains non-zero or equals zero. Apparently, the staker could restake the withdrawn LPs (of course with an amount greater than 0.01 LP tokens) to keep boosting the rewards emission in the LP Pool.

Recommended Mitigation Steps

It is recommended making the first multiplicand, i.e. amount, scaled to a higher order. In the case of the LP pool, the arithmetic operations could also benefit from simplifying two divisions to one division.