Inverse Finance contest - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L56-L60 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L49-L53 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L16-L21 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L281-L283

Vulnerability details

Impact

SimpleERC20Escrow and GovTokenEscrow contracts do not have onDeposit function that can be called from Market contract. That means that no one will be allowed to deposit when Market is initialized with callOnDepositCallback == true and escrow is of type SimpleERC20Escrow or GovTokenEscrow, because the deposit function will revert.

Proof of Concept

function deposit(address user, uint amount) public {
        IEscrow escrow = getEscrow(user);
        collateral.transferFrom(msg.sender, address(escrow), amount);
        if(callOnDepositCallback) {
            escrow.onDeposit();
        }
        emit Deposit(user, amount);
    }

Market.deposit function calls IEscrow.onDeposit if callOnDepositCallback is true. In case when escrow implementation is of type SimpleERC20Escrow or GovTokenEscrow the function will always revert as both contracts has this function commented.

As a result no one can deposit and market is not working.

Tools Used

VsCode

Recommended Mitigation Steps

Extend SimpleERC20Escrow and GovTokenEscrow from IEscrow directly in the contract and implement method onDeposit.

Lines of code

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

Vulnerability details

Impact

If no one called Oracle.getPrice during the day, then no minimal day price will be saved and Oracle price decreasing function becomes useless.

Proof of Concept

Oracle.getPrice function stores the lowest price per day to the Oracle state. Then this is used for price normalization. If the current price is too high, the oracle takes min of yesterdays and todays lowest prices and compare if current price * 80% is bigger then lowest price of 2 days. If it's still begger(the price increased a lot) then oracle check if this lowest price * 125%(as sponsor said) is still less then current price. If yes, then lowest price * 125% is returned as price. So this mechanism is created to defeat the sharp increase of price.

However it will not be working correctly, if there was no price info saved for the day.

Oracle has 2 functions: viewPrice and getPrice. Only getPrice function saves prices as lowest in the day. Oracle.getPrice will be invoked only through the Market.liquidate, Market.forceReplenish, Market.borrow, Market.withdraw. Functions Market.getWithdrawalLimit and Market.getCollateralValue use Oracle.viewPrice.

1.Suppose in t1 price has increased strongly and Oracle has yesterdays and todays lowest price saved before, so the price was normalized and Oracle did his function well. 2.Suppose in t10 price has increased again and Oracle do not have yesterdays and todays lowest price saved, because no one called Market functions that call Oracle.getPrice during the previous 2 days. In this case Oracle can't normalize the price and returns the one received form chainlink.

As you can see if no one calls Oracle.getPrice during the previous day, then Oracle can't normalize the price. The price can be too big.

Tools Used

VsCode

Recommended Mitigation Steps

Add function to Market to call Oracle.getPrice that can be called once per day and create some js bot the will trigger the function.

Lines of code

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

Vulnerability details

Impact

Market do not deal with collateral token decimal value. It suppose that collateral asset has 18 decimals like DOLA. Because of that the prices are wrong for the non 18 decimals tokens.

Proof of Concept

Let's look at one example.

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

function borrow(uint amount) public {
        borrowInternal(msg.sender, msg.sender, amount);
    }

Which calls https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L389-L401

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);
    }

Here users deposit(credit param) is checked to be less then his debt. This credit is returned as 18 decimals value. But there is no conversion to 18 decimals of amount provided by borrower. So while credit is 18 decimals, debt can have more or less decimals and comparing them is not correct. You can see that amount provided by user is then sent to him in DOLA. DOLA is 18 decimals token.

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;
    }

It calls getCollateralValueInternal. 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;
    }

This function takes amount from user's escrow and then multiplies it with oracle.getPrice which is 18 decimal price. The decimals of asset token is not counted here.

That means that the prices that are stored to user debt are incorrect and can be less or greater depending on the collateral assets decimals count.

Tools Used

VsCode

Recommended Mitigation Steps

You need to convert user amount value to the 18 decimals as well.

Lines of code

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

Vulnerability details

Impact

Oracle contract uses latestAnswer function to get price from price feed. But according to Chainlink's documentation, it is deprecated. Though the returned value is checked for 0, it's not enough to be safe. This function might suddenly stop working if Chainlink stop supporting deprecated APIs.

Proof of Concept

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

Also you can see, similar finding here.

Tools Used

VsCode

Recommended Mitigation Steps

Use the latestRoundData function to get the price instead.

1. For the safety reasons it will be good to check if _escrowImplementation.token == _collateral. In this way you sure, that the escrow works with same token as market. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L80

2. Market. DOMAIN_SEPARATOR function can’t be called from another chain. So computeDomainSeparator() function will never be called. Looks like this function should just return INITIAL_CHAIN_ID value always. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L97-L99

3. Ownership of Market contract can be lost. Market.gov address is very important in the contract as it can call functions that change Market behaviour. However it’s not secured from changing this address to wrong account or 0. If gov will be lost, then you can’t adjust Market anymore. And one more important thing is that if you pause Market then it’s only the gov who can unpause it. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130 I suggest to add 2 step ownership changing like it is done in DBR. https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L70-L75 I would also recommend to use same flow everywhere.

4. There are multiple functions where dev comments say that provided values can be in range [x;y] however it's not exactly like that. Whether code or comments should be changed. a. Market. setReplenismentIncentiveBps says @dev Must be set between 1 and 10000., but it’s not possible to set 10000. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L173 Also, though it’s not allowed to provide 0 in setter, you can do that in constructor. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L76 b. Market. setLiquidationIncentiveBps says @dev Must be set between 0 and 10000 - liquidation fee , but possible values are from 1 to 9999-liquidation fee. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L184 c. Market. setLiquidationFeeBps says @dev Must be set between 0 and 10000 - liquidation factor, but possible values are from 1 to 9999-liquidation factor. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L184

5. In Market.liquidate function there is fee transfer to the protocol in case when fee is switched on. But if escrow balance is less then calculated fee, then the fee transfer is skipped. I propose in this case to send all balance of escrow to the protocol. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L605-L610

6. In Oracle.setFeed it’s possible to make mistake and provide incorrect token decimals. I propose instead call token.decimals() on the provided token address to get the 100% correct value. https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L605-L610