Neo Tokyo contest - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] Missing 0 address checks in the constructor

The constructor in BYTES2 is missing 0 address checks. Allowing zero-addresses can lead to contract reverts and force redeployments if there are no setters for such address variables. This counts for BYTES1 and S1_CITIZEN, which are both immutable.

The same applies to the constructor in NeoTokyoStaker.sol#L588-L606.

[L-02] identityCreditYield returns empty string if one combination is missing

The identityCreditYields gets supplied in the function configureIdentityCreditYields(). However the admin must supply all possible combinations for citizenRewards and vaultRewards. If one combination is missing the function getCreditYield() will return an empty string. There are 105 combinations so it's definitely possible to miss one.

An example: IdentityCreditYields put in by the admin:

• identityCreditYield[5]["LOW"] = "LOW"
• identityCreditYield[5]["HIGH"] = "MEDIUM"
• identityCreditYield[10]["VERY HIGH"] = "HIGH"

When user stakes a citizen, getCreditYield() gets called.

Good scenario:

• rewardRate is 5 and and vaultMultiplier is HIGH: getCreditYield() returns "MEDIUM"

Bad scenario:

• rewardRate is 4 and vaultMultiplier is HIGH: getCreditYield() returns an empty string

When the function returns an empty string, this will result in StakedS1Citizen having 0 points.

I put this as low because it's an admin mistake if this happens and the identityCreditYields can always get resupplied. However the first few users will be disadvantaged by this a lot. Because this will mean that the staked Citizen will get no rewards. I suggest to have an extra require check in the stake function to check if the returned string is not empty.

L938:
		// Determine the base worth in points of the S1 Citizen's Identity.
		string memory citizenCreditYield = getCreditYield(
			citizenId,
			citizenVaultId
		);
+       require (!_stringEquals(memory, ""), "Credit Yield is empty string");
		uint256 identityPoints = identityCreditPoints[citizenCreditYield];

The same applies to vaultCreditMultiplier but this has a much lower chance because it only has 6 values.

[L-03] Function shouldn't revert when class is not equal to Hand of Citadel

If a user attempts to claim a Hand of The Citadel bonus it will put 1 as last parameter of the stake() function. If the handClaimant parameter is 1 and isn't equal to "Hand of Citadel" the transaction will revert. It's better to get the real vaultMultiplier instead of reverting. This way when a sets the parameter by accident to 1, the transaction will still succeed.

NeoTokyoStaker.sol#L947-L959

		uint256 vaultMultiplier = 100;
		if (handClaimant == 1) {
			uint256 identityId = citizen.getIdentityIdOfTokenId(citizenId);
			string memory class = IGenericGetter(IDENTITY).getClass(identityId);
			if (_stringEquals(class, "Hand of Citadel")) {
				vaultMultiplier = vaultCreditMultiplier["?"];
-           } else {
+			} else if (vaultId != 0) {
-				revert CitizenIsNotHand(citizenId);
+			    vaultMultiplier = getConfiguredVaultMultiplier(vaultId);
			}

		// Otherwise use the configured Vault multiplier, if any.
		} else if (vaultId != 0) {
			vaultMultiplier = getConfiguredVaultMultiplier(vaultId);
		}

[L-04] No guarantee that timelock and timelockMultiplier is always the same

When you stake LP tokens, the function checks if the current multiplier is equal to the new multiplier. However there is no guarantee that the duration is still the same due to a mistake when configuring the timelock options. This will mean that the staker will have the same multiplier but maybe a longer or shorter duration. This can happen because the duration of the timelock is in its upper 128 bits and the multiplier is in the lower 128 bits. To fix this the duration should also be saved to the stakerLPPosition and added into the if check.

NeoTokyoStaker.sol#L1143-L1150

		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();
		}

[L-05] Maximum value of AssetType is 3

When you want to stake or withdraw an asset you have the pass a parameter AssetType. This is an enum with 4 values. The first item of the enum always has index 0. So this enum has a maximum index of 3. However the transaction will only revert if the enum index is bigger than 4. This should be changed to >=4 or >3. NeoTokyoStaker.sol#L1205

		if (uint8(_assetType) > 4) {
			revert InvalidAssetType(uint256(_assetType));
		}

NeoTokyoStaker.sol#L1668-L1670

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

[L-06] Staking gets more expensive the more assets you have staked

When you stake, the function stake() calls getReward(). GetReward will by his turn call claimReward().

When you claim it will iterate over all your staked assets. The more staked assets you have, the more gas it cost. A whale with 50 staked assets can easily pay double in gas when he calls stake(). A solution to this is to not call getReward() in the stake function and let the user claim his rewards whenever he want.

Non critical

[N-01] 100 / 100 = 1

When you stake a season 2 citizen, the points are calculated as follow: 100 * timelockMultiplier / _DIVISOR. Divisor is also 100 so it's useless do to all the calculations.

NeoTokyoStaker.sol#L1022

		unchecked {
-			citizenStatus.points = 100 * timelockMultiplier / _DIVISOR;
+			citizenStatus.points = timelockMultiplier;
			citizenStatus.timelockEndTime = block.timestamp + timelockDuration;

			// Record the caller's staked S2 Citizen.
			_stakerS2Position[msg.sender].push(citizenId);

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

[N-02] Duplicated code

getCreditYield() and getConfiguredVaultMultiplier() use the same code to get the vaultMultiplier. A small code cleanup can be done by making a seperate function.

[N-03] Unnecessary if statements

Code can be made cleaner if you remove these useless if checks

[N-04] Constant values such as a call to keccak256(), should be immutable rather than constant

Constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

• BYTES2.sol#L37
• BYTES2.sol#L40
• NeoTokyoStaker.sol#L206-L220

[N-05] Missing checks for zero address

Some functions are missing a zero address check. This is not an issue but it will do the whole process for a 0 address and will unneccesary waste gas. This can be prevented by having a 0 address check.

• BYTES2.sol#L114-L129

Summary

Details

[G-01] Save to memory first when you plan to use it later

When you use a variable twice, save it first to memory so you can read twice from memory instead of storage. This saves 1 sload which is 100 gas. With the other calculations substracted this saves on average 40 gas. The more you need to read from memory, the more gas you will save.

NeoTokyoStaker.sol#L967-L978

		unchecked {
-			citizenStatus.points =
-				identityPoints * vaultMultiplier * timelockMultiplier /
-				_DIVISOR / _DIVISOR;
+			uint256 points = identityPoints * vaultMultiplier * timelockMultiplier /
+				_DIVISOR / _DIVISOR;
+			citizenStatus.points = points;
			citizenStatus.timelockEndTime = block.timestamp + timelockDuration;

			// Record the caller's staked S1 Citizen.
			_stakerS1Position[msg.sender].push(citizenId);

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

The same can also be done for staking a season 2 citizen NeoTokyoStaker.sol#L1154-L1165

[G-02] if or require statements that can revert should be at the top of the function

This is to prevent the function from doing all sort of calculations when it's destined to revert and hereby save gas.

[G-03] Using double if/require statement instead of && consumes less gas

Saves around 15 gas for require statements and 40 for if statements

-	if (j != 0 && _inputs[i].rewardWindows[j].startTime <= lastTime) {
+   if (j!=0) {
+       if ( _inputs[i].rewardWindows[j].startTime <= lastTime){
            revert RewardWindowTimesMustIncrease();
-       } 
    }

Also used at:

• NeoTokyoStaker.sol#L910
• NeoTokyoStaker.sol#L917
• NeoTokyoStaker.sol#L926

[G-04] Unnecessary if statements

[G-05] IGenericGetter can be made immutable

+  	IGenericGetter immutable public vault;
+	IGenericGetter immutable public citizen;

L588
    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;
		VAULT_CAP = _vaultCap;
		NO_VAULT_CAP = _noVaultCap;
+		vault = IGenericGetter(VAULT);
+		citizen = IGenericGetter(S1_CITIZEN);
	}

The rest of the IGenericGetter initializations can be removed. Around 40 gas saved with this optimization. It also makes the code a bit cleaner.

[G-06] LastRewardTime doesn't need to be a mapping

When a user claims the reward, lastRewardTime is set the same for all asset types. So a nested mapping is unnecessary and it can be a single mapping.

This optimization saves two cold loads because we now access 3 times the same value so we only have to perform one cold load and then 2 hot loads. Also 2 sloads are saved because we only need to write the current timestamp to storage once.

When I run the tests:

• stake(): avg gas saved: 26202
• withdraw(): avg gas saved: 9884

L319:
-	mapping ( address => mapping ( AssetType => uint256 )) public lastRewardTime;
+	mapping ( address => uint256) public lastRewardTime;

L1310:
-	uint256 lastPoolRewardTime = lastRewardTime[_recipient][_assetType];
+	uint256 lastPoolRewardTime = lastRewardTime[_recipient];

L1433:
-	lastRewardTime[_recipient][AssetType.S1_CITIZEN] = block.timestamp;
-	lastRewardTime[_recipient][AssetType.S2_CITIZEN] = block.timestamp;
-	lastRewardTime[_recipient][AssetType.LP] = block.timestamp;
+   lastRewardTime[_recipient]= block.timestamp;

[G-07] Don't call getReward() when staking

It's extremely expensive if you always call getReward() when you stake. This includes a lot of iterations and 2 mints to the caller and the DAO. Almost 80k gas can be saved when you let the user call getReward() whenever he wants and not do it when he stakes an asset.