Platform: Code4rena
Start Date: 19/04/2022
Pot Size: $30,000 USDC
Total HM: 10
Participants: 43
Period: 3 days
Judges: moose-code, JasoonS
Total Solo HM: 7
Id: 90
League: ETH
Rank: 11/43
Findings: 2
Award: $247.11
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0xDjango, 0xkatana, Dravee, Kenshin, Tadashi, TerrierLover, abhinavmir, defsec, ellahi, fatima_naz, foobar, gzeon, hyh, joestakey, kebabsec, kenta, minhquanym, oyc_109, rayn, robee, sseefried, xpriment626, z3s
104.9942 USDC - $104.99
Few vulnerabilities were found examining the contracts. The main concerns are with standard token methods such as
transfer(),mint()orburn()not being implemented properlyThe presence of an
assertstatement was also found, which is bad practice for functions that can be called by external users.Most of the contracts are using
uintas an alias foruint256. For readability, it is best practice to useuint256
transfer() and transferFrom() should return a boolean. It can be confusing when some functions are expected to return something and others are silent.
Low
Instances include:
vToken.sol:76 vToken.sol:81
IvToken.sol:24 IvToken.sol:48
Manual Analysis
Add a return boolean to these functions
Properly functioning code should never reach a failing assert statement. If it happened, it would indicate the presence of a bug in the contract. A failing assert uses all the remaining gas, which can be financially painful for a user.
Low
Instances include:
IndexLogic.sol:72: assert(minAmountInBase != type(uint).max);
Manual Analysis
Replace the assert statement with a require statement or a custom error
mint() and burn() should check that _recipient is not the zero address.
Low
Instances include:
BaseIndex.sol:43 BaseIndex.sol:59
Manual Analysis
Add a zero address check before performing the delegatecall
uint is an alias for uint256.
It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint
Non-Critical
Instances include:
BaseIndex.sol:78: uint i
AssetInfo.sol:25: uint lastAssetPerBaseInUQ AssetInfo.sol:25: uint assetPerBaseInUQ
IndexLayout.sol:20: uint internal lastTransferTime
IndexLogic.sol:37: uint lastAssetBalanceInBase IndexLogic.sol:38: uint minAmountInBase IndexLogic.sol:39: uint i IndexLogic.sol:44: uint assetPerBaseInUQ IndexLogic.sol:48: uint amountInAsset IndexLogic.sol:49: uint weightedPrice IndexLogic.sol:50: uint _minAmountInBase IndexLogic.sol:54: uint lastBalanceInAsset IndexLogic.sol:56: uint balanceInBase IndexLogic.sol:60: uint i IndexLogic.sol:62: uint lastBalanceInAsset IndexLogic.sol:74: uint value IndexLogic.sol:85: uint fee IndexLogic.sol:97: uint value IndexLogic.sol:99: uint length IndexLogic.sol:102: uint i IndexLogic.sol:112: uint fee IndexLogic.sol:124: uint lastOrderId IndexLogic.sol:125: uint i IndexLogic.sol:132: uint indexAssetBalance IndexLogic.sol:133: uint accountBalance
ManagedIndex.sol:30: uint i
ManagedIndexReweightingLogic.sol:36: uint virtualEvaluationInBase ManagedIndexReweightingLogic.sol:38: uint i ManagedIndexReweightingLogic.sol:39: uint priceAssetPerBaseInUQ ManagedIndexReweightingLogic.sol:40: uint availableAssets ManagedIndexReweightingLogic.sol:46: uint orderId ManagedIndexReweightingLogic.sol:48: uint _totalWeight ManagedIndexReweightingLogic.sol:50: uint i ManagedIndexReweightingLogic.sol:74: uint amountInBase ManagedIndexReweightingLogic.sol:75: uint amountInAsset ManagedIndexReweightingLogic.sol:76: uint newShares ManagedIndexReweightingLogic.sol:76: uint oldShares ManagedIndexReweightingLogic.sol:96: uint i ManagedIndexReweightingLogic.sol:97: uint shares
PhutureIndex.sol:47: uint _value PhutureIndex.sol:56: uint timePassed PhutureIndex.sol:58: uint fee
PhuturePriceOracle.sol:68: uint _baseAmount
TopNMarketCapIndex.sol:22: uint public category TopNMarketCapIndex.sol:23: uint public snapshot TopNMarketCapIndex.sol:39: uint _category TopNMarketCapIndex.sol:40: uint _snapshot TopNMarketCapIndex.sol:43: uint _totalCapitalization TopNMarketCapIndex.sol:48: uint i TopNMarketCapIndex.sol:49: uint _i
TopNMarketCapIndex.sol:31: uint _category TopNMarketCapIndex.sol:32: uint _snapshot TopNMarketCapIndex.sol:33: uint _topN TopNMarketCapIndex.sol:36: uint virtualEvaluationInBase TopNMarketCapIndex.sol:37: uint i TopNMarketCapIndex.sol:38: uint priceAssetPerBaseInUQ TopNMarketCapIndex.sol:39: uint availableAssets TopNMarketCapIndex.sol:48: uint orderId TopNMarketCapIndex.sol:51: uint _i TopNMarketCapIndex.sol:52: uint i TopNMarketCapIndex.sol:57: uint shares TopNMarketCapIndex.sol:89: uint amountInBase TopNMarketCapIndex.sol:90: uint amountInAsset TopNMarketCapIndex.sol:94: uint newShares TopNMarketCapIndex.sol:94: uint oldShares TopNMarketCapIndex.sol:104: uint i TopNMarketCapIndex.sol:105: uint shares
TrackedIndex.sol:28: uint _totalCapitalization TrackedIndex.sol:33: uint maxCapitalization TrackedIndex.sol:35: uint i
TrackedIndexReweightingLogic.sol:30: uint _totalCapitalization TrackedIndexReweightingLogic.sol:33: uint virtualEvaluationInBase TrackedIndexReweightingLogic.sol:35: uint maxCapitalization TrackedIndexReweightingLogic.sol:37: uint i TrackedIndexReweightingLogic.sol:40: uint priceAssetPerBaseInUQ TrackedIndexReweightingLogic.sol:41: uint availableAssets TrackedIndexReweightingLogic.sol:64: uint orderId TrackedIndexReweightingLogic.sol:66: uint i TrackedIndexReweightingLogic.sol:68: uint amountInBase TrackedIndexReweightingLogic.sol:69: uint amountInAsset TrackedIndexReweightingLogic.sol:70: uint newShares TrackedIndexReweightingLogic.sol:70: uint oldShares
UniswapV2PathPriceOracle.sol:32: uint currentAssetPerBaseInUQ UniswapV2PathPriceOracle.sol:34: uint i UniswapV2PathPriceOracle.sol:47: uint currentAssetPerBaseInUQ UniswapV2PathPriceOracle.sol:49: uint i
UniswapV2PriceOracle.sol:19: uint private constant MIN_UPDATE_INTERVAL = 24 hours; UniswapV2PriceOracle.sol:27: uint private price0CumulativeLast; UniswapV2PriceOracle.sol:28: uint private price1CumulativeLast; UniswapV2PriceOracle.sol:30: uint private price0Average; UniswapV2PriceOracle.sol:31: uint private price1Average; UniswapV2PriceOracle.sol:48: uint _price0CumulativeLast; UniswapV2PriceOracle.sol:49: uint _price1CumulativeLast; UniswapV2PriceOracle.sol:50: uint price0Cml; UniswapV2PriceOracle.sol:50: uint price1Cml; UniswapV2PriceOracle.sol:62: uint price0Cumulative; UniswapV2PriceOracle.sol:62: uint price1Cumulative;
vToken.sol:70: uint _amount vToken.sol:76: uint _amount vToken.sol:84: uint _shares vToken.sol:90: uint shares vToken.sol:95: uint _amount vToken.sol:125: uint _amount vToken.sol:145: uint _shares vToken.sol:147: uint amountInAsset vToken.sol:152: uint _amountInAsset vToken.sol:156: uint newShares vToken.sol:156: uint oldShares vToken.sol:159: uint _totalSupply vToken.sol:161: uint _balance vToken.sol:162: uint _assetBalance vToken.sol:163: uint availableAssets vToken.sol:183: uint shares vToken.sol:184: uint _totalAssetSupply vToken.sol:193: uint amount vToken.sol:194: uint shares vToken.sol:208: uint _amount vToken.sol:213: uint _amount vToken.sol:218: uint balance
Manual Analysis
replace uint with
uint256
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0xDjango, 0xNazgul, 0xkatana, Dravee, Kenshin, MaratCerby, Tadashi, TerrierLover, Tomio, TrungOre, defsec, ellahi, fatherOfBlocks, fatima_naz, gzeon, joestakey, kenta, minhquanym, oyc_109, rayn, rfa, robee, simon135, slywaters, windhustler, z3s
142.1175 USDC - $142.12
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.
In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high
Instances include:
scope: mint()
registry is read (2 + assets.length + inactiveAssets.length) times:
number of reads depending on length of assets and inactiveAssets as it is in a for loopIndexLogic.sol:32 IndexLogic.sol:35 IndexLogic.sol:40 IndexLogic.sol:61
assets.length() is read (assets.length) times:
number of reads depending on length of assets as it is in a for loopIndexLogic.sol:39
inactiveAssets.length() is read (inactiveAssets.length) times:
number of reads depending on length of inactiveAssets as it is in a for loopIndexLogic.sol:60
vTokenFactory is read (assets.length + inactiveAssets.length) times:
number of reads depending on length of assets and inactiveAssets as it is in a for loopIndexLogic.sol:47 IndexLogic.sol:63
scope: burn()
registry is read (2 + assets.length + inactiveAssets.length) times:
number of reads depending on length of assets and inactiveAssets as it is in a for loopIndexLogic.sol:103 IndexLogic.sol:110 IndexLogic.sol:123 IndexLogic.sol:127
inactiveAssets.length() is read (assets.length + inactiveAssets.length) times:
number of reads depending on length of assets and inactiveAssets as it is in a for loopIndexLogic.sol:125
vTokenFactory is read (assets.length + inactiveAssets.length) times:
number of reads depending on length of assets and inactiveAssets as it is in a for loopIndexLogic.sol:131
scope: reweight()
registry is read (3 + _updatedAssets.length) times:
number of reads depending on length of _updatedAssets as it is in a for loopManagedIndexReweightingLogic.sol:32 ManagedIndexReweightingLogic.sol:37 ManagedIndexReweightingLogic.sol:45 ManagedIndexReweightingLogic.sol:62
assets.length() is read (assets.length) times:
number of reads depending on length of assetsas it is in a for loopManagedIndexReweightingLogic.sol:38
vTokenFactory is read (assets.length + _updatedAssets.length + _inactiveAssets.length) times:
number of reads depending on length of assets, _updatedAssets and _inactiveAssets as it is in a for loopManagedIndexReweightingLogic.sol:40 ManagedIndexReweightingLogic.sol:76 ManagedIndexReweightingLogic.sol:40
scope: reweight()
registry is read (2 + diff.assetCount) times:
number of reads depending on length of _updatedAssets as it is in a for loopTopNMarketCapReweightingLogic.sol:35 TopNMarketCapReweightingLogic.sol:47 TopNMarketCapReweightingLogic.sol:67
assets.length() is read (assets.length) times:
number of reads depending on length of assetsas it is in a for loopTopNMarketCapReweightingLogic.sol:37
vTokenFactory is read (assets.length + diff.assetCount + _inactiveAssets.length) times:
number of reads depending on length of assets, diff.assetCount and _inactiveAssets as it is in a for loopTopNMarketCapReweightingLogic.sol:39 TopNMarketCapReweightingLogic.sol:54 TopNMarketCapReweightingLogic.sol:105
scope: reweight()
registry is read (3 + assets.length) times:
number of reads depending on length of assets as it is in a for loopTrackedIndexReweightingLogic.sol:29 TrackedIndexReweightingLogic.sol:30 TrackedIndexReweightingLogic.sol:38 TrackedIndexReweightingLogic.sol:63
assets.length() is read (2 * assets.length) times:
number of reads depending on length of assetsas it is in a for loopTrackedIndexReweightingLogic.sol:37 TrackedIndexReweightingLogic.sol:66
vTokenFactory is read (2 * assets.length) times:
number of reads depending on length of assetsas it is in a for loopTrackedIndexReweightingLogic.sol:41 TrackedIndexReweightingLogic.sol:70
scope: refreshedAssetPerBaseInUQ()
path.length is read (path.length - 1) times:
number of reads depending on length of path as it is in a for loopUniswapV2PathPriceOracle.sol:34
scope: lastAssetPerBaseInUQ()
path.length is read (path.length - 1) times:
number of reads depending on length of path as it is in a for loopUniswapV2PathPriceOracle.sol:49
scope: _transferAsset()
asset is read twice:vToken.sol:218 vToken.sol:219
Manual Analysis
cache these storage variables in memory
>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
Instances include:
IndexLogic.sol:76 IndexLogic.sol:98
Manual Analysis
Replace > 0 with !0
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
Instances include:
ManagedIndexReweightingLogic.sol:32
UniswapV2PathPriceOracle.sol:24
UniswapV2PriceOracle.sol:65
Manual Analysis
Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable
example:
-_updatedAssets.length <= IIndexRegistry(registry).maxComponents(); +_updatedAssets.length < IIndexRegistry(registry).maxComponents() + 1;
When the comparison is with a constant storage variable, you can also do the increment in the storage variable declaration
example:
-if (timeElapsed >= MIN_UPDATE_INTERVAL); +if (timeElapsed > MIN_UPDATE_INTERVAL_MINUS_ONE);
and
-uint private constant MIN_UPDATE_INTERVAL = 24 hours; +uint private constant MIN_UPDATE_INTERVAL_MINUS_ONE = 24 hours - 1;
However in this case, the 1 second difference is negligible compared to the value of MIN_UPDATE_INTERVAL, so the following is the best option:
-if (timeElapsed >= MIN_UPDATE_INTERVAL); +if (timeElapsed > MIN_UPDATE_INTERVAL);
and MIN_UPDATE_INTERVAL remains unchanged
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
Instances include:
BaseIndex.sol:34 BaseIndex.sol:49 BaseIndex.sol:65
ChainlinkPriceOracle.sol:51 ChainlinkPriceOracle.sol:61 ChainlinkPriceOracle.sol:62 ChainlinkPriceOracle.sol:86
IndexLogic.sol:40 IndexLogic.sol:76 IndexLogic.sol:98
ManagedIndex.sol:28 ManagedIndex.sol:44 ManagedIndex.sol:54
ManagedIndexReweightingLogic.sol:29 ManagedIndexReweightingLogic.sol:52 ManagedIndexReweightingLogic.sol:58 ManagedIndexReweightingLogic.sol:62 ManagedIndexReweightingLogic.sol:85 ManagedIndexReweightingLogic.sol:104
PhuturePriceOracle.sol:46 PhuturePriceOracle.sol:47 PhuturePriceOracle.sol:56 PhuturePriceOracle.sol:63 PhuturePriceOracle.sol:83 PhuturePriceOracle.sol:93
TopNMarketCapIndex.sol:45 TopNMarketCapIndex.sol:55 TopNMarketCapIndex.sol:73
TopNMarketCapReweightingLogic.sol:67
TrackedIndex.sol:30 TrackedIndex.sol:63
TrackedIndexReweightingLogic.sol:38
UniswapV2PathPriceOracle.sol:24 UniswapV2PathPriceOracle.sol:25
UniswapV2PriceOracle.sol:46 UniswapV2PriceOracle.sol:83
vToken.sol:59 vToken.sol:60 vToken.sol:71
Manual Analysis
Replace require and revert statements with custom errors.
For instance, in TopNMarketCapIndex.sol:
Replace
require(msg.sender == factory, "TopNMarketCapIndex: FORBIDDEN");
with
if (msg.sender != factory) { revert IsNotFactory(msg.sender); }
and define the custom error in the contract
error IsNotFactory(address _address);
Functions that are not used should be removed as they increase the gas cost during deployment
Instances include:
IndexLibrary.sol:24: //amountInAsset() is never used in the contracts.
Manual Analysis
Remove amountInAsset()
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.
Instances include:
UniswapV2PathPriceOracle.sol:34 UniswapV2PathPriceOracle.sol:49
Manual Analysis
Remove explicit initialization for default values.
Prefix increments are cheaper than postfix increments.
Instances include:
UniswapV2PathPriceOracle.sol:34 UniswapV2PathPriceOracle.sol:49
UniswapV2PathPriceOracle.sol:124
Manual Analysis
change i++ to ++i in UniswapV2PathPriceOracle.sol.
change result++ to ++result in FullMath.sol
Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.
Instances include:
ManagedIndexReweightingLogic:29: require( _updatedAssets.length > 1 && _updatedWeights.length == _updatedAssets.length && _updatedAssets.length <= IIndexRegistry(registry).maxComponents(), "ManagedIndex: INVALID" );
Manual Analysis
require(_updatedAssets.length > 1); require(_updatedWeights.length == _updatedAssets.length); require(_updatedAssets.length <= IIndexRegistry(registry).maxComponents(),"ManagedIndex: INVALID");
The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.
if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas
Instances include:
BaseIndex.sol:78: i bounded by _assets.length, overflow check unnecessary
IndexLogic.sol:39: i bounded by assets.length, overflow check unnecessary IndexLogic.sol:60: i bounded by inactiveAssets.length, overflow check unnecessary IndexLogic.sol:102: i bounded by assets.length, overflow check unnecessary IndexLogic.sol:125: i bounded by assets.length+inactiveAssets.length, overflow check unnecessary
ManagedIndex.sol:30: i bounded by _assets.length, overflow check unnecessary
ManagedIndexReweightingLogic.sol:38: i bounded by assets.length, overflow check unnecessary ManagedIndexReweightingLogic.sol:50: i bounded by _updatedAssets.length, overflow check unnecessary ManagedIndexReweightingLogic.sol:96: i bounded by _inactiveAssets.length, overflow check unnecessary
TopNMarketCapIndex.sol:48: i bounded by _assets.length, overflow check unnecessary TopNMarketCapIndex.sol:49: i bounded by _assets.length_ underflow check unnecessary
TopNMarketCapReweightingLogic.sol:37: i bounded by assets.length, overflow check unnecessary TopNMarketCapReweightingLogic.sol:51: i bounded by diff.assetCount, overflow check unnecessary TopNMarketCapReweightingLogic.sol:104: i bounded by _inactiveAssets.length, overflow check unnecessary
TrackedIndex.sol:35: i bounded by _assets.length, overflow check unnecessary
TrackedIndexReweightingLogic.sol:37: i bounded by assets.length, overflow check unnecessary TrackedIndexReweightingLogic.sol:66: i bounded by assets.length, overflow check unnecessary
UniswapV2PathPriceOracle.sol:34: i bounded by path.length, overflow check unnecessary UniswapV2PathPriceOracle.sol:49: i bounded by path.length, overflow check unnecessary
Manual Analysis
Place the arithmetic operations in an unchecked block
Inlining computation can save gas
Instances include:
IndexLogic.sol:56: uint balanceInBase = lastBalanceInAsset.mulDiv(FixedPoint112.Q112, assetPerBaseInUQ); lastAssetBalanceInBase += balanceInBase;
Manual Analysis
lastAssetBalanceInBase += lastBalanceInAsset.mulDiv(FixedPoint112.Q112, assetPerBaseInUQ);
we can remove if statements where unnecessary to save gas.
BaseIndex.sol:51: if (_minAmountInBase < minAmountInBase) { minAmountInBase = _minAmountInBase; }
line 38, minAmountInBase = type(uint).max, hence the condition check line 51 is redundant.
Manual Analysis
Remove the if condition check
minAmountInBase = _minAmountInBase;