Lines of code
<https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1124-L1174&gt;
<https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1264-L1396&gt;

Vulnerability details

Impact

When calling the following NeoTokyoStaker._stakeBytes and NeoTokyoStaker._stakeLP functions, the higher the specified amount to be staked is, the higher the pool.totalPoints is increased by.

<https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1044-L1116&gt;

	function _stakeBytes (
		uint256
	) private {
		uint256 amount;
		uint256 citizenId;
		uint256 seasonId;
		assembly{
			amount := calldataload(0x44)
			citizenId := calldataload(0x64)
			seasonId := calldataload(0x84)
		}
		...
		_assetTransferFrom(BYTES, msg.sender, address(this), amount);
		...
		if (seasonId == 1) {
			...
			PoolData storage pool = _pools[AssetType.S1_CITIZEN];
			unchecked {
				uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
				citizenStatus.stakedBytes += amount;
				citizenStatus.points += bonusPoints;
				pool.totalPoints += bonusPoints;
			}
		...
		} else if (seasonId == 2) {
			...
			PoolData storage pool = _pools[AssetType.S2_CITIZEN];
			unchecked {
				uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
				citizenStatus.stakedBytes += amount;
				citizenStatus.points += bonusPoints;
				pool.totalPoints += bonusPoints;
			}
		...
		} else {
			revert InvalidSeasonId(seasonId);
		}
		...
	}

<https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1124-L1174&gt;

	function _stakeLP (
		uint256 _timelock
	) private {
		uint256 amount;
		assembly{
			amount := calldataload(0x44)
		}
		...
		_assetTransferFrom(LP, msg.sender, address(this), amount);
		...
		uint256 timelockDuration = _timelock >> 128;
		uint256 timelockMultiplier = _timelock & type(uint128).max;
		...
		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;
		}

		...
	}

When Staker A, who has staked some tokens already, calls the following NeoTokyoStaker.getPoolReward function, the share to be minted to this staker is calculated by executing uint256 share = points * _PRECISION / pool.totalPoints * totalReward and share /= _PRECISION. When such staker’s NeoTokyoStaker.stake or NeoTokyoStaker.withdraw transaction, which calls the NeoTokyoStaker.getPoolReward function, is in the mempool, Staker B, who has many BYTES tokens or LP tokens, can maliciously frontrun such transaction by calling the NeoTokyoStaker.stake function with a gas price that is higher than such transaction’s, which can increase the pool.totalPoints by a lot. When this happens, the pool.totalPoints could have been increased to an extent in which executing uint256 share = points * _PRECISION / pool.totalPoints * totalReward would cause share to be smaller than _PRECISION when Staker A’s transaction is executed. As a result, 0 share will be minted to Staker A and 0 daoShare will be minted to the DAO’s treasury while both Staker A and the DAO should deserve some reward shares; essentially, Staker A and the DAO lose these reward shares that they are entitled to.

<https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1264-L1396&gt;

	function getPoolReward (
		AssetType _assetType,
		address _recipient
	) public view returns (uint256, uint256) {

		...
		PoolData storage pool = _pools[_assetType];
		if (pool.totalPoints != 0) {

			...

			// Return final shares.
			unchecked {
				uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
				uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);
				share /= _PRECISION;
				daoShare /= _PRECISION;
				return ((share - daoShare), daoShare);
			}
		}
		return (0, 0);
	}

Proof of Concept

The following steps can occur for the described scenario.

1. Staker A has staked some LP tokens, which has increased both its stakerLPPosition’s points and the pool.totalPoints to 100.
2. Staker A decides to stake more LP tokens and calls the NeoTokyoStaker.stake function, which further calls the NeoTokyoStaker.getPoolReward function, accordingly.
3. After seeing Staker A’s transaction in the mempool, Staker B, who owns lots of the LP tokens, frontruns Staker A’s transaction by calling the NeoTokyoStaker.stake function to stake many LP tokens so the pool.totalPoints is increased to 100500.
4. When Staker B’s transaction is executed, totalReward can be 1000 at this moment. When calling the NeoTokyoStaker.getPoolReward function, uint256 share = points * _PRECISION / pool.totalPoints * totalReward is evaluated to share = 100 * 1e12 / 100500 * 1000 = 995024875000, and share /= _PRECISION is evaluated to share = 995024875000 / 1e12 = 0.
5. As a result, 0 shares are minted to Staker A and the DAO’s treasury when they are entitled to some shares.

Tools Used

VSCode

Recommended Mitigation Steps

Flashbots can be used to keep the NeoTokyoStaker.stake and NeoTokyoStaker.withdraw transactions, which call the NeoTokyoStaker.getPoolReward function, away from the mempool for counteracting frontrunning.

---

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

All reactions