Sturdy contest - fatherOfBlocks's results

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L140

Vulnerability details

Impact

In lines 140 and 141 a low-level transfer is performed and first it is reversed and then it is validated with the require if it was done correctly. This can generate many problems, since the transaction may not be carried out and a message may be returned as if it had been carried out correctly.

Recommended Mitigation Steps

You must first perform the validation (line 140) and then perform the return (line 141)

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.

GeneralVault.sol

• L29/L34: You can save 20,000 gas if instead of using modifiers you use a private view function.

• L218: In the for loop you could save 25,000 gas if instead of using uint256 i= 0, you didn't set it to its default value. Also, instead of terminating with i++ to raise the value of i, use unchecked with ++i on the last line of the loop.

YieldManager.sol

• L51: the onlyOwner() could generate a saving of 20,000 gas, if it is created as a private view.

• L120/130/156: In the for loop you could save 25,000 gas if instead of using uint256 i= 0, you didn't set it to its default value. Also, instead of terminating with i++ to raise the value of i, use unchecked with ++i on the last line of the loop.

CollateralAdapter.sol

• L17: the onlyOwner() could generate a saving of 20,000 gas, if it is created as a private view.

ConvexCurveLPVault.sol

• L106: In the for loop you could save 25,000 gas if instead of using uint256 i= 0, you didn't set it to its default value. Also, instead of terminating with i++ to raise the value of i, use unchecked with ++i on the last line of the loop.