Neo Tokyo contest - Inspex's results

Low Issues

[L-01] Preventing the user from getting the reward due to the lost precision

Line References

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

Description

The getPoolReward() function is used to calculate the amount of reward that the user will get. https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1388-L1392

uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
uint256 daoShare = share * pool.daoTax / (100 * _DIVISOR);
share /= _PRECISION;
daoShare /= _PRECISION;
return ((share - daoShare), daoShare);

As a result of dividing before multiplying, the reward will be less than it should be.

Recommended Mitigation Steps

Modify the function to multiply before dividing.

[L-02] The user will get the reward without staking time

Line References

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L962 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1016 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1140 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1737

Description

The stake() function is used to stake the user's asset and earn reward points depending on the _timelock state, which is set by the configureTimelockOptions() function.

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

uint256 timelockDuration = _timelock >> 128;
uint256 timelockMultiplier = _timelock & type(uint128).max;

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

function configureTimelockOptions (
    AssetType _assetType,
    uint256[] memory _timelockIds,
    uint256[] memory _encodedSettings
) external hasValidPermit(bytes32(uint256(_assetType)), CONFIGURE_TIMELOCKS) {
    for (uint256 i; i < _timelockIds.length; ) {
        timelockOptions[_assetType][_timelockIds[i]] = _encodedSettings[i];
        unchecked { ++i; }
    }
}

However, the duration can be set to any number below type(uint256).max, for example.

uint256 _timelock = uint256(bytes32(type(uint256).max << 128) ^ bytes32(type(uint256).max));
/// which is 340282366920938463463374607431768211455

Any number below 340282366920938463463374607431768211455 will make the timelockDuration state be 0 and the timelockMultiplier be greater than0.

Resulting in breaking the business logic that the timelockMultiplier will be relevant with the timelockDuration.

Recommended Mitigation Steps

Add a validation check to prevent the value below 340282366920938463463374607431768211455 from being passed into the configureTimelockOptions() function.

function configureTimelockOptions (
    AssetType _assetType,
    uint256[] memory _timelockIds,
    uint256[] memory _encodedSettings
) external hasValidPermit(bytes32(uint256(_assetType)), CONFIGURE_TIMELOCKS) {
    uint256 upperBound = uint256(bytes32(type(uint256).max << 128) ^ bytes32(type(uint256).max));
    for (uint256 i; i < _timelockIds.length; ) {
        if(_encodedSettings[i] <= upperBound){
          revert DoNotSetTooHighNumber();
        }
        timelockOptions[_assetType][_timelockIds[i]] = _encodedSettings[i];
        unchecked { ++i; }
    }
}

[L-03] Lack of zero address checks

Line References

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L75 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L162 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/BYTES2.sol#L173 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L588 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1708

Description

If these variable get configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.

Recommended Mitigation Steps

Add a validation check when setting the address in functions.

[L-04] The getReward() function can be called by anyone to claim rewards for another user

Line References

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

Description

In the current implementation of BYTES2, the getReward() function can be called by anyone to claim rewards for another user.

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

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

    // Mint both reward BYTES and the DAO tax to targeted recipients.
    if (reward > 0) {
        _mint(_to, reward);
    }
    if (daoCommision > 0) {
        _mint(TREASURY, daoCommision);
    }
}

As a result, the reward is claimed arbitrarily, violating the business logic that only the STAKER and S1_CITIZEN contracts can claim rewards for users.

Recommended Mitigation Steps

Add the code below to prevent the call from other users.

function getReward (
    address _to
) external {
    if (msg.sender != S1_CITIZEN && msg.sender != STAKER){
      revert DontCallForOther(_to);
    } 
    (
        uint256 reward,
        uint256 daoCommision
    ) = IStaker(STAKER).claimReward(_to);

    // Mint both reward BYTES and the DAO tax to targeted recipients.
    if (reward > 0) {
        _mint(_to, reward);
    }
    if (daoCommision > 0) {
        _mint(TREASURY, daoCommision);
    }
}

Non-Critical Issues

[N-01] Critical Address Changes Should Use Two-step Procedure

Line References

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

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

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

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

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

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

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

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

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

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

Description

The critical procedures should be carried out in two steps.

Recommended Mitigation Steps

We suggest adding two step procedure on the critical functions.

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

Line References

All Contracts.

Description

Organizing functions within a contract can help users know which ones they can call, but some contracts in the project do not follow this practice.

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

Recommended Mitigation Steps

To improve code readability, it's recommended that functions are:

Grouped by their visibility and ordered as follows:

• Constructor
• Receive function (if it exists)
• Fallback function (if it exists)
• External
• Public
• Internal
• Private

Within each grouping, view and pure functions should be placed last.

[N-03] Missing return in NatSpec

Line References

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

Description

The NatSpec comment block for the function is incomplete, as it does not include a return parameter to describe the format and meaning of the function's return values.

File: NeoTokyoStaker.sol

	/**
		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) {

Recommended Mitigation Steps

Adding return parameter in NatSpec comments.

[N-04] Compliance with Solidity Style rules in Upper Case expressions

Line References

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

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

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

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

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

Description

Variables that are not declared as constants or immutable should be named using the lower_case format.

File: BYTES2.sol

49:     address public STAKER;

52:	address public TREASURY;

File: NeoTokyoStaker.sol

232:	address public LP;
    
249:	uint256 public VAULT_CAP;

255:	uint256 public NO_VAULT_CAP;

Recommended Mitigation Steps

We suggest using the lower_case format when naming variables that are not declared as constants or immutable, as this helps distinguish them from constant variables and complies with Solidity naming conventions.

[N-05] Lock pragmas to specific compiler version

Line References

All Contracts.

Description

All the contracts in scope are floating the pragma version.

Recommended Mitigation Steps

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.

[N-06] Lack of array length check in the configuration function

Line References

https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1737 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1760 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1783 https://github.com/code-423n4/2023-03-neotokyo/blob/main/contracts/staking/NeoTokyoStaker.sol#L1802

Description

The configuration functions that have input parameters as arrays will be reverted due to the lack of a validation length check.

Recommended Mitigation Steps

Add a validation length check when input parameters are arrays.

Gas Optimizations

In total, there were 18 instances of 5 issues, resulting in a total gas savings of at least 4200 (this represents the minimum amount of gas saved).

[Gas-01] Constructors can be marked payable

Description

Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.

Proof of concept: https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5?u=pcaversaccio

Instances(2):

File: BYTES2.sol

75:	constructor (

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

File: NeoTokyoStaker.sol

588:	constructor (

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

Recommendation

We suggest setting the constructor to payable.

[Gas-02] Use of Named Returns for Local Variables Saves Gas

Description

Using named return values as temporary local variables can help you conserve gas. Instances(1):

File: NeoTokyoStaker.sol

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

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

Recommendation

We suggest using named returns for local variables.

For example:

File: NeoTokyoStaker.sol

1267:	) public view returns (uint256 share, uint256 points) {


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

[Gas-03] Using storage instead of memory for struct/arrays saves gas

Description

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read.

Instances(2):

File: NeoTokyoStaker.sol

1281:	StakedS1Citizen memory s1Citizen = stakedS1[_recipient][citizenId];

1290:	StakedS2Citizen memory s2Citizen = stakedS2[_recipient][citizenId];

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

Recommendation

We suggest using storage instead of memory for struct/arrays to save gas.

For example:

File: NeoTokyoStaker.sol

1281:	StakedS1Citizen storage s1Citizen = stakedS1[_recipient][citizenId];

1290:	StakedS2Citizen storage s2Citizen = stakedS2[_recipient][citizenId];

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

Description

Using assembly to store an address can save gas because it allows direct access to the EVM, which enables more efficient manipulation of storage.

Instances(13):

File: BYTES2.sol

81:		BYTES1 = _bytes;

82:		S1_CITIZEN = _s1Citizen;

83:		STAKER = _staker;

84:		TREASURY = _treasury;

165:		STAKER = _staker;

176:		TREASURY = _treasury;

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

File: NeoTokyoStaker.sol

598:		BYTES = _bytes;

599:		S1_CITIZEN = _s1Citizen;

600:		S2_CITIZEN = _s2Citizen;

601:		LP = _lpToken;

602:		IDENTITY = _identity;

603:		VAULT = _vault;

1714:		LP = _lp;

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

Recommendation

We suggest using assembly to write address storage value to save gas.

For example:

File: BYTES2.sol

75:    constructor (
76:        address _bytes,
77:        address _s1Citizen,
78:        address _staker,
79:        address _treasury
80:    ) {
81:        assembly {
82:            sstore(0x0a, _bytes)
83:            sstore(0x0b, _s1Citizen)
84:            sstore(0x08, _staker)
85:            sstore(0x09, _treasury)
86:      }
87:    }

[Gas-05] Optimize names to save gas

Description

Each function has a unique Method ID, and for each function, the EVM increases the gas cost by 22 for every function that comes before it in the Method ID order. This means that arranging frequently used functions with lower Method IDs can help reduce gas costs.

Proof of concept: https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

For example: The following table contains all functions in the BYTES2 contract sorted by MethodID from lower to higher.

Recommendation

We suggest optimizing the names of frequently called functions with lower Method ID.