Phuture Finance contest - robee's results

Lines of code

https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/IndexLogic.sol#L115

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

Vault.sol#addValue() assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. But, the actual transferred amount can be lower for those tokens. Therefore it's recommended to use the balance change before and after the transfer instead of the amount. This way you also support the tokens with transfer fee - that are popular.

    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/IndexLogic.sol#L115

Title: Solidity compiler versions mismatch Severity: Low Risk

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    vToken.sol, mint
    AUMCalculationLibrary.sol, rpow
    FullMath.sol, mulDiv
    vToken.sol, burn

Title: Assert instead require to validate user inputs Severity: Low Risk

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
    
    IndexLogic.sol : reachable assert in line 71

Title: Add a timelock Severity: Low Risk

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/PhuturePriceOracle.sol#L55

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    vToken.sol, constructor, 51
    vToken.sol, initialize, 55

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    PhuturePriceOracle.sol.setOracleOf _asset
    IndexLogic.sol.burn _recipient
    vToken.sol.transferFrom _to
    vToken.sol.transfer _recipient
    vToken.sol.transferFrom _from

Title: Check transfer receiver is not 0 to avoid burned money Severity: Low Risk

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

    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/vToken.sol#L210
    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/IndexLogic.sol#L115
    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/vToken.sol#L187
    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/vToken.sol#L77
    https://github.com/code-423n4/2022-04-phuture/tree/main/contracts/vToken.sol#L196

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    PhutureIndex._chargeAUMFee (_feePool)

Title: Require with empty message Severity: Low Risk

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: FullMath.sol, In line 44 with Empty Require message.

Title: Missing commenting Severity: Low Risk

    The following functions are missing commenting as describe below:
    
    NAV.sol, transfer (internal), parameter self not commented

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    PhuturePriceOracle.sol, lastAssetPerBaseInUQ
    UniswapV2PathPriceOracle.sol, anatomy
    PhuturePriceOracle.sol, refreshedAssetPerBaseInUQ
    PhutureIndex.sol, supportsInterface
    ManagedIndex.sol, supportsInterface

Title: Caching array length can save gas Severity: GAS

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    TrackedIndex.sol, _assets, 35
    ManagedIndexReweightingLogic.sol, _updatedAssets, 50
    ManagedIndex.sol, _assets, 30
    ManagedIndexReweightingLogic.sol, _inactiveAssets, 96
    BaseIndex.sol, _assets, 78

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    PhutureIndex.sol, _chargeAUMFee
    NAV.sol, _burn
    FullMath.sol, mulDiv
    PhutureIndex.sol, _transfer

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    IndexLayout.sol, registry
    vToken.sol, __gap
    IndexLayout.sol, vTokenFactory
    FixedPoint112.sol, RESOLUTION
    IndexLayout.sol, lastTransferTime

Title: Change if -> revert pattern to require Severity: GAS

Change if -> revert pattern to 'require' to save gas and improve code quality, if (some_condition) { revert(revert_message) }

to: require(!some_condition, revert_message)

In the following locations:

    TrackedIndex.sol, 62
    TopNMarketCapIndex.sol, 73
    ManagedIndex.sol, 53
    BaseIndex.sol, 48
    BaseIndex.sol, 64

Title: Unused declared local variables Severity: GAS

Unused local variables are gas consuming, since the initial value assignment costs gas. And are a bad code practice. Removing those variables will decrease the gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    vToken.sol, assetDataOf, amountInAsset

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    vToken.sol.constructor
    PhutureIndex.sol.constructor
    TrackedIndex.sol.constructor
    TopNMarketCapIndex.sol.constructor

Title: Unnecessary index init Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    UniswapV2PathPriceOracle.sol, 49
    UniswapV2PathPriceOracle.sol, 34

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: UniswapV2PathPriceOracle.sol, In line 25, Require message length to shorten: 33, The message: UniswapV2PathPriceOracle: ORACLES

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    FullMath.sol, 35: change 'denominator > 0' to 'denominator != 0'
    IndexLibrary.sol, 29: change '_assetPerBaseInUQ > 0' to '_assetPerBaseInUQ != 0'

Title: Rearrange state variables Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In UniswapV2PriceOracle.sol,rearranging the storage fields can optimize to: 8 slots from: 9 slots. The new order of types (you choose the actual variables): 1. uint 2. IUniswapV2Pair 3. uint 4. uint 5. uint 6. uint 7. address 8. uint32 9. address

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    PhutureIndex.sol, line 4, import "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";
    vToken.sol, line 12, import "./libraries/BP.sol";
    IOrderer.sol, line 4, import "./IvToken.sol";
    IOrderer.sol, line 5, import "./IPhuturePriceOracle.sol";
    BaseIndex.sol, line 8, import "./libraries/IndexLibrary.sol";

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    TopNMarketCapIndex.sol; the inherited contracts BaseIndex not used
    TrackedIndex.sol; the inherited contracts BaseIndex not used
    BaseIndex.sol; the inherited contracts PhutureIndex not used

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    baseDecimals in PhuturePriceOracle.sol
    registry in PhuturePriceOracle.sol
    REWEIGHT_INDEX_ROLE in ManagedIndex.sol
    registry in vToken.sol
    asset in vToken.sol

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    NAV.sol, mintableShares
    NAV.sol, assetBalanceForShares
    IndexLibrary.sol, amountInAsset
    AUMCalculationLibrary.sol, rpow
    NAV.sol, transfer

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    NAV.sol, mintableShares
    PhutureIndex.sol, _chargeAUMFee
    vToken.sol, _burn
    NAV.sol, assetBalanceForShares
    IndexLibrary.sol, amountInAsset

Title: Cache powers of 10 used several times Severity: GAS

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

ChainlinkPriceOracle.sol, 89 : You should cache the used power of 10 as constant state variable since it's used several times (2): (uint(quotePrice) * 10**baseDecimals)

ChainlinkPriceOracle.sol, 87 : You should cache the used power of 10 as constant state variable since it's used several times (2): uint assetPerBaseInUQ = ((uint(basePrice) * 10**assetInfo.decimals).mulDiv(

ChainlinkPriceOracle.sol, 90 : You should cache the used power of 10 as constant state variable since it's used several times (2): ) * 10assetInfo.answerDecimals) / 10baseAnswerDecimals;

Title: Prefix increments are cheaper than postfix increments Severity: GAS

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    just change to unchecked: BaseIndex.sol, i, 78
    just change to unchecked: TopNMarketCapReweightingLogic.sol, i, 37
    just change to unchecked: IndexLogic.sol, i, 125
    just change to unchecked: ManagedIndex.sol, i, 30
    just change to unchecked: ManagedIndexReweightingLogic.sol, i, 96