Inverse Finance contest - adriro's results

Lines of code

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

Vulnerability details

Impact

The Oracle contract normalizes prices in both viewPrices and getPrices functions to adjust for potential decimal differences between feed and token decimals and the expected return value.

However these functions assume that feedDecimals and tokenDecimals won't exceed 18 since the normalization calculation is 36 - feedDecimals - tokenDecimals, or that at worst case the sum of both won't exceed 36.

This assumption should be safe for certain cases, for example WETH is 18 decimals and the ETH/USD chainlink is 8 decimals, but may cause an overflow (and a revert) for the general case, rendering the Oracle useless in these cases.

Proof of Concept

If feedDecimals + tokenDecimals > 36 then the expression 36 - feedDecimals - tokenDecimals will be negative and (due to Solidity 0.8 default checked math) will cause a revert.

Recommended Mitigation Steps

In case feedDecimals + tokenDecimals exceeds 36, then the proper normalization procedure would be to divide the price by 10 ** decimals. Something like this:

uint normalizedPrice;

if (feedDecimals + tokenDecimals > 36) {
    uint decimals = feedDecimals + tokenDecimals - 36;
    normalizedPrice = price / (10 ** decimals)
} else {
    uint8 decimals = 36 - feedDecimals - tokenDecimals;
    normalizedPrice = price * (10 ** decimals);
}

Validate operator is not address(0) in setOperator function of BorrowController

Validate the new operator is not address(0) so that the operator role remains accessible in case of mistakenly setting a zero/empty value.

Duplicated code in Oracle viewPrices and getPrices functions

Consider refactoring duplicate code between these two functions since most of the code is exactly the same except for the difference in the update to the daily low.

Consider using a onlyGov / onlyChair modifier in FED contract

Most of the protected functions in the FED contract have the explicit require to check the sender's role. Consider adding a onlyGov and onlyChair modifier to protect these functions.

Ensure gov and chair addresses in the FED contract are initialized and non-empty

Check that gov and chair are not zero/empty (address(0)) in the contract's constructor and in the functions changeGov and changeChair.

Unused interface functions in Market contract

Both balanceOf functions of IERC20 and IDolaBorrowingRights interfaces aren't used in the scope of this contract and can be safely removed.

Validate _replenishmentIncentiveBps is greater than 0 in Market constructor

Validate _replenishmentIncentiveBps is greater than 0 during initialization to match the semantics defined in the setter setReplenismentIncentiveBps.

Validate gov address is initialized and non-empty in the Market contract

Validate _gov != address(0) in the contract's constructor and the setGov setter. A wrong value here will lead to the contract being ungovernable.

Typo in setReplenismentIncentiveBps function of Market contract

Missing h in the function's name.

Change _totalSupply to private in DolaBorrowingRights contract

The storage variable _totalSupply is defined as public, which will define a _totalSupply() getter, which could cause confusion with the totalSupply() function that properly tracks debt.

Validate _operator address is initialized and non-empty in the DolaBorrowingRights contract

Validate _operator != address(0) in the contract's constructor. A wrong value here will leave role protected functions inaccessible.

Unneeded check in Oracle getPrice function

The twoDayLow > 0 check in line 136 isn't needed since twoDayLow will always take at least the current normalizedPrice value, which is greater than 0 due to the check in line 117.

Move check condition in expansion function of FED contract to save gas

The check in line 93 can be moved up in the stack to early exit the function in case the condition fails in order to save gas.

uint256 newGlobalSupply = globalSupply + amount;
require(newGlobalSupply <= supplyCeiling);
...
globalSupply = newGlobalSupply;
emit Expansion(market, amount);

Use unchecked math in function contraction of FED contract

Lines 110 and 111 can be placed under an unchecked math group since amount <= supply due to check in line 107 and can't possibly underflow these two updates.

Force replenish checks are done twice in Market and DBR contracts

The checks around the deficit value (deficit > 0 and deficit >= amount) are done twice and repeated over the Market and DBR contracts in the functions forceReplenish and onForceReplenish respectively.

Store result of debts[borrower] += amount in borrowInternal function of Market contract

The calculation executed in line 395 is stored in storage and re-read from storage in the next line to check against the credit limit. Store the calculation locally to prevent an unnecessary re-read to storage.

name and symbol variables in DolaBorrowingRights contract can be changed to immutable to save gas

These two variables are defined at construction time and can't be updated. Consider defining them as immutable to avoid reading from storage to save gas.

Use unchecked math in transfer and transferFrom functions of DolaBorrowingRights contract

The updates in lines 172 and 196 can be done using unchecked math since balances[msg.sender] >= amount and balances[from] >= amount due to the checks in lines 171 and 195 respectively.

Store lastUpdated[user] locally to prevent a second read in function accrueDueTokens of DolaBorrowingRights contract

Value is loaded in line 286 and read again in the next line. Store the first read locally to avoid a second sload.

Check if debt is zero in accrueDueTokens to skip unnecessary calculations and save gas

If debt is zero in the accrueDueTokens function (line 285), then operations in lines 287, 288 and 290 can be skipped to save gas.

Use unchecked math in _burn function of DolaBorrowingRights contract

The update in line 374 can be done using unchecked math since balances[from] >= amount due to the check in line 373.