Inverse Finance contest - eierina's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L136

Vulnerability details

Impact

A lender may be swapped out by governance at any time, or by enough voting power, depending on the governance implementation, without returning the free and/or borrowed balance to the lender. If a single or a group may benefit from it, then there should be measures to prevent it.

Proof of Concept

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L136

Tools Used

n/a

Recommended Mitigation Steps

As a minimum I would expect something like the following: gov proposes new lender, old lender executes/finalize the swap in a way that arrangements between the two lenders can be settled outside of the market. This works also in the case the new lender and old lender are owned by same entity.

While the external agreement takes place (or not)

• new borrowings are paused until swap completes
• free dola balance are recalled from market to the lender
• new lender can execute the swap if all borrowings are repaid or liquidated and old lender does not execute the swap itself (e.g. external agreement not reached)

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L87-L88 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L121-L122

Vulnerability details

Impact

The Oracle's viewPrice() and getPrice() functions retrieve the price of a specific token adjusted for token and feed decimals before applying the collateral factor. While it uses the token's and feed's decimals() function to retrieve their respective decimals, it uses a hardcoded (36) value during normalization.

Given the potential consequences of an invalid result, if the feed and/or token decimals are expected to be fixed, either there is no point in querying the decimals count or there shoud be a guard condition (require / if-revert). The hardcoding solution is only mentioned to point out the need for a guard condition or a proper normalization code.

With the current code, depending on the token/feed pair's decimals count, the normalizedPrice may either result in an invalid scaling by excess or by defect. A corrected normalization is proposed, with a tiny change, in the mitigation section.

Proof of Concept

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L87-L88

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L121-L122

Tools Used

n/a

Recommended Mitigation Steps

Replace the following two lines of code found here and here

uint8 decimals = 36 - feedDecimals - tokenDecimals;
uint normalizedPrice = price * (10 ** decimals);

with the following code

uint normalizedPrice = tokenDecimals >= feedDecimals
        ? (price * 10**(tokenDecimals - feedDecimals))
        : (price / 10**(feedDecimals - tokenDecimals));

While not recommending a fit-some-cases in exchange of the above's fit-all-cases solution, if still willing to keep the partially hardcoded / partially programmatic decimals normalization code (fit-some-cases), then at least add the following check to the Oracle's viewPrice() and getPrice() functions or to the setFeed() function.

require(tokenDecimals == 18, "Unsupported token")

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L116

Vulnerability details

Impact

There are a two issues with the Oracle's viewPrice() and getPrice() functions:

1. Use of deprecated latestAnswer() API which may stop working in the future;
2. Lack of validation for stale data, which may create conditions for financial loss / attacks.

Proof of Concept

Use of deprecated API without stale data validation

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Oracle.sol#L116

Deprecated API reference doc

https://docs.chain.link/docs/data-feeds/price-feeds/api-reference/#latestanswer

Real life case of DeFi hack due to Chainlink stale data

https://blog.venus.io/venus-protocol-luna-incident-update-2-c334475d9214

Tools Used

n/a

Recommended Mitigation Steps

Use latestRoundData() API and do verify for stale data eventually suspending the service if stale data is found like in cases feed suspension during extreme market volatility (e.g. see Venus Protocol hack during Chainlink LUNA price feed suspension)

diff --git a/src/Oracle.sol b/src/Oracle.sol
index 14338ed..d56d118 100644
--- a/src/Oracle.sol
+++ b/src/Oracle.sol
@@ -79,8 +79,10 @@ contract Oracle {
         if(fixedPrices[token] > 0) return fixedPrices[token];
         if(feeds[token].feed != IChainlinkFeed(address(0))) {
             // get price from feed
-            uint price = feeds[token].feed.latestAnswer();
-            require(price > 0, "Invalid feed price");
+            (, int256 answer, , uint256 updatedAt, ) = feeds[token].feed.latestRoundData();
+            require(block.timestamp <= updatedAt + PRICE_MAX_AGE, "Stale feed detected");
+            require(answer > 0, "Invalid feed price");
+            uint price = uint(answer);
             // normalize price
             uint8 feedDecimals = feeds[token].feed.decimals();
             uint8 tokenDecimals = feeds[token].tokenDecimals;
@@ -113,8 +115,11 @@ contract Oracle {
         if(fixedPrices[token] > 0) return fixedPrices[token];
         if(feeds[token].feed != IChainlinkFeed(address(0))) {
             // get price from feed
-            uint price = feeds[token].feed.latestAnswer();
-            require(price > 0, "Invalid feed price");
+            (, int256 answer, , uint256 updatedAt, ) = feeds[token].feed.latestRoundData();
+            require(block.timestamp <= updatedAt + PRICE_MAX_AGE, "Stale feed detected");
+            require(answer > 0, "Invalid feed price");
+            uint price = uint(answer);
             // normalize price
             uint8 feedDecimals = feeds[token].feed.decimals();
             uint8 tokenDecimals = feeds[token].tokenDecimals;

Refer to Chainlink's StalenessFlaggingValidator.

If deploying to L2, extra caution should be taken for L2 Sequencer uptime as shown in the following Chainlink example.