Neo Tokyo contest - Sathish9098's results

LOW FINDINGS

[1] MISSING CHECKS FOR ADDRESS(0X0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES

It is important to always check for the value address(0x0) (also known as address(0)) when assigning values to address state variables. This is because address(0x0) represents the Ethereum zero address, which has a special meaning and can cause unexpected behavior if not properly handled

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

  81: BYTES1 = _bytes;
  82: S1_CITIZEN = _s1Citizen;
  83: STAKER = _staker;
  84: TREASURY = _treasury;

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

   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)

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

   598: BYTES = _bytes;
   599: S1_CITIZEN = _s1Citizen;
   600: S2_CITIZEN = _s2Citizen;
   601: LP = _lpToken;
   602: IDENTITY = _identity;
   603: VAULT = _vault;

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

Recommended Mitigation:

Add address(0) checks before assigning value to state variables

[2] Using the safeMint() instead of the normal mint()

The safeMint() function is typically used in contracts that implement the ERC20 or ERC721 standards, which are widely adopted standards for creating fungible and non-fungible tokens on the Ethereum blockchain. The safeMint() function is designed to prevent potential security vulnerabilities in these token contracts by including additional checks and safeguards to prevent certain types of attacks, such as integer overflows or reentrancy attacks

OpenZeppelin recommendation is to use the safe variant of _mint.

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

102 : _mint(msg.sender, _amount);

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

124: _mint(_to, reward);

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

127 : _mint(TREASURY, daoCommision);

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

154: _mint(TREASURY, treasuryShare);

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

Recommended Mitigation:

Replace _mint() with _safeMint()

[3] LACK OF CHECKS THE INTEGER RANGES

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

 604: VAULT_CAP = _vaultCap;
 605: NO_VAULT_CAP = _noVaultCap;

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

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

_amountis not checked to be != 0 before calling the _burn()

function burn (address _from,uint256 _amount) hasValidPermit(UNIVERSAL, BURN) external {

_burn(_from, _amount);

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

[4] CRITICAL ADDRESS CHANGES SHOULD USE TWO-STEP PROCEDURE

The critical procedures should be two step process.

FILE : 2023-03-neotokyo/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)

 function configureLP (
	address _lp
) external hasValidPermit(UNIVERSAL, CONFIGURE_LP) {
	if (lpLocked) {
		revert LockedConfigurationOfLP();
	}
	LP = _lp;
}

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

[5] LOSS OF PRECISION DUE TO ROUNDING

Rounding is a process of approximating a number by replacing it with a nearby value that has fewer digits. In computing, rounding is often used to reduce the number of decimal places or significant digits in a calculation to a level that is appropriate for the context in which the result will be used. However, rounding can lead to a loss of precision in the calculation, which can be a problem in some situations

For example, suppose we want to calculate the average of two numbers, 1.23456789 and 9.87654321, to two decimal places. The exact answer is 5.55555555, but if we round each number to two decimal places before averaging, we get 1.23 and 9.88, which give an average of 5.555. This answer is less precise than the exact answer, and the error introduced by rounding can accumulate in more complex calculations

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

   968: citizenStatus.points = identityPoints * vaultMultiplier * timelockMultiplier /_DIVISOR / _DIVISOR;

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

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

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

1077: uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);

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

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

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

1388: uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
1389: uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);

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

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

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

[6] Use OpenZeppelin safeCast library instead of normal casting

The SafeCast library provides a set of functions for safely casting between integer types in Solidity. These functions perform various checks to prevent integer overflows and underflows that can occur when performing type conversions

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

1205: if (uint8(_assetType) > 4) { //@audit uint8 safecast

1206: revert InvalidAssetType(uint256(_assetType)); //@audit uint256 safecast

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

1211: revert UnconfiguredPool(uint256(_assetType));

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

1216: revert InactivePool(uint256(_assetType));

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

1222: revert InvalidTimelockOption(uint256(_assetType), _timelockId);

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

1307: revert InvalidAssetType(uint256(_assetType));

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

1668: if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {
1669: revert InvalidAssetType(uint256(_assetType));

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

Recommended Mitigation:

Use OpenZeppelin safeCast library

[7] 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 : 2023-03-neotokyo/contracts/staking/BYTES2.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)

[L-8] Add an event for critical parameter changes

Adding events for critical parameter changes will facilitate offchain monitoring and indexing

FILE : 2023-03-neotokyo/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)

 function configureLP (
	address _lp
) external hasValidPermit(UNIVERSAL, CONFIGURE_LP) {
	if (lpLocked) {
		revert LockedConfigurationOfLP();
	}
	LP = _lp;
}

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

Recommended Mitigation Steps :

Add events for every critical changes happen in protocol with old and new values

[L-9] Tokens accidentally sent to the contract cannot be recovered

It can’t be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts.

Recommended Mitigation Steps

Add this code:

/**

• @notice Sends ERC20 tokens trapped in contract to external address
• @dev Onlyowner is allowed to make this function call
• @param account is the receiving address
• @param externalToken is the token being sent
• @param amount is the quantity being sent
• @return boolean value indicating whether the operation succeeded.

*/ function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) { IERC20(externalToken).transfer(account, amount); return true; } }

[L-10] Use safest version of openzeppelin/contracts-upgradeable instead of vulnerable one

"@openzeppelin/contracts-upgradeable": "^4.4.2" version used in this protocol

Recommended mitigation:

Use safest openzeppelin version 4.8.2

NONCRITICAL FINDINGS

[1] Named imports can be used

It’s possible to name the imports to improve code readability. E.g. import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; can be rewritten as import {IERC20} from “import “@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol”;

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

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

 import "../access/PermitControl.sol";
 import "../interfaces/IByteContract.sol";
 import "../interfaces/IStaker.sol";

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "../access/PermitControl.sol";
import "../interfaces/IByteContract.sol";
import "../interfaces/IGenericGetter.sol";

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

[2] EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. 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.

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

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

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

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

206: bytes32 public constant CONFIGURE_LP = keccak256("CONFIGURE_LP");

209: bytes32 public constant CONFIGURE_TIMELOCKS = keccak256( "CONFIGURE_TIMELOCKS" );

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");

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

[3] Use "public immutable " constantly instead of "immutable public"

in Solidity, the order of modifiers in a variable declaration matters. The public modifier should come before the immutable modifier.

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

43: address immutable public BYTES1;

46: address immutable public S1_CITIZEN;

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

223:  address immutable public BYTES;

226:  address immutable public S1_CITIZEN;

229:  address immutable public S2_CITIZEN;

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

239: address immutable public IDENTITY;

242: address immutable public VAULT;

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

[4] FOR STATE VARIBAES, FOLLOW SOLIDITY STANDARD NAMING CONVENTIONS

it is common practice to use lowercase letters for variable names, and to use capital letters for contract names and event names Using capital letters for state variable names may cause confusion or make the code harder to read, especially if the convention is not consistent throughout the codebase. Therefore, it is generally recommended to use lowercase letters for state variable names.

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

49: address public STAKER;

52: address public TREASURY;

(https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L49-L52) FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

232: address public LP;

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

[5] Use @notice tag to explain the functions as per NATSPEC instructions

CONTEXT: ALL FUNCTIONS

When this function is compiled, the Natspec comments containing the @notice and @param tags are included in the contract's metadata

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

     /**
            Allow holders of the old BYTES contract to change them for BYTES 2.0; old 
	BYTES tokens will be burnt.
	@param _amount The amount of old BYTES tokens to exchange.
*/
function upgradeBytes (

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

    /**
	This function is called by the S1 Citizen contract to emit BYTES to callers 
	based on their state from the staker contract.
	@param _to The reward address to mint BYTES to.
*/
function getReward (
	address _to
) external {

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

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

(https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L131-L140) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L157-L162) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L169-L173) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/BYTES2.sol#L179-L189)

[6] Lack of address(0) checks before calling the functions.

If the _to is zero address the overall functions executions waste of work

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

    function getReward (
	address _to
) external {
    (uint256 reward,uint256 daoCommision) = IStaker(STAKER).claimReward(_to);

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

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

[7] Need Fuzzing test

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

 151: unchecked {
      treasuryShare = _amount * 2 / 3;
      }

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

728 : unchecked { i++; }

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

743: unchecked { i++; }

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

unchecked { citizenStatus.points = identityPoints * vaultMultiplier * timelockMultiplier / _DIVISOR / _DIVISOR; 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;
	}

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

  unchecked {
		citizenStatus.points = 100 * timelockMultiplier / _DIVISOR;
		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;
	}

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

      unchecked {
			uint256 bonusPoints = (amount * 100 / _BYTES_PER_POINT);
			citizenStatus.stakedBytes += amount;
			citizenStatus.points += bonusPoints;
			pool.totalPoints += bonusPoints;
		}

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

 unchecked {
 points += s1Citizen.points;
 i++;
 }

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

  1291 : unchecked {

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

 1297: unchecked {

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

1330 : unchecked {

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

1341: unchecked {

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

1354: unchecked {

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

1369 : unchecked { j++; }

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

1377: unchecked {

1383 : unchecked { i++; }

1387: unchecked {

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

1509 : unchecked { stakedIndex++; }

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

[8] Omissions in Events

Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, some events are missing important parameters

The events should include the new value and old value where possible

FILE : 2023-03-neotokyo/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)

[9] NO SAME VALUE INPUT CONTROL

Add an additional check to ensure that the input is not the same as the previously used value, thus preventing same value inputs in a more robust way

FILE : 2023-03-neotokyo/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)

 function configureLP (
	address _lp
) external hasValidPermit(UNIVERSAL, CONFIGURE_LP) {
	if (lpLocked) {
		revert LockedConfigurationOfLP();
	}
	LP = _lp;
}

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

Recommended Mitigation:

require(oldAddress!= _newAddress, "Same Address");

[10] CONSTANTS SHOULD BE DEFINED RATHER THAN USING MAGIC NUMBERS

Even assembly can benefit from using readable constants instead of hex/numeric literals

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

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

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

[11] Use "private constant" instead of "constant private"

in Solidity, the order of modifiers in a variable declaration matters. The private modifier should come before the immutable modifier.

FILE : 2023-03-neotokyo/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;

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

[12] NON-LIBRARY/INTERFACE FILES SHOULD USE FIXED COMPILER VERSIONS, NOT FLOATING ONES

It is generally considered best practice to specify a fixed compiler version in non-library/interface files in Solidity, rather than using a floating version. This is because different versions of the Solidity compiler can introduce changes to the language syntax or behavior, which may cause compatibility issues or unexpected behavior in your code

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

2: pragma solidity ^0.8.19;

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

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

2: pragma solidity ^0.8.19;

[13] GENERATE PERFECT CODE HEADERS EVERY TIME

Description I recommend using header for Solidity code layout and readability

(https://github.com/transmissions11/headers)

[14] Contract layout and order of functions

Another recommendation is to declare internal functions below external functions.

Functions should be grouped according to their visibility and ordered:

constructor

receive function (if exists)

fallback function (if exists)

external

public

internal

private

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

The instances below highlights private above external. If possible, consider adding private functions below external functions for the contract layout

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

[15] Use constants instead of hardcoding numbers

It is generally recommended to use constants instead of hardcoding numbers directly in your code. This can help make your code more readable and maintainable, as well as avoid errors that may arise from accidentally mistyping a number

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

962: uint256 timelockDuration = _timelock >> 128;

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

1016: uint256 timelockDuration = _timelock >> 128;

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

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

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

1113: (seasonId << 128) + citizenId,

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

1140: uint256 timelockDuration = _timelock >> 128;

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

1205: if (uint8(_assetType) > 4) {

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

1668: if (uint8(_assetType) == 2 || uint8(_assetType) > 4) {

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

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

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

   bytes4 constant private _TRANSFER_FROM_SELECTOR = 0x23b872dd;
   bytes4 constant private _TRANSFER_SELECTOR = 0xa9059cbb;
   uint256 constant private _PRECISION = 1e12;
   uint256 constant private _DIVISOR = 100;
   uint256 constant private _BYTES_PER_POINT = 200 * 1e18;
   bytes32 public constant CONFIGURE_LP = keccak256("CONFIGURE_LP");
   bytes32 public constant CONFIGURE_TIMELOCKS = keccak256(
	"CONFIGURE_TIMELOCKS"
);
   bytes32 public constant CONFIGURE_CREDITS = keccak256("CONFIGURE_CREDITS");
   bytes32 public constant CONFIGURE_POOLS = keccak256("CONFIGURE_POOLS");
   bytes32 public constant CONFIGURE_CAPS = keccak256("CONFIGURE_CAPS");

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

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

    bytes32 public constant BURN = keccak256("BURN");
    bytes32 public constant ADMIN = keccak256("ADMIN");

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

[17] Assembly Codes Specific – Should Have Comments

Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does.

This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code.

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

833: assembly {

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

886: assembly {

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

1001: assembly {

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

1050: assembly{

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

1128: assembly{

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

1236: assembly {

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

1461: assembly {

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

1536: assembly {

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

1599: assembly{

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

1682: assembly {

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

[18] Convert repeated validation statements into a function modifier to improve code reusability

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

693: if (_assetType == AssetType.S1_CITIZEN) { 1278: if (_assetType == AssetType.S1_CITIZEN) {

NeoTokyoStaker.sol#L693,NeoTokyoStaker.sol#L1278

1287: } else if (_assetType == AssetType.S2_CITIZEN) { 695: } else if (_assetType == AssetType.S2_CITIZEN) {

NeoTokyoStaker.sol#L1287,NeoTokyoStaker.sol#L695

1071: if (citizenStatus.timelockEndTime == 0) { 1092: if (citizenStatus.timelockEndTime == 0) {

NeoTokyoStaker.sol#L1071,NeoTokyoStaker.sol#L1092

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

NeoTokyoStaker.sol#L1472,NeoTokyoStaker.sol#L1547

1477: if (stakedCitizen.stakedBytes > 0) { 1552: if (stakedCitizen.stakedBytes > 0) {

NeoTokyoStaker.sol#L1477,NeoTokyoStaker.sol#L1552

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

NeoTokyoStaker.sol#L1502,NeoTokyoStaker.sol#L1567

GAS OPTIMIZATIONS

[G-1] OPTIMIZE NAMES TO SAVE GAS

public/external function names and public member variable names can be optimized to save gas. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

  /// @audit 
  upgradeBytes(),getReward(),burn(),changeStakingContractAddress(),updateReward(),updateRewardOnMint()
  34: contract BYTES2 is PermitControl, ERC20("BYTES", "BYTES") {

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

  /// @audit getCreditYield(),getConfiguredVaultMultiplier(),getStakerPosition(),getStakerPositions(),stake(),
  getPoolReward(),claimReward(),withdraw(),configureLP(),lockLP(),configureTimelockOptions(),
  configureIdentityCreditYields(),configureIdentityCreditPoints()configureVaultCreditMultipliers(),
  configurePools(),configureCaps
 188: contract NeoTokyoStaker is PermitControl, ReentrancyGuard {

[G-2] Setting the constructor to payable

INSTANCES(2):

APROXIMATE GAS SAVED : 26 GAS

ou 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 : 2023-03-neotokyo/contracts/staking/BYTES2.sol

 75 : constructor (

staking/BYTES2.sol#L75

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

588: constructor (

NeoTokyoStaker.sol#L588

[G-3] EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

Using immutable variables instead of constant variables for expressions that evaluate to constant values, such as a call to keccak256(), can potentially save gas in smart contracts

The reason for this is that immutable variables are evaluated at compile time and the resulting values are hardcoded into the bytecode of the contract. This means that the gas cost of evaluating the expression is incurred only once, during contract compilation, and not at runtime

In contrast, constant variables are evaluated at runtime, which can result in additional gas costs. Specifically, when a constant variable is accessed, the EVM must perform a lookup to retrieve the value of the variable from storage, which can add to the gas cost of executing the contract

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

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

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

BYTES2.sol#L37-L40

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

 206: bytes32 public constant CONFIGURE_LP = keccak256("CONFIGURE_LP");

 209: bytes32 public constant CONFIGURE_TIMELOCKS = keccak256(
	"CONFIGURE_TIMELOCKS"
);

 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");

NeoTokyoStaker.sol#L206-L220

[G-4] MULTIPLE ADDRESS/ID MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS/ID TO A STRUCT, WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

316: mapping ( AssetType => PoolData ) private _pools;

319: mapping ( address => mapping ( AssetType => uint256 )) public lastRewardTime;

326: mapping ( uint256 => mapping ( string => string )) public identityCreditYield;

329: mapping ( string => uint256 ) public identityCreditPoints;

332:  mapping ( string => uint256 ) public vaultCreditMultiplier;

NeoTokyoStaker.sol#L316-L332

372 : mapping ( address => mapping( uint256 => StakedS1Citizen )) public stakedS1;

378:  mapping ( address => uint256[] ) private _stakerS1Position;

NeoTokyoStaker.sol#L372-L378

405: mapping ( address => mapping( uint256 => StakedS2Citizen )) public stakedS2;

411: mapping ( address => uint256[] ) private _stakerS2Position;

NeoTokyoStaker.sol#L405-L411

434: mapping ( address => LPPosition ) public stakerLPPosition;

NeoTokyoStaker.sol#L434

[G-5] CAN MAKE THE VARIABLE OUTSIDE THE LOOP TO SAVE GAS

INSTANCES(10):

APROXIMATE GAS SAVED : 90 GAS

In Solidity, declaring a variable inside a loop can result in higher gas costs compared to declaring it outside the loop. This is because every time the loop runs, a new instance of the variable is created, which can lead to unnecessary memory allocation and increased gas costs

On the other hand, if you declare a variable outside the loop, the variable is only initialized once, and you can reuse the same instance of the variable inside the loop. This approach can save gas and optimize the execution of your code

GAS SAMPLE TEST IN remix ide(Without gas optimizations) :

Before Mitigation :

 function testGas() public view {

    for(uint256 i = 0; i < 10; i++) {
      address collateral = msg.sender;
      address  collateral1 = msg.sender;
        
    }

The execution Cost : 2176

After Mitigation :

 function testGas() public view {
 address collateral; address  collateral1;
    for(uint256 i = 0; i < 10; i++) {
     collateral = msg.sender;
     collateral1 = msg.sender;
        
    }

The execution Cost : 2086

Hence clearly after mitigation the gas cost is reduced. Approximately its possible to save 9 gas for every iterations

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

  for (uint256 i; i < _stakerS1Position[_staker].length; ) {
  uint256 citizenId = _stakerS1Position[_staker][i];
  StakedS1Citizen memory citizenDetails = stakedS1[_staker][citizenId];

NeoTokyoStaker.sol#L717-L719

 for (uint256 i; i < _stakerS2Position[_staker].length; ) {
 uint256 citizenId = _stakerS2Position[_staker][i];
 StakedS2Citizen memory citizenDetails = stakedS2[_staker][citizenId];

NeoTokyoStaker.sol#L734-L736

 for (uint256 i; i < _stakerS1Position[_recipient].length; ) {
 uint256 citizenId = _stakerS1Position[_recipient][i];
 StakedS1Citizen memory s1Citizen = stakedS1[_recipient][citizenId];

NeoTokyoStaker.sol#L1279-L1281

for (uint256 i; i < _stakerS2Position[_recipient].length; ) {
uint256 citizenId = _stakerS2Position[_recipient][i];
StakedS2Citizen memory s2Citizen = stakedS2[_recipient][citizenId];

NeoTokyoStaker.sol#L1288-L1290

for (uint256 i; i < windowCount; ) {
RewardWindow memory window = pool.rewardWindows[i];

NeoTokyoStaker.sol#L1312-L1313

  1320 : uint256 currentRewardRate = pool.rewardWindows[i - 1].reward;

NeoTokyoStaker.sol#L1320

  1331: uint256 timeSinceReward = block.timestamp - lastPoolRewardTime;

NeoTokyoStaker.sol#L1331

  1342: uint256 timeSinceReward = window.startTime - lastPoolRewardTime;

NeoTokyoStaker.sol#L1342

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

For each && split can save 10-20 gas

INSTANCES(10):

APROXIMATE GAS SAVED : 100 GAS

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

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

  910: if (citizenVaultId != 0 && vaultId != 0) {

[NeoTokyoStaker.sol#L910] (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L910)

  917: } else if (citizenVaultId != 0 && vaultId == 0) {

NeoTokyoStaker.sol#L917

  926: } else if (citizenVaultId == 0 && vaultId != 0) {

NeoTokyoStaker.sol#L926

[G-7] INSTEAD OF CALCULATING A STATEVAR WITH KECCAK256() EVERY TIME THE CONTRACT IS MADE PRE CALCULATE THEM BEFORE AND ONLY GIVE THE RESULT TO A CONSTANT

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

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

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

BYTES2.sol#L37-L40

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

 206: bytes32 public constant CONFIGURE_LP = keccak256("CONFIGURE_LP");

 209: bytes32 public constant CONFIGURE_TIMELOCKS = keccak256(
	"CONFIGURE_TIMELOCKS"
);

 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");

NeoTokyoStaker.sol#L206-L220

[G-8] <x> += <y> OR <x>-=<y> costs more gas than <x> = <x> + <y> OR <x> = <x> - <y> for state variables

Using the addition/Subtraction operator instead of plus-equals/minus-equals saves 113 gas

INSTANCES(13):

APROXIMATE GAS SAVED : 1469 GAS

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

  977: pool.totalPoints += citizenStatus.points;

NeoTokyoStaker.sol#L977

  1029 : pool.totalPoints += citizenStatus.points;

NeoTokyoStaker.sol#L1029

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

NeoTokyoStaker.sol#L1078-L1080

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

 1164: pool.totalPoints += points;

NeoTokyoStaker.sol#L1160-L1164

 1519 : pool.totalPoints -= stakedCitizen.points;

NeoTokyoStaker.sol#L1515

 1580 : pool.totalPoints -= stakedCitizen.points;

NeoTokyoStaker.sol#L1580

 1626: lpPosition.amount -= amount;
 1627: lpPosition.points -= points;
 1630: pool.totalPoints -= points;

NeoTokyoStaker.sol#L1626-L1627

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

INSTANCES(3):

Using constants instead of type(uintx).max can save gas in some cases because constants are precomputed at compile-time and can be reused throughout the code, whereas type(uintx).max requires computation at runtime

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

963: uint256 timelockMultiplier = _timelock & type(uint128).max;

1017 : uint256 timelockMultiplier = _timelock & type(uint128).max;

NeoTokyoStaker.sol#L1017

1141 : uint256 timelockMultiplier = _timelock & type(uint128).max;

NeoTokyoStaker.sol#L1141

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

INSTANCES(10):

     598: BYTES = _bytes;
     599: S1_CITIZEN = _s1Citizen;
     600: S2_CITIZEN = _s2Citizen;
     601: LP = _lpToken;
     602: IDENTITY = _identity;
     603: VAULT = _vault;

NeoTokyoStaker.sol#L598

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

 81: BYTES1 = _bytes;
 82: S1_CITIZEN = _s1Citizen;
 83: STAKER = _staker;
 84: TREASURY = _treasury;

BYTES2.sol#L81-L84

[G-11] Use calldata instead of memory for function arguments that do not get mutated

INSTANCES(2):

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

   824: function _stringEquals (
	string memory _a,
	string memory _b
       ) private pure returns (bool) {

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

[G-12] Save gas with the use of the import statement

INSTANCES(2):

While the following two critical VAULT_CAP,NO_VAULT_CAP values are assigned in the constructor, there is no zero value control. This means that if both state variables are started with a possible value of 0, the contract must be deployed again. This possibility means gas consumption.

Zero value control is the most error-prone value control since zero value is assigned in case of no value entry due to EVM design.

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

 604: VAULT_CAP = _vaultCap;
 605: NO_VAULT_CAP = _noVaultCap;

NeoTokyoStaker.sol#L604-L605

Recommendation:

It is recommended to perform a zero value check for critical value assignments.

[G-13] Using delete instead of setting struct 0 saves gas

INSTANCES(8):

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

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

NeoTokyoStaker.sol#L1517-L1521

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

NeoTokyoStaker.sol#L1582-L1584

[G-14] Gas overflow during iteration (DoS)

Each iteration of the cycle requires a gas flow. A moment may come when more gas is required than it is allocated to record one block. In this case, all iterations of the loop will fail

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

(https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L717-L728) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L734-L743) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1279-L1302) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1312-L1383) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1499-L1509) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1564-L1574) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1742-L1745) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1765-L1772) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1787-L1791) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1806-L1809) (https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1822-L1840)Reco

Recommendations:

Check the maxProcessingLengt allowed before start iterating the loops

require(Varibale.length < maxProcessingLengt, "max length");

[G-15] Save gas with the use of the import statement

INSTANCES(8):

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";

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

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

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

NeoTokyoStaker.sol#L4-L8

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

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

import "../access/PermitControl.sol"; import "../interfaces/IByteContract.sol"; import "../interfaces/IStaker.sol";

NeoTokyoStaker.sol#L4-L8

[G-16] Avoid contract existence checks by using solidity version 0.8.10 or later

INSTANCES(5):

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

           (
		uint256 reward,
		uint256 daoCommision
	) = IStaker(STAKER).claimReward(_to);

   96: if (IERC20(BYTES1).balanceOf(msg.sender) < _amount) {

BYTES2.sol#L117-L120

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

632: uint256 rewardRate = citizen.getRewardRateOfTokenId(_citizenId);

NeoTokyoStaker.sol#L632

904: uint256 citizenVaultId = citizen.getVaultIdOfTokenId(citizenId);

948: uint256 identityId = citizen.getIdentityIdOfTokenId(citizenId);

[G-17] Make 3 event parameters indexed when possible

INSTANCES(4):

It’s the most gas efficient to make up to 3 event parameters indexed. If there are less than 3 parameters, you need to make all parameters indexed.

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

The same extra indexed parameter can be applied to BytesUpgraded()

  event BytesUpgraded (
	address indexed caller,
	uint256 amount
);

BYTES2.sol#L61-L64

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

The same extra indexed parameter can be applied to Stake()

    event Stake (
	address indexed staker,
	address indexed asset,
	uint256 timelockOption,
	uint256 amountOrTokenId
);

NeoTokyoStaker.sol#L539-L544

The same extra indexed parameter can be applied to Claim()

   event Claim (
	address indexed recipient,
	uint256 reward,
	uint256 tax
);

NeoTokyoStaker.sol#L553-L557

The same extra indexed parameter can be applied to Withdraw()

 event Withdraw (
	address indexed caller,
	address indexed asset,
	uint256 amountOrTokenId
);

NeoTokyoStaker.sol#L567-L571

[G-18] Duplicated if() checks should be refactored to a modifier or function

INSTANCES(12):

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

693: if (_assetType == AssetType.S1_CITIZEN) { 1278: if (_assetType == AssetType.S1_CITIZEN) {

NeoTokyoStaker.sol#L693,NeoTokyoStaker.sol#L1278

1287: } else if (_assetType == AssetType.S2_CITIZEN) { 695: } else if (_assetType == AssetType.S2_CITIZEN) {

NeoTokyoStaker.sol#L1287,NeoTokyoStaker.sol#L695

1071: if (citizenStatus.timelockEndTime == 0) { 1092: if (citizenStatus.timelockEndTime == 0) {

NeoTokyoStaker.sol#L1071,NeoTokyoStaker.sol#L1092

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

NeoTokyoStaker.sol#L1472,NeoTokyoStaker.sol#L1547

1477: if (stakedCitizen.stakedBytes > 0) { 1552: if (stakedCitizen.stakedBytes > 0) {

NeoTokyoStaker.sol#L1477,NeoTokyoStaker.sol#L1552

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

NeoTokyoStaker.sol#L1502,NeoTokyoStaker.sol#L1567

[G-19] Create the instance of IGenericGetter(VAULT),IGenericGetter(S1_CITIZEN) outside the functions . We can reuse the instance variable whenever needed instead of creating new instance variables every time . This will save the gas cost

INSTANCES(2):

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

638: IGenericGetter vault = IGenericGetter(VAULT);

664: IGenericGetter vault = IGenericGetter(VAULT);

NeoTokyoStaker.sol#L638,NeoTokyoStaker.sol#L664

631: IGenericGetter citizen = IGenericGetter(S1_CITIZEN); 903: IGenericGetter citizen = IGenericGetter(S1_CITIZEN);

NeoTokyoStaker.sol#L631,NeoTokyoStaker.sol#L903

[G-20] Cache the S1_CITIZEN immutable address variable value with local address variable . This will save the 60 gas as per remix gas reports

PROOF OF CONCEPT (REMIX WITHOUT OPTIMIZATIONS) :

pragma solidity ^0.8.7;

contract MappingTest {

address public immutable A=0xE0f5206BBD039e7b0592d8918820024e2a7437b9; // execution cost 195 gas function withoutImmutableCache() public view { if(A==0xE0f5206BBD039e7b0592d8918820024e2a7437b9){ if(A==0xE0f5206BBD039e7b0592d8918820024e2a7437b9){

    }
}

} // execution cost 135 gas function withImmutableCache() public view {

address a=A;
 if(a==0xE0f5206BBD039e7b0592d8918820024e2a7437b9){
     if(a==0xE0f5206BBD039e7b0592d8918820024e2a7437b9){

    }
}

}

}

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

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

903: IGenericGetter citizen = IGenericGetter(S1_CITIZEN);

 emit Stake(
 msg.sender,
 S1_CITIZEN,
 _timelock,
 citizenId
);

NeoTokyoStaker.sol#L897,NeoTokyoStaker.sol#L903,NeoTokyoStaker.sol#L983

S2_CITIZEN should be cached with local address variable

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

emit Stake(
		msg.sender,
		S2_CITIZEN,
		_timelock,
		citizenId
	);

NeoTokyoStaker.sol#L1010,NeoTokyoStaker.sol#L1035

[G-21] Cache _PRECISION constant variable with local uint256 variable instead of calling multiple times . This will save the 9 gas as per remix gas reports

PROOF OF CONCEPT (REMIX) :

pragma solidity ^0.8.7;

contract MappingTest { uint256 public constant A=100; // execution cost 802 gas function withoutContantCache() public pure { uint256 b=200;

b+=A; b/=A; b*=A; } // execution cost 793 gas function withConstantCache() public pure { uint256 b=200; uint256 c=A; b+=c; b/=c; b*=c; } }

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

1388: uint256 share = points * _PRECISION / pool.totalPoints * totalReward;

1390: share /= _PRECISION;

1391: daoShare /= _PRECISION;

NeoTokyoStaker.sol#L1388-L1391

[G-21] Structs can be packed into fewer storage slots

APROXIMATE GAS SAVED : 40000 (2 SLOTS)

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

 struct StakedS1Citizen {
   uint256 stakedBytes; //32-SLOT 1
   uint256 timelockEndTime; //32- SLOT 2
   uint256 points; //32 SLOT-3
   uint256 stakedVaultId; //32 SLOT-4
   bool hasVault; //1 SLOT-5
}

Total slots 5

NeoTokyoStaker.sol#L359-L365

We can declare timelockEndTime uint128 instead of uint256. Using uint128 we can store 10,774,146,971,163 years this is more than enough for timelockEndTime . So we can save 1 slot and saves 20000 gas

Recommended Mitigation:

  /// @audit Variable ordering with 4 slots instead of the current 5:

   struct StakedS1Citizen {
   uint256 stakedBytes; //32-SLOT 1
-  uint256 timelockEndTime; 
   uint256 points; //32 SLOT-2
   uint256 stakedVaultId; //32 SLOT-3

• uint128 timelockEndTime; //16- SLOT 4 bool hasVault; //1 SLOT-4 }

Same like last example we can reduce one slot

    struct StakedS1CitizenOutput {
	uint256 citizenId; //32 -SLOT1
	uint256 stakedBytes; //32 -SLOT2
	uint256 timelockEndTime; //32 -SLOT3
	uint256 points; //32 -SLOT4
	uint256 stakedVaultId; //32 -SLOT5
	bool hasVault; //1 -SLOT6
}

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

Total slots 6

Recommended Mitigation:

   /// @audit Variable ordering with 5 slots instead of the current 6:
    struct StakedS1CitizenOutput {
	uint256 citizenId; //32 -SLOT1
	uint256 stakedBytes; //32 -SLOT2
       -    uint256 timelockEndTime; 
	uint256 points; //32 -SLOT3
	uint256 stakedVaultId; //32 -SLOT4
        +   uint128 timelockEndTime; //16 -SLOT5 
	bool hasVault; //1 -SLOT5
}

 

[G-22] State variables can be packed into fewer storage slots

INSTANCES(1):

APROXIMATE GAS SAVED : 20000 (1 SLOT)

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

232: address public LP; //20 bytes

511: bool public lpLocked; //1byte

NeoTokyoStaker.sol#L232,NeoTokyoStaker.sol#L511

Move the (bool public lpLocked) bellow the (address public LP) this wills save the one slot

232: address public LP; //20 bytes

• 233: bool public lpLocked;

• 511: bool public lpLocked;

Once moved possible to pack both variable in single slot instead of 2 slots . So possible to save 20000 gas

[G-23] PRIVATE FUNCTIONS ONLY CALLED ONCE CAN BE INLINED TO SAVE GAS

INSTANCES(5):

APROXIMATE GAS SAVED : 200 GAS

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

FILE : 2023-03-neotokyo/contracts/staking/NeoTokyoStaker.sol

function _stringEquals ( string memory _a, string memory _b ) private pure returns (bool) {

NeoTokyoStaker.sol#L824-L827

 function _stakeS1Citizen (
	uint256 _timelock
) private {

NeoTokyoStaker.sol#L875-L877

 function _stakeS2Citizen (
	uint256 _timelock
) private {

NeoTokyoStaker.sol#L995-L998

function _stakeBytes (
	uint256
) private {

NeoTokyoStaker.sol#L1044-L1046

function _stakeLP (
	uint256 _timelock
) private {

NeoTokyoStaker.sol#L1124-L1126

[G-24] Empty blocks should be removed to save deployment cost

INSTANCES(2):

FILE : 2023-03-neotokyo/contracts/staking/BYTES2.sol

function updateRewardOnMint ( address, uint256 ) external { }

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

 function updateReward (
	address,
	address,
	uint256
) external {
}

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

[G-25] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

INSTANCES(6):

windowCount - 1 is not possible to overflow . We already checked the value in for loop

   1353: if (j == windowCount - 1) {

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

 1376: } else if (i == windowCount - 1) {

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

1503: if (stakedIndex != oldPosition.length - 1) {
1504: oldPosition[stakedIndex] = oldPosition[oldPosition.length - 1];

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

 1568: if (stakedIndex != oldPosition.length - 1) {
 1569: oldPosition[stakedIndex] = oldPosition[oldPosition.length - 1]; 

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