Neo Tokyo contest - 0x1f8b's results

• Low
  • 1. Lack of address checks
  • 2. Lack of event emit
  • 3. Unsafe transfer
• Non critical
  • 4. Typo
  • 5. Avoid hardcoded values

Low

1. Lack of checks address(0)

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• BYTES2.sol:76-79
• NeoTokyoStaker.sol:589-594

2. Lack of event emit

The changeStakingContractAddress and changeTreasuryContractAddress methods do not emit an event when the state changes, something that it's very important for dApps and users.

Affected source code:

• BYTES2.sol:165
• BYTES2.sol:176

3. Unsafe transfer

It does not check the return returned in the case of ERC-20, so a token that returns false could incur economic losses for the user or the project.

It is necessary to check that if data is a boolean, it is positive, just like Open Zeppelin's SafeTransfer does.

Affected source code:

• NeoTokyoStaker.sol:755-786
• NeoTokyoStaker.sol:796-814

---

Non critical

4. Typo

Tab was used instead of a space:

S2) and BYTES for time-locked emission rewards. The staker operates on a

Affected source code:

• NeoTokyoStaker.sol:181

5. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

Use the selector instead of the raw value:

• NeoTokyoStaker.sol:191
• NeoTokyoStaker.sol:194

• Gas
  • 1. Optimize stringEquals
  • 2. Optimize stake

Gas

1. Optimize stringEquals

It is possible to optimize the comparison of 2 string using a hash function.

Sample:

pragma solidity ^0.8.19;

contract testA
{
	function stringEquals (
		string memory _a,
		string memory _b
	) public pure returns (bool) {
		bytes memory a = bytes(_a);
		bytes memory b = bytes(_b);
		
		// Check equivalence of the two strings by comparing their contents.
		bool equal = true;
		assembly {
			let length := mload(a)
			switch eq(length, mload(b))

			// Proceed to compare string contents if lengths are equal. 
			case 1 {
				let cb := 1

				// Iterate through the strings and compare contents.
				let mc := add(a, 0x20)
				let end := add(mc, length)
				for {
					let cc := add(b, 0x20)
				} eq(add(lt(mc, end), cb), 2) {
					mc := add(mc, 0x20)
					cc := add(cc, 0x20)
				} {

					// If any of these checks fails then arrays are not equal.
					if iszero(eq(mload(mc), mload(cc))) {
						equal := 0
						cb := 0
					}
				}
			}

			// By default the array length is not equal so the strings are not equal.
			default {
				equal := 0
			}
		}
		return equal;
	}
}

contract testB
{
	function stringEquals (
		string memory _a,
		string memory _b
	) public pure returns (bool) {
        bytes memory a = bytes(_a);
		bytes memory b = bytes(_b);
		if (a.length != b.length) return false;
		return keccak256(a) == keccak256(b);
	}
}

With my result it's cheaper using keccak256:

+ With one character ("a") or 32 characters ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"): 1251 before, 1173 after
+ With 64 characters ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"): 1356 before, 1197 after

Because is only used for if (_stringEquals(class, "Hand of Citadel")) { that is less than 32 bytes, it will be cheaper using keccak256

Affected source code:

• NeoTokyoStaker.sol:824

2. Optimize stake

The following logic is redundant an can be removed because it's not possible to send a different type and also it's checked that the rewardCount has a value with _pools[_assetType].rewardCount == 0.

	function stake (
		AssetType _assetType,
		uint256 _timelockId,
		uint256,
		uint256,
		uint256
	) external nonReentrant {

-		// Validate that the asset being staked is of a valid type.
-		if (uint8(_assetType) > 4) {
-			revert InvalidAssetType(uint256(_assetType));
-		}

		// Validate that the asset being staked matches a configured pool.
		if (_pools[_assetType].rewardCount == 0) {
			revert UnconfiguredPool(uint256(_assetType));
		}

Affected source code:

• NeoTokyoStaker.sol:1205-1207