Inverse Finance contest - immeas's results

Lines of code

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

Vulnerability details

Impact

An attacker can prevent someone from repaying his whole debt. Either just to grief or if the timing is right and the borrower is repaying just before the DBR goes into deficit it could incur an unwanted force replenish for the borrower.

Proof of Concept

There's actually already a test for this, just modified it a bit to split into one frontrunning tx:

    function testRepay_Fails_WhenAmountGtDebtFrontRun() public {
        gibWeth(user, wethTestAmount);
        gibDBR(user, wethTestAmount);
        gibDOLA(user, 500e18);

        vm.startPrank(user);

        deposit(wethTestAmount);
        uint borrowAmount = getMaxBorrowAmount(wethTestAmount);
        market.borrow(borrowAmount);

        market.repay(user, 1); // front run DoS

        vm.expectRevert("Insufficient debt");
        market.repay(user, borrowAmount);
    }

Tools Used

vscode, old reports

Recommended Mitigation Steps

Allow overpay of the debt and just let the diff count as profit.

Lines of code

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

Vulnerability details

Impact

If a user wants to completely forceReplenish a borrower with deficit, the borrower or any other malicious party can front run this with a dust amount to prevent the replenish.

Proof of Concept

    function testForceReplenishFrontRun() public {
        gibWeth(user, wethTestAmount);
        gibDBR(user, wethTestAmount / 14);
        uint initialReplenisherDola = DOLA.balanceOf(replenisher);

        vm.startPrank(user);
        deposit(wethTestAmount);
        uint borrowAmount = getMaxBorrowAmount(wethTestAmount);
        market.borrow(borrowAmount);
        uint initialUserDebt = market.debts(user);
        uint initialMarketDola = DOLA.balanceOf(address(market));
        vm.stopPrank();

        vm.warp(block.timestamp + 5 days);
        uint deficitBefore = dbr.deficitOf(user);
        vm.startPrank(replenisher);

        market.forceReplenish(user,1); // front run DoS

        vm.expectRevert("Amount > deficit");
        market.forceReplenish(user, deficitBefore); // fails due to amount being larger than deficit
        
        assertEq(DOLA.balanceOf(replenisher), initialReplenisherDola, "DOLA balance of replenisher changed");
        assertEq(DOLA.balanceOf(address(market)), initialMarketDola, "DOLA balance of market changed");
        assertEq(DOLA.balanceOf(replenisher) - initialReplenisherDola, initialMarketDola - DOLA.balanceOf(address(market)),
            "DOLA balance of market did not decrease by amount paid to replenisher");
        assertEq(dbr.deficitOf(user), deficitBefore-1, "Deficit of borrower was not fully replenished");

        // debt only increased by dust
        assertEq(market.debts(user) - initialUserDebt, 1 * replenishmentPriceBps / 10000, "Debt of borrower did not increase by replenishment price");
    }

This requires that the two txs end up in the same block. If they end up in different blocks the front run transaction will need to account for the increase in deficit between blocks.

Tools Used

vscode, forge

Recommended Mitigation Steps

Use min(deficit,amount) as amount to replenish

Lines of code

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

Vulnerability details

Impact

A borrower could get an undue liquidation losing parts or almost all of their collateral, depending on how much an attacker manages to manipulate the price. If this is a governance token this could be beneficial, other than just getting tokens for a low price, since that could increase their influence over the organization in question.

Proof of Concept

Since the oracle takes the lowest seen value of two days, an attacker only need to manipulate an artificially low price once to liquidate one or multiple borrowers.

    function testBorrow_oraclePriceManipulated() public {
        gibWeth(user, wethTestAmount);
        gibDBR(user, wethTestAmount);
        vm.startPrank(user);
        deposit(wethTestAmount);

        uint borrowAmount = getMaxBorrowAmount(wethTestAmount);
        market.borrow(borrowAmount*3/4); // only borrow 75% of credit amount leaving a healthy 25% cushion in the debt

        assertEq(market.getLiquidatableDebt(user),0);

        ethFeed.changeAnswer(ethFeed.latestAnswer()/2); // price mainpulated

        oracle.getPrice(address(market.collateral()),market.collateralFactorBps()); // record price

        ethFeed.changeAnswer(1600e18); // price returns

        assertGt(market.getLiquidatableDebt(user), 0);
    }

Perhaps not applicable when ETH is used as collateral but for governance tokens. Since their trading volume is lower and the price could more easily be manipulated. A large holder of that governance token could drop the price and record it in the oracle. Force liquidate tokens used as collateral for loans, buy their initial tokens back, gaining more influence where the governance tokens are used.

Tools Used

vs code, forge

Recommended Mitigation Steps

Do not take the low of two days but the high of the low/mid of two or three days or some other sort of low time weighted average.

Lines of code

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#L119

Vulnerability details

Impact

The api doesn't provide the necessary data points to validate the feed. Also, Chainlink can stop providing this deprecated api.

Proof of Concept

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

the same issue was considered medium here: https://code4rena.com/reports/2022-01-yield/#m-01-oracle-data-feed-is-insufficiently-validated https://code4rena.com/reports/2021-09-wildcredit/#m-01-use-of-deprecated-chainlink-api https://code4rena.com/reports/2021-07-wildcredit/#m-01-chainlink---use-latestrounddata-instead-of-latestanswer-to-run-more-validations

Tools Used

vscode, previous reports and chainlink docs

Recommended Mitigation Steps

use latest: https://docs.chain.link/docs/data-feeds/price-feeds/api-reference/#latestrounddata

then validate that the oracle prices are not stale:

    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
            (uint80 roundID, int256 price, , uint256 timestamp, uint80 answeredInRound) = feeds[token].feed.latestRoundData();
            require(price > 0, "Invalid feed price");
            require(answeredInRound >= roundID, "Stale price");
            require(timestamp > 0, "Round not complete");
            // normalize price
            ...

        }
        revert("Price not found");
    }

Low

L-1 consider two step ownership transfer for Market and Fed

As in DBR.sol, to prevent accidental transfer to invalid addresses.

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

File: src/Market.sol

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

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L48-L51

File: src/Fed.sol

48:    function changeGov(address _gov) public {
49:        require(msg.sender == gov, "ONLY GOV");
50:        gov = _gov;
51:    }

L-2 consider using safeTransfer/From or a safeTransfer/From library

Even though there might be a strict vetting of what tokens can be used as collateral, there's little cost of using safeTransfer. That will give you the option of using more types of collateral. Otherwise you would have to develop a new Market contract, which could impact trust.

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

File: src/Market.sol

280:        collateral.transferFrom(msg.sender, address(escrow), amount);

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L38

File: src/escrows/SimpleERC20Escrow.sol

38:        token.transfer(recipient, amount);

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L45

File: src/escrows/GovTokenEscrow.sol

45:        token.transfer(recipient, amount);

Non-crit

NC-1 use 1e18 instead of 1 ether

Please use scientific notation for better code readability. 1 ether also implies that it has something to do with native ether which it doesn't.

There are 6 instances of this issue:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L315 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L326 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L360 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L597 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L606

File: src/Market.sol:

315:        return collateralBalance * oracle.viewPrice(address(collateral), collateralFactorBps) / 1 ether;

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

360:        uint minimumCollateral = debt * 1 ether / oracle.getPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps;

597:        uint liquidatorReward = repaidDebt * 1 ether / price;

606:            uint liquidationFee = repaidDebt * 1 ether / price * liquidationFeeBps / 10000;

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

File: src/escrows/INVEscrow.sol:

72:        uint invBalanceInXInv = xINV.balanceOf(address(this)) * xINV.exchangeRateStored() / 1 ether;

NC-2 oracle.getPrice() is better named updatePrice()

get and view are very easy to mix up. Also, get implies no side effects while update/getAndUpdate would imply that there are changes as well.

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

File: src/Oracle.sol

78:     function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
    
112:    function getPrice(address token, uint collateralFactorBps) external returns (uint) {

NC-3 open todos

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

File: src/escrows/INVEscrow.sol

35:         xINV = _xINV; // TODO: Test whether an immutable variable will persist across proxies

NC-4 very long row

GitHub starts using a scroll bar when the length is over 164 characters, the lines below should be split when they reach that length.

There are 4 instances of this issue:

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

File: src/Oracle.sol

12:The Pessimistic Oracle introduces collateral factor into the pricing formula. 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.

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

File: src/Fed.sol

99:     @dev Markets can have more DOLA in them than they've been supplied, due to force replenishes. This call will revert if trying to contract more than have been supplied.

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L16

File: src/escrows/GovTokenEscrow.sol

16: This specific escrow is meant as an example of how an escrow can be implemented that allows depositors to delegate votes with their collateral, unlike pooled deposit protocols.

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

File: src/escrows/INVEscrow.sol

25: This escrow allows user to deposit INV collateral directly into the xINV contract, earning APY and allowing them to delegate votes on behalf of the xINV collateral

NC-5 misspelled comment

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

File: src/Market.sol

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