Yieldy contest - 0xNazgul's results

Max/Infinite Approvals are Dangerous

Severity: Low Context: Staking.sol#L38-L101, Migration.sol#L18-L36, LiquidityReserve.sol#L57-L86

Description: Giving max/infinite approvals to contracts are dangerous because if those contracts are exploited then they can remove all the funds from the approving addresses.

Recommendation: Check allowance and approve as much as required.

Value Range Validity for Setters

Severity Low Context: Staking.sol#L167-L170, Staking.sol#L217-L251

Description: These functions doesn't have any checks to ensure that the variables being set is within some kind of value range.

Recommendation: Each variable input parameter updated should have it's own value range checks to ensure their validity.

Missing Equivalence Checks in Setters

Severity: Low Context: Staking.sol#L157-L251, LiquidityReserve.sol#L57-L86

Description: Setter functions are missing checks to validate if the new value being set is the same as the current value already set in the contract. Such checks will showcase mismatches between on-chain and off-chain states.

Recommendation: This may hinder detecting discrepancies between on-chain and off-chain states leading to flawed assumptions of on-chain state and protocol behavior.

Missing Zero-address Validation

Severity: Low Context: Staking.sol#L157-L160, Staking.sol#L177-L180, LiquidityReserve.sol#L57-L86

Description: Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.

Recommendation: Add explicit zero-address validation on input parameters of address type.

Lack of Event Emission For Critical Functions

Severity: Low Context: Staking.sol#L38-L101, Staking.sol#L246-L251, Yieldy.sol#L29-L47, LiquidityReserve.sol#L36-L50, LiquidityReserve.sol#L57-L86, BatchRequests.sol#L81-L99

Description: Several functions update critical parameters that are missing event emission. These should be performed to ensure tracking of changes of such critical parameters off-chain.

Recommendation: Add events to functions that change critical parameters.

Spelling Errors

Severity: Informational Context: Staking.sol#L32 (affilateAddress => affiliateAddress)

Description: Spelling errors in comments can cause confusion to both users and developers.

Recommendation: Check all misspellings to ensure they are corrected.

Be Aware of the Elastic Supply Tokens

Severity: Informational Context: All Contracts

Description: Elastic supply tokens could dynamically adjust their price, supply, user's balance, etc. Such a mechanism makes a DeFi system complex, while many security accidnets are caused by the elastic tokens. For example, a DEX using deflationary token must double check the token transfer amount when taking swap action because of the difference of actual transfer amount and parameter.

Recommendation: In terms of confidentiality, integrity and availability, it is highly recommend that one should not use elastic supply tokens.

Missing or Incomplete NatSpec

Severity: Informational Context: All Contracts

Description: Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.

Recommendation: Add in full NatSpec comments for all functions to have complete code documentation for future use.

In require(), Use != 0 Instead of > 0 With Uint Values

Context: Staking.sol#L111-L123, Staking.sol#L406-L450 (For L410), Staking.sol#L571-L592 (For L572), Staking.sol#L600-L627 (For L604), Yieldy.sol#L78-L102 (For L83 && L96)

Description: In a require, when checking a uint, using != 0 instead of > 0 saves 6 gas. This will jump over or avoid an extra ISZERO opcode.

Recommendation: Use != 0 instead of > 0 with uint values but only in require() statements.

Same State Variable Read More Than Once

Context: Staking.sol#L129-L134 (For both FEE_ADDRESS && affiliateFee), Staking.sol#L319-L327 (For TOKE_POOL), Staking.sol#L517-L565 (For YIELDY_TOKEN), Staking.sol#L571-L592 (For LIQUIDITY_RESERVE), Staking.sol#L571-L592 (For both TOKE_POOL && CURVE_POOL)

Description: Functions that read state variables more than once can cach it into a local variable for repeated reads saving gas by converting expensive SLOADs into much cheaper MLOADs.

Recommendation: It's best to cach the state variables into memory when read more than once.

Use of Custom Errors Instead of Strings

Context: All Contracts

Description: To save some gas the use of custom errors leads to cheaper deploy time cost and run time cost. The run time cost is only relevant when the revert condition is met.

Recommendation: Use Custom Errors instead of strings.

Setting The Constructor To Payable

Context: All Contracts

Description: You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 21 gas on deployment with no security risks.

Recommendation: Set the constructor to payable.

Function Ordering via Method ID

Context: All Contracts

Description: Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. One could use This tool to help find alternative function names with lower Method IDs while keeping the original name intact.

Recommendation: Find a lower method ID name for the most called functions for example mostCalled() vs. mostCalled_41q() is cheaper by 44 gas.