Neo Tokyo contest - BPZ's results

Lines of code

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

Vulnerability details

Impact

This vulnerability allows a user to cause their LP position points to underflow which will then allow a user to receive a massively disproportionate amount of the emission rewards relative to their stake because they now practically have an infinite amount of points in addition to breaking the the contract's internal controls and accounting system (for example, users's lpPosition.points > pool.totalPoints). This finding should be considered high risk because it will allow the user to redirect almost all emission rewards to their position.

Proof of Concept

This attack involves the user creating a small stake position for the LPToken asset. If a user specifies an amount of 0.000000000000000001 LPToken when invoking the stake function in the NeoTokyoStaker contract, the user's points will be equal to 0 as specified by this equation:

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

Because the user can append additional amounts to a respective LP staking position, a user can repeat this multiple times so that their cumulative amount will be equal to at least 1 point when they withdraw their position once the respective timelockDuration has passed. Withdrawing their position will then cause their position's points to underflow because the current value of their lpPosition.points = 0 while their lpPosition.amount will equal an amount greater than 0 and this subtraction happens in an unchecked block. Here is the affected block of code within the '_withdrawLP' function:

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

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

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

This user will then have a practically infinite amount of lpPosition.points allowing them to receive most of the emission rewards. The following details a step-by-step outline of this attack for when a respective timelockDuration of 30 days and timelockMultiplier of 100 is specified:

1) User creates a small stake position of 0.000000000000000001 LP token with the mentioned timelockOption. This will cause their position's points to be equal to 0. Here is the calculation for this with the mentioned example values used in the points equation:

uint256 points = 1 * 100 / 1e18 * 100/ 100 = 0;

2) User repeats the previously mentioned step, appending the same amount to the respective stake position, until their cumulative points will calculate to a non zero value when they withdraw the position at the end of the timelockDuration of 30 days. The user can calculate how many stake transactions will be needed by determining how many points a stake amount of 0.000000000000000001 LP token is worth:

points = 1 * 100 / 1e18 * 100/ 100 ->  points = 1e-20

In other words 1e-18 points = 1e-16 LP Tokens, so the user would have to repeat this specified stake transaction at least 100 times to have a position valued at at least 1 point.

3) User waits the timelockDuration of 30 days, and withdraws the entirety of their stake causing their lpPosition.points to underflow.

Tools Used

VS Code

Recommended Mitigation Steps

As a mitigation, it is recommended that the protocol implements a safeguard to ensure that the user has not created an LP stake position that amounts to 0 points. This can be done by implementing a require statement directly after the position has been updated in the state within the '_stakeLP' function on line 1166 as shown below:

// Update caller staking information and asset data.
PoolData storage pool = _pools[AssetType.LP];
unchecked {
	uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;

	// Update the caller's LP token stake.
	stakerLPPosition[msg.sender].timelockEndTime =
	block.timestamp + timelockDuration;
	stakerLPPosition[msg.sender].amount += amount;
	stakerLPPosition[msg.sender].points += points;

	// Update the pool point weights for rewards.
	pool.totalPoints += points;
}
require(stakerLPPosition[msg.sender].points != 0, "Amount requirement not met!");

DOS in getPoolReward

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

If i = 0 , it could be reverted this function since 0-1 = -1 .

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

Lack of checks for Address(0) and Unit 0 .

The following methods have a lack of checks if the received argument is an address, it’s good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0) and uint 0.

Affected Source Code

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

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