Phuture Finance contest - Dravee's results

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L83-L86

Vulnerability details

Impact

When using Chainlink Price feeds, it is important to ensure the price feed data was updated recently. While getting started with chainlink requires just one line of code, it is best to add additional checks for "in production" environments.

Here, latestRoundData() is missing additional validation to ensure that the round is complete and has returned a valid/expected price. This is documented here: https://docs.chain.link/ethereum/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round.

Proof of Concept

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L83-L86

File: ChainlinkPriceOracle.sol
80:     function refreshedAssetPerBaseInUQ(address _asset) public override returns (uint) {
...
83:         (, int basePrice, , , ) = baseAggregator.latestRoundData();
84:         (, int quotePrice, , , ) = assetInfo.aggregator.latestRoundData();
85: 
86:         require(basePrice > 0 && quotePrice > 0, "ChainlinkPriceOracle: NEGATIVE");

Tools Used

Manual code review. Chainlink best practices.

Recommended Mitigation Steps

Consider adding missing checks for stale data.

As an example:

        (uint80 baseRoundID, int256 basePrice, , uint256 baseTimestamp, uint80 baseAnsweredInRound) = baseAggregator.latestRoundData();
        require(basePrice > 0, "ChainlinkPriceOracle: Base price <= 0");
        require(baseAnsweredInRound >= baseRoundID, "ChainlinkPriceOracle: Stale Base price");
        require(baseTimestamp > 0, "ChainlinkPriceOracle: Base Round not complete");

        (uint80 quoteRoundID, int256 quotePrice, , uint256 quoteTimestamp, uint80 quoteAnsweredInRound) = assetInfo.aggregator.latestRoundData();
        require(quotePrice > 0, "ChainlinkPriceOracle: Quote price <= 0");
        require(quoteAnsweredInRound >= quoteRoundID, "ChainlinkPriceOracle: Stale Quote price");
        require(quoteTimestamp > 0, "ChainlinkPriceOracle: Quote round not complete");

QA Report

[L-01] Add constructor initializers

As per OpenZeppelin’s (OZ) recommendation, “The guidelines are now to make it impossible for anyone to run initialize on an implementation contract, by adding an empty constructor with the initializer modifier. So the implementation contract gets initialized automatically upon deployment.”

Note that this behaviour is also incorporated the OZ Wizard since the UUPS vulnerability discovery: “Additionally, we modified the code generated by the Wizard 19 to include a constructor that automatically initializes the implementation when deployed.”

Furthermore, this thwarts any attempts to frontrun the initialization tx of these contracts:

contracts/ManagedIndex.sol:
  27:     function initialize(address[] calldata _assets, uint8[] calldata _weights) external {

contracts/TopNMarketCapIndex.sol:
  37:     function initialize(

contracts/TrackedIndex.sol:
  25:     function initialize(

contracts/vToken.sol:
  55:     function initialize(address _asset, address _registry) external override initializer {

[L-02] require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:

IndexLogic.sol:72:        assert(minAmountInBase != type(uint).max);

As the Solidity version is > 0.8.0 the remaining gas would still be refunded in case of failure.

[L-03] Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

vToken.sol:210:        _NAV.transfer(_from, _to, _amount);

[N-01] Avoid floating pragmas: the version should be locked

The declared pragma in the solution is pragma solidity >=0.8.7; but this should be locked.

[N-02] Missing comments

The following comments are missing (see @audit tags):

contracts/interfaces/IFeePool.sol:
   8      /// @notice Minting fee in base point format
   9:     /// @return Returns minting fee in base point (BP) format  //@audit NC: missing @param _index
  10      function mintingFeeInBPOf(address _index) external view returns (uint16);
  11  
  12      /// @notice Burning fee in base point format
  13:     /// @return Returns burning fee in base point (BP) format  //@audit NC: missing @param _index
  14      function burningFeeInBPOf(address _index) external view returns (uint16);
  15  
  16      /// @notice AUM scaled per seconds rate
  17:     /// @return Returns AUM scaled per seconds rate  //@audit NC: missing @param _index
  18      function AUMScaledPerSecondsRateOf(address _index) external view returns (uint);

contracts/interfaces/IPriceOracle.sol:
   8      /// @notice Updates and returns asset per base
   9:     /// @return Asset per base in UQ  //@audit NC: missing @param _asset
  10      function refreshedAssetPerBaseInUQ(address _asset) external returns (uint);
  11  
  12      /// @notice Returns last asset per base
  13:     /// @return Asset per base in UQ  //@audit NC: missing @param _asset
  14      function lastAssetPerBaseInUQ(address _asset) external view returns (uint);

contracts/interfaces/IvToken.sol:
  72      /// @notice Returns amount of assets for the given account with the given shares amount 
  73:     /// @return Amount of assets for the given account with the given shares amount  //@audit NC: missing @param * 2
  74      function assetDataOf(address _account, uint _shares) external view returns (AssetData memory);

contracts/interfaces/IvTokenFactory.sol:
  8      /// @notice Creates or returns address of previously created vToken for the given asset
  9:     /// @param _asset Asset to create or return vToken for  //@audit NC: missing @return address
  10      function createOrReturnVTokenOf(address _asset) external returns (address);

contracts/libraries/NAV.sol:
  18      /// @notice Transfer `_amount` of shares between given addresses
  19:     /// @param _from Account to send shares from //@audit NC: missing @param self
  20      /// @param _to Account to send shares to
  21      /// @param _amount Amount of shares to send
  22      function transfer(

  32      /// @notice Mints shares to the `_recipient` account
  33      /// @param self Data structure reference
  34      /// @param _balance New shares maximum limit
  35:     /// @param _recipient Recipient that will receive minted shares //@audit NC: missing @return shares
  36      function mint(

  53      /// @notice Burns shares from the `_recipient` account
  54      /// @param self Data structure reference
  55:     /// @param _balance Shares balance //@audit NC: missing @return amount
  56      function burn(Data storage self, uint _balance) internal returns (uint amount) {

Table of Contents:

• No need to explicitly initialize variables with default values
• Some variables should be immutable
• Caching storage values in memory
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Increments can be unchecked
• Unchecking arithmetics operations that can't underflow/overflow
• Reduce the size of error messages (Long revert Strings)
• Use Custom Errors instead of Revert Strings to save Gas

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

UniswapV2PathPriceOracle.sol:34:        for (uint i = 0; i < path.length - 1; i++) {
UniswapV2PathPriceOracle.sol:49:        for (uint i = 0; i < path.length - 1; i++) {

I suggest removing explicit initializations for default values.

Some variables should be immutable

These variables are only set in the constructor and are never edited after that:

File: PhuturePriceOracle.sol
23:     /// @notice Base asset address
24:     address public base; //@audit gas: should be immutable
25: 
26:     /// @notice Index registry address
27:     address public registry; //@audit gas: should be immutable

Consider marking them as immutable.

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

contracts/PhuturePriceOracle.sol:
  83:         require(priceOracleOf[_asset] != address(0), "PhuturePriceOracle: UNSET"); //@audit gas: should cache priceOracleOf[_asset]
  84:         return IPriceOracle(priceOracleOf[_asset]).refreshedAssetPerBaseInUQ(_asset); //@audit gas: should use cached priceOracleOf[_asset]
  93:         require(priceOracleOf[_asset] != address(0), "PhuturePriceOracle: UNSET"); //@audit gas: should cache priceOracleOf[_asset]
  94:         return IPriceOracle(priceOracleOf[_asset]).lastAssetPerBaseInUQ(_asset); //@audit gas: should use cached priceOracleOf[_asset]

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

libraries/FullMath.sol:35:                require(denominator > 0);
libraries/IndexLibrary.sol:29:        require(_assetPerBaseInUQ > 0, "IndexLibrary: ORACLE");
libraries/NAV.sol:49:        require(shares > 0, "NAV: INSUFFICIENT_AMOUNT");
libraries/NAV.sol:59:        require(amount > 0, "NAV: INSUFFICIENT_SHARES_BURNED");
ChainlinkPriceOracle.sol:86:        require(basePrice > 0 && quotePrice > 0, "ChainlinkPriceOracle: NEGATIVE");
IndexLogic.sol:76:            require(lastAssetBalanceInBase > 0, "Index: INSUFFICIENT_AMOUNT");
IndexLogic.sol:98:        require(value > 0, "Index: INSUFFICIENT_AMOUNT");

Also, please enable the Optimizer.

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

BaseIndex.sol:78:        for (uint i; i < _assets.length; ++i) {
IndexLogic.sol:39:        for (uint i; i < assets.length(); ++i) {
IndexLogic.sol:60:        for (uint i; i < inactiveAssets.length(); ++i) {
IndexLogic.sol:125:        for (uint i; i < length + inactiveAssets.length(); ++i) {
ManagedIndex.sol:30:        for (uint i; i < _assets.length; ++i) {
ManagedIndexReweightingLogic.sol:38:        for (uint i; i < assets.length(); ++i) {
ManagedIndexReweightingLogic.sol:50:        for (uint i; i < _updatedAssets.length; ++i) {
ManagedIndexReweightingLogic.sol:96:        for (uint i; i < _inactiveAssets.length; ++i) {
TopNMarketCapIndex.sol:48:        for (uint i; i < _assets.length; ++i) {
TopNMarketCapReweightingLogic.sol:37:        for (uint i; i < assets.length(); ++i) {
TopNMarketCapReweightingLogic.sol:51:        for (uint _i; _i < diff.assetCount; ++_i) {
TopNMarketCapReweightingLogic.sol:104:        for (uint i; i < _inactiveAssets.length; ++i) {
TrackedIndex.sol:35:        for (uint i; i < _assets.length; ++i) {
TrackedIndexReweightingLogic.sol:37:        for (uint i; i < assets.length(); ++i) {
TrackedIndexReweightingLogic.sol:66:        for (uint i; i < assets.length(); ++i) {
UniswapV2PathPriceOracle.sol:34:        for (uint i = 0; i < path.length - 1; i++) {
UniswapV2PathPriceOracle.sol:49:        for (uint i = 0; i < path.length - 1; i++) {

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

Instances include:

libraries/FullMath.sol:124:                result++;
UniswapV2PathPriceOracle.sol:34:        for (uint i = 0; i < path.length - 1; i++) {
UniswapV2PathPriceOracle.sol:49:        for (uint i = 0; i < path.length - 1; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

BaseIndex.sol:78:        for (uint i; i < _assets.length; ++i) {
IndexLogic.sol:39:        for (uint i; i < assets.length(); ++i) {
IndexLogic.sol:60:        for (uint i; i < inactiveAssets.length(); ++i) {
IndexLogic.sol:102:        for (uint i; i < length; ++i) {
IndexLogic.sol:125:        for (uint i; i < length + inactiveAssets.length(); ++i) {
ManagedIndex.sol:30:        for (uint i; i < _assets.length; ++i) {
ManagedIndexReweightingLogic.sol:38:        for (uint i; i < assets.length(); ++i) {
ManagedIndexReweightingLogic.sol:50:        for (uint i; i < _updatedAssets.length; ++i) {
ManagedIndexReweightingLogic.sol:96:        for (uint i; i < _inactiveAssets.length; ++i) {
TopNMarketCapIndex.sol:48:        for (uint i; i < _assets.length; ++i) {
TopNMarketCapReweightingLogic.sol:37:        for (uint i; i < assets.length(); ++i) {
TopNMarketCapReweightingLogic.sol:51:        for (uint _i; _i < diff.assetCount; ++_i) {
TopNMarketCapReweightingLogic.sol:104:        for (uint i; i < _inactiveAssets.length; ++i) {
TrackedIndex.sol:35:        for (uint i; i < _assets.length; ++i) {
TrackedIndexReweightingLogic.sol:37:        for (uint i; i < assets.length(); ++i) {
TrackedIndexReweightingLogic.sol:66:        for (uint i; i < assets.length(); ++i) {
UniswapV2PathPriceOracle.sol:34:        for (uint i = 0; i < path.length - 1; i++) {
UniswapV2PathPriceOracle.sol:49:        for (uint i = 0; i < path.length - 1; i++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tags for more details):

contracts/ManagedIndexReweightingLogic.sol:
  80:                     orderer.addOrderDetails(orderId, asset, newShares - oldShares, IOrderer.OrderSide.Buy); //@audit gas: can be unchecked
  82:                     orderer.addOrderDetails(orderId, asset, oldShares - newShares, IOrderer.OrderSide.Sell); //@audit gas: can be unchecked

contracts/TopNMarketCapReweightingLogic.sol:
  96:                         orderer.addOrderDetails(orderId, asset, newShares - oldShares, IOrderer.OrderSide.Buy); //@audit gas: can be unchecked
  98:                         orderer.addOrderDetails(orderId, asset, oldShares - newShares, IOrderer.OrderSide.Sell); //@audit gas: can be unchecked

contracts/TrackedIndex.sol:
  51:             weightOf[maxCapitalizationAsset] += IndexLibrary.MAX_WEIGHT - totalWeight;  //@audit gas: can be unchecked

contracts/TrackedIndexReweightingLogic.sol:
  59:             weightOf[maxCapitalizationAsset] += IndexLibrary.MAX_WEIGHT - totalWeight; //@audit gas: can be unchecked
  75:                 orderer.addOrderDetails(orderId, asset, newShares - oldShares, IOrderer.OrderSide.Buy); //@audit gas: can be unchecked
  77:                 orderer.addOrderDetails(orderId, asset, oldShares - newShares, IOrderer.OrderSide.Sell); //@audit gas: can be unchecked

contracts/UniswapV2PathPriceOracle.sol:
  25:         require(_oracles.length == _path.length - 1, "UniswapV2PathPriceOracle: ORACLES"); //@audit gas: can be unchecked

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

TopNMarketCapIndex.sol:74:                revert("TopNMarketCapIndex: REWEIGH_FAILED");
TopNMarketCapReweightingLogic.sol:67:                require(IAccessControl(registry).hasRole(ASSET_ROLE, asset), "TopNMarketCapIndex: INVALID_ASSET");
UniswapV2PathPriceOracle.sol:25:        require(_oracles.length == _path.length - 1, "UniswapV2PathPriceOracle: ORACLES"); 

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

libraries/FullMath.sol:35:                require(denominator > 0);
libraries/FullMath.sol:44:            require(denominator > prod1);
libraries/FullMath.sol:123:                require(result < type(uint256).max);
libraries/IndexLibrary.sol:29:        require(_assetPerBaseInUQ > 0, "IndexLibrary: ORACLE");
libraries/NAV.sol:49:        require(shares > 0, "NAV: INSUFFICIENT_AMOUNT");
libraries/NAV.sol:59:        require(amount > 0, "NAV: INSUFFICIENT_SHARES_BURNED");
BaseIndex.sol:29:        require(IAccessControl(registry).hasRole(role, msg.sender), "GovernableIndex: FORBIDDEN");
BaseIndex.sol:34:        require(_factory.supportsInterface(type(IIndexFactory).interfaceId), "BaseIndex: INTERFACE");
ChainlinkPriceOracle.sol:51:        require(_baseAggregator != address(0) && _base != address(0), "ChainlinkPriceOracle: ZERO");
ChainlinkPriceOracle.sol:61:        require(registry.hasRole(ASSET_MANAGER_ROLE, msg.sender), "ChainlinkPriceOracle: FORBIDDEN");
ChainlinkPriceOracle.sol:62:        require(_asset != address(0), "ChainlinkPriceOracle: ZERO");
ChainlinkPriceOracle.sol:86:        require(basePrice > 0 && quotePrice > 0, "ChainlinkPriceOracle: NEGATIVE");
IndexLogic.sol:40:            require(IAccessControl(registry).hasRole(ASSET_ROLE, assets.at(i)), "Index: INVALID_ASSET");
IndexLogic.sol:76:            require(lastAssetBalanceInBase > 0, "Index: INSUFFICIENT_AMOUNT");
IndexLogic.sol:98:        require(value > 0, "Index: INSUFFICIENT_AMOUNT");
ManagedIndex.sol:28:        require(msg.sender == factory, "ManagedIndex: FORBIDDEN");
ManagedIndex.sol:44:        require(
ManagedIndexReweightingLogic.sol:29:        require(
ManagedIndexReweightingLogic.sol:52:            require(asset != address(0), "ManagedIndex: ZERO");
ManagedIndexReweightingLogic.sol:58:                require(_updatedAssets[i - 1] < asset, "ManagedIndex: SORT");
ManagedIndexReweightingLogic.sol:62:                require(IAccessControl(registry).hasRole(ASSET_ROLE, asset), "ManagedIndex: INVALID_ASSET");
ManagedIndexReweightingLogic.sol:85:                require(assets.remove(asset), "ManagedIndex: INVALID");
ManagedIndexReweightingLogic.sol:104:        require(_totalWeight == IndexLibrary.MAX_WEIGHT, "ManagedIndex: MAX");
PhuturePriceOracle.sol:38:        require(IAccessControl(registry).hasRole(_role, msg.sender), "PhuturePriceOracle: FORBIDDEN");
PhuturePriceOracle.sol:46:        require(_registry.supportsAllInterfaces(interfaceIds), "PhuturePriceOracle: INTERFACE");
PhuturePriceOracle.sol:47:        require(_base != address(0), "PhuturePriceOracle: ZERO");
PhuturePriceOracle.sol:56:        require(_oracle.supportsInterface(type(IPriceOracle).interfaceId), "PhuturePriceOracle: INTERFACE");
PhuturePriceOracle.sol:63:        require(priceOracleOf[_asset] != address(0), "PhuturePriceOracle: UNSET");
PhuturePriceOracle.sol:83:        require(priceOracleOf[_asset] != address(0), "PhuturePriceOracle: UNSET");
PhuturePriceOracle.sol:93:        require(priceOracleOf[_asset] != address(0), "PhuturePriceOracle: UNSET");
TopNMarketCapIndex.sol:45:        require(msg.sender == factory, "TopNMarketCapIndex: FORBIDDEN");
TopNMarketCapIndex.sol:55:            require(asset != address(0), "TopNMarketCapIndex: ZERO");
TopNMarketCapReweightingLogic.sol:67:                require(IAccessControl(registry).hasRole(ASSET_ROLE, asset), "TopNMarketCapIndex: INVALID_ASSET");
TrackedIndex.sol:30:        require(msg.sender == factory, "TrackedIndex: FORBIDDEN");
TrackedIndexReweightingLogic.sol:38:            require(IAccessControl(registry).hasRole(ASSET_ROLE, assets.at(i)), "TrackedIndex: INVALID_ASSET");
UniswapV2PathPriceOracle.sol:24:        require(_path.length >= 2, "UniswapV2PathPriceOracle: PATH");
UniswapV2PathPriceOracle.sol:25:        require(_oracles.length == _path.length - 1, "UniswapV2PathPriceOracle: ORACLES");
UniswapV2PriceOracle.sol:46:        require(reserve0 != 0 && reserve1 != 0, "UniswapV2PriceOracle: RESERVES");
UniswapV2PriceOracle.sol:83:            require(_asset == asset1, "UniswapV2PriceOracle: UNKNOWN");
vToken.sol:46:        require(IAccessControl(registry).hasRole(_role, msg.sender), "vToken: FORBIDDEN");
vToken.sol:59:        require(_registry.supportsAllInterfaces(interfaceIds), "vToken: INTERFACE");
vToken.sol:60:        require(_asset != address(0), "vToken: ZERO");
vToken.sol:71:        require(msg.sender == IIndexRegistry(registry).orderer(), "vToken: FORBIDDEN");

I suggest replacing revert strings with custom errors.