QA Report

[code423n4]

GeneralVault.sol

• In general, they only put the contracts and no interfaces or other components inside the repository, therefore
  we cannot validate how correct a call to an interface is, for example. How can we know if they are respecting the interface or not.

• Another example of this is the function withdrawCollateral() which is called on line 119 and has the comment: In Lido vault, it will return ETH or stETH to user, but we don't know how it is implemented.

• In the L51 it appears that the vault fee is 20%, it is not clear what that comment refers to.
  This generates confusion since in the setTreasuryInfo() function it allows setting up to 30% of the fee.

LidoVault.sol

• By not having access to the dependencies we cannot know how some calls are specifically implemented,
  for example, on line 57 when TransferHelper.transferFrom() is called, it is not known if some type of reentrance could be generated, for example.

• In most methods you start by setting the LIDO address variable = _addressesProvider.getAddress('LIDO');
  Since both are used and the LIDO value is set in the constructor, the LIDO variable could be created in storage
  and in the constructor set it with this line: "LIDO = _addressesProvider.getAddress('LIDO');"

• Inside the function _withdrawFromYieldPool() when swapExactTokensForTokens(L130) is called, the value of 200 is found as the last parameter reading the code that represents that 200 is very difficult to understand.

YieldManager.sol

• L219 - Validate that _asset meets the interface directly in the signature, this would avoid spending unnecessary gas.

ConvexCurveLPVault.sol

• L40/27/62 - convexBooster address is hardcoded, that means it can only be run on one network, therefore

• it only serves for testing on a certain testnet or only for mainnet.

• It should be a constant and should be directly hardcoded into storage.

• It would be better to simplify the use of interfaces, if in storage convexBooster instead of address directly it is of type IConvexBooster.

• L137 - It is not necessary to request a parameter that can only be unique, therefore it is better not to request the _asset variable in the signature of _depositToYieldPool and remove the require.

• L178 - It is not necessary to request a parameter that can only be unique, therefore it is better not to request the _asset variable in the signature of withdrawOnLiquidationy() removes the require.

• L192 - It is not necessary to request a parameter that can only be unique, therefore it is better not to request the _asset variable in the signature of _withdrawFromYieldPool() removes the require.

[HickupHH3]

working through the bullet pts:

• Disagree, link to full repo was provided. behaviour can be verified.
• Implementation in LidoVault. ETH/stETH were given as an example
• Agree, issue with comment. (L1)
• Invalid, as per first pt.
• Possibly, I would argue it's done the way it is in case LIDO contract changes (NC1)
• Readability issue, but lacks further explanation on what the issue really is. (NC2)
• Only 1 other point is valid of hardcoding an address, but it honestly isn't that big of an issue. Constant declaration probably more valid as a gas opt.

Hence, 1 low and 2 NC have been raised