Neo Tokyo contest - joestakey's results

Lines of code

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

Vulnerability details

User rewards are updated upon staking actions (ie stake() or withdraw()):

File: contracts/staking/NeoTokyoStaker.sol
1225: 		// Grant the caller their total rewards with each staking action.
1226: 		IByteContract(BYTES).getReward(msg.sender);

Which are computed as follows:

File: contracts/staking/NeoTokyoStaker.sol
1387: 			unchecked {
1388: 				uint256 share = points * _PRECISION / pool.totalPoints * totalReward; @audit - here
1389: 				uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);
1390: 				share /= _PRECISION;
1391: 				daoShare /= _PRECISION;
1392: 				return ((share - daoShare), daoShare);
1393: 			}

Breaking down how the share of rewards is computed, it is a function of:

• point, ie "how much" the user has staked since the last staking action
• pool.totalPoints, ie the current total of points in the staking pool.
• totalReward, ie how much rewards are earned since the last staking action (computed by adding the products of the reward members of the windows and the time length of each window)

The issue is that their share of rewards is a function of the current pool.totalPoints:

This means a staker will lose part of their reward if other users have started staking after them.

Impact

Loss of rewards

Proof of Concept

• Alice stakes some LP tokens into the staking pool (say 10 tokens, for a timelock of 30 days)
• After some time, Alice decides to withdraw, she calls getPoolReward to check how much she would receive, then calls withdraw()
• Bob front-runs her call with a stake() call, depositing some LP tokens in the pool (the same amount as Alice)
• Alice withdraws her LP token, but the reward BYTES she receives is less than expected (half), because Bob's deposit increased totalPoints.

Add this test to NeoTokyoStaker.test.js, recreating the steps described above: Compared to the existing test, the Bob stake() call is removed from the beforeEach block in describe('with staked LP tokens' so that only Alice stakes at the beginning.

• Run it once with the block commented out, which corresponds to Alice claiming her reward without Bob staking.
• Run it a second time with the block not commented out , which corresponds to Bob staking just before Alice's reward claim.

Logging the result shows that Alice receives half the amount in the second case.

// Simulate LP staking.
		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;
			});

			// rewards theft
			it.only('rewards theft', async function () {
				
				await ethers.provider.send('evm_setNextBlockTimestamp', [
					aliceStakeTime + (60 * 60 * 24)
				]);
				
				// @audit diff - Stake Bob's LP tokens for 30 days. -> changes reward Alice receives
				// @audit following block to toggle - when "commented out", Alice is the only staker, when "on", Bob stakes just before she claims her reward
				// await NTStaking.connect(bob.signer).stake(
				// 	ASSETS.LP.id,
				// 	TIMELOCK_OPTION_IDS['30'],
				// 	ethers.utils.parseEther('40'),
				// 	0,
				// 	0
				// );
				
				// Alice claims her rewards
				let aliceBalance = await NTBytes2_0.balanceOf(alice.address);
				await NTCitizenDeploy.connect(alice.signer).getReward();
				let aliceBalanceInitial = aliceBalance;
				aliceBalance = await NTBytes2_0.balanceOf(alice.address);
				console.log("alice reward received: ", aliceBalance - aliceBalanceInitial);  // @audit 48499999999999870000 when Bob did not stake
				// @audit 24250280671296094000 when Bob staked just before Alice claimed (ie half)
			});

Tools Used

Manual Analysis, Hardhat

Mitigation

Refactor the reward computation, removing the use of pool.totalPoints in it, and instead add:

• an accumulatedRewardsPerShare member in PoolData , that should be updated in claimRewards
• a rewardDebt member in StakedS1Citizen, StakedS2Citizen and LPPosition, that should be updated at the end of stake() and withdraw() The amount of reward to send will essentially be a product of staked.accumulatedRewardsPerShare and staked.points, to which will be subtracted staked.rewardDebt (which is how many MasterChef implementations compute rewards).

QA Report

Low Issues

<a name="L-1"></a>[L-1] Loss of precision

When staking LP tokens, there is a loss of precision in the computation of points due to a division before multiplication

File: contracts/staking/NeoTokyoStaker.sol
1154: 		unchecked {
1155: 			uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

The loss is generally minor and happens only if timelockMultiplier > 100. The only case where the loss is significant is when amount < 1e16, which depending on the state of the BYTES/ETH pool, may be of value greater than $1. In such case, the points computed are 0, meaning the user share of the pool is 0.

Refactor as follows:

File: contracts/staking/NeoTokyoStaker.sol
1154: 		unchecked {
-1155: 			uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
+1155: 			uint256 points = amount * 100 * timelockMultiplier / 1e18 / _DIVISOR;

<a name="L-2"></a>[L-2] Wrong error logic of assetType in stake

The function reverts if assetType > 4. This should be >=, as 4 is not a valid type.

File: staking/NeoTokyoStaker.sol

1205: 		if (uint8(_assetType) > 4) { //@audit-info should be >=, 4 is not a valid type
      			revert InvalidAssetType(uint256(_assetType));
      		}

<a name="L-3"></a>[L-3] Wrong error logic of startTime in stake

The function reverts if _pools[_assetType].rewardWindows[0].startTime >= block.timestamp. This should be >, as currently the function reverts when the startTime is reached at the block.timestamp.

File: staking/NeoTokyoStaker.sol

1214: 		// Validate that the asset being staked matches an active pool.
1215: 		if (_pools[_assetType].rewardWindows[0].startTime >= block.timestamp)
1216: 			revert InactivePool(uint256(_assetType));
1217: 		}

<a name="L-4"></a>[L-4] _withdrawLP should reset timelockEndTime

This does not create any vulnerability, but may make it confusing when a user calls getStakerPositions if they have withdrawn their LP. Note that resetting timelockEndTime is done for the the citizen staking functions

File: staking/NeoTokyoStaker.sol

1634: 		if (lpPosition.amount == 0) {
1635: 			lpPosition.multiplier = 0;
+						lpPosition.timelockEndTime = 0
1636: 		}

Non Critical Issues

<a name="NC-1"></a>[NC-1] Capital letters should be reserved for constants

File: staking/BYTES2.sol

49: 	address public STAKER;

52: 	address public TREASURY;

<a name="NC-2"></a>[NC-2] Constants on the left are better

This is safer in case you forget to put a double equals sign, as the compiler will throw an error instead of assigning the value to the variable.

File: staking/NeoTokyoStaker.sol

633: 		if (rewardRate == 0) {

693: 		if (_assetType == AssetType.S1_CITIZEN) {

695: 		} else if (_assetType == AssetType.S2_CITIZEN) {

917: 		} else if (citizenVaultId != 0 && vaultId == 0) {

926: 		} else if (citizenVaultId == 0 && vaultId != 0) {

947: 		if (handClaimant == 1) {

1060: 		if (seasonId == 1) {

1071: 			if (citizenStatus.timelockEndTime == 0) {

1084: 		} else if (seasonId == 2) {

1092: 			if (citizenStatus.timelockEndTime == 0) {

1144: 		if (stakerLPPosition[msg.sender].multiplier == 0) {

1210: 		if (_pools[_assetType].rewardCount == 0) {

1221: 		if (timelockOption == 0) {

1278: 			if (_assetType == AssetType.S1_CITIZEN) {

1287: 			} else if (_assetType == AssetType.S2_CITIZEN) {

1296: 			} else if (_assetType == AssetType.LP) {

1301: 				revert InvalidAssetType(uint256(_assetType));

1320: 					uint256 currentRewardRate = pool.rewardWindows[i - 1].reward;

1353: 							if (j == windowCount - 1) {

1376: 				} else if (i == windowCount - 1) {

1472: 		if (stakedCitizen.timelockEndTime == 0) {

1502: 			if (citizenId == oldPosition[stakedIndex]) {

1547: 		if (stakedCitizen.timelockEndTime == 0) {

1567: 			if (citizenId == oldPosition[stakedIndex]) {

1634: 		if (lpPosition.amount == 0) {

1697: 		_withdraw();

<a name="NC-3"></a>[NC-3] Scientific Notation

It is good practice to use scientific notation instead of using exponents or explicitly writing number of high decimals

File: staking/NeoTokyoStaker.sol

200: 	uint256 constant private _DIVISOR = 100;

203: 	uint256 constant private _BYTES_PER_POINT = 200 * 1e18;

946: 		uint256 vaultMultiplier = 100;

1022: 			citizenStatus.points = 100 * timelockMultiplier / _DIVISOR;

1077: 				uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);

1098: 				uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);

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

1331: 								uint256 timeSinceReward = block.timestamp - lastPoolRewardTime;

1342: 								uint256 timeSinceReward = window.startTime - lastPoolRewardTime; // 1- 1000-500 // 2-2000-1000 //3-

1389: 				uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);

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

<a name="NC-4"></a>[NC-4] Imports should be cleaner

For readability, it is good practice to only import the libraries and contracts your need from a file.

Update your imports as such import {contract1 , contract2} from "file.sol";

File: staking/BYTES2.sol

4: import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

7: import "../access/PermitControl.sol";

8: import "../interfaces/IByteContract.sol";

9: import "../interfaces/IStaker.sol";

File: staking/NeoTokyoStaker.sol

4: import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

6: import "../access/PermitControl.sol";

7: import "../interfaces/IByteContract.sol";

8: import "../interfaces/IGenericGetter.sol";