Sturdy contest - joestakey's results

QA Report

Table of Contents

• summary
• Coding convention not respected
• Comment Missing function parameter
• Event should be emitted in setters
• Function missing comments
• Related data should be grouped in struct
• Solidity version not up to date
• Typos
• Unchecked inputs

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the lack of events emitted in setters.

Coding convention not respected

PROBLEM

The code layout should follow Solidity's conventions and modifiers should be placed after state variables.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CollateralAdapter.sol

The modifier onlyAdmin() is placed at the beginning of the contract, before the state variables.

TOOLS USED

Manual Analysis

MITIGATION

Place onlyAdmin() after the last state variable, ie _collateralAssets

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

YieldManager.sol

YieldManager.sol:106 function getCurvePool(address _tokenIn, address _tokenOut) //returns missing comment
YieldManager.sol:142 _getAssetYields(uint256 _totalYieldAmount) //_totalYieldAmount missing comment

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ConvexCurveLPVault.sol

ConvexCurveLPVault.sol:37: function setConfiguration

YieldManager.sol

YieldManager.sol:64: function setConfiguration
YieldManager.sol:92: function setCurvePool

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CollateralAdapter.sol

CollateralAdapter.sol:39: getRevision()
CollateralAdapter.sol:43: addCollateralAsset()
CollateralAdapter.sol:52: getAcceptableVault()
CollateralAdapter.sol:56: getInternalCollateralAsset()

ConvexCurveLPVault.sol

ConvexCurveLPVault.sol:87: processYield()
ConvexCurveLPVault.sol:164: _withdraw()
ConvexCurveLPVault.sol:178: withdrawOnLiquidation()

GeneralVault.sol

GeneralVault.sol:65: getRevision()
GeneralVault.sol:235: _depositYield()
GeneralVault.sol:178: withdrawOnLiquidation()
GeneralVault.sol:56: getInternalCollateralAsset()

YieldManager.sol

YieldManager.sol:64: setExchangeToken()
YieldManager.sol:69: getRevision()
YieldManager.sol:73: registerAsset()
YieldManager.sol:78: getAssetCount()
YieldManager.sol:82: getAssetInfo()

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

CollateralAdapter.sol

CollateralAdapter.sol: 27 mapping(address => address) internal _assetToVaults;
CollateralAdapter.sol: 29 mapping(address => address) internal _collateralAssets;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, the mitigation could be:

struct CollateralAsset {
address vault;
address internalAsset;
}

And it would be used as a state variable:

mapping(address =>  CollateralAsset) internal collateralAsset;

Solidity version not up to date

PROBLEM

It is good practice to use the latest versions of Solidity available. If not, you might use some less secure features, and you might get a bigger bytecode out of the compilation process, which will make you pay more gas for deployment.

SEVERITY

Non-critical

PROOF OF CONCEPT

All the contracts in scope are using Solidity 0.6.12

TOOLS USED

Manual Analysis

MITIGATION

Make sure you deploy the contracts using a Solidity version > 0.8.0. You can also remove the pragma experimental ABIEncoderV2 statement as it is a (default from the latest versions of Solidity)[https://docs.soliditylang.org/en/develop/layout-of-source-files.html#abi-coder-pragma], along with the SafeMath library, as overflow/underflow checks are now performed by default in Solidity from 0.8.0 onwards.

Typos

PROBLEM

There are typos present in the contracts.

SEVERITY

Non-critical

PROOF OF CONCEPT

Instances include:

YieldManager.sol

YieldManager.sol:217: The mount of asset

TOOLS USED

Manual Analysis

MITIGATION

change mount to amount

Unchecked inputs

PROBLEM

There should be a zero address check in all setters. Particularly in the setter that sets the Curve token used in the vault, as a wrong address being passed would mean the contract would need to be re-deployed.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ConvexCurveLPVault.sol

ConvexCurveLPVault.sol:37: function setConfiguration() //no check on _lpToken

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check.

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Comparisons with zero for unsigned integers
• Comparison operators
• Custom errors
• Default value initialization
• Prefix increments

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

ConvexCurveLPVault.sol

scope: processYield()

• _addressesProvider is read twice:

ConvexCurveLPVault.sol:93
ConvexCurveLPVault.sol:99

scope: _depositToYieldPool()

• curveLPToken is read 3 times:

ConvexCurveLPVault.sol:137
ConvexCurveLPVault.sol:138
ConvexCurveLPVault.sol:141

• convexBooster is read twice:

ConvexCurveLPVault.sol:141
ConvexCurveLPVault.sol:142

LidoVault.sol

scope: processYield()

• _addressesProvider is read twice:

LidoVault.sol:32
LidoVault.sol:43
LidoVault.sol:44
LidoVault.sol:52
LidoVault.sol:56
LidoVault.sol:59

scope: _depositToYieldPool()

• _addressesProvider is read twice:

LidoVault.sol:84
LidoVault.sol:102

scope: _withdrawFromYieldPool()

• _addressesProvider is read twice:

LidoVault.sol:127
LidoVault.sol:131
LidoVault.sol:132

YieldManager.sol

scope: _convertToStableCoin()

• _exchangeToken is read 3 times:

YieldManager.sol:199
YieldManager.sol:202
YieldManager.sol:205

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Comparisons with zero for unsigned integers

IMPACT

>0 is less gas efficient than != 0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes here

PROOF OF CONCEPT

Instances include:

GeneralVault.sol

GeneralVault.sol:179

LidoVault.sol

LidoVault.sol:88

TOOLS USED

Manual Analysis

MITIGATION

Replace > 0 with != 0

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

GeneralVault.sol

GeneralVault.sol:125: withdrawAmount >= _amount.percentMul(99_00)
GeneralVault.sol:167: _fee <= 30_00
GeneralVault.sol:196: stAssetBalance >= aTokenBalance

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable.

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

ConvexCurveLPVault.sol

ConvexCurveLPVault.sol:38
ConvexCurveLPVault.sol:71
ConvexCurveLPVault.sol:95
ConvexCurveLPVault.sol:101
ConvexCurveLPVault.sol:137
ConvexCurveLPVault.sol:183
ConvexCurveLPVault.sol:184
ConvexCurveLPVault.sol:197

GeneralVault.sol

GeneralVault.sol:125
GeneralVault.sol:166
GeneralVault.sol:167
GeneralVault.sol:179

LidoVault.sol

LidoVault.sol:88
LidoVault.sol:92
LidoVault.sol:97
LidoVault.sol:142
LidoVault.sol:145

YieldManager.sol

YieldManager.sol:65
YieldManager.sol:97
YieldManager.sol:122
YieldManager.sol:203

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in GeneralVault.sol:

Replace

require(yieldStAsset > 0, Errors.VT_PROCESS_YIELD_INVALID

with

if (yieldStAsset == 0) {
		revert YieldInvalid(_stAsset);
}

and define the custom error in the contract

error YieldInvalid(address _stAsset);

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

ConvexCurveLPVault.sol

ConvexCurveLPVault.sol:106: uint256 i = 0;

GeneralVault.sol

GeneralVault.sol:218: uint256 i = 0;

YieldManager.sol

YieldManager.sol:120: uint256 i = 0;
YieldManager.sol:130: uint256 i = 0;
YieldManager.sol:156: uint256 i = 0;

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

LidoVault.sol

LidoVault.sol:59: emit ProcessYield(_addressesProvider.getAddress('WETH'), receivedETHAmount);

TOOLS USED

Manual Analysis

MITIGATION

The storage variable is read multiple times in the function and it is recommended to cache them into memory, then passing the cached variable in the emit statement, as explained in the cache paragraph

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

ConvexCurveLPVault.sol

ConvexCurveLPVault.sol:106: i++

GeneralVault.sol

GeneralVault.sol:218: i++

YieldManager.sol

YieldManager.sol:75: _assetsCount + 1;
YieldManager.sol:120: i++
YieldManager.sol:130: i++
YieldManager.sol:156: i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ and variable + 1 to ++variable.