Inverse Finance contest - codexploder's results

Lines of code

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

Vulnerability details

Impact

If the Governance set _collateralFactorBps as 0 while calling setCollateralFactorBps function then User full collateral will be gone as shown in POC

Proof of Concept

1. The setCollateralFactorBps function allows Governance to set 0 value for collateralFactorBps which can have disastrous consequences

function setCollateralFactorBps(uint _collateralFactorBps) public onlyGov {
        require(_collateralFactorBps < 10000, "Invalid collateral factor");
        collateralFactorBps = _collateralFactorBps;
    }

2. This will turn credit limit and withdrawal limit of user to 0 even though User has collateral

function getCreditLimit(address user) public view returns (uint) {
        uint collateralValue = getCollateralValue(user);
        return collateralValue * collateralFactorBps / 10000;
    }

Recommended Mitigation Steps

Add a minimum value for _collateralFactorBps

Lines of code

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

Vulnerability details

Impact

Decimal normalization is done incorrectly. From code it seems that tokens are to be normalized to 18 decimal places.

1. This normalization should be done based on feedDecimals values and not tokenDecimals since price is determined using feed price by latestAnswer
2. The decimals for token could be more than 18 so current calculation might cause underflow

Proof of Concept

1. The normalizedPrice is currently calculated as:

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

2. Decimal returned by feedDecimals and tokenDecimals could be more than 18, say 20 which means decimals = 36 - 20-20 = underflow.

3. Only feedDecimals consideration (feeds[token].feed.decimals()) should be enough since price is derived from feed (feeds[token].feed.latestAnswer())

Recommended Mitigation Steps

Revise the normalizedPrice calculation as below:

uint8 feedDecimals = feeds[token].feed.decimals();
uint8 decimals;
uint normalizedPrice;

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

Lines of code

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

Vulnerability details

Impact

The latestAnswer function has been deprecated , in favor of latestRoundData function. latestAnswer method will return the last value, but you won't be able to check if the data is fresh. On the other hand, calling the method latestRoundData allow you to run some extra validations like the round to which price belongs

Proof of Concept

1. getPrice function at Oracle.sol#L77 makes use of latestAnswer function which is now deprecated in favor of latestRoundData

uint price = feeds[token].feed.latestAnswer();

Recommended Mitigation Steps

Make use of latestRoundData function instead and make basic validations

(uint80 roundID ,price,, uint256 timestamp, uint80 answeredInRound) = feeds[token].feed.latestAnswer();
require(answer > 0, "Chainlink price <= 0"); 
require(answeredInRound >= roundID, "Stale price");
require(timestamp != 0, "Round not complete");