Sturdy contest - 0x4non's results

Lines of code

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

Vulnerability details

Vulnerability details

Impact

Sending ETH to LIDO can fail makes a return without checking the sent variable

Proof of Concept

    (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
    return receivedETHAmount;
    require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

Imagine that sent is false, then it will return receivedETHAmount but no ETH was received.

Tools Used

Manual review

Recommended Mitigation Steps

Change

    (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
    return receivedETHAmount;
    require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

For

    (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
    require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);
    return receivedETHAmount;

QA

File CollateralAdapter.sol

Missing event emission

Missing event emission, critical function addCollateralAsset doesnt emmit any event L43

Missing address(0) check

Function addCollateralAsset doesnt validate vars _acceptVault and _internalAsset

L43

Recommentation add validation;

  function addCollateralAsset(
    address _externalAsset,
    address _internalAsset,
    address _acceptVault
  ) external onlyAdmin {
    require(_externalAsset != address(0), "External asset address is not valid");
    require(_internalAsset != address(0), "Internal asset address is not valid");
    _assetToVaults[_externalAsset] = _acceptVault;
    _collateralAssets[_externalAsset] = _internalAsset;
  }

Unused variable

Variable _acceptVault its not used in function addCollateralAsset L43

File ConvexCurveLPVault.sol

Missing event emmision

Critical function withdrawOnLiquidation(address _asset, uint256 _amount) doesnt emit any event L178

File GeneralVault.sol

Unused constant variable

Consider remove the next line thats never used; L47

Simplify if

Consider change >= to > in L196

--- a/contracts/protocol/vault/GeneralVault.sol
+++ b/contracts/protocol/vault/GeneralVault.sol
@@ -193,7 +193,7 @@ contract GeneralVault is VersionedInitializable {
     // when deposit for collateral, stAssetBalance = aTokenBalance
     // But stAssetBalance should increase overtime, so vault can grab yield from lendingPool.
     // yield = stAssetBalance - aTokenBalance
-    if (stAssetBalance >= aTokenBalance) return stAssetBalance.sub(aTokenBalance);
+    if (stAssetBalance > aTokenBalance) return stAssetBalance.sub(aTokenBalance);
 
     return 0;
   }

File YieldManager.sol

Add address(0) validation

Consider add a validario on _asset variable

L73

function registerAsset(address _asset) external onlyAdmin {
    require(_asset != address(0), "Asset address cannot be 0");
    _assetsList[_assetsCount] = _asset;
    _assetsCount = _assetsCount + 1;
  }

Missing event emission on critical functions

Critical Functions setExchangeToken, registerAsset, setCurvePool and distributeYield dont emmit events,

setExchangeToken L64

registerAsset L73

setCurvePool L92

distributeYield L118

Gas optimizations

File ConvexCurveLPVault.sol

Variable convexBooster should be a constant

--- a/contracts/protocol/vault/ethereum/ConvexVault/ConvexCurveLPVault.sol
+++ b/contracts/protocol/vault/ethereum/ConvexVault/ConvexCurveLPVault.sol
@@ -24,7 +24,7 @@ interface IRewards {
 contract ConvexCurveLPVault is GeneralVault {
   using SafeERC20 for IERC20;
 
-  address public convexBooster;
+  address public constant convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
   address internal curveLPToken;
   address internal internalAssetToken;
   uint256 internal convexPoolId;
@@ -37,7 +37,6 @@ contract ConvexCurveLPVault is GeneralVault {
   function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {
     require(internalAssetToken == address(0), Errors.VT_INVALID_CONFIGURATION);
 
-    convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
     curveLPToken = _lpToken;
     convexPoolId = _poolId;
     SturdyInternalAsset _interalToken = new SturdyInternalAsset(

File GeneralVault.sol

i++ is generally more expensive because it must increment a value and "return" the old value, so it may require holding two numbers in memory. ++i only ever uses one number in memory.

--- a/contracts/protocol/vault/GeneralVault.sol
+++ b/contracts/protocol/vault/GeneralVault.sol
@@ -215,7 +215,7 @@ contract GeneralVault is VersionedInitializable {
     AssetYield[] memory assetYields = new AssetYield[](length);
     uint256 extraWETHAmount = _WETHAmount;
 
-    for (uint256 i = 0; i < length; i++) {
+    for (uint256 i = 0; i < length; ++i) {
       assetYields[i].asset = assets[i];
       if (i != length - 1) {
         // Distribute wethAmount based on percent of asset volume

File YieldManaget.sol

i++ is generally more expensive because it must increment a value and "return" the old value, so it may require holding two numbers in memory. ++i only ever uses one number in memory.

--- a/contracts/protocol/vault/YieldManager.sol
+++ b/contracts/protocol/vault/YieldManager.sol
@@ -117,7 +117,7 @@ contract YieldManager is VersionedInitializable, Ownable {
    **/
   function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
     // 1. convert from asset to exchange token via uniswap
-    for (uint256 i = 0; i < _count; i++) {
+    for (uint256 i = 0; i < _count; ++i) {
       address asset = _assetsList[_offset + i];
       require(asset != address(0), Errors.UL_INVALID_INDEX);
       uint256 _amount = IERC20Detailed(asset).balanceOf(address(this));
@@ -127,7 +127,7 @@ contract YieldManager is VersionedInitializable, Ownable {
 
     // 2. convert from exchange token to other stable assets via curve swap
     AssetYield[] memory assetYields = _getAssetYields(exchangedAmount);
-    for (uint256 i = 0; i < assetYields.length; i++) {
+    for (uint256 i = 0; i < assetYields.length; ++i) {
       if (assetYields[i].amount > 0) {
         uint256 _amount = _convertToStableCoin(assetYields[i].asset, assetYields[i].amount);
         // 3. deposit Yield to pool for suppliers
@@ -153,7 +153,7 @@ contract YieldManager is VersionedInitializable, Ownable {
     AssetYield[] memory assetYields = new AssetYield[](length);
     uint256 extraYieldAmount = _totalYieldAmount;
 
-    for (uint256 i = 0; i < length; i++) {
+    for (uint256 i = 0; i < length; ++i) {
       assetYields[i].asset = assets[i];
       if (i != length - 1) {
         // Distribute yieldAmount based on percent of asset volume