Debt DAO contest - Awesome's results

Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

For example: 🤦 Bad:

pragma solidity ^0.8.0;

🚀 Good:

pragma solidity 0.8.4;

for more info: Consensys Audit of 1inch

Solidity docs

Typos

Consider sticking to the proper spelling of words otherwise, it will decrease readability

There are 6 instances of this issue consider making the following changes:

Line 37, Line 45, Line 107, Line 213, Line 355, Line 416, Line 510

Line 37:     // Line Financials aggregated accross all existing  Credit

Line 45:     * @param arbiter_    - A neutral party with some special priviliges on behalf of Borrower and Lender.

Line 107:    * @dev - updates `status` variable in storage if current status is diferent from existing status

Line 213:    @notice - accrues token demoninated interest on a lender's position.

Line 335:    // ensure that borrowing doesnt cause Line to be LIQUIDATABLE

Line 416:    * @notice - updates `status` variable in storage if current status is diferent from existing status.

Line 510:    * @notice - Insert `p` into the next availble FIFO position in the repayment queue

Could be refactored to

Line 37:     // Line Financials aggregated across all existing  Credit

Line 45:     * @param arbiter_    - A neutral party with some special privileges on behalf of Borrower and Lender.

Line 107:    * @dev - updates `status` variable in storage if current status is different from existing status

Line 213:    @notice - accrues token denominated interest on a lender's position.

Line 335:    // ensure that borrowing doesn't cause Line to be LIQUIDATABLE

Line 416:    * @notice - updates `status` variable in storage if current status is different from existing status.

Line 510:    * @notice - Insert `p` into the next available FIFO position in the repayment queue

Use CamelCase

Consider sticking to using the CamelCase naming convention as it is highly recommended to follow these guidelines to help improve the readability of the source code

here are some instances:

Line 110, Line 121

Line 110:    function healthcheck() external returns (LineLib.STATUS) {

Line 121:    function _healthcheck() internal virtual returns (LineLib.STATUS) {

Could be refactored to

Line 110:    function healthCheck() external returns (LineLib.STATUS) {

Line 121:    function _healthCheck() internal virtual returns (LineLib.STATUS) {

Constants should be named with all capital letters

with underscores separating words. (e.g, MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION). There are 2 instances with this issue:

Line 12, Line 14

block.timestamp is unreliable

Using block.timestamp as part of the time checks could be slightly modified by miners/validators to favor them in contracts that contain logic heavily dependent on them.

Consider this problem and warn users that a scenario like this could occur. If the change of timestamps will not 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: Line 132, Line 48-50, Line 82

Uncheck arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible, some gas can be saved by using an unchecked block. For example:

Line 203-207

        for (uint256 i = 0; i < selectorsLength; ++i) {
            bytes4 selector = _selectors[i];
            SelectorToFacet memory oldFacet = ds.selectorToFacet[selector];
            require(oldFacet.facetAddress != address(0), "a2"); // Can't delete a non-existent facet


            _removeOneFunction(oldFacet.facetAddress, selector);
        }

Using Storage Instead of Memory for Structs/Arrays Saves Gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incurring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

There are 3 instances of this issue: Line 100, Line 139, Line 273

Function Order Affects Gas Consumption

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

Remove Shorthand Addition/Subtraction Assignment

Instead of using the shorthand of addition/subtraction assignment operators (+=, -=) it costs less to remove the shorthand (x += y same as x = x+y) saves ~22 gas There are 3 instances of this issue:

Line 276

Line 276:    credit.deposit += amount;

Could be refactored as

Line 276:    credit.deposit = credit.deposit + amount;

Line 144

Line 144:    unusedTokens[credit.token] -= amount;

Could be refactored as

Line 144:    unusedTokens[credit.token] = unusedTokens[credit.token] - amount;