Phuture Finance contest - 0xkatana's results

Lines of code

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

Vulnerability details

Impact

The data returned by the Chainlink latestRoundData() function may be stale. There should be checks applied on the data received from Chainlink to validate that it is not stale. https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

The ChainlinkPriceOracle contract has these two lines https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L83-L84

(, int basePrice, , , ) = baseAggregator.latestRoundData();
(, int quotePrice, , , ) = assetInfo.aggregator.latestRoundData();

The Chainlink oracle data is not validated properly. There is no check for stale price and round completeness. The price returned can be stale and can lead to inaccurate return values.

Tools Used

Manual analysis

Recommended Mitigation Steps

Validate the round and timestamp returned by the oracle data

(uint80 roundID, int basePrice, , uint256 timestamp, uint80 answeredInRound) = baseAggregator.latestRoundData();
require(basePrice > 0, "price is 0"); 
require(answeredInRound >= roundID, "stale price");
require(timestamp > 0, "incomplete round");

(uint80 roundID, int quotePrice, , uint256 timestamp, uint80 answeredInRound) = assetInfo.aggregator.latestRoundData();
require(quotePrice > 0, "price is 0");
require(answeredInRound >= roundID, "stale price");
require(timestamp > 0, "incomplete round");

1. Low - factory address never set

Impact

The factory state variable is never set in ManagedIndex and TopNMarketCapIndex. This will cause the initialize function to be uncalled unless the zero address calls the function, which is not possible for the Phuture Finance team to do.

Proof of Concept

These line checks if the caller of the initialize function is the factory address https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndex.sol#L28 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TopNMarketCapIndex.sol#L45 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TrackedIndex.sol#L30

But the factory address is never set before it is referenced, so it will have a value of address(0). The factory address state variable is imported from the BaseIndex contract, but the value of this state variable is not borrowed from importing the contract.

Tools Used

Manual analysis

Recommended Mitigation Steps

Set the value of the factory state variable in the constructor of ManagedIndex

2. Low - Missing initializer modifier

Impact

The ManagedIndex initializer function does not have an initializer modifier. This could allow the function to be called more than once y the factory contract, unexpectedly changing important state variable values.

Proof of Concept

These initialize functions should have the initializer modifier, like the other initialize functions in the project have https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndex.sol#L27 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TopNMarketCapIndex.sol#L37 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TrackedIndex.sol#L25

Tools Used

Manual analysis

Recommended Mitigation Steps

Add the initializer modifier to all initialize functions

1. Split up require statements instead of &&

Impact

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into separate require statements

Proof of Concept

Several instances of this issue was found https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L51 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L86 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndexReweightingLogic.sol#L30-L31 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/UniswapV2PriceOracle.sol#L46

Tools Used

Manual analysis

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

2. Use prefix not postfix in loops

Impact

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Proof of Concept

There are three examples of this https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/UniswapV2PathPriceOracle.sol#L34 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/UniswapV2PathPriceOracle.sol#L49

Tools Used

Manual analysis

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

3. Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

1. https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/UniswapV2PathPriceOracle.sol#L25
2. https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TopNMarketCapIndex.sol#L74
3. https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TopNMarketCapReweightingLogic.sol#L67

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

4. Use != 0 instead of > 0

Impact

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Proof of Concept

Locations where this was found include https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndexReweightingLogic.sol#L56 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndexReweightingLogic.sol#L61 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndexReweightingLogic.sol#L98 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L76 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L86 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L98 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L114 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L141 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/PhutureIndex.sol#L56 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/PhutureIndex.sol#L64

Tools Used

grep

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

5. Cache array length before loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Proof of Concept

This optimization is already used in some places, but is not used in these places https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L39 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L60 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L125 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndexReweightingLogic.sol#L38 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TopNMarketCapReweightingLogic.sol#L37 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TrackedIndexReweightingLogic.sol#L37 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TrackedIndexReweightingLogic.sol#L66

Tools Used

Manual analysis

Recommended Mitigation Steps

Cache the array length before the for loop