Neo Tokyo contest - Rolezn's results

Summary<a name="Summary">

Low Risk Issues

Total: 21 contexts over 7 issues

Non-critical Issues

Total: 56 contexts over 14 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> Add to blacklist function

NFT thefts have increased recently, so with the addition of hacked NFTs to the platform, NFTs can be converted into liquidity. To prevent this, I recommend adding the blacklist function.

Marketplaces such as Opensea have a blacklist feature that will not list NFTs that have been reported theft, NFT projects such as Manifold have blacklist functions in their smart contracts.

Here is the project example; Manifold

Manifold Contract https://etherscan.io/address/0xe4e4003afe3765aca8149a82fc064c0b125b9e5a#code

     modifier nonBlacklistRequired(address extension) {
         require(!_blacklistedExtensions.contains(extension), "Extension blacklisted");
         _;
     }

<ins>Recommended Mitigation Steps</ins>

Add to Blacklist function and modifier.

<a href="#Summary">[LOW‑2]</a><a name="LOW&#x2011;2"> Function can risk gas exhaustion on large receipt calls due to multiple mandatory loops

The function contains loops over arrays that the user cannot influence. In any call for the function, users spend gas to iterate over the array. There are loops in the following functions that iterate all array values. Which could risk gas exhaustion on large array calls.

This risk is small, but could be lessened somewhat by allowing separate calls for small loops. Consider allowing partial calls via array arguments, or detecting expensive call bundles and only partially iterating over them. Since the logic already filters out calls, this would make the later loops smaller.

<ins>Proof Of Concept</ins>

1459: function _withdrawS1Citizen () private {
        ...
        uint256[] storage oldPosition = _stakerS1Position[msg.sender];
        for (uint256 stakedIndex; stakedIndex < oldPosition.length; ) {

            
            if (citizenId == oldPosition[stakedIndex]) {
                if (stakedIndex != oldPosition.length - 1) {
                    oldPosition[stakedIndex] = oldPosition[oldPosition.length - 1];
                }
                oldPosition.pop();
                break;
            }
            unchecked { stakedIndex++; }
        }
        ...
    }

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

<a href="#Summary">[LOW‑3]</a><a name="LOW&#x2011;3"> Possible rounding issue

Division by large numbers may result in the result not rounding properlly, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator

<ins>Proof Of Concept</ins>

152: treasuryShare = _amount * 2 / 3;

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

<a href="#Summary">[LOW‑4]</a><a name="LOW&#x2011;4"> Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

<ins>Proof of Concept</ins>

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

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

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

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

<ins>Recommended Mitigation Steps</ins>

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts. See https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/tree/master/contracts for list of available upgradeable contracts

<a href="#Summary">[LOW‑6]</a><a name="LOW&#x2011;6"> Upgrade OpenZeppelin Contract Dependency

An outdated OZ version is used (which has known vulnerabilities, see: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).

<ins>Proof Of Concept</ins>

Require dependencies to be at least version of 4.8.1 to include critical vulnerability patches.

    "@openzeppelin/contracts-upgradeable": "^4.4.2"

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

    "@openzeppelin/contracts": "^4.3.1"

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

<ins>Recommended Mitigation Steps</ins>

Update OpenZeppelin Contracts Usage in package.json and require at least version of 4.8.1 via >=4.8.1

<a href="#Summary">[LOW‑7]</a><a name="LOW&#x2011;7"> Missing Non Reentrancy Modifiers

The claimReward is missing nonrenonReentrantentrant modifier although some other public/external functions does use the nonReentrant modifer such as stake and withdraw. It seems like the claimReward function should have the nonReentrant modifier as the other functions have it as well.

<ins>Proof Of Concept</ins>

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

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

Non Critical Issues

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> Avoid Floating Pragmas: The Version Should Be Locked

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

<ins>Proof Of Concept</ins>

Found usage of floating pragmas ^0.8.19 of Solidity in [BYTES2.sol]

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

Found usage of floating pragmas ^0.8.19 of Solidity in [NeoTokyoStaker.sol]

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

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> Variable names that consist of all capital letters should be reserved for constant/immutable variables

Variable names with all capital letters should be restricted to be used only with constant or immutable variables. If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like <a href="https://github.com/OpenZeppelin/openzeppelin-contracts/blob/76eee35971c2541585e05cbf258510dda7b2fbc6/contracts/token/ERC20/extensions/draft-IERC20Permit.sol#L59">this</a>).

<ins>Proof Of Concept</ins>

49: address public STAKER;

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

52: address public TREASURY;

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

232: address public LP;

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

249: uint256 public VAULT_CAP;

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

255: uint256 public NO_VAULT_CAP;

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

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> Constants Should Be Defined Rather Than Using Magic Numbers

<ins>Proof Of Concept</ins>

1022: citizenStatus.points = 100 * timelockMultiplier / _DIVISOR;

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

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

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

```solidity
1623: uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;

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

<a href="#Summary">[NC‑6]</a><a name="NC&#x2011;6"> Constants in comparisons should appear on the left side

Doing so will prevent <a href="https://www.moserware.com/2008/01/constants-on-left-are-better-but-this.html">typo bugs</a>

<ins>Proof Of Concept</ins>

633: if (rewardRate == 0) {

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

1144: if (stakerLPPosition[msg.sender].multiplier == 0) {

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

1221: if (timelockOption == 0) {

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

1472: if (stakedCitizen.timelockEndTime == 0) {

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

1547: if (stakedCitizen.timelockEndTime == 0) {

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

<a href="#Summary">[NC‑7]</a><a name="NC&#x2011;7"> Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function

Saves deployment costs

<ins>Proof Of Concept</ins>

784: revert(string(data));
812: revert(string(data));

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

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

<a href="#Summary">[NC‑8]</a><a name="NC&#x2011;8"> Function writing that does not comply with the Solidity Style Guide

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

<ins>Proof Of Concept</ins>

• NeoTokyoStaker.sol

<a href="#Summary">[NC‑9]</a><a name="NC&#x2011;9"> Use delete to Clear Variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic. Consider replacing assignments of zero with delete statements.

<ins>Proof Of Concept</ins>

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

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

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

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

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

1582: stakedCitizen.stakedBytes = 0;
1583: stakedCitizen.timelockEndTime = 0;
1584: stakedCitizen.points = 0;

https://github.com/code-423n4/2023-03-neotokyo/tree/main/contracts/staking/NeoTokyoStaker.sol#L1582-L1584

1635: lpPosition.multiplier = 0;

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

<a href="#Summary">[NC‑10]</a><a name="NC&#x2011;10"> NatSpec return parameters should be included in contracts

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

<ins>Proof Of Concept</ins>

• NeoTokyoStaker.sol

<ins>Recommended Mitigation Steps</ins>

Include return parameters in NatSpec comments

Recommendation Code Style: (from Uniswap3)

    /// @notice Adds liquidity for the given recipient/tickLower/tickUpper position
    /// @dev The caller of this method receives a callback in the form of IUniswapV3MintCallback#uniswapV3MintCallback
    /// in which they must pay any token0 or token1 owed for the liquidity. The amount of token0/token1 due depends
    /// on tickLower, tickUpper, the amount of liquidity, and the current price.
    /// @param recipient The address for which the liquidity will be created
    /// @param tickLower The lower tick of the position in which to add liquidity
    /// @param tickUpper The upper tick of the position in which to add liquidity
    /// @param amount The amount of liquidity to mint
    /// @param data Any data that should be passed through to the callback
    /// @return amount0 The amount of token0 that was paid to mint the given amount of liquidity. Matches the value in the callback
    /// @return amount1 The amount of token1 that was paid to mint the given amount of liquidity. Matches the value in the callback
    function mint(
        address recipient,
        int24 tickLower,
        int24 tickUpper,
        uint128 amount,
        bytes calldata data
    ) external returns (uint256 amount0, uint256 amount1);

For example see NeoTokyoStaker.getPoolReward()

    /**
        This function supports retrieving the reward and tax earned by a particular 
        `_recipient` on a specific pool of type `_assetType`.

        @param _assetType The type of the asset to calculate rewards for.
        @param _recipient The recipient of the reward.
    */
    function getPoolReward (
        AssetType _assetType,
        address _recipient
    ) public view returns (uint256, uint256) {

<a href="#Summary">[NC‑11]</a><a name="NC&#x2011;11"> NatSpec comments should be increased in contracts

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

<ins>Proof Of Concept</ins>

• NeoTokyoStaker.sol

For example see NeoTokyoStaker._stakeBytes()

    /**
        A private function for managing the staking of BYTES into a Citizen.
    */
    function _stakeBytes (
        uint256
    ) private {

<ins>Recommended Mitigation Steps</ins>

NatSpec comments should be increased in contracts

<a href="#Summary">[NC‑12]</a><a name="NC&#x2011;12"> Non-usage of specific imports

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files

A good example:

import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
import {SafeCastLib} from "solmate/utils/SafeCastLib.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";
import {IProducer} from "src/interfaces/IProducer.sol";
import {GlobalState, UserState} from "src/Common.sol";

<ins>Proof Of Concept</ins>

7: import "../access/PermitControl.sol";
8: import "../interfaces/IByteContract.sol";
9: import "../interfaces/IStaker.sol";

https://github.com/code-423n4/2023-03-neotokyo/tree/main/contracts/staking/BYTES2.sol#L7-L9

6: import "../access/PermitControl.sol";
7: import "../interfaces/IByteContract.sol";
8: import "../interfaces/IGenericGetter.sol";

https://github.com/code-423n4/2023-03-neotokyo/tree/main/contracts/staking/NeoTokyoStaker.sol#L6-L8

<ins>Recommended Mitigation Steps</ins>

Use specific imports syntax per solidity docs recommendation.

<a href="#Summary">[NC‑13]</a><a name="NC&#x2011;13"> Empty blocks should be removed or emit something

<ins>Proof Of Concept</ins>

193: ) external {
    }
207: ) external {
    }

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

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

1250: default {}
1693: default {}

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

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

<a href="#Summary">[NC‑14]</a><a name="NC&#x2011;14"> Use private constant consistently

Replace constant private with private constant.

<ins>Proof Of Concept</ins>

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;

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

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

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

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

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

Summary<a name="Summary">

Gas Optimizations

Total: 52 contexts over 14 issues

Gas Optimizations

<a href="#Summary">[GAS‑1]</a><a name="GAS&#x2011;1"> Use calldata instead of memory for function parameters

In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

contract C {
	function add(uint[] memory arr) external returns (uint sum) {
		uint length = arr.length;
		for (uint i = 0; i < arr.length; i++) {
		    sum += arr[i];
		}
	}
}

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

contract C {
	function add(uint[] calldata arr) external returns (uint sum) {
		uint length = arr.length;
		for (uint i = 0; i < arr.length; i++) {
		    sum += arr[i];
		}
	}
}

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

Examples Note: The following pattern is prevalent in the codebase:

function f(bytes memory data) external {
	(...) = abi.decode(data, (..., types, ...));
}

Here, changing to bytes calldata will decrease the gas. The total savings for this change across all such uses would be quite significant.

<ins>Proof Of Concept</ins>

function getStakerPosition (
		address _staker,
		AssetType _assetType
	) external view returns (uint256[] memory) {

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

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

<ins>Proof Of Concept</ins>

203: uint256 constant private _BYTES_PER_POINT = 200 * 1e18;

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

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> Duplicated require()/revert() Checks Should Be Refactored To A Modifier Or Function

Saves deployment costs

<ins>Proof Of Concept</ins>

784: revert(string(data));
812: revert(string(data));

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

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

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> Empty Blocks Should Be Removed Or Emit Something

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{...}})

<ins>Proof Of Concept</ins>

1250: default {}

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

1693: default {}

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

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> Using delete statement can save gas

<ins>Proof Of Concept</ins>

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

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

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

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

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

1582: stakedCitizen.stakedBytes = 0;
1583: stakedCitizen.timelockEndTime = 0;
1584: stakedCitizen.points = 0;

https://github.com/code-423n4/2023-03-neotokyo/tree/main/contracts/staking/NeoTokyoStaker.sol#L1582-L1584

1635: lpPosition.multiplier = 0;

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

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> Use assembly to write address storage values

<ins>Proof Of Concept</ins>

1231: _b = _stakeBytes;

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

1232: _lp = _stakeLP;

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

1678: _lp = _withdrawLP;

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

<a href="#Summary">[GAS‑7]</a><a name="GAS&#x2011;7"> Use hardcode address instead address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry's script.sol and solmate's LibRlp.sol contracts can help achieve this.

References: https://book.getfoundry.sh/reference/forge-std/compute-create-address

https://twitter.com/transmissions11/status/1518507047943245824

<ins>Proof Of Concept</ins>

897: _assetTransferFrom(S1_CITIZEN, msg.sender, address(this), citizenId);
927: _assetTransferFrom(VAULT, msg.sender, address(this), vaultId);

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

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

1010: _assetTransferFrom(S2_CITIZEN, msg.sender, address(this), citizenId);

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

1057: _assetTransferFrom(BYTES, msg.sender, address(this), amount);

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

1137: _assetTransferFrom(LP, msg.sender, address(this), amount);

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

1485: address(this),
1492: _assetTransferFrom(S1_CITIZEN, address(this), msg.sender, citizenId);

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

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

1557: _assetTransferFrom(S2_CITIZEN, address(this), msg.sender, citizenId);

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

<ins>Recommended Mitigation Steps</ins>

Use hardcoded address

<a href="#Summary">[GAS‑8]</a><a name="GAS&#x2011;8"> Optimize names to save gas

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.

See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>

<ins>Proof Of Concept</ins>

All in-scope contracts

<ins>Recommended Mitigation Steps</ins>

Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Gauge.sol contract will be the most used; A lower method ID may be given.

<a href="#Summary">[GAS‑9]</a><a name="GAS&#x2011;9"> <x> += <y> Costs More Gas Than <x> = <x> + <y> For State Variables

<ins>Proof Of Concept</ins>

977: pool.totalPoints += citizenStatus.points;

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

1029: pool.totalPoints += citizenStatus.points;

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

1078: citizenStatus.stakedBytes += amount;
1079: citizenStatus.points += bonusPoints;
1080: pool.totalPoints += bonusPoints;
1078: citizenStatus.stakedBytes += amount;
1079: citizenStatus.points += bonusPoints;
1080: pool.totalPoints += bonusPoints;

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

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

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

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

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

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

1160: stakerLPPosition[msg.sender].amount += amount;
1161: stakerLPPosition[msg.sender].points += points;
1164: pool.totalPoints += points;

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

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

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

<a href="#Summary">[GAS‑10]</a><a name="GAS&#x2011;10"> Structs can be packed into fewer storage slots by editing time variables

The following structs contain time variables that are unlikely to ever reach max uint256 then it is possible to set them as uint64, saving gas slots. In total, it is possible to save around 5-8 storage slots

<ins>Proof Of Concept</ins>

359: struct StakedS1Citizen {
		uint256 stakedBytes;
		uint256 timelockEndTime;
		uint256 points;
		uint256 stakedVaultId;
		bool hasVault;
	}

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

Can save one storage slot by editing timelockEndTime to uint64 and changing to:

359: struct StakedS1Citizen {
		uint256 stakedBytes;
		uint256 points;
		uint256 stakedVaultId;
		bool hasVault;
		uint64 timelockEndTime;
	}

The same applies to:

462: struct StakedS1CitizenOutput {
		uint256 citizenId;
		uint256 stakedBytes;
		uint256 timelockEndTime;
		uint256 points;
		uint256 stakedVaultId;
		bool hasVault;
	}

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

For the following, each struct can save between 1-2 storage slot by changing timelockEndTime to uint64 and the rest to a smaller uint such as uint128 if uint256 if the value is unlikely to go over max uin128

394: struct StakedS2Citizen { uint256 stakedBytes; uint256 timelockEndTime; uint256 points; } 423: struct LPPosition { uint256 amount; uint256 timelockEndTime; uint256 points; uint256 multiplier; } 486: struct StakedS2CitizenOutput { uint256 citizenId; uint256 stakedBytes; uint256 timelockEndTime; uint256 points; }

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

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

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







### <a href="#Summary">[GAS&#x2011;11]</a><a name="GAS&#x2011;11"> Public Functions To External

The following functions could be set external to save gas and improve code quality.
External call cost is less expensive than of public functions.

#### <ins>Proof Of Concept</ins>


```solidity
function getCreditYield (
		uint256 _citizenId,
		uint256 _vaultId
	) public view returns (string memory) {

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

function getConfiguredVaultMultiplier (
		uint256 _vaultId
	) public view returns (uint256) {

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

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

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

<a href="#Summary">[GAS‑12]</a><a name="GAS&#x2011;12"> Non-usage of specific imports

The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files

A good example:

import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/access/OwnableUpgradeable.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
import {SafeCastLib} from "solmate/utils/SafeCastLib.sol";
import {ERC20} from "solmate/tokens/ERC20.sol";
import {IProducer} from "src/interfaces/IProducer.sol";
import {GlobalState, UserState} from "src/Common.sol";

<ins>Proof Of Concept</ins>

7: import "../access/PermitControl.sol";
8: import "../interfaces/IByteContract.sol";
9: import "../interfaces/IStaker.sol";

https://github.com/code-423n4/2023-03-neotokyo/tree/main/contracts/staking/BYTES2.sol#L7-L9

6: import "../access/PermitControl.sol";
7: import "../interfaces/IByteContract.sol";
8: import "../interfaces/IGenericGetter.sol";

https://github.com/code-423n4/2023-03-neotokyo/tree/main/contracts/staking/NeoTokyoStaker.sol#L6-L8

<ins>Recommended Mitigation Steps</ins>

Use specific imports syntax per solidity docs recommendation.

<a href="#Summary">[GAS‑13]</a><a name="GAS&#x2011;13"> Help The Optimizer By Saving A Storage Variable's Reference Instead Of Repeatedly Fetching It

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. The effect can be quite significant. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

<ins>Proof Of Concept</ins>

1502: if (citizenId == oldPosition[stakedIndex]) {

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

<a href="#Summary">[GAS‑14]</a><a name="GAS&#x2011;14"> Use v4.8.1 OpenZeppelin contracts

The upcoming v4.8.1 version of OpenZeppelin provides many small gas optimizations. https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.8.1

<ins>Proof Of Concept</ins>

    "@openzeppelin/contracts-upgradeable": "^4.4.2"

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

    "@openzeppelin/contracts": "^4.3.1"

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

<ins>Recommended Mitigation Steps</ins>

Update OpenZeppelin Contracts Usage in package.json and require at least version of 4.8.1