Neo Tokyo contest - nadin's results

Low Risk Issues List

Issue

[L-01] Gas griefing/theft is possible on unsafe external call

Total: 01 contexts over 01 issues

[L-01] Gas griefing/theft is possible on unsafe external call

return data (bool success,) has to be stored due to EVM architecture, if in a usage like below, ‘out’ and ‘outsize’ values are given (0,0) . Thus, this storage disappears and may come from external contracts a possible Gas griefing/theft problem is avoided https://twitter.com/pashovkrum/status/1607024043718316032?t=xs30iD6ORWtE2bTTYsCFIQ&s=19

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

		(bool success, bytes memory data) = 
			_asset.call(
				abi.encodeWithSelector(
					_TRANSFER_FROM_SELECTOR,
					_from,
					_to, 
					_idOrAmount
				)
			);

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

Non-Critical Issues List

Issue

[N-01] Use a single file for all system-wide constants

[N-02] Need Fuzzing test

[N-03] Function writing that does not comply with the Solidity Style Guide

[N-04] For modern and more readable code; update import usages

[N-05] Include return parameters in NatSpec comments

[N-06] Upgradable openzeppelin contracts are recommended

[N-07] LOCK PRAGMAS TO SPECIFIC COMPILER VERSION

[N-08] PRAGMA VERSION^0.8.19 VERSION TOO RECENT TO BE TRUSTED.

[N-09] MISSING EVENT FOR CRITICAL PARAMETERS INIT AND CHANGE

[N-10] Note that using Assembly removes several important security features of Solidity, which can make the code more insecure and more error-prone.

Total: 56 contexts over 10 issues

[N-01] Use a single file for all system-wide constants

Context: 14

There are many addresses and constants used in the system. It is recommended to put the most used ones in one file (for example constants.sol, use inheritance to access these values).

This will help with readability and easier maintenance for future changes. This also helps with any issues, as some of these hard-coded values are admin addresses.

constants.sol

Use and import this file in contracts that require access to these values. This is just a suggestion, in some use cases this may result in higher gas usage in the distribution.

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

37:	bytes32 public constant BURN = keccak256("BURN");
40: 	bytes32 public constant ADMIN = keccak256("ADMIN");

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

191:	bytes4 constant private _TRANSFER_FROM_SELECTOR = 0x23b872dd;
194:	bytes4 constant private _TRANSFER_SELECTOR = 0xa9059cbb;
197:	uint256 constant private _PRECISION = 1e12;
200:	uint256 constant private _DIVISOR = 100;
203:	uint256 constant private _BYTES_PER_POINT = 200 * 1e18;
206:	bytes32 public constant CONFIGURE_LP = keccak256("CONFIGURE_LP");
209:	bytes32 public constant CONFIGURE_TIMELOCKS = keccak256(
214:	bytes32 public constant CONFIGURE_CREDITS = keccak256("CONFIGURE_CREDITS");
217: 	bytes32 public constant CONFIGURE_POOLS = keccak256("CONFIGURE_POOLS");
220:	bytes32 public constant CONFIGURE_CAPS = keccak256("CONFIGURE_CAPS");

[N-02] Need Fuzzing test

Context: 30

Description

In total 2 contracts, 30 unchecked are used, the functions used are critical. For this reason, there must be fuzzing tests in the tests of the project. Not seen in tests.

Recommendation

Use should fuzzing test like Echidna. As Alberto Cuesta Canada said: Fuzzing is not easy, the tools are rough, and the math is hard, but it is worth it. Fuzzing gives me a level of confidence in my smart contracts that I didn’t have before. Relying just on unit testing anymore and poking around in a testnet seems reckless now. https://medium.com/coinmonks/smart-contract-fuzzing-d9b88e0b0a05

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

151:		unchecked {

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

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

728:			unchecked { i++; }
743:			unchecked { i++; }
967:		unchecked {
1021:		unchecked {
1076:			unchecked {
1097:			unchecked {
1154:		unchecked {
1282:					unchecked {
1291:					unchecked {
1297:				unchecked {
1330:							unchecked {
1341:							unchecked {
1354:								unchecked {
1369:						unchecked { j++; }
1377:					unchecked {
1383:				unchecked { i++; }
1387:			unchecked {
1440:		unchecked {
1509:			unchecked { stakedIndex++; }
1514:		unchecked {
1574:			unchecked { stakedIndex++; }
1579:		unchecked {
1622:		unchecked {
1744:			unchecked { ++i; }
1771:			unchecked { ++i; }
1789:			unchecked { ++i; }
1808:			unchecked { ++i; }
1838:				unchecked { j++; }
1840:			unchecked { ++i; }

[N-03] Function writing that does not comply with the Solidity Style Guide

Context: 01

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

Description

Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

Functions should be grouped according to their visibility and ordered:

constructor receive function (if exists) fallback function (if exists) external public internal private within a grouping, place the view and pure functions last

[N-04] For modern and more readable code; update import usages

Context: 02

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

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

Description:

Solidity code is also cleaner in another way that might not be noticeable: the struct Point. We were importing it previously with global import but not using it. The Point struct polluted the source code with an unnecessary object we were not using because we did not need it. This was breaking the rule of modularity and modular programming: only import what you need Specific imports with curly braces allow us to apply this rule better.

Recommendation:

import {contract1 , contract2} from "filename.sol"; A good example from the ArtGobblers project;

import {Owned} from "solmate/auth/Owned.sol";
import {ERC721} from "solmate/tokens/ERC721.sol";
import {LibString} from "solmate/utils/LibString.sol";
import {MerkleProofLib} from "solmate/utils/MerkleProofLib.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {ERC1155, ERC1155TokenReceiver} from "solmate/tokens/ERC1155.sol";
import {toWadUnsafe, toDaysWadUnsafe} from "solmate/utils/SignedWadMath.sol";

[N-05] Include return parameters in NatSpec comments

Context: 02

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

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

Description

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

https://docs.soliditylang.org/en/v0.8.15/natspec-format.html

Recommendation

Include return parameters in NatSpec comments

[N-06] Upgradable openzeppelin contracts are recommended

Context: 03

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

4: import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L4-L5

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

4: import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

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

Recommendation

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts/token/ERC20 https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/security/ReentrancyGuardUpgradeable.sol

[N-07] LOCK PRAGMAS TO SPECIFIC COMPILER VERSION

Context: 02

01: https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L2 02: https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L2

Description:

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally. https://swcregistry.io/docs/SWC-103

Recommendation:

Ethereum Smart Contract Best Practices - Lock pragmas to specific compiler version. solidity-specific/locking-pragmas

[N-08] PRAGMA VERSION^0.8.19 VERSION TOO RECENT TO BE TRUSTED.

https://github.com/ethereum/solidity/blob/develop/Changelog.md Unexpected bugs can be reported in recent versions; Risks related to recent releases Risks of complex code generation changes Risks of new language features Risks of known bugs Use a non-legacy and more battle-tested version

[N-09] MISSING EVENT FOR CRITICAL PARAMETERS INIT AND CHANGE

Context: 02

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

	function changeStakingContractAddress (
		address _staker
	) hasValidPermit(UNIVERSAL, ADMIN) external {
		STAKER = _staker;
	}

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L162-L166

	function changeTreasuryContractAddress (
		address _treasury
	) hasValidPermit(UNIVERSAL, ADMIN) external {
		TREASURY = _treasury;
	}

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L173-L177

Description:

Events help non-contract tools to track changes, and events prevent users from being surprised by changes

Recommendation:

Add Event-Emit

[N-10] Note that using Assembly removes several important security features of Solidity, which can make the code more insecure and more error-prone.

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

Suggestion

Issue

[S-01] Project Upgrade and Stop Scenario should be

[S-02] GENERATE PERFECT CODE HEADERS EVERY TIME

Total: 02 issues

[S-01] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an ” EMERGENCY STOP (CIRCUIT BREAKER) PATTERN “. https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[S-02] GENERATE PERFECT CODE HEADERS EVERY TIME

Description:

I recommend using header for Solidity code layout and readability https://github.com/transmissions11/headers

/*//////////////////////////////////////////////////////////////
                           TESTING 123
//////////////////////////////////////////////////////////////*/

Gas Optimizations

Issue

[G-01] Avoid using state variable in emit

[G-02] Gas savings can be achieved by changing the model for assigning value to the structure

[G-03] Use nested if and, avoid multiple check combinations

[G-04] Use assembly to write address storage values

[G-05] Setting the constructor to payable

[G-06] Upgrade Solidity’s optimizer

[G-07] Optimize names to save gas

[G-08] Using delete instead of setting stakedCitizen struct to 0 saves gas

[G-09] Empty blocks should be removed or emit something

[G-10] Use constants instead of type(uintx).max

[G-11] x += y (x -= y) costs more gas than x = x + y (x = x - y) for state variables

Total: 58 contexts over 11 issues

[G-01] Avoid using state variable in emit

Context: 01

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

Using a state variable in Claim emits wastes gas.

		emit Claim (
			_recipient,
			totalReward,
			totalTax
		);

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

[G-02] Gas savings can be achieved by changing the model for assigning value to the structure

Description:

By changing the pattern of assigning value to the structure, gas savings of ~130 per instance are achieved. In addition, this use will provide significant savings in distribution costs.

Context: 02

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

			stakedS1Details[i] = StakedS1CitizenOutput({
				citizenId: citizenId,
				stakedBytes: citizenDetails.stakedBytes,
				timelockEndTime: citizenDetails.timelockEndTime,
				points: citizenDetails.points,
				hasVault: citizenDetails.hasVault,
				stakedVaultId: citizenDetails.stakedVaultId
			});

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

			stakedS2Details[i] = StakedS2CitizenOutput({
				citizenId: citizenId,
				stakedBytes: citizenDetails.stakedBytes,
				timelockEndTime: citizenDetails.timelockEndTime,
				points: citizenDetails.points
			});

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

Recommendation:

The following model, which is more gas efficient, can be preferred to assign value to the building elements. Ex:

+ stakedS1Details[i] .citizenId = citizenId
+ ...

[G-03] Use nested if and, avoid multiple check combinations

Context: 04

Description:

Using nested is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.

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

910:		if (citizenVaultId != 0 && vaultId != 0) {
917:		} else if (citizenVaultId != 0 && vaultId == 0) {
926:		} else if (citizenVaultId == 0 && vaultId != 0) {
1834:				if (j != 0 && _inputs[i].rewardWindows[j].startTime <= lastTime) {

[G-04] Use assembly to write address storage values

Context: 10

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

	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/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L75-L84

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

	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;

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

[G-05] Setting the constructor to payable

Context: 02

Description:

You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 13 gas on deployment with no security risks.

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

75:	constructor (

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

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

588:	constructor (

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

[G-06] Upgrade Solidity’s optimizer

Context: 02

Description:

Make sure Solidity’s optimizer is enabled. It reduces gas costs. If you want to gas optimize for contract deployment (costs less to deploy a contract) then set the Solidity optimizer at a low number. If you want to optimize for run-time gas costs (when functions are called on a contract) then set the optimizer to a high number. Set the optimization value higher than 800 in your hardhat.config.js file. https://github.com/code-423n4/2023-03-neotokyo/blob/main/hardhat.config.js

33:						runs: 200, 
45:						runs: 200, 

[G-07] Optimize names to save gas

Description:

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas. https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

[G-08] Using delete instead of setting stakedCitizen struct to 0 saves gas

Contexts: 07

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

1517:		stakedCitizen.stakedBytes = 0;
1518:		stakedCitizen.timelockEndTime = 0;
1519:		stakedCitizen.points = 0;
1521:		stakedCitizen.stakedVaultId = 0;
1582:		stakedCitizen.stakedBytes = 0;
1583:		stakedCitizen.timelockEndTime = 0;
1584:		stakedCitizen.points = 0;

[G-09] Empty blocks should be removed or emit something

Contexts: 02

Description:

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}). Empty receive()/fallback() payable functions that are not used, can be removed to save deployment gas.

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

189:	function updateReward (
204:	function updateRewardOnMint (

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L189-L194 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L204-L209

[G-10] Use constants instead of type(uintx).max

Contexts: 06

Description:

type(uint128).max or type(uint256).max, etc. it uses more gas in the distribution process and also for each transaction than constant usage.

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

963:		uint256 timelockMultiplier = _timelock & type(uint128).max;
1017:		uint256 timelockMultiplier = _timelock & type(uint128).max;
1141:		uint256 timelockMultiplier = _timelock & type(uint128).max;
1206:			revert InvalidAssetType(uint256(_assetType));
1301:				revert InvalidAssetType(uint256(_assetType));
1669:			revert InvalidAssetType(uint256(_assetType));

[G-11] x += y (x -= y) costs more gas than x = x + y (x = x - y) for state variables

Contexts: 22

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

977:			pool.totalPoints += citizenStatus.points;
1029:			pool.totalPoints += citizenStatus.points;
1078:				citizenStatus.stakedBytes += amount;
1079:				citizenStatus.points += bonusPoints;
1080:				pool.totalPoints += bonusPoints;
1099:				citizenStatus.stakedBytes += amount;
1100:				citizenStatus.points += bonusPoints;
1101:				pool.totalPoints += bonusPoints;
1160:			stakerLPPosition[msg.sender].amount += amount;
1161:			stakerLPPosition[msg.sender].points += points;
1164:			pool.totalPoints += points;
1283:						points += s1Citizen.points;
1292:						points += s2Citizen.points;
1298:                 points += stakerLPPosition[_recipient].points;
1332:		totalReward += currentRewardRate * timeSinceReward;	
1343:		totalReward += currentRewardRate * timeSinceReward;
1357:                           totalReward += currentRewardRate * timeSinceReward;
1515:			pool.totalPoints -= stakedCitizen.points;
1580:			pool.totalPoints -= stakedCitizen.points;
1626:			lpPosition.amount -= amount;
1627:			lpPosition.points -= points;
1620:			pool.totalPoints -= points;