Neo Tokyo contest - 0xAgro's results

QA Report

Finding Summary

Low Severity

1. Early Use of Compiler Version
2. Rewards Sent on Behalf
3. Precision Loss
4. Lack of Function Misuse Protection

Non-Critical

1. Spelling Mistakes
2. Respect Your Code
3. Multivariate Event Parameters

Low Severity

1. Early Use of Compiler Version

All functions in scope operate under Solidity 0.8.19. Solidity 0.8.19 as of the start of the contest, is less than 3 weeks old.

By using a compiler version early you are volunteering as guinea pigs for potential bugs when it is not necessary. As an example in Solidity 0.8.13, a compiler bug was found 81 days after the release announcement (see links).

Consider downgrading contracts of version less 0.8.19 to a more battle-tested Solidity version.

2. Rewards Sent on Behalf

The getReward function of BYTES2.sol is callable on behalf of any user. There is a comment that states:

This function is called by the S1 Citizen contract to emit BYTES to callers

If the desire is to have getReward only callable by the S1 Citizen contract, consider adding a check that the caller is the S1 Citizen contract only.

By having an external rewards function callable on behalf of users, there is a risk of funds being lost through external user contract assumptions.

3. Precision Loss

There are some instances of precision loss from a divide before mulitply:

1155:	uint256 points = amount * 100 / 1e18 * timelockMultiplier / _DIVISOR;
1388:	uint256 share = points * _PRECISION / pool.totalPoints * totalReward;
1623:	uint256 points = amount * 100 / 1e18 * lpPosition.multiplier / _DIVISOR;

For large inputs, the wrong values are calculated. Consider doing all multiplication before division in all expressions.

4. Lack of Function Misuse Protection

The stake and withdraw functions use ambiguous calldata that depends on the underlying function being called.

For example this comment states:

The third parameter is overloaded to have different meaning depending on the assetType selected.

There is no checks in either the underlying stake or withdraw functions that makes sure the user parameters are reasonable inputs. If a user accidently uses the wrong parameters, it can result in unwanted actions. Consider making each action a different function or add input sanitation if possible.

Non-Critical

1. Spelling Mistakes

There are some spelling mistakes throughout the codebase. Spelling mistakes can confuse readers and possibly result in uninformed user decisions. Consider fixing all spelling mistakes. The following were not found in the automated report.

contracts/staking/NeoTokyoStaker.sol

• The word time is misspelled as tiume.
• The word citizen is misspelled as ctiizen.
• The word configured is misspelled as configued.
• The word entirety is misspelled as entireity.
• The word function is misspelled as funciton.

2. Respect Your Code

Consider respecting your code:

/contracts/staking/NeoTokyoStaker.sol

1335:	// We have no forward goto and thus include this bastardry.

3. Multivariate Event Parameters

In the NeoTokyoStaker.sol contract the Stake and Withdraw events have a multivariate parameter amountOrTokenId. Consider making individual events per staking type.