search

code-423n4 / 2022-10-inverse-findings

0 stars 0 forks source link

Wrong decimals calculation in Oracle contract #540

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The price returned by the Oracle is wrong in certain conditions, leading to prices multiplied or divided by 10^n. This situation makes the calculation of the collateral value, the withdrawal limit and the liquidation reward in Market.sol absolutely wrong, which could potentially be a loss of funds for the protocol. See that the price is supposed to be 18 decimals, and getting it in another number of decimals would be a disaster for the protocol. See for example of use of the getPrice function of the oracle in line 326 of Market.sol:

return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether;

how the multiplication is divided by 1 ether (10^18, or one with 18 decimals).

Proof of Concept

In both getPrice and viewPrice functions in Oracle.sol, the error is located when trying to set the price to return as a 18 decimal value.

function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
    ...
    uint price = feeds[token].feed.latestAnswer();
    require(price > 0, "Invalid feed price");
    // normalize price
    uint8 feedDecimals = feeds[token].feed.decimals();
    uint8 tokenDecimals = feeds[token].tokenDecimals;
    uint8 decimals = 36 - feedDecimals - tokenDecimals;
    uint normalizedPrice = price * (10 ** decimals);
    ...
}

Feed decimals is the number of decimals of the Chainlink feed. The price that returns the feed is with feedDecimals so the last line above uint normalizedPrice = price * (10 ** decimals); just should add (18 - feedDecimals) to the price, so that normalizedPrice turns into a 18 decimal value. Instead of this, the functions calculate the decimals as uint8 decimals = 36 - feedDecimals - tokenDecimals; which works for tokenDecimals = 18, but won't work otherwise. So let's see the different possibilities here:

  1. If tokenDecimals = 18 the functions will work OK as long as feedDecimals <= 18. If it's greater than 18 the calls to both functions will revert because 36 - 18 - feedDecimals < 0.
  2. If tokenDecimals != 18 the calculation won't work OK. Even worse, if tokenDecimals + feedDecimals > 36 both functions will revert.

The following forge test (which I included in the Market.t.sol file) illustrates some wrong returns by the oracle supposing that the feedDecimals are always 18, for different values of tokenDecimals:

function testWrongPriceDecimalsWith18DecimalsFeed() public {
    // The Eth feed has 18 decimals and is supposed to return a price of 1,600 with 18 decimals
    assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e18);

    // We change the feed setting a value of tokenDecimals of 8
    vm.prank(gov);
    oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 8);

    // Check that the price returned by the oracle is now 1,600 with 28 decimals
    assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e28);

    vm.prank(gov);
    // We change the feed again setting a value of tokenDecimals of 24
    oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 24);
    // Check that the call to viewPrice reverts because of the underflow [36 - 18 - 24 = -6]
    vm.expectRevert(stdError.arithmeticError);
    uint price = oracle.viewPrice(address(WETH), collateralFactorBps);
}

To test the cases for when the feed is different than 18 decimals I first changed file EthFeed.sol, precisely these two lines:

...
uint8 decimals_ = 18;
uint price_ = 1600e18;
...

into

...
uint8 decimals_ = 8;
uint price_ = 1600e8;
...

Then added this test to Market.t.sol:

function testWrongPriceDecimalsWith8DecimalsFeed() public {
    // The Eth feed now has 8 decimals, and the token decimals are 18
    // It is supposed to return a price of 1,600 with 18 decimals
    assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e18);

    // We change the feed setting a value of tokenDecimals of 8
    vm.prank(gov);
    oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 8);

    // Check that the price returned by the oracle is now 1,600 with 28 decimals
    assertEq(oracle.viewPrice(address(WETH), collateralFactorBps), 1_600e28);

    vm.prank(gov);
    // We change the feed again setting a value of tokenDecimals of 30
    oracle.setFeed(address(WETH), IChainlinkFeed(address(ethFeed)), 30);
    // Check that the call to viewPrice reverts because of the underflow [36 - 8 - 30 = -2]
    vm.expectRevert(stdError.arithmeticError);
    uint price = oracle.viewPrice(address(WETH), collateralFactorBps);
}

So it is clear that the returned price has the wrong decimals in some of the cases, and so the calculations in the Market contract would lead to disastrous consequences.

Tools Used

Forge tests and manual analysis

Recommended Mitigation Steps

If the oracle is supposed to return the price of the feed with 18 decimals, a simple solution would be changing this lines in both viewPrice and getPrice:

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

with this:

uint normalizedPrice = price;
if(feedDecimals <= 18){
    normalizedPrice *= 10 ** (18 - feedDecimals);
}
else{
    normalizedPrice /= 10 ** (feedDecimals - 18);
}

This way, if price feed has less than 18 decimals, it would be right padded with zeroes up to 18, and if it has more than 18 decimals, the excess decimals would be taken out.

c4-judge commented 2 years ago

0xean marked the issue as primary issue

c4-judge commented 2 years ago

0xean marked the issue as selected for report

08xmt commented 2 years ago

This issue is only partially correct.

The intention of the oracle is not to return a 18 decimal price, but a price that will give a correct dollar price as if the underlying token had 18 decimals, since we do no decimal math in the market.

Let's take an example:

Their collateral value is calculated as follows by the market: return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether;

Leaving them with 1e8 * 1.6e32 / 1e18 = 1.6e22 collateral value, or what amounts to 16,000 DOLA which is correct.

The part about the oracle failing if the token decimals + feed decimals > 36 is correct, but considered a lower severity issue, is that is highly unorthodox. Severity considered medium for the correct part of the issue.

c4-sponsor commented 2 years ago

08xmt marked the issue as disagree with severity

08xmt commented 2 years ago

Fixed in https://github.com/InverseFinance/FrontierV2/pull/25

c4-sponsor commented 2 years ago

08xmt marked the issue as sponsor confirmed

c4-judge commented 2 years ago

0xean marked the issue as not selected for report

0xean commented 2 years ago

I think M is reasonable here, it requires some pre-existing conditions that are atypical for the issue to occur.

c4-judge commented 2 years ago

0xean changed the severity to 2 (Med Risk)

c4-judge commented 2 years ago

0xean marked the issue as satisfactory

c4-judge commented 1 year ago

Simon-Busch marked the issue as duplicate of #533