Neo Tokyo contest - mrpathfindr's results

Compiler version too recent

pragma solidity ^0.8.19;

The compiler version detected in the code is too recent. Therefore, it is not time-tested and may be susceptible to multiple bugs and vulnerabilities, both from the usage and security perspectives.

Mitigation: Make use of a more stable version of solidity.

The Use of Pragma can be problematic

^0.8.19;

The contract was found to be using a floating pragma which is not considered safe as it can be compiled with all the versions described. The following affected files were found to be using floating pragma:

Making use of 'require' instead of 'revert'

The require Solidity function guarantees the validity of the condition(s) passed as a parameter that cannot be detected before execution. It checks inputs, contract state variables, and return values from calls to external contracts.

Using require instead of revert improves the overall readability of the contract code.

The construction if (condition) { revert(); } is equivalent to require(!condition);

The Use of Inline Assembly

Inline assembly is a way to access the Ethereum Virtual Machine at a low level. This bypasses several important safety features and checks of Solidity.

May not be necessary in code such as

assembly { amount := calldataload(0x24) }

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

Multiple vulnerabilities have been detected previously when the assembly is not properly used within the Solidity code; therefore, caution should be exercised while using them.

Use Custom Errors to save gas

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

revert(string(data)); Could be long or short depending on the response. A standard custom error here will save gas

Use less strict inequalities

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

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

When inside the if statements, non-strict inequalities (>=, <=) are usually cheaper than the strict equalities (>, <).

Array Length Loop Caching

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

Mitigation:

uint256 stakePositionLength = _stakerS1Position[_staker].length

for (uint256 i; i < stakePositionLength; ) {

During each iteration of the loop, reading the length of the array uses more gas than is necessary. In the most favorable scenario, in which the length is read from a memory variable, storing the array length in the stack can save about 3 gas per iteration. In the least favorable scenario, in which external calls are made during each iteration, the amount of gas wasted can be significant.

Cheaper Conditional Operations

Sample case can be found here: https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L639

string memory vaultMultiplier = (_vaultId > 0) is cheaper than string memory vaultMultiplier = (_vaultId != 0)