Lines of code
<https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1388&gt;

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).

---

The text was updated successfully, but these errors were encountered:

All reactions