Inverse Finance contest - Franfran's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L119-L121

Vulnerability details

Impact

The line 122 in the Oracle.sol contract purpose is to normalize the price of the feed to 18 decimals in order to return the price of the underlying token in DOLA. The issue is that the decimals calculation is done wrong, leading to incorrect calculations and for most of the time (when the underlying decimals are less than 18) an hyper-inflated price of the token in DOLA.

Proof of Concept

Let's take the BTC/USD price feed contract. We can see that the feed decimals is 8, and the BTC decimals is 8 as well. By doing a quick calculation, the decimals will be

uint8 decimals = 36 - feedDecimals (8) - feedDecimals (8);
          └──  (decimals = 20)

So at a today price of BTC at about 21k$, the normalized price would be

uint normalizedPrice = price * (10 ** decimals);
          |               |              └  (20)
          |               └  (21,000 * 1e8)
          └  (21,000 * 1e28) 

While the "fair" price should be 21,000 * 1e18 (DOLA). This would of course lead to disastrous consequence because every lender loan in BTC would be scaled by 1e10.

Tools Used

CLion

Recommended Mitigation Steps

// Oracle.sol L.121
- | uint8 decimals = 36 - feedDecimals - tokenDecimals;
+ | uint8 decimals = 18 - feedDecimals;

Thanks to this, tokenDecimals is now useless and there is no need for a FeedData struct.

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L6 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L82

Vulnerability details

Impact

Chainlink latestAnswer() should never be used as it is not a reliable source of data. From one of the deployed oracles at: 0x72AFAECF99C9d9C8215fF44C77B94B99C28741e8 on etherscan

overridden function to add the checkAccess() modifier "deprecated". Use latestRoundData instead. 
This does not error if no answer has been reached, it will simply return 0. 
Either wait to point to an already answered Aggregator or use the recommended latestRoundData instead which includes better verification information.

It is also explained in the Chainlink docs that the latestAnswer() function is deprecated.

Even if it is checked that the price is not null, an invalid price could be returned by the aggregators during an unfinished round. As explained, we should use latestRoundData and take care of the return parameters.

Proof of Concept

Chainlink docs

Tools Used

CLion

Recommended Mitigation Steps

- uint price = feeds[token].feed.latestAnswer();
+ (uint80 roundId, int256 price, uint256 timestamp, uint80 ansRoundId) = feeds[token].feed.latestRoundData();
+ require(price > 0, "Invalid feed price"); 
+ require(ansRoundId >= roundID, "Stale feed price");
+ require(timestamp != 0, "Incomplete feed round");