Inverse Finance contest - pashov's results

Lines of code

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

Vulnerability details

Proof of Concept

The contract’s admin has control to set values in setCollateralFactorBps(), setLiquidationFactorBps(), setReplenismentIncentiveBps(), setLiquidationIncentiveBps(), setLiquidationFeeBps(). All of them have upper bounds, most of them have lower bounds, but they are mostly serving a sanity check role, they aren’t really upper/lower bounds.

For example admin can set the liquidationIncentive to 99.99% which will result in almost all of the user liquidated collateral going to the liquidator. This is not expected by protocol users and can be seen as an unexpected loss of value for them.

Another issue is admin can set collateralFactorBps to zero, which will DoS withdrawals for users, because of this check in getWithdrawalLimit()

if(collateralFactorBps == 0) return 0;

and since on withdraw you have the following check

uint limit = getWithdrawalLimitInternal(from);
require(limit >= amount, "Insufficient withdrawal limit");

then all withdraws will result in a revert (DoS)

Impact

The admin has too much control over setting protocol params and a malicious or a compromised owner can make it so that users can’t withdraw any of their assets or that they get almost all of their liquidated collateral lost, resulting in a value loss for them.

Recommendation

Add sensible upper & lower bounds for all bps field setters, don’t let collateralFactorBps be set to zero.

Lines of code

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

Vulnerability details

Proof of Concept

Chainlink price feeds usually have 18 decimals, but this is not guaranteed. Also tokens usually have 18 decimals or less but this is also not the case for 100% of widely used tokens (YAM-v2 has 24).

So the normal use case is when both the feed an the token have 18 decimals or less. There are three options when the following code will revert

uint8 decimals = 36 - feedDecimals - tokenDecimals;

1. If we have feedDecimals to be 18, but tokenDecimals to be >18
2. If we have tokenDecimals to be 18, but feedDecimals to be >18
3. If there is some other combination that tokenDecimals + feedDecimals > 36

If some of the examples are the case, then the viewPrice() and getPrice() functions in Oracle.sol will always revert because of the uint underflow, resulting in all of the protocol functions being in a DoS state.

Impact

The impact is 100% malfunctioning of the protocol, but it will happen only under some Market collateral circumstances, hence the Medium severity.

Recommendation

When adding a new token and price feed in Oracle.sol add a require statement that makes sure tokenDecimals + feedDecimals <= 36

Lines of code

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

Vulnerability details

Proof of Concept

Chainlink has market the latestAnswer() method as deprecated for his price feeds, but the code is using it.

Impact

The latestAnswer() method just returns the price and has no way to check if it is stale. If the project is using a stale price it can result in miscalculations of collateral price and in value stolen from the protocol.

Recommendation

Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, for example:

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = feed.latestRoundData();
require(answeredInRound >= roundID, "...");
require(timeStamp != 0, "...");