Inverse Finance contest - RaymondFam's results

Zero Address Checks

Zero address checks should be implemented at the constructor to avoid human error(s) that could result in non-functional calls associated with them particularly when the incidents involve immutable variables that could lead to contract redeployment at its worst. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L77-L83 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L37-L40

Better yet, incorporate complementary codehash checks just to make sure the address inputs are the matching ones if these have not been deemed an overkill from the developer team's perspective.

block.timestamp Unreliable

The use of block.timestamp as part of the time checks can be slightly altered by miners/validators to favor them in contracts that have logic strongly dependent on them.

Consider taking into account this issue and warning the users that such a scenario could happen. If the alteration of timestamps cannot affect the protocol in any way, consider documenting the reasoning and writing tests enforcing that these guarantees will be preserved even if the code changes in the future.

Here are some of the instances entailed:

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

Inadequately Commented Assembly Block

Assembly block is used in numerous contracts of Inverse Finance. While this does not pose a security risk per se, it is at the same time a complicated and critical part of the system. Moreover, as this is a low-level language that is harder to parse by readers, consider including extensive documentation regarding the rationale behind its use, clearly explaining what every single assembly instruction does. This will make it easier for users to trust the code, for reviewers to verify it, and for developers to build on top of it or update it. Note that the use of assembly discards several important safety features of Solidity, which may render the code less safe and more error-prone.

Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L229-L235 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L296-L305

constant Variables Should be Capitalized

Constants should be named with all capital letters with underscores separating words if need be. Here are some of the instances entailed:

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

Use of uint Instead of uint256

Across the codebase, there are numerous instances of uint, as opposed to uint256. In favor of explicitness, consider replacing all instances of uint with uint256.

Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L6-L33 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L47-L54 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L58 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L69-L71

Lack of Events for Critical Operations

Critical operations not triggering events will make it difficult to review the correct behavior of the deployed contracts. Users and blockchain monitoring systems will not be able to detect suspicious behaviors at ease without events. Consider adding events where appropriate for all critical operations for better support of off-chain logging API. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L118 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L124 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L136 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L149 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L161 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L172 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L183 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L194 https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L26 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L53 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L61

Add a Timelock to Critical Parameter Change

It is a good practice to give time for users to react and adjust to critical changes with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period. This allows users that do not accept the change to withdraw within the grace period. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious Owner making any malicious or ulterior intention). Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L118 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L124 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L136 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L149 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L161 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L172 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L183 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L194 https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L26 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L53 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L61

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here are some of the instances entailed:

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

Events Associated With Setter Functions

Consider having events associated with setter functions emit both the new and old values instead of just the new value. Here are two of the instances entailed which may have the functions unanimously refactored as follows:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L70-L75 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L66-L71

    event ChangeOperator(address indexed oldOperator, address indexed newOperator);

    function claimOperator() public {
        require(msg.sender == pendingOperator, "ONLY PENDING OPERATOR");
        emit ChangeOperator(operator, msg.sender);
        operator = pendingOperator;
        pendingOperator = address(0);
    }

Sanity/Threshold/Limit checks on Setter Functions

Devoid of sanity/threshold/limit checks, certain critical parameters of the contracts can be configured to invalid values, causing a variety of issues and breaking expected interactions between contracts. Consider adding proper validation checks in the setter function logic. If the validation procedure is unclear or too complex to implement on-chain, document the potential issues that could produce invalid values. Here are some of the instances entailed that would need a boundary limit in place:

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

Add a Constructor Initializer

As per Openzeppelin's recommendation:

https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/6

The guidelines are now to prevent front-running of initialize() on an implementation contract, by adding an empty constructor with the initializer modifier. Hence, the implementation contract gets initialized atomically upon deployment.

This feature is readily incorporated in the Solidity Wizard since the UUPS vulnerability discovery. You would just need to check UPGRADEABILITY to have the following constructor code block added to the contract:

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

Here are some of the contract instances entailed:

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

No Storage Gap for Upgradeable Contracts

Consider adding a storage gap at the end of an upgradeable contract, just in case it would entail some child contracts in the future. This would ensure no shifting down of storage in the inheritance chain. Here are some of the contract instances entailed:

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

uint256[50] private __gap;

Commented Code

Throughout the codebase SimpleERC20Escrow.sol and GovTokenEscrow.sol, there are lines of code that have been commented out with //. This can lead to confusion and is detrimental to overall code readability. Consider removing commented out lines of code that are no longer needed.

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/escrows/GovTokenEscrow.sol#L56-L60

Open TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved. Consider resolving them before deploying.

Here is one of the instances entailed:

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

Missing Zero Value Checks

_replenishmentIncentiveBps at the constructor should include a zero value check to be consistent with its setter function:

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

That said, a zero value check for _collateralFactorBps should also be included unless there is a good reason for not doing so:

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

Comment and Code Mistmatch

The comment said _replenishmentIncentiveBps must be set between 1 and 10000, but the require statement implemented a threshold check between 0 and 10000:

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

Similar inconsistency has also been found on the following instance pertaining to _liquidationFactorBps:

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

Immutable Boolean

callOnDepositCallback is an immutable state boolean in Market.sol that will have its literal state or value determined at the constructor upon contract deployment. It does not make much sense implementing it in deposit() involving the following code lines:

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

        if(callOnDepositCallback) {
            escrow.onDeposit();
        }

If it has been set to true, the if condition can be removed since it is going to always run escrow.onDeposit().

If it has been set to false, lines 281 - 283 can be removed since it is going to always skip escrow.onDeposit().

Based on the code logic, it would make better sense setting it to true considering the collateral amount has been transferred from msg.sender to the escrow on line 280.

State Variable Associated With a Setter Function

liquidationFeeBps in Market.sol should be assigned its needed value at the constructor like its other counterparts unless it has been pre-assigned a non-zero value.

Had this been done, the following if statement could have been removed:

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

considering it would have been set between 0 and 10000 as documented in the comment:

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

Another good reason for setting its value upon deployment is to avoid the following problem:

1. liquidationIncentiveBps has been set to 9999,
2. setLiquidationFeeBps() is going to revert for any value greater than 0.

Lines Too Long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible. Here are some of the instances entailed:

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

Minimization of Truncation

Each of the respective three instances entailed below could have the code line refactored to minimize the frequency of truncation:

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

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

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

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

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

    uint liquidationFee = repaidDebt * 1 ether * liquidationFeeBps / (price * 10000);

Note: Comment the above operations on converting their double/multiple divisions into just one division where deemed fit.

Non-Assembly Method Alternative

Automated tools would typically flag a contract using inline-assembly as having high complexity, poor readability and error prone as far as security is concerned. As such, avoid using it where possible. For instance, there is a newer syntax way to invoke create2 without assembly. You would just need to pass salt and the contract constructor arguments as opposed to the following instance:

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

    address instance = address(new <ContractName>{salt: user}(<constructor argument(s)>));

Please visit the following link for further details:

https://solidity-by-example.org/app/create2/ https://docs.soliditylang.org/en/latest/control-structures.html#salted-contract-creations-create2

Parameterized Instead of Hard Coding

It is a good practice to parameterize the immutable contract instance, dola, at the constructor instead of getting it directly assigned in the state variable declaration, just it has been done to collateral.

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

abi.encode() Costs More Gas Than abi.encodePacked()

Changing abi.encode() to abi.encodePacked() can save gas considering the former pads extra null bytes at the end of the call data, which is unnecessary. Please visit the following the link delineating how abi.encodePacked() is more gas efficient in general:

https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

Here is one of the instances entailed:

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

Split Require Statements Using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L75 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L162 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L173 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L184 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L195 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L448 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L512

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =). As an example, consider replacing >= with the strict counterpart > in the following line of code:

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

        require(debt > amount - 1, "Insufficient debt");

Similarly, as an example, consider replacing <= with the strict counterpart < in the following line of code:

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

        if(collateralBalance < minimumCollateral + 1) return 0;

Ternary Over if ... else

Using ternary operator instead of the if else statement saves gas. For instance the following code block may be rewritten as:

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

        _value 
            ? {
                require(msg.sender == pauseGuardian || msg.sender == gov, "Only pause guardian or governance can  pause");
               }
            : {
                require(msg.sender == gov, "Only governance can unpause");
            }

External costs less gas than public visibility

Functions not internally called may have its visibility changed to external to save gas. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L118 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L124 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L136 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L149 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L161 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L172 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L183 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L194 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L203 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L212

Payable Access Control Functions Costs Less Gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L118 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L124 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L136 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L149 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L161 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L172 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L183 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L194

Private/Internal Function Embedded Modifier Reduces Contract Size

Consider having the logic of a modifier embedded through an internal or private (if no contracts inheriting) function to reduce contract size if need be. For instance, the following instance of modifier may be rewritten as follows:

    function _onlyGov() private view {
        require(msg.sender == gov, "Only gov can call this function");
    }

    modifier onlyGov() {
        _onlyGov();
        _;
    }

Function Order Affects Gas Consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the Optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
  solidity: {
    version: "0.8.11",
    settings: {
      optimizer: {
        enabled: true,
        runs: 1000,
      },
    },
  },
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization PUSH1 0x00 DUP2 EQ ISZERO PUSH1 [cont offset] JUMPI

after optimization DUP1 PUSH1 [revert offset] JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

uint256 Can be Cheaper Than uint8 and Other Unsigned Integer Type of Smaller Bit Size

When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values. Your contract’s gas usage may be higher because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. The EVM needs to properly enforce the limits of this smaller type.

It is only more efficient when you can pack variables of uint8 into the same storage slot with other neighboring variables smaller than 32 bytes. Here are some of the instances entailed:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L220 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L422 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L486

|| Costs Less Gas Than Its Equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the following code line may be rewritten as:

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

            require(!(recoveredAddress == address(0) || recoveredAddress != owner), "INVALID_SIGNER");

+= and -= Costs More Gas

+= generally costs 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop. As an example, the following line of code could be rewritten as:

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

        debts[user] =  debts[user] + additionalDebt;

Similarly, the following code line should be refactored as:

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

        debts[user] = debts[user] - repaidDebt;

Avoid Emitting State Variable When an Equivalent Alternative is Available

The following instance could have had msg.sender emitted instead of operator to save gas. In fact, msg.sender' instead of pendingOperatorshould be assigned tooperator` to save more gas. Here are two of the instances entailed which may have the functions unanimously refactored as follows:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L74 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L66-L71

    function claimOperator() public {
        require(msg.sender == pendingOperator, "ONLY PENDING OPERATOR");
        operator = msg.sender;
        pendingOperator = address(0);
        emit ChangeOperator(msg.sender);
    }

Use of Named Returns for Local Variables Saves Gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable. There are numerous instances entailed throughout all codebases:

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

As an example, the following code block can be refactored as:

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

    function getEscrow(address user) internal returns (IEscrow) {
        if(escrows[user] != IEscrow(address(0))) return escrows[user];
        escrow = createEscrow(user);
        escrow.initialize(collateral, user);
        escrows[user] = escrow;
    }

State Variables Repeatedly Read Should be Cached

SLOADs are cost 100 gas each after the 1st one whereas MLOADs/MSTOREs only incur 3 gas each. As such, storage values read multiple times should be cached in the stack memory the first time (costing only 1 SLOAD) and then re-read from this cache to avoid multiple SLOADs.

For instance, collateralFactorBps should be cached in the following two instances:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L359-L360 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L376-L377

The following code line should respectively be inserted before lines 359 and 376:

        uint256 collateralFactorBps = collateralFactorBps;

Use of Modifiers for Repeated Checks

Consider using modifiers for common checks within different functions. This will result in less code duplication in the given contract and add significant readability into the code base. For instance, the first if statement of viewPrice() and getPrice() in Oracle.sol may have a modifier in place for the identical check:

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

    modifier fixedPrices () {
        if(fixedPrices[token] > 0) return fixedPrices[token];
        _;
    }

Identical Code Block Should Be Grouped Into an Internal Function

The logic block of viewPrice() and getPrice() in Oracle.sol are almost identical except for a minor central part. They could be grouped and refactored into an internal function as follows:

    function getOrViewPrice(address token, uint collateralFactorBps, bool write) external returns (uint) {
        if(fixedPrices[token] > 0) return fixedPrices[token];
        if(feeds[token].feed != IChainlinkFeed(address(0))) {
            // get price from feed
            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);
            // potentially store price as today's low
            uint day = block.timestamp / 1 days;
            uint todaysLow = dailyLows[token][day];
            if (write) {
                if(todaysLow == 0 || normalizedPrice < todaysLow) {
                    dailyLows[token][day] = normalizedPrice;
                    todaysLow = normalizedPrice;
                    emit RecordDailyLow(token, normalizedPrice);
                }
            }
            // get yesterday's low
            uint yesterdaysLow = dailyLows[token][day - 1];
            // calculate new borrowing power based on collateral factor
            uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
            uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
            if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
                uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
                return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
            }
            return normalizedPrice;

        }
        revert("Price not found");
    }

viewPrice() and getPrice() may respectively be refactored as:

    function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
        getOrViewPrice (token, collateralFactorBps, false);
    }

    function getPrice(address token, uint collateralFactorBps) external view returns (uint) {
        getOrViewPrice (token, collateralFactorBps, true);
    }

Return Value Check

It is a good practice to allow an anticipated failed transaction to transpire as early as possible to avoid any unnecessary wastage of gas. This would object to the following line of code commented thereupon:

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

Functions of Similar Logic Could be Merged

The access controlled allow() and deny() in BorrowController.sol could be merged into and replaced by one single function as refactored below:

    function allowOrDeny(address contract, bool auth)  public onlyOperator { contractAllowlist[allowedContract] = auth; }

Unchecked SafeMath Saves Gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas.

For instance, the following code block could be refactored as:

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

    unchecked { 
        debts[borrower] += amount;
        require(credit >= debts[borrower], "Exceeded credit limit");
        totalDebt += amount;
    }