Sturdy contest - simon135's results

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L150 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L154

Vulnerability details

Impact

(bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}(''); return receivedETHAmount; require statement is never reached. this can be a failed call and not know about it causes function to go on with the desired outcome / return ends the function before the requrire can check for call was succecfull

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. poc in remix : function addnum(uint x, uint y ) public payable returns(uint256) { if (x>y){ bool success=x>y; return x ; require(success,"nice"); }else{ return y; } }

require statement is never reached so you will not know if the call is successfull

Tools Used

remix and vscode ,manaul

Recommended Mitigation Steps

put the require statment before return statmen because return stops execution

Pragma 6.12 old version of solidity use a newer version ,because otherwise you can run into weird bugs and security issues. https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/CollateralAdapter.sol#L2 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L3 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L2 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L2

audit why erc20Detailed uses more gas bec of the extra functions pick and choose your erc20 contract or just pick erc20-detailed because it has also the optional functions —--------------------------------------------------------------------------------------------------- using static varibles that you cant change now what happens if convex contract needs a update and they change addresses may a onlyadmin to change the contracts address https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L54 —-------------------------------------------------------------------------------------------- Dint specify return statement and the contract https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L79

why not use call instead of safetranfer then erc20 transfer and uses the bools and require statement just do it yourself just incase not enough gas is supplied https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L108 put this into a require statement otherwise it can fail and not know if it failed or not a correct input https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L132 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L142 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L177 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L178 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L182 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L181 —-------------------------------------------------------------------- add require statement or bool to make sure it function happend https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L237 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L239

Require statement not needed https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L222 should remove todo in the comment after you implement the _depositToYieldPool https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L84 best practice to remove commented out code https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L165

Solidity 6.12 you need to use safemath library and in newer version of solidity the compiler checks overflow/underflow which saves gas https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/CollateralAdapter.sol#L2 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L2 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L2

Make onlyadmin and initializer functions payable because it doesn't check msg.value == 0 because it saves gas https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/CollateralAdapter.sol#L37 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/CollateralAdapter.sol#L45 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L49 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L114 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L68 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L183 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L32 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L63 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L67 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L76 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L96 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L124

—------------------------------------------------------------------------------------------------------ Use revert instead of require statement and if you upgrade to 0.8.0 and up then gas is lower
https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L51 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L88 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L124 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L173 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L237 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L34 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L40 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L199 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L95 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L106 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L158 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L102 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L218

—---------------------------------------------------------------------------------------------------------------------------- https://docs.soliditylang.org/en/v0.8.13/abi-spec.html#:~:text=Through%20abi.,place%20and%20without%20the%20length. abi.encoded packed, packs the string to ? mayabe use bytes to save gas bec it's less than 32 bytes https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L60 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L59 —---------------------------------------------------------------------------------------------------------------------------- We can make this != because _vault fee cant be less than zero,uint256 internal _vaultFee This will save gas because it only needs to check for anything less than zero https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L95 ///@audit change >0 to != because its uint and is always greater or equal to zero require(yieldStAsset > 0, Errors.VT_PROCESS_YIELD_INVALID);

https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/LidoVault.sol#L40

Waste of variables in memory because we can just put it into function instead https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L97 yieldAmount=yieldAmount.sub(_processTreasury(_asset, yieldAmount)); this instead

you can get rid of baseRewardPool and just put the function inside function bec view function does not cast gas its just reading of blockchain but the variable is mstore https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L117 saves 6 gas mstore and mload just combine and save gas _transferYield(IRewards(IConvexBaseRewardPool(getBaseRewardPool()).extraRewards(i)).rewardToken()); https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L142 —-------------------------------------------- already returns uint8 no need for a extra variable 10**IERC20Detailed(internalAssetToken).decimals()

https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L159 address baseRewardPool = getBaseRewardPool(); IConvexBaseRewardPool(baseRewardPool).withdrawAndUnwrap(_amount, true); We can remove baseRewardPool and just put into the function https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L203 —---------------------------------------------------------------------------------------------------- ///@audit save gas by removing decimal varible bec it already returns a uint8 // uint256 decimal = IERC20Detailed(_asset).decimals();

  _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**IERC20Detailed(_asset).decimals());

Remove decimal variable https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L136 —--------------------------------------------------------------- uint256 yieldStAsset = _getYieldAmount(_stAsset); Waste of yieldstasset you can just return the function and put it into the line after https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L196 —---------------------------------------------------------- //uint256 treasuryStETH = _processTreasury(yieldStETH); yieldStETH = yieldStETH.sub(_processTreasury(yieldStETH)); You can just remove treasury stEth and save mstore and mload —--------------------------------- a waste of variable you can just use _amount or msg.value in the field assetAmount = msg.value; This it a waste just use msg.value and with the assetAmount you can get rid off because you can just use _amount —------------------------------------------------------- Instead of _amount just put it into the function // uint256 _amount = IERC20Detailed(asset).balanceOf(address(this)); _convertAssetToExchangeToken(asset, IERC20Detailed(asset).balanceOf(address(this)));

https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L132 uint256 _amount = _convertToStableCoin(assetYields[i].asset, assetYields[i].amount); // 3. deposit Yield to pool for suppliers _depositYield(assetYields[i].asset, _amount);

Take out _amount and just put that into _depositYield https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L145

—-------------------------------------------------------------------------------------------------------------------------- ++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

https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/ConvexCurveLPVault.sol#L137 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L238 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L127 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L141

make indexed for 3 fields if there are more than 3 fields https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L27 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L29 you can add 256-160 uint96 to this struct if you want without any gas cost https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/GeneralVault.sol#L49 https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L32

you can add 96 uint for free bec mapping starts a new slot and address takes up 160 bits out of 256 bits https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L45 make it _assetList array in memory bec of mappings isn't to bad for gas but its still a lot of gas of storage and its still in a function in a for loop, find way to not use it for a memory variable at least address asset = _assetsList[_offset + i]; https://github.com/code-423n4/2022-05-sturdy/blob/6cc44472f6321d0be6844d6fe7fbd7b78d7602a9/smart-contracts/YieldManager.sol#L129

make it default variable of zero and not initialized the variable to save gas becuase this var is only assigned after and its assignment now is doesn't need it bec it's already a variable in use Also because _totalYieldAmount is used in the math calculation only. uint256 extraYieldAmount = _totalYieldAmount;