code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Oracle#getPrice returns incorrect price for asset

Proof of Concept

There are a few issues with the final return value of the function.

uint normalizedPrice = price * (10 ** decimals);
// potentially store price as today's low
uint day = block.timestamp / 1 days;
uint todaysLow = dailyLows[token][day];
if(todaysLow == 0 || normalizedPrice < todaysLow) {
    dailyLows[token][day] = normalizedPrice;
    todaysLow = normalizedPrice;
    emit RecordDailyLow(token, normalizedPrice);
}

uint yesterdaysLow = dailyLows[token][day - 1];

uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;

The first issue is that todaysLow and by extent yesterdaysLow and also twoDayLow are all set using normalizedPrice which doesn't account for collateralFactorBPS at all. The problem stems from the following line:

if(twoDayLow > 0 && newBorrowingPower > twoDayLow)

newBorrowingPower is adjusted by collateralFactorBPS while twoDayLow isn't, which isn't a useful comparison.

uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;

The second issue is that dampenedPrice is scaled by the inverse collateralFactorBps but then compared against normalizedPrice which isn't adjusted for collateralFactorBps at all.

Example:

normalizedPrice = 100 todaysLow = 90 yesterdaysLow = 95

collateralFactorBps = 6000

newBorrowingPower = 100 * 6000 / 10000 = 60 twoDayLow = todaysLow = 90

(60 > 90) = FALSE

NewBorrowingPower is compared with twoDayLow. newBorrowingPower is lower so it returns nomalizedPrice. In this scenario it returned 100 when it should have returned 90.

Tools Used

Manual Review

Recommended Mitigation Steps

The end of the function can be simplified. Market contracts factor in collateral BPS elsewhere, so no need to add any extra complexity:

    uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
+   return twoDayLow > 0 && normalizedPrice > twoDayLow ? twoDayLow : normalizedPrice;

-   if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
-       uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
-       return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
-   }
-   return normalizedPrice;

d3e4 commented 2 years ago

The first issue is that todaysLow and by extent yesterdaysLow and also twoDayLow are all set using normalizedPrice which doesn't account for collateralFactorBPS at all.

They are set from the price feed, which is the true price, not the normalized price even though this variable is used here before it has had a chance to be modified by collateralFactorBPS.

In this scenario it returned 100 when it should have returned 90.

Why should it? Are you referring to "It ensures that any given oracle price is dampened to prevent borrowers from borrowing more than the lowest recorded value of their collateral over the past 2 days."? By returning a price of at most dampenedPrice = twoDayLow * 10000 / collateralFactorBps the borrow amount is restricted to at most the two day low, as can be seen by following the borrowing logic:

function borrowInternal(address borrower, address to, uint amount) internal {
    require(!borrowPaused, "Borrowing is paused");
    if(borrowController != IBorrowController(address(0))) {
        require(borrowController.borrowAllowed(msg.sender, borrower, amount), "Denied by borrow controller");
    }
    uint credit = getCreditLimitInternal(borrower);
    debts[borrower] += amount;
    require(credit >= debts[borrower], "Exceeded credit limit");
    totalDebt += amount;
    dbr.onBorrow(borrower, amount);
    dola.transfer(to, amount);
    emit Borrow(borrower, amount);
}

function getCreditLimitInternal(address user) internal returns (uint) {
    uint collateralValue = getCollateralValueInternal(user);
    return collateralValue * collateralFactorBps / 10000;
}

function getCollateralValueInternal(address user) internal returns (uint) {
    IEscrow escrow = predictEscrow(user);
    uint collateralBalance = escrow.balance();
    return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether;
}

twoDayLow * 10000 / collateralFactorBps => collateralBalance * twoDayLow * 10000 / collateralFactorBps / 1 ether => collateralBalance * twoDayLow * 10000 / collateralFactorBps / 1 ether * collateralFactorBps / 10000 = collateralBalance * twoDayLow / 1 ether which is the credit compared to the debt in require(credit >= debts[borrower], "Exceeded credit limit"); for borrowing.

It seems that what is claimed here is that a design choice is incorrect, or that a refactoring should be done (this I may agree with). I see no justification for why the comparisons should be done as claimed here. But in order to bring clarity to the price calculation, note that newBorrowingPower is not really needed and that normalizedPrice is really just the minimum of dampenedPrice and normalizedPrice, except if twoDayLow == 0 in which case it returns normalizedPrice. See my QA-report NC-03 for a demonstration of this.

0xean commented 2 years ago

going to leave open for sponsor review and comment from the warden, the logic here does seem to be a little off and there is the potential for a price that isn't correctly dampened (the 2 day low) to be returned, even though the dampened price is in fact less.