Phuture Finance contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L48 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L97-L98

Vulnerability details

Impact

Both in the mint and burn cases all the {user supplied} / {due to a user} assets can be stolen by an attacker, who detects correspondingly {asset transfer calls} / {Index token transfer call} and front runs Index contract's mint() / burn() call with own address as a recipient.

This is the common approach shared by all Indices who inherit from BaseIndex. The contracts use pre-supplied balance for minting and burning amount detection, which requires strict atomic execution as in all other cases simple front running becomes a critical attack surface. This is especially important for the burn case, where there is no complication of many asset transfers and a user can unknowingly choose to run the burn() directly and not necessary in an atomic way. As the function is permissionless and will work correctly per se it cannot be deemed simply as a user mistake.

Setting severity to high as this is full principal loss scenario when the system is used in a formally correct way, i.e. a user unaware of front running issues transfers Index token to Index contract manually and then calls burn(). Without front running that's a correct way, with it all the user funds to be lost.

Proof of Concept

As Index Mint is permissionless, a malicious actor can call it with her address as recipient:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/BaseIndex.sol#L43

Same for burn:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/BaseIndex.sol#L59

IndexLogic uses the balance difference approach:

mint:

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

burn:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L97-L98

There are no other checks, i.e. whoever runs the function gets the assets.

For example in burn case VToken use the recipient address only as a transfer destination:

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

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/vToken.sol#L196

That is, whenever the mint() or burn() call is not atomic with the asset transfer, it can be front run by an attacker, using the assets transferred or due to mint() or burn() to her address instead.

Recommended Mitigation Steps

Consider using safeTransferFrom mechanics instead of requiring for assets and index tokens to be on the Index contract balance to perform the operation.

In the burn case it is straightforward as only one transferFrom is in question, for mint signatory based approach can be examined for many assets pooling.

1. TrackedIndex, TopNMarketCapIndex, ManagedIndex can be reinitialized multiple times (low)

Impact

Reinitialization can reset index composition to an entirely different set.

If done in production it will ruin the system as user's mint/burn operations are done using current index composition.

Placing severity to be low as it is factory only method.

Proof of Concept

As an example, TrackedIndex's initialize can be run repeatedly as no flag is set after the run:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TrackedIndex.sol#L25-L54

Recommended Mitigation Steps

Index update should be done via the corresponding functions, not reinitialization.

Consider disallowing of running initialize twice, by following initializer pattern or introducing a flag, for example:

bool private initialized;
...
function initialize(
	... {
		require(!initialized, "Already initialized");
		...
		initialized = true;
}

2. ChainlinkPriceOracle can produce stale prices as latestRoundData fields are ignored (low)

Impact

System can use stale prices for business logic as ChainlinkPriceOracle ’s refreshedAssetPerBaseInUQ is being used in minting and across all reweighting logic. Correct underlying asset price tracking is a core function of an index.

Proof of Concept

latestRoundData returned results aren't checked to be fresh:

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

Positive price check there is vital, but performed alone it doesn't ensure that the reading is not stale.

refreshedAssetPerBaseInUQ is a part of reweighting:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndexReweightingLogic.sol#L75

And minting:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/IndexLogic.sol#L44-L67

Recommended Mitigation Steps

Consider implementing the checks for the round id and timestamp fields:

https://docs.chain.link/docs/feed-registry/#latestrounddata

An example:

(int256 roundID, int256 priceInUsd, , int256 updatedAt, int256 answeredInRound) = baseAggregator.latestRoundData();

require(priceInUsd > 0 && updatedAt > 0 && answeredInRound >= roundID , "Price invalid");

3. Assert is used instead of require in IndexLogic (non-critical)

Impact

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message.

Proof of Concept

assert in IndexLogic's mint:

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

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases.

4. Floating pragma is used in most contracts (non-critical)

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced

Proof of Concept

pragma solidity >=0.8.7 is used in most contracts, for example:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/libraries/NAV.sol#L3

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndex.sol#L3

Recommended Mitigation Steps

Consider fixing the version to 0.8.x across all the codebase, for example set x to 12

5. User facing functions miss emergency lever (non-critical)

Impact

If there be any emergency with system contracts or outside infrastructure, there is no way to temporary stop the operations.

Proof of Concept

Most core user-facing contracts do not have pausing functionality for new operation initiation.

For example, BaseIndex mint cannot be temporary paused:

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/BaseIndex.sol#L43

Recommended Mitigation Steps

Consider making all new actions linked user facing functions pausable, first of all ones without access controls.

For example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol