Sturdy contest - 0x1f8b's results

Lack of empty address checks

The following methods have a lack 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).

Source code:

• CollateralAdapter.sol#L36
• CollateralAdapter.sol#L44-L46
• ConvexCurveLPVault.sol#L37
• GeneralVault.sol#L62
• YieldManager.sol#L61
• YieldManager.sol#L73

Change important values without emitting an event

When features are modified that affect the project's economics or ecology, it's critical to send out an event so that users and dapps can react appropriately.

Source code:

• CollateralAdapter.sol#L44-L46

Wrong type used decimals uint8

The ERC20 standard establishes that the decimals method is of type uint8, however a variable of type uint256 is used to handle it.

Source code:

• ConvexCurveLPVault.sol#L124
• GeneralVault.sol#L122

As stated in the OpenZeppelin source comments, the OpenZeppeling ERC20 safeApprove() function has been deprecated.

Using this deprecated function could result in accidental reverts and possibly fund locking. The deprecation of this function is discussed in greater depth in the OZ issue #2219.

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead.

Source code:

• ConvexCurveLPVault.sol#L141-L146
• LidoVault.sol#L102

The variable _assetsCount in the YieldManager contract could be out of sync with the total assets.

The number could be different if the owner sent the same _asset twice because the assets were not checked to see if they were already in _assetsList.

Source code:

• YieldManager.sol#L75

unsafe approve

It was found some approve without checking the boolean result, ERC20 standard specify that the token can return false if this call was not made, so it's mandatory to check the result of these methods.

Source code:

• YieldManager.sol#L221

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Source code:

• ConvexCurveLPVault.sol#L106
• GeneralVault.sol#L218
• YieldManager.sol#L120
• YieldManager.sol#L130
• YieldManager.sol#L156