Inverse Finance contest - rbserver's results

Rethink the way you borrow.

General Information

Platform: Code4rena

Start Date: 25/10/2022

Pot Size: $50,000 USDC

Total HM: 18

Participants: 127

Period: 5 days

Judge: 0xean

Total Solo HM: 9

Id: 175

League: ETH

Inverse Finance

Findings Distribution

Researcher Performance

Rank: 2/127

Findings: 4

Award: $3,638.23

QA:
grade-b

🌟 Selected for report: 3

🚀 Solo Findings: 1

Findings Information

🌟 Selected for report: rbserver

Also found by: 0xRobocop, Ch_301, ElKu, Jeiwan, MiloTruck, Picodes, sam_cunningham

Labels

bug
2 (Med Risk)
primary issue
sponsor disputed
selected for report
M-16

Awards

203.1474 USDC - $203.15

External Links

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559-L572 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531-L539 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L472-L474 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L460-L466

Vulnerability details

Impact

When a user incurs a DBR deficit, a replenisher can call the forceReplenish function to force the user to replenish DBR. However, there is no guarantee that the forceReplenish function will always be called. When the forceReplenish function is not called, such as because that the replenisher does not notice the user's DBR deficit promptly, the user can just call the repay function to repay the origianl debt and the withdraw function to receive all of the deposited collateral even when the user has a DBR deficit already. Yet, in the same situation, if the forceReplenish function has been called, more debt should be added for the user, and the user needs to repay more in order to get back all of the deposited collateral. Hence, when the forceReplenish function is not called while it could be called, the Market contract would receive less DOLA if the user decides to repay the debt and withdraw the collateral both in full.

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559-L572

    function forceReplenish(address user, uint amount) public {
        uint deficit = dbr.deficitOf(user);
        require(deficit > 0, "No DBR deficit");
        require(deficit >= amount, "Amount > deficit");
        uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000;
        uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000;
        debts[user] += replenishmentCost;
        uint collateralValue = getCollateralValueInternal(user);
        require(collateralValue >= debts[user], "Exceeded collateral value");
        totalDebt += replenishmentCost;
        dbr.onForceReplenish(user, amount);
        dola.transfer(msg.sender, replenisherReward);
        emit ForceReplenish(user, msg.sender, amount, replenishmentCost, replenisherReward);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531-L539

    function repay(address user, uint amount) public {
        uint debt = debts[user];
        require(debt >= amount, "Insufficient debt");
        debts[user] -= amount;
        totalDebt -= amount;
        dbr.onRepay(user, amount);
        dola.transferFrom(msg.sender, address(this), amount);
        emit Repay(user, msg.sender, amount);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L472-L474

    function withdraw(uint amount) public {
        withdrawInternal(msg.sender, msg.sender, amount);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L460-L466

    function withdrawInternal(address from, address to, uint amount) internal {
        uint limit = getWithdrawalLimitInternal(from);
        require(limit >= amount, "Insufficient withdrawal limit");
        IEscrow escrow = getEscrow(from);
        escrow.pay(to, amount);
        emit Withdraw(from, to, amount);
    }

Proof of Concept

Please add the following test in src\test\Market.t.sol. This test will pass to demonstrate the described scenario.

    function testRepayAndWithdrawInFullWhenIncurringDBRDeficitIfNotBeingForcedToReplenish() public {
        gibWeth(user, wethTestAmount);
        gibDBR(user, wethTestAmount);

        vm.startPrank(user);

        // user deposits wethTestAmount WETH and borrows wethTestAmount DOLA
        deposit(wethTestAmount);
        market.borrow(wethTestAmount);

        assertEq(DOLA.balanceOf(user), wethTestAmount);
        assertEq(WETH.balanceOf(user), 0);

        vm.warp(block.timestamp + 60 weeks);

        // after some time, user incurs DBR deficit
        assertGt(dbr.deficitOf(user), 0);

        // yet, since no one notices that user has a DBR deficit and forces user to replenish DBR,
        //   user is able to repay wethTestAmount DOLA that was borrowed previously and withdraw wethTestAmount WETH that was deposited previously
        market.repay(user, wethTestAmount);
        market.withdraw(wethTestAmount);

        vm.stopPrank();

        // as a result, user is able to get back all of the deposited WETH, which should not be possible if user has been forced to replenish DBR
        assertEq(DOLA.balanceOf(user), 0);
        assertEq(WETH.balanceOf(user), wethTestAmount);
    }

Tools Used

VSCode

When calling the repay function, the user's DBR deficit can also be checked. If the user has a DBR deficit, an amount, which is similar to replenishmentCost that is calculated in the forceReplenish function, can be calculated; it can then be used to adjust the repay function's amount input for updating the states regarding the user's and total debts in the relevant contracts.

#0 - d3e4

2022-11-02T08:09:44Z

This seems to be a design choice. The user should perhaps be allowed to repay his debt without penalty if he does so quickly enough. That is, if forced replenishment is designed as a threat rather than an actual cost of being in deficit.

#1 - c4-judge

2022-11-05T23:06:07Z

0xean marked the issue as duplicate

#2 - c4-judge

2022-12-01T15:57:32Z

0xean marked the issue as selected for report

#3 - c4-judge

2022-12-07T08:15:30Z

Simon-Busch marked the issue as not a duplicate

#4 - c4-judge

2022-12-07T08:15:37Z

Simon-Busch marked the issue as primary issue

#5 - c4-sponsor

2022-12-14T14:19:21Z

08xmt marked the issue as sponsor disputed

#6 - 08xmt

2022-12-14T14:19:29Z

Working as intended.

Awards

0.5004 USDC - $0.50

Labels

bug
2 (Med Risk)
primary issue
sponsor confirmed
selected for report
M-17

External Links

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L78-L105 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L112-L144 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363

Vulnerability details

Impact

Calling the Oracle contract's viewPrice or getPrice function executes uint price = feeds[token].feed.latestAnswer() and require(price > 0, "Invalid feed price"). Besides that Chainlink's latestAnswer function is deprecated, only verifying that price > 0 is true is also not enough to guarantee that the returned price is not stale. Using a stale price can cause the calculations for the credit and withdrawal limits to be inaccurate, which, for example, can mistakenly consider a user's debt to be under water and unexpectedly allow the user's debt to be liquidated.

To avoid using a stale answer returned by the Chainlink oracle data feed, according to Chainlink's documentation:

  1. The latestRoundData function can be used instead of the deprecated latestAnswer function.
  2. roundId and answeredInRound are also returned. "You can check answeredInRound against the current roundId. If answeredInRound is less than roundId, the answer is being carried over. If answeredInRound is equal to roundId, then the answer is fresh."
  3. "A read can revert if the caller is requesting the details of a round that was invalid or has not yet been answered. If you are deriving a round ID without having observed it before, the round might not be complete. To check the round, validate that the timestamp on that round is not 0."

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

    function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
        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");
            // normalize price
            uint8 feedDecimals = feeds[token].feed.decimals();
            uint8 tokenDecimals = feeds[token].tokenDecimals;
            uint8 decimals = 36 - feedDecimals - tokenDecimals;
            uint normalizedPrice = price * (10 ** decimals);
            uint day = block.timestamp / 1 days;
            // get today's low
            uint todaysLow = dailyLows[token][day];
            // get yesterday's low
            uint yesterdaysLow = dailyLows[token][day - 1];
            // calculate new borrowing power based on collateral factor
            uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
            uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
            if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
                uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
                return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
            }
            return normalizedPrice;

        }
        revert("Price not found");
    }

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

    function getPrice(address token, uint collateralFactorBps) external returns (uint) {
        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");
            // normalize price
            uint8 feedDecimals = feeds[token].feed.decimals();
            uint8 tokenDecimals = feeds[token].tokenDecimals;
            uint8 decimals = 36 - feedDecimals - tokenDecimals;
            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);
            }
            // get yesterday's low
            uint yesterdaysLow = dailyLows[token][day - 1];
            // calculate new borrowing power based on collateral factor
            uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
            uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
            if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
                uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
                return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
            }
            return normalizedPrice;

        }
        revert("Price not found");
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347

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

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327

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

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363

    function getWithdrawalLimitInternal(address user) internal returns (uint) {
        IEscrow escrow = predictEscrow(user);
        uint collateralBalance = escrow.balance();
        if(collateralBalance == 0) return 0;
        uint debt = debts[user];
        if(debt == 0) return collateralBalance;
        if(collateralFactorBps == 0) return 0;
        uint minimumCollateral = debt * 1 ether / oracle.getPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps;
        if(collateralBalance <= minimumCollateral) return 0;
        return collateralBalance - minimumCollateral;
    }

Proof of Concept

The following steps can occur for the described scenario.

  1. Alice calls the depositAndBorrow function to deposit some WETH as the collateral and borrows some DOLA against the collateral.
  2. Bob calls the liquidate function for trying to liquidate Alice's debt. Because the Chainlink oracle data feed returns an up-to-date price at this moment, the getCreditLimitInternal function calculates Alice's credit limit accurately, which does not cause Alice's debt to be under water. Hence, Bob's liquidate transaction reverts.
  3. After some time, Bob calls the liquidate function again for trying to liquidate Alice's debt. This time, because the Chainlink oracle data feed returns a positive but stale price, the getCreditLimitInternal function calculates Alice's credit limit inaccurately, which mistakenly causes Alice's debt to be under water.
  4. Bob's liquidate transaction is executed successfully so he gains some of Alice's WETH collateral. Alice loses such WETH collateral amount unexpectedly because her debt should not be considered as under water if the stale price was not used.

Tools Used

VSCode

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L82-L83 and https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L116-L117 can be updated to the following code.

            (uint80 roundId, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = feeds[token].feed.latestRoundData();
            require(answeredInRound >= roundId, "answer is stale");
            require(updatedAt > 0, "round is incomplete");
            require(answer > 0, "Invalid feed answer");

            uint256 price = uint256(answer);

#0 - neumoxx

2022-10-31T08:40:48Z

Duplicate of #601

#1 - c4-judge

2022-11-05T17:48:17Z

0xean marked the issue as duplicate

#2 - c4-judge

2022-12-03T20:32:40Z

0xean marked the issue as selected for report

#3 - c4-judge

2022-12-07T08:13:18Z

Simon-Busch marked the issue as not a duplicate

#4 - c4-judge

2022-12-07T08:13:26Z

Simon-Busch marked the issue as primary issue

#5 - 08xmt

2022-12-14T14:21:10Z

#6 - c4-sponsor

2022-12-14T14:21:19Z

08xmt marked the issue as sponsor confirmed

Findings Information

🌟 Selected for report: rbserver

Labels

bug
2 (Med Risk)
disagree with severity
satisfactory
sponsor acknowledged
selected for report
M-18

Awards

3397.8465 USDC - $3,397.85

External Links

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L78-L105 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L112-L144 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363

Vulnerability details

Impact

Based on the current implementation, when the protocol wants to use Chainlink oracle data feed for getting a collateral token's price, the fixed price for the token should not be set. When the fixed price is not set for the token, calling the Oracle contract's viewPrice or getPrice function will execute uint price = feeds[token].feed.latestAnswer(). As https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ mentions, it is possible that Chainlink’s "multisigs can immediately block access to price feeds at will". When this occurs, executing feeds[token].feed.latestAnswer() will revert so calling the viewPrice and getPrice functions also revert, which cause denial of service when calling functions like getCollateralValueInternal andgetWithdrawalLimitInternal. The getCollateralValueInternal andgetWithdrawalLimitInternal functions are the key elements to the core functionalities, such as borrowing, withdrawing, force-replenishing, and liquidating; with these functionalities facing DOS, the protocol's usability becomes very limited.

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

    function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
        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");
            // normalize price
            uint8 feedDecimals = feeds[token].feed.decimals();
            uint8 tokenDecimals = feeds[token].tokenDecimals;
            uint8 decimals = 36 - feedDecimals - tokenDecimals;
            uint normalizedPrice = price * (10 ** decimals);
            uint day = block.timestamp / 1 days;
            // get today's low
            uint todaysLow = dailyLows[token][day];
            // get yesterday's low
            uint yesterdaysLow = dailyLows[token][day - 1];
            // calculate new borrowing power based on collateral factor
            uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
            uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
            if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
                uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
                return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
            }
            return normalizedPrice;

        }
        revert("Price not found");
    }

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

    function getPrice(address token, uint collateralFactorBps) external returns (uint) {
        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");
            // normalize price
            uint8 feedDecimals = feeds[token].feed.decimals();
            uint8 tokenDecimals = feeds[token].tokenDecimals;
            uint8 decimals = 36 - feedDecimals - tokenDecimals;
            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);
            }
            // get yesterday's low
            uint yesterdaysLow = dailyLows[token][day - 1];
            // calculate new borrowing power based on collateral factor
            uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
            uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
            if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
                uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
                return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
            }
            return normalizedPrice;

        }
        revert("Price not found");
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347

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

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327

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

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363

    function getWithdrawalLimitInternal(address user) internal returns (uint) {
        IEscrow escrow = predictEscrow(user);
        uint collateralBalance = escrow.balance();
        if(collateralBalance == 0) return 0;
        uint debt = debts[user];
        if(debt == 0) return collateralBalance;
        if(collateralFactorBps == 0) return 0;
        uint minimumCollateral = debt * 1 ether / oracle.getPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps;
        if(collateralBalance <= minimumCollateral) return 0;
        return collateralBalance - minimumCollateral;
    }

Proof of Concept

The following steps can occur for the described scenario.

  1. Chainlink oracle data feed is used for getting the collateral token's price so the fixed price for the token is not set.
  2. Alice calls the depositAndBorrow function to deposit some of the collateral token and borrows some DOLA against the collateral.
  3. Chainlink's multisigs suddenly blocks access to price feeds so executing feeds[token].feed.latestAnswer() will revert.
  4. Alice tries to borrow more DOLA but calling the borrow function, which eventually executes feeds[token].feed.latestAnswer(), reverts.
  5. Alice tries to withdraw the deposited collateral but calling the withdraw function, which eventually executes feeds[token].feed.latestAnswer(), reverts.
  6. Similarly, calling the forceReplenish and liquidate functions would all revert as well.

Tools Used

VSCode

The Oracle contract's viewPrice and getPrice functions can be updated to refactor feeds[token].feed.latestAnswer() into try feeds[token].feed.latestAnswer() returns (int256 price) { ... } catch Error(string memory) { ... }. The logic for getting the collateral token's price from the Chainlink oracle data feed should be placed in the try block while some fallback logic when the access to the Chainlink oracle data feed is denied should be placed in the catch block. If getting the fixed price for the collateral token is considered as a fallback logic, then setting the fixed price for the token should become mandatory, which is different from the current implementation. Otherwise, fallback logic for getting the token's price from a fallback oracle is needed.

#0 - c4-sponsor

2022-11-14T08:58:47Z

08xmt marked the issue as disagree with severity

#1 - c4-sponsor

2022-11-14T08:58:54Z

08xmt marked the issue as sponsor acknowledged

#2 - 08xmt

2022-11-14T09:00:33Z

In the unlikely event of a chainlink msig block, the protocol can still recover through the use of governance actions to insert a new feed. I'd consider this a Low Severity, as protocol is only DOS'ed for a short period, and can't be repeatedly DOS'ed.

#3 - 0xean

2022-11-28T18:03:07Z

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

I don't think a M requires some amount of time for the DOS to be valid, so I think without a mitigation or fallback in place, this is a valid issue and should qualify as M

#4 - c4-judge

2022-11-28T19:35:19Z

0xean marked the issue as satisfactory

#5 - 08xmt

2022-11-30T11:30:54Z

@0xean That's fair.

#6 - c4-judge

2022-12-01T15:53:20Z

0xean marked the issue as selected for report

[L-01] DIFFERENT RESTRICTIONS FOR SOME STATE VARIABLES IN constructor AND SETTERS

Some state variables have different restrictions when setting them in constructor and the corresponding setter functions. It is possible that the values, which can be set in the constructor, cannot be set in the setter functions or vice versa. These inconsistencies can confuse users and developers. For better user experience and maintainability, please consider updating the restrictions for the following state variables to be the same in constructor and the corresponding setter functions.

replenishmentPriceBps's setter function requires that newReplenishmentPriceBps_ > 0 but constructor does not require that at all. https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L30-L42

    constructor(
        uint _replenishmentPriceBps,
        string memory _name,
        string memory _symbol,
        address _operator
    ) {
        replenishmentPriceBps = _replenishmentPriceBps;
        ...
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L62-L65

    function setReplenishmentPriceBps(uint newReplenishmentPriceBps_) public onlyOperator {
        require(newReplenishmentPriceBps_ > 0, "replenishment price must be over 0");
        replenishmentPriceBps = newReplenishmentPriceBps_;
    }

replenishmentIncentiveBps's setter function requires that _replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000 but constructor only requires _replenishmentIncentiveBps < 10000.

liquidationIncentiveBps's setter function requires that _liquidationIncentiveBps + liquidationFeeBps < 10000 but constructor only requires that _liquidationIncentiveBps < 10000.

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L61-L90

    constructor (
        address _gov,
        address _lender,
        address _pauseGuardian,
        address _escrowImplementation,
        IDolaBorrowingRights _dbr,
        IERC20 _collateral,
        IOracle _oracle,
        uint _collateralFactorBps,
        uint _replenishmentIncentiveBps,
        uint _liquidationIncentiveBps,
        bool _callOnDepositCallback
    ) {
        require(_collateralFactorBps < 10000, "Invalid collateral factor");
        require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps < 10000, "Invalid liquidation incentive");
        require(_replenishmentIncentiveBps < 10000, "Replenishment incentive must be less than 100%");
        ...
        collateralFactorBps = _collateralFactorBps;
        replenishmentIncentiveBps = _replenishmentIncentiveBps;
        liquidationIncentiveBps = _liquidationIncentiveBps;
        ...
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L172-L175

    function setReplenismentIncentiveBps(uint _replenishmentIncentiveBps) public onlyGov {
        require(_replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000, "Invalid replenishment incentive");
        replenishmentIncentiveBps = _replenishmentIncentiveBps;
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L183-L186

    function setLiquidationIncentiveBps(uint _liquidationIncentiveBps) public onlyGov {
        require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps + liquidationFeeBps < 10000, "Invalid liquidation incentive");
        liquidationIncentiveBps = _liquidationIncentiveBps;
    }

[L-02] MISSING TWO-STEP PROCEDURES FOR SETTING gov, lender, and pauseGuardian

The following setGov, setLender, and setPauseGuardian functions can be called to set gov, lender, and pauseGuardian. Because these functions do not include a two-step procedure, it is possible to assign these roles to wrong addresses. When this occurs, the functions that are only callable by these roles can become no-ops or be maliciously called. To reduce the potential attack surface, please consider implementing a two-step procedure for assigning each of these roles.

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130

    function setGov(address _gov) public onlyGov { gov = _gov; }

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

    function setLender(address _lender) public onlyGov { lender = _lender; }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142

    function setPauseGuardian(address _pauseGuardian) public onlyGov { pauseGuardian = _pauseGuardian; }

[L-03] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

To prevent unintended behaviors, critical addresses inputs should be checked against address(0).

Please consider checking _operator in the following constructor. https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L30-L42

    constructor(
        uint _replenishmentPriceBps,
        string memory _name,
        string memory _symbol,
        address _operator
    ) {
        ...
        operator = _operator;
        ...
    }

Please consider checking _gov, _lender, _pauseGuardian, and _escrowImplementation in the following constructor. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L61-L90

    constructor (
        address _gov,
        address _lender,
        address _pauseGuardian,
        address _escrowImplementation,
        ...
    ) {
        ...
        gov = _gov;
        lender = _lender;
        pauseGuardian = _pauseGuardian;
        escrowImplementation = _escrowImplementation;
        ...
    }

Please consider checking _operator in the following constructor. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L29-L33

    constructor(
        address _operator
    ) {
        operator = _operator;
    }

[L-04] UNRESOLVED TODO COMMENT

A comment regarding todo indicates that there is an unresolved action item for implementation, which needs to be addressed before the protocol deployment. Please review the todo comment in the following code.

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L34-L36

    constructor(IXINV _xINV) {
        xINV = _xINV; // TODO: Test whether an immutable variable will persist across proxies
    }

[N-01] TYPOS IN COMMENTS

The phrase in the following comment should be "the user" instead of "th user". https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L128

    @notice Get the DBR deficit of an address. Will return 0 if th user has zero DBR or more.

The word in the following comment should be "debt" instead of "deb". https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L129

    @dev The deficit of a user is calculated as the difference between the user's accrued DBR deb + due DBR debt and their balance.

The phrase in the following comment should be "The amount" instead of "Th amount". https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L589

    @param repaidDebt Th amount of user user debt to liquidate.

[N-02] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers, such as 10000, in the following code with constants.

src\DBR.sol
  122: uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
  135: uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
  330: uint replenishmentCost = amount * replenishmentPriceBps / 10000;    

src\Market.sol
  75: require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps < 10000, "Invalid liquidation incentive"); 
  150: require(_collateralFactorBps < 10000, "Invalid collateral factor"); 
  162: require(_liquidationFactorBps > 0 && _liquidationFactorBps <= 10000, "Invalid liquidation factor");
  173: require(_replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000, "Invalid replenishment incentive");
  184: require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps + liquidationFeeBps < 10000, "Invalid liquidation incentive");
  195: require(_liquidationFeeBps > 0 && _liquidationFeeBps + liquidationIncentiveBps < 10000, "Invalid liquidation fee");
  377: uint minimumCollateral = debt * 1 ether / oracle.viewPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps;
  563: uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000;  
  564: uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000;  
  583: return debt * liquidationFactorBps / 10000; 

src\Oracle.sol
  87: uint8 decimals = 36 - feedDecimals - tokenDecimals;
  89: uint day = block.timestamp / 1 days; 
  95: uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; 
  98: uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; 
  121: uint8 decimals = 36 - feedDecimals - tokenDecimals; 
  124: uint day = block.timestamp / 1 days; 
  134: uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; 
  137: uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; 

[N-03] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param and/or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.

src\Market.sol
  226: function createEscrow(address user) internal returns (IEscrow instance) {   
  245: function getEscrow(address user) internal returns (IEscrow) {   
  292: function predictEscrow(address user) public view returns (IEscrow predicted) {
  312: function getCollateralValue(address user) public view returns (uint) {  
  323: function getCollateralValueInternal(address user) internal returns (uint) {  
  334: function getCreditLimit(address user) public view returns (uint) {  
  344: function getCreditLimitInternal(address user) internal returns (uint) {  
  353: function getWithdrawalLimitInternal(address user) internal returns (uint) { 
  370: function getWithdrawalLimit(address user) public view returns (uint) { 

src\Oracle.sol
  78: function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {  

[N-04] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

src\DBR.sol
  262: function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { 
  266: function computeDomainSeparator() internal view virtual returns (bytes32) { 

src\Market.sol
  97: function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { 
  101: function computeDomainSeparator() internal view virtual returns (bytes32) { 

#0 - c4-judge

2022-11-08T00:52:31Z

0xean marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter