Neo Tokyo contest - rbserver's results

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1044-L1116 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1124-L1174 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1264-L1396

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

	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

	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

	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.

Lines of code

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

Vulnerability details

Impact

When withdrawing the staked LP tokens, the staker can divide the total staked token amount into smaller amounts and call the NeoTokyoStaker.withdraw function, which further calls the following NeoTokyoStaker._withdrawLP function, to withdraw each of such smaller token amounts. When such token amount is small enough, executing uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR in the NeoTokyoStaker._withdrawLP function can cause points to equal 0, which does not decrease the staker's lpPosition.points at all. In this situation, given some time between two NeoTokyoStaker.withdraw function calls, the staker can benefit from the unchanged lpPosition.points in each subsequent NeoTokyoStaker.withdraw function call, which eventually calls the lpPosition.getPoolReward function below. When executing uint256 share = points * _PRECISION / pool.totalPoints * totalReward in each lpPosition.getPoolReward function call, the unchanged lpPosition.points would cause a positive share to be minted to the staker while the staker should deserve 0 extra reward shares if it has withdrawn the staked LP tokens all at once and reduced its lpPosition.points to 0. As a result, the total number of reward shares minted to the staker becomes more than the staker is entitled to.

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

	function _withdrawLP () private {
		uint256 amount;
		assembly{
			amount := calldataload(0x24)
		}

		...
		_assetTransfer(LP, msg.sender, amount);

		// Update caller staking information and asset data.
		PoolData storage pool = _pools[AssetType.LP];
		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;
		}

		...
	}

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

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

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

			// Calculate the total number of points accrued to the `_recipient`.
			uint256 points;
			if (_assetType == AssetType.S1_CITIZEN) {
				...
			} else if (_assetType == AssetType.S2_CITIZEN) {
				...
			} else if (_assetType == AssetType.LP) {
				unchecked {
					points += stakerLPPosition[_recipient].points;
				}
			} else {
				revert InvalidAssetType(uint256(_assetType));
			}

			...

			// 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. The staker has staked 0.9e17 LP tokens. Since these staked tokens' end time is reached, the staker wants to withdraw them. Instead of withdrawing the 0.9e17 LP tokens all at once, it decides to withdraw 0.9e16 LP tokens at a time for ten times.
2. When the NeoTokyoStaker.withdraw function is called to withdraw 0.9e16 LP tokens, the lpPosition.getPoolReward function is eventually called. share that is (points * _PRECISION / pool.totalPoints * totalReward) / _PRECISION is minted to the staker.
3. Then, when the NeoTokyoStaker.withdraw function calls the NeoTokyoStaker._withdrawLP function, lpPosition.multiplier can be 100, and uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR is evaluated to points = 0.9e16 * 100 / 1e18 * 100 / 100 = 0. Therefore, the staker's lpPosition.points is not decreased at all.
4. After some time, when the NeoTokyoStaker.withdraw function is called again to withdraw another 0.9e16 LP tokens, the lpPosition.getPoolReward function is called again, and a positive share, which is (points * _PRECISION / pool.totalPoints * totalReward) / _PRECISION, is minted to the staker because the staker's lpPosition.points did not change and was not reduced to 0.
5. Steps 3 and 4 can be repeated until the total 0.9e17 staked LP tokens are all withdrawn. In this way, the extra reward shares, which the staker is not entitled to, are minted to the staker.
6. If the staker withdraws the 0.9e17 staked LP tokens all at once, its lpPosition.points would be reduced to 0 when the NeoTokyoStaker._withdrawLP function is called, and any subsequent lpPosition.getPoolReward function call would calculate the share to be 0 so no extra reward shares would be minted to the staker. However, such extra reward shares that the staker should not deserve are minted to the staker for the described scenario.

Tools Used

VSCode

Recommended Mitigation Steps

The NeoTokyoStaker._withdrawLP function can be updated to revert when the calculated points is 0 to ensure that the staker must withdraw enough staked LP tokens to reduce its lpPosition.points properly.

QA REPORT

[01] NeoTokyoStaker.stake AND NeoTokyoStaker.withdraw FUNCTIONS SHOULD CHECK uint8(_assetType) > 3) INSTEAD OF uint8(_assetType) > 4)

The uint8 representation of the following AssetType enum's maximum is 3. Yet, calling the NeoTokyoStaker.stake and NeoTokyoStaker.withdraw functions below would revert if uint8(_assetType) is more than 4. In other words, if uint8(_assetType) equals 4, calling these functions would not revert. This is inconsistent with the AssetType enum in which its maximum cannot be 4. To improve these sanity checks, please consider replacing uint8(_assetType) > 4) with uint8(_assetType) > 3) in these functions.

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

	enum AssetType {
		S1_CITIZEN,
		S2_CITIZEN,
		BYTES,
		LP
	}

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

	function stake (
		AssetType _assetType,
		uint256 _timelockId,
		uint256,
		uint256,
		uint256
	) external nonReentrant {

		// Validate that the asset being staked is of a valid type.
		if (uint8(_assetType) > 4) {
			revert InvalidAssetType(uint256(_assetType));
		}

		...
	}

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

	function withdraw (
		AssetType _assetType,
		uint256
	) external nonReentrant {

		/*
			Validate that the asset being withdrawn is of a valid type. BYTES may not 
			be withdrawn independently of the Citizen that they are staked into.
		*/
		if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {
			revert InvalidAssetType(uint256(_assetType));
		}

		...
	}

[02] STAKING FUNCTIONALITY IS UNAVAILABLE WHEN block.timestamp AND _pools[_assetType].rewardWindows[0].startTime ARE SAME

The comment of the following RewardWindow struct indicates that its startTime is "the time at which the daily reward activated". Yet, calling the NeoTokyoStaker.stake function below reverts when block.timestamp and _pools[_assetType].rewardWindows[0].startTime are the same though the pool would be considered as active at _pools[_assetType].rewardWindows[0].startTime. This means that the staking functionality becomes unavailable to users when block.timestamp and _pools[_assetType].rewardWindows[0].startTime are the same.

As a mitigation, please consider updating the NeoTokyoStaker.stake function to check _pools[_assetType].rewardWindows[0].startTime > block.timestamp instead of _pools[_assetType].rewardWindows[0].startTime >= block.timestamp.

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

		@param startTime The time at which the daily reward activated.
		@param reward The reward emission rate beginning at `startTime`.
	*/
	struct RewardWindow {
		uint128 startTime;
		uint128 reward;
	}

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

	function stake (
		AssetType _assetType,
		uint256 _timelockId,
		uint256,
		uint256,
		uint256
	) external nonReentrant {
		...

		// Validate that the asset being staked matches an active pool.
		if (_pools[_assetType].rewardWindows[0].startTime >= block.timestamp) {
			revert InactivePool(uint256(_assetType));
		}

		...
	}

[03] unchecked USAGE CAN EVENTUALLY BECOME UNSAFE

unchecked is used in multiple places of the codebase but can eventually become unsafe. For example, the following NeoTokyoStaker.claimReward function executes totalReward = (s1Reward + s2Reward + lpReward) in an unchecked block. If the corresponding tokens have been staked for a very long time, calling the NeoTokyoStaker.getPoolReward function can return very large numbers of reward shares; then executing s1Reward + s2Reward + lpReward in the NeoTokyoStaker.claimReward function can potentially overflow to an amount that is much smaller than it should be without reverting. In this case, such small number of BYTES token are minted to the user when the user actually is entitled to a much bigger number of BYTES token for the rewards.

As a mitigation, please consider not using unchecked in the relevant code. For example, the NeoTokyoStaker.claimReward function can be updated to not wrap totalReward = (s1Reward + s2Reward + lpReward) and totalTax = (s1Tax + s2Tax + lpTax) in the unchecked block. In addition, another function can be added and used for claiming a pre-determined maximum number of reward shares if executing s1Reward + s2Reward + lpReward ever overflows to revert in the NeoTokyoStaker.claimReward function.

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

	function claimReward (
		address _recipient
	) external returns (uint256, uint256) {
		...

		// Retrieve the `_recipient` reward share from each pool.
		(uint256 s1Reward, uint256 s1Tax) = getPoolReward(
			AssetType.S1_CITIZEN,
			_recipient
		);
		(uint256 s2Reward, uint256 s2Tax) = getPoolReward(
			AssetType.S2_CITIZEN,
			_recipient
		);
		(uint256 lpReward, uint256 lpTax) = getPoolReward(
			AssetType.LP,
			_recipient
		);

		...

		// Calculate total reward and tax.
		uint256 totalReward;
		uint256 totalTax;
		unchecked {
			totalReward = (s1Reward + s2Reward + lpReward);
			totalTax = (s1Tax + s2Tax + lpTax);
		}

		...
	}

[04] BYTES 2.0 AND BYTES 1.0 TOKENS USE SAME NAME AND SYMBOL

As shown by the following code, the name and symbol are BYTES for the BYTES 2.0 token, which are the same for the BYTES 1.0 token. This can cause confusion to users because tools like a scanner can display the same meta information for both tokens. Also, this can introduce difficulties for other protocols when integrating with this protocol.

As a mitigation, please consider updating the BYTES 2.0 token to use name and symbol that are different than BYTES.

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

contract BYTES2 is PermitControl, ERC20("BYTES", "BYTES") {

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/s1/BYTESContract.sol#L798

contract BYTESContract is Ownable, ERC20("BYTES", "BYTES") {

[05] ADDITIONAL LP TOKENS CANNOT BE STAKED WITH DIFFERENT timelockMultiplier, WHICH CAN BE INCONVENIENT TO USERS

Calling the following NeoTokyoStaker._stakeLP function reverts if stakerLPPosition[msg.sender].multiplier != timelockMultiplier is true. This means that after staking some LP tokens with a timelockMultiplier, the user cannot stake additional LP tokens with a different timelockMultiplier. In order to stake additional LP tokens with a different timelockMultiplier, the user has to wait until the timelock expires for the previously staked LP tokens and withdraw them, which can be very inconvenient. To improve the user experience, please consider adding a mechanism for staking additional LP tokens with a different timelockMultiplier.

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

	function _stakeLP (
		uint256 _timelock
	) private {
		...

		// Decode the timelock option's duration and multiplier.
		uint256 timelockDuration = _timelock >> 128;
		uint256 timelockMultiplier = _timelock & type(uint128).max;

		// If this is a new stake of this asset, initialize the multiplier details.
		if (stakerLPPosition[msg.sender].multiplier == 0) {
			stakerLPPosition[msg.sender].multiplier = timelockMultiplier;

		// If a multiplier exists already, we must match it.
		} else if (stakerLPPosition[msg.sender].multiplier != timelockMultiplier) {
			revert MismatchedTimelock();
		}

		...
	}

[06] USERS ARE UNABLE TO WITHDRAW SPECIFIED BYTES TOKEN AMOUNTS OVER TIME

When staking the BYTES tokens, users can stake various BYTES token amounts over time; however, they have to withdraw the staked BYTES tokens all at once because only the following NeoTokyoStaker._withdrawS1Citizen and NeoTokyoStaker._withdrawS2Citizen functions, which would transfer all staked BYTES tokens to msg.sender, are available. To improve the user experience, please consider adding a mechanism to support the use case for withdrawing specified BYTES token amounts over time.

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

	function _withdrawS1Citizen () private {
		...
		// Return any staked BYTES.
		if (stakedCitizen.stakedBytes > 0) {
			_assetTransfer(BYTES, msg.sender, stakedCitizen.stakedBytes);
		}
		...
		stakedCitizen.stakedBytes = 0;
		...
	}

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

	function _withdrawS2Citizen () private {
		...
		// Return any staked BYTES.
		if (stakedCitizen.stakedBytes > 0) {
			_assetTransfer(BYTES, msg.sender, stakedCitizen.stakedBytes);
		}
		...
		stakedCitizen.stakedBytes = 0;
		...
	}

[07] NeoTokyoStaker.stake FUNCTION'S COMMENT IS NOT COMPLETE FOR SITUATION WHERE ASSET BEING STAKED IS BYTES

The following comment for the NeoTokyoStaker.stake function only describes the input parameter for the situation where the asset being staked is an S1 Citizen. Yet, this input parameter is also used as the corresponding citizenId when the asset being staked is BYTES. This can confuse and mislead users for calling this function. To avoid such confusion, please consider completing this comment to explain the situation where the asset being staked is BYTES.

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

		@custom:param If the asset being staked is an S1 Citizen, this is the ID of 
			a Vault to attempt to optionally attach.

[08] FUNCTIONS IN NeoTokyoStaker CONTRACT DO NOT FOLLOW CHECKS-EFFECTS-INTERACTIONS PATTERN

The NeoTokyoStaker._stakeS1Citizen, NeoTokyoStaker._stakeS2Citizen, NeoTokyoStaker._stakeBytes, NeoTokyoStaker._stakeLP, NeoTokyoStaker._withdrawS1Citizen, NeoTokyoStaker._withdrawS2Citizen, and NeoTokyoStaker._withdrawLP functions execute the following code to transfer the corresponding tokens before updating the relevant states, which do not follow the checks-effects-interactions pattern. To reduce the potential attack surface and increase the level of security, please consider updating these functions to follow the checks-effects-interactions pattern.

contracts\staking\NeoTokyoStaker.sol
  897: 		   _assetTransferFrom(S1_CITIZEN, msg.sender, address(this), citizenId);	
  1010: 		_assetTransferFrom(S2_CITIZEN, msg.sender, address(this), citizenId);	
  1057: 		_assetTransferFrom(BYTES, msg.sender, address(this), amount);	
  1137: 		_assetTransferFrom(LP, msg.sender, address(this), amount);	
  1492: 		_assetTransferFrom(S1_CITIZEN, address(this), msg.sender, citizenId);	
  1557: 		_assetTransferFrom(S2_CITIZEN, address(this), msg.sender, citizenId);	
  1618: 		_assetTransfer(LP, msg.sender, amount);	

[09] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

To prevent unintended behaviors, critical address inputs should be checked against address(0). address(0) checks are missing for the address input variables in the following constructors. Please consider checking them.

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L75-L85

	constructor (
		address _bytes,
		address _s1Citizen,
		address _staker,
		address _treasury
	) {
		BYTES1 = _bytes;
		S1_CITIZEN = _s1Citizen;
		STAKER = _staker;
		TREASURY = _treasury;
	}

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

	constructor (
		address _bytes,
		address _s1Citizen,
		address _s2Citizen,
		address _lpToken,
		address _identity,
		address _vault,
		uint256 _vaultCap,
		uint256 _noVaultCap
	) {
		BYTES = _bytes;
		S1_CITIZEN = _s1Citizen;
		S2_CITIZEN = _s2Citizen;
		LP = _lpToken;
		IDENTITY = _identity;
		VAULT = _vault;
		...
	}

[10] VULNERABILITIES IN VERSION 4.4.2 OF @openzeppelin/contracts-upgradeable AND VERSION 4.3.1 OF @openzeppelin/contracts

As shown in the following code in package.json, version 4.4.2 of @openzeppelin/contracts-upgradeable and version 4.3.1 of @openzeppelin/contracts can be used. As described in https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts-upgradeable/4.4.2 and https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts/4.3.1, these versions have vulnerabilities that are related to ECDSA.recover, initializer, etc. To reduce the potential attack surface and be more future-proofed, please consider upgrading these packages to at least version 4.7.3.

https://github.com/code-423n4/2023-03-neotokyo/blob/main/package.json#L9-L23

    "@openzeppelin/contracts-upgradeable": "^4.4.2",
    ...
    "@openzeppelin/contracts": "^4.3.1",

[11] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 100, used in the following code with constants.

contracts\staking\NeoTokyoStaker.sol
  946: 		uint256 vaultMultiplier = 100;
  962: 		uint256 timelockDuration = _timelock >> 128;
  1016: 	uint256 timelockDuration = _timelock >> 128;
  1022: 	citizenStatus.points = 100 * timelockMultiplier / _DIVISOR;
  1077: 	uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
  1098: 	uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
  1113: 	(seasonId << 128) + citizenId,
  1140: 	uint256 timelockDuration = _timelock >> 128;
  1155: 	uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
  1389: 	uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);

[12] HARDCODED STRINGS THAT HAVE SPECIAL MEANINGS CAN BE REPLACED WITH CONSTANTS

For better maintainability, the following Hand of Citadel and ? that are hardcoded and have special meanings can be replaced with constants.

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

			if (_stringEquals(class, "Hand of Citadel")) {
				vaultMultiplier = vaultCreditMultiplier["?"];

[13] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragmas for the following files.

contracts\staking\BYTES2.sol
  2: pragma solidity ^0.8.19;

contracts\staking\NeoTokyoStaker.sol
  2: pragma solidity ^0.8.19;

[14] WORD TYPING TYPO

( Please note that the following instance is not found in https://gist.github.com/Picodes/01427c59b07c651699136589541159a7#nc-1-typos. )

ctiizen can be changed to citizen in the following comment. https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L533-L535

		@param timelockOption Data encoding the parameters surrounding the timelock 
			option used in staking the particular asset. Alternatively, this encodes 
			ctiizen information for BYTES staking.

[15] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following function misses the @return comments. Please consider completing the NatSpec comments for this function.

contracts\staking\NeoTokyoStaker.sol
  1264: 	function getPoolReward (

[16] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following function misses NatSpec comments. Please consider adding NatSpec comments for this function.

contracts\staking\NeoTokyoStaker.sol
  1044: 	function _stakeBytes (