Sturdy contest - oyc_109's results

Lines of code

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

Vulnerability details

description

the require statement in the _withdrawFromYieldPool function is never checked since it is after the return statement

/2022-05-sturdy/smart-contracts/LidoVault.sol
140:       (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
141:       return receivedETHAmount;
142:       require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

since the _to address is an user controlled parameter, it is possible that if the user inputs a bad address the transfer will fail silently and cause the user to lose funds

missing checks for zero address

description

the function addCollateralAsset does not check if invalid addresses are added to a vault, eg zero address checks

Checking addresses against zero-address during initialization or during setting is a security best-practice.

findings

/2022-05-sturdy/smart-contracts/CollateralAdapter.sol
43:   function addCollateralAsset(
44:     address _externalAsset,
45:     address _internalAsset,
46:     address _acceptVault
47:   ) external onlyAdmin {
48:     _assetToVaults[_externalAsset] = _acceptVault;
49:     _collateralAssets[_externalAsset] = _internalAsset;
50:   }
51: 

/2022-05-sturdy/smart-contracts/YieldManager.sol
60:  function initialize(ILendingPoolAddressesProvider _provider) public initializer {
61:     _addressesProvider = _provider;
62:   }

hard coded addresses

description

Use of hard-coded addresses may cause errors: Each contract needs contract addresses in order to be integrated into other protocols and systems. These addresses are currently hard-coded, which may cause errors and result in the codebase’s deployment with an incorrect asset. Using hard-coded values instead of deployer-provided values makes these contracts incredibly difficult to test.

Recommendation: set addresses when contracts are created rather than using hard-coded values. This practice will facilitate testing and reused across networks

findings

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
40: convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;

unnecessary parameter

description

The function _depositToYieldPool() takes in an address parameter _asset.

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
131: function _depositToYieldPool(address _asset, uint256 _amount)

This parameter is not used within the function except in the require statement

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
137: require(_asset == curveLPToken, Errors.VT_COLLATERAL_DEPOSIT_INVALID);

therefore this parameter can be removed from the function

this also applies to

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
154: function _getWithdrawalAmount(address _asset, uint256 _amount)
178: function withdrawOnLiquidation(address _asset, uint256 _amount)
192:   function _withdrawFromYieldPool(
193:     address _asset,

missing payable modifier

description

the function _depositToYieldPool may deposit ETH however it does not have the payable modifier

/2022-05-sturdy/smart-contracts/LidoVault.sol
79:   function _depositToYieldPool(address _asset, uint256 _amount)
80:     internal
81:     override
82:     returns (address, uint256)

do not cache variable used only once

description

for variables only used once, changing it to inline saves gas

findings

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
62: IConvexBooster.PoolInfo memory poolInfo = IConvexBooster(convexBooster).poolInfo(convexPoolId);
76: uint256 treasuryAmount = _processTreasury(_asset, yieldAmount);
81: address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER');
107:       address _extraReward = IConvexBaseRewardPool(baseRewardPool).extraRewards(i);
108:       address _rewardToken = IRewards(_extraReward).rewardToken();

/2022-05-sturdy/smart-contracts/GeneralVault.sol
119: uint256 withdrawAmount = _withdrawFromYieldPool(_asset, _amountToWithdraw, _to);
122: uint256 decimal = IERC20Detailed(_asset).decimals();

/2022-05-sturdy/smart-contracts/LidoVault.sol
37: uint256 treasuryStETH = _processTreasury(yieldStETH);

cache in variables instead of loading

description

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

findings

/2022-05-sturdy/smart-contracts/YieldManager.sol
130: for (uint256 i = 0; i < assetYields.length; i++) {

using prefix increments save gas

description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

findings

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
106: for (uint256 i = 0; i < extraRewardsLength; i++) {

/2022-05-sturdy/smart-contracts/GeneralVault.sol
218: for (uint256 i = 0; i < length; i++) {

/2022-05-sturdy/smart-contracts/YieldManager.sol
120: for (uint256 i = 0; i < _count; i++) {
130: for (uint256 i = 0; i < assetYields.length; i++) {
156: for (uint256 i = 0; i < length; i++) {

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

/2022-05-sturdy/smart-contracts/ConvexCurveLPVault.sol
106: for (uint256 i = 0; i < extraRewardsLength; i++) {

/2022-05-sturdy/smart-contracts/GeneralVault.sol
218: for (uint256 i = 0; i < length; i++) {

/2022-05-sturdy/smart-contracts/YieldManager.sol
120: for (uint256 i = 0; i < _count; i++) {
130: for (uint256 i = 0; i < assetYields.length; i++) {
156: for (uint256 i = 0; i < length; i++) {