ToB: Code Quality Recommendations

[ewilz]

Code Quality Recommendations The following recommendations are not associated with specific vulnerabilities. However, they enhance code readability and may prevent the introduction of vulnerabilities in the future.

• Informal Specification
  • ~Specify the expected behavior of claimReward when the amount requested is zero. The current implementation of claimReward transfers the complete reward when the amountRequested parameter is zero. However, this could be unexpected by the users and should be properly documented in the specification.~ close via #210
• Staking Contract.
  • Considering disallowing the usage of 0-length arrays in onERC721Received. The function documentation states "if data.length>0, [it] stakes the token". However, if a 0-length array is encoded in the data parameter, then no token will be staked.
  • ~Considering disallowing reentrant calls in withdrawToken. The withdrawToken function can trigger a reentrant call using onERC721Received if a user calls it using the staking contract address as destination. Moreover, the staking contract can force to stake the received tokens using the data parameter. While we cannot find any security or correctness issues in this corner case, it is unclear how such functionality can be used by users.~ closed via #201
  • ~Properly document the design decision on the usage of the fields in onERC721Received. In the onERC721Received function, the operator field is ignored and the from field is used as the owner for the staking. While we understand this decision, it should be properly documented to make sure the code is easy to understand, review and maintain.~ closed via #210
  • Considering simplifying the code that keeps track of staked liquidity. The staked liquidity is counted in two fields: liquidityNoOverflow and liquidityIfOverflo. However, it is possible to keep a single uint128 liquidity field. This will make the code easier to understand, review and maintain.
  • Considering refactoring the getRewardInfo and unstakeToken. These two functions share a common logic related with the computation of the reward. Reducing code duplication will make the codebase easier to understand, review and maintain.

[moodysalem]

IMO the only remaining things we should implement are the two documentation notes

Specify the expected behavior of claimReward when the amount requested is zero

and

Properly document the design decision on the usage of the fields in onERC721Received

The others are not important or deliberate (e.g. splitting liquidity into two slots is a gas optimization)