Debt DAO contest - delfin454000's results

QA Report - low risk

Unused/empty receive()/fallback() function

Leaving the receive() below without a revert could result in a loss of funds for a user who sends Ether to the contract (having no way to get anything back)

SpigotedLine.sol: L271-272

    // allow claiming/trading in ETH
    receive() external payable {}

---

---

Missing checks for address(0x0) when assigning values to address state variables

---

Checks for address(0x0) are missing for arbiter_ and borrower_:

LineOfCredit.sol: L56-57

        arbiter = arbiter_;
        borrower = borrower_;

Similarly for the following:

Escrow.sol: L49-50

---

---

Solidity pragma version should be upgraded to latest available version before finalization

The current solidity version in contracts is ^0.8.9, compared to the latest version of ^0.8.17. Only the latest version receives security fixes. However, the latest version often has bugs, so it's safer to use releases a few versions older at first, as has been done here.

However, some Debt DAO contracts use pragma solidity 0.8.9; rather than pragma solidity ^0.8.9;. These should be corrected:

EscrowedLine.sol: L1

Spigot.sol: L1

Escrow.sol: L1

LineFactory.sol: L1

CreditLib.sol: L1

LineLib.sol: L1

CreditListLib.sol: L1

EscrowLib.sol: L1

SpigotLib.sol: L1

SpigotedLineLib.sol: L1

MutualConsent.sol: L3

LineFactoryLib.sol: L1

pragma solidity 0.8.9;

Change to pragma solidity ^0.8.9;

---

---

QA Report: non-critical

Typos

---

LineOfCredit.sol: L37

    // Line Financials aggregated accross all existing  Credit

Change accross to across

---

The same typo (priviliges) occurs in both lines referenced below:

LineOfCredit.sol: L45

SpigotedLine.sol: L46

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

Change priviliges to privileges in both cases

---

The same typo (diferent) occurs in both lines referenced below:

LineOfCredit.sol: L107

LineOfCredit.sol: L416

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

Change diferent to different in both cases

---

The same typo (demoninated) occurs in both lines referenced below:

LineOfCredit.sol: L213

CreditLib.sol: L250

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

Change demoninated to denominated in both cases

---

LineOfCredit.sol: L395

        // ensure all money owed is accounted for. Accrue facility fee since prinicpal was paid off

Change prinicpal to principal

---

LineOfCredit.sol: L501

        // If all credit lines are closed the the overall Line of Credit facility is declared 'repaid'.

Remove repeated word the

---

LineOfCredit.sol: L510

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

Change availble to available

---

SpigotedLine.sol: L38

     * @dev    - private variable so other Line modules do not interfer with Spigot functionality

Change interfer to interfere

---

SpigotedLine.sol: L84

      * @notice requires Spigot contract itselgf to be transfered to Arbiter and sold off to a 3rd party before declaring insolvent

Change itselgf to itself

---

The same typo (priviliegad) occurs in all four lines referenced below:

SpigotedLine.sol: L85

EscrowedLine.sol: L45

EscrowedLine.sol: L74

EscrowedLine.sol: L85

      *(@dev priviliegad internal function.

Change priviliegad to privileged in each case

---

The same typo (priviliged) occurs in both lines referenced below:

SpigotedLine.sol: L85

SpigotedLineLib.sol: L44

     * @dev     - priviliged internal function

Change priviliged to privileged in both cases

---

SecuredLine.sol: L110

    * @return isInsolvent - if the entire Line including all collateral sources is fuly insolvent.

Change fuly to fully

---

EscrowedLine.sol: L84

   * @notice helper function to allow borrower to easily swithc collateral to a new Line after repyment

Change swithc to switch and repyment to repayment

---

Spigot.sol: L133

     * @dev - revenuContract's transfer func MUST only accept one paramteter which is the new owner.

Change paramteter to parameter

---

Escrow.sol: L94

     * @notice - allows  the lines arbiter to  enable thdeposits of an asset

Change thdeposits to the deposits

---

Escrow.sol: L95

     *        - gives  better risk segmentation forlenders

Change forlenders to for lenders

---

InterestRateCredit.sol: L8

    // Must divide by 100 too offset bps in numerator and divide by another 100 to offset % and get actual token amount

Change too to to

---

InterestRateCredit.sol: L17

     * @notice Interest rate / acrrued interest calculation contract for Line of Credit contracts

Change acrrued to accrued

---

The same typo (interset) occurs in both lines referenced below:

CreditLib.sol: L123

CreditLib.sol: L237

    * @param oracle - interset rate contract used by line that will calculate interest owed

Change interset to interest in both cases

---

CreditLib.sol: L200

    * @param credit - The lender position that is being bwithdrawn from

Change bwithdrawn to being withdrawn

---

CreditLib.sol: L221

          // emit events before seeting to 0

Change seeting to setting

---

SpigotedLineLib.sol: L48

     * @param spigot        - The Spigot to claim from. Must be owned by adddress(this)

Change adddress to address

---

SpigotedLineLib.sol: L188

   * @notice -  Transfers ownership of the entire Spigot and its revenuw streams from its then Owner to either 

Change revenuw to revenue

---

SpigotedLineLib.sol: L189

                the Borrower (if a Line of Credit has been been fully repaid) or 

Remove repeated word been

---

---

Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

---

SpigotLib.sol: L255-260

    event ClaimRevenue(
        address indexed token,
        uint256 indexed amount,
        uint256 escrowed,
        address revenueContract
    );

---

SpigotLib.sol: L262-266

    event ClaimEscrow(
        address indexed token,
        uint256 indexed amount,
        address owner
    );

---

---

TODOs and other open items

Comments that refer to open items should be addressed and more fully explained, or else modified or removed.

---

LineFactory.sol: L140

        // TODO: test

---

LineFactory.sol: L145

        // TODO: test

---

CreditListLib.sol: L18

     * Bob - consider renaming this function removeId()

---

SpigotLib.sol: L117

        self.escrowed[token] = 0; // keep 1 in escrow for recurring call gas optimizations?

---

---

Use scientific notation for large multiples of 10 rather than decimal literals

For readability, used scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000) for large multiples of ten

---

InterestRateCredit.sol: L6-11

    // 1 Julian astronomical year in seconds to use in calculations for rates = 31557600 seconds
    uint256 constant ONE_YEAR = 365.25 days;
    // Must divide by 100 too offset bps in numerator and divide by another 100 to offset % and get actual token amount
    uint256 constant BASE_DENOMINATOR = 10000;
     // = 31557600 * 10000 = 315576000000;
    uint256 constant INTEREST_DENOMINATOR = ONE_YEAR * BASE_DENOMINATOR;

Suggestion:

    // 1 Julian astronomical year in seconds to use in calculations for rates = 3.15576e7 seconds
    uint256 constant ONE_YEAR = 365.25 days;
    // Must divide by 100 to offset bps in numerator and divide by another 100 to offset % and get actual token amount
    uint256 constant BASE_DENOMINATOR = 1e4;
     // = 3.15576e7 * 1e4 = 3.15576e11;
    uint256 constant INTEREST_DENOMINATOR = ONE_YEAR * BASE_DENOMINATOR;

---

---

Extra-long single-line comments

In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if longer comments (up to 120 characters) are acceptable and a scroll bar is provided, very long comments can interfere with readability.

Some of the long comments in Debt DAO do wrap but the treatment of very long comments is inconsistent. Also, long spaces within some comments elongate them unnecessarily. Below, I divide the long comments in the contest into three types, with examples of each and references to additional long comments of the type.

---

Type 1: Long NatSpec statements

Example

CreditListLib.sol: L12-13

     * @dev assumes that `id` of a single credit line within the Line of Credit facility (same lender/token) is stored only once in the `positions` array 
     since there's no reason for them to be stored multiple times.

Suggestion:

     * @dev assumes that `id` of a single credit line within the Line of Credit facility (same lender/token)   
       is stored only once in the `positions` array since there's no reason for them to be stored multiple times.

Below are more extra-long (at least 120 characters) NatSpec statements:

LineOfCredit.sol

L42-43, L47

SpigotedLine.sol

L18, L28, L36-37, L49, L50

L51, L84, L182

Spigot.sol

L12-13, L26, L27, L28-29

L68, L131-132

Escrow.sol

L17, L36, L38-39, L40

L62, L63, L71

LineFactory.sol

L171

CreditLib.sol

L31-32, L55, L101

CreditListLib.sol

L17

SpigotedLineLib.sol

L39-42, L46, L164, L212-214

LineFactoryLib.sol

L34

---

Type 2: Extra-long lines that include both code and comments

If a line containing a comment is longer than 120 characters, it makes sense to put the comment in the line above, as shown:

Example:

LineOfCredit.sol: L35

    mapping(bytes32 => Credit) public credits; // id -> Reference ID for a credit line provided by a single Lender for a given token on a Line of Credit

Suggestion:

    // id -> Reference ID for a credit line provided by a single Lender for a given token on a Line of Credit
    mapping(bytes32 => Credit) public credits;

Similarly for the following:

LineOfCredit.sol: L58

---

Type 3: Extra-long comment lines

If a line containing a comment is longer than 120 characters, it makes sense to wrap the comment, as shown:

Example:

CreditLib.sol: L42-43

  /** Emits that a Borrower has repaid an amount of interest 
  (N.B. results in an increase in interestRepaid, i.e. interest not yet withdrawn by a Lender). There is no corresponding function

Suggestion:

  /** Emits that a Borrower has repaid an amount of interest 
      (N.B. results in an increase in interestRepaid, i.e. interest not yet withdrawn by a Lender). 
      There is no corresponding function.

Similarly for the following:

CreditListLib.sol: L14

SpigotLib.sol: L15

SpigotLib.sol: L69

---

---

Strong language in comment

While a comment may contain abbreviations, shorthand for well-known or previously defined terms, inconsistent or nonstandard punctuation, and use of minor grammatical errors, it should not contain strong language that could be offensive.

---

SpigotLib.sol: L69

        // cant claim revenue via operate() because that fucks up accounting logic. Owner shouldn't whitelist it anyway but just in case

---

---

Update sensitive terms in both comments and code

Terms incorporating "black," "white," "slave" or "master" are potentially problematic. Substituting more neutral terminology is becoming common practice.

---

Spigot.sol: L187-195

     * @notice - Allows Owner to whitelist function methods across all revenue contracts for Operator to call.
     *           Can whitelist "transfer ownership" functions on revenue contracts
     *           allowing Spigot to give direct control back to Operator.
     * @dev - callable by `owner`
     * @param func - smart contract function signature to whitelist
     * @param allowed - true/false whether to allow this function to be called by Operator
     */
     function updateWhitelistedFunction(bytes4 func, bool allowed) external returns (bool) {
        return state.updateWhitelistedFunction(func, allowed);

Suggestion: Change whitelist to allowlist throughout. Similarly for its variants.

---

---

NatSpec is partially missing

NatSpec is partially missing for the following external or public functions (NatSpec is not required for internal or private functions).

Note that I'm excluding functions preceded by comments such as /// see ILineOfCredit.increaseCredit. However, such references appear to be insufficient and inconvenient substitutes for full NatSpec for complex functions with multiple parameters.

---

Spigot.sol: L144-148

    // Changes the revenue split between the Treasury and the Owner based upon the status of the Line of Credit
    // or otherwise if the Owner and Borrower wish to change the split.
    function updateOwnerSplit(address revenueContract, uint8 ownerSplit)
        external
        returns(bool)

Missing: @param revenueContract, @param ownerSplit and @return

---

Spigot.sol: L128-139

    /**
     *   @dev   We don't transfer the ownership of Escrow and Spigot internally
     *          because they aren't owned by the factory, the responsibility falls
     *          on the [owner of the line]
     *   @dev   The `cratio` in the CoreParams are not used, due to the fact
     *          they're passed in when the Escrow is created separately.
     */
    function deploySecuredLineWithModules(
        CoreLineParams calldata coreParams,
        address mSpigot,
        address mEscrow
    ) external returns (address line) {

Missing: @param coreParams, @param mSpigot, @param mEscrow and @return

---

EscrowLib.sol: L28-34

    /**
     * @notice updates the cratio according to the collateral value vs line value
     * @dev calls accrue interest on the line contract to update the latest interest payable
     * @param oracle - address to call for collateral token prices
     * @return cratio - the updated collateral ratio in 4 decimals
    */
    function _getLatestCollateralRatio(EscrowState storage self, address oracle) public returns (uint256) {

Missing: @param self

---

EscrowLib.sol: L46-51

    /**
    * @notice - Iterates over all enabled tokens and calculates the USD value of all deposited collateral
    * @param oracle - address to call for collateral token prices
    * @return totalCollateralValue - the collateral's USD value in 8 decimals
    */
    function _getCollateralValue(EscrowState storage self, address oracle) public returns (uint256) {

Missing: @param self

---

SpigotedLineLib.sol: L142-148

    /**
     * @notice cleanup function when a Line of Credit facility has expired.
        Used in the event that we want to reuse a Spigot instead of starting from scratch
     */
    function rollover(address spigot, address newLine) external returns(bool) {
      require(ISpigot(spigot).updateOwner(newLine));
      return true;

Missing: @param spigot, @param newLine and @return

---

LineFactoryLib.sol: L33-47

    /**
      @notice sets up new line based of config of old line. Old line does not need to have REPAID status for this call to succeed.
      @dev borrower must call rollover() on `oldLine` with newly created line address
      @param oldLine  - line to copy config from for new line.
      @param borrower - borrower address on new line
      @param ttl      - set total term length of line
      @return newLine - address of newly deployed line with oldLine config
     */
    function rolloverSecuredLine(
        address payable oldLine,
        address borrower, 
        address oracle,
        address arbiter,
        uint ttl
    ) external returns(address) {

Missing: @param oracle and @param arbiter

---

@return is the only type of NatSpec statement missing for the following functions:

---

Spigot.sol: L101-109

    /**
     * @notice - Allows Operator to call whitelisted functions on revenue contracts to maintain their product
     *           while still allowing Spigot Owner to receive its revenue stream
     * @dev - cannot call revenueContracts claim or transferOwner functions
     * @dev - callable by `operator`
     * @param revenueContract - contract to call. Must have existing settings added by Owner 
     * @param data - tx data, including function signature, to call contract with
     */
    function operate(address revenueContract, bytes calldata data) external returns (bool) {
        return state.operate(revenueContract, data);

@return similarly missing for the following functions:

Spigot.sol

L118-125, L131-139, L154-159, L165-170

L176-181, L187-194, L203-206, L211-216

Escrow.sol

L54-57, L61-65, L69-74, L93-100

LineFactory.sol

L170-182

LineLib.sol

L28-40, L53-65, L76-80

---

---

NatSpec is completely missing

NatSpec is completely missing for the following external or public functions:

LineOfCredit.sol: L64-67

    function init() external virtual returns(LineLib.STATUS) {
      if(status != LineLib.STATUS.UNINITIALIZED) { revert AlreadyInitialized(); }
      return _updateStatus(_init());
    }

NatSpec is similarly missing for the following functions:

SpigotedLine.sol

L78-80

Spigot.sol

L41-43, L45-47, L49-51, L220-222

LineFactory.sol

L59-61, L87-89

SpigotedLineLib.sol

L120-127, L151

LineFactoryLib.sol

L57, L77-86

NatSpec is completely missing for the following constructor:

SecuredLine.sol: L13-22

    constructor(
        address oracle_,
        address arbiter_,
        address borrower_,
        address payable swapTarget_,
        address spigot_,
        address escrow_,
        uint ttl_,
        uint8 defaultSplit_
    ) SpigotedLine(

NatSpec is similarly missing for the following constructors:

EscrowedLine.sol: L16-17

LineFactory.sol: L20-25

---

NatSpec is completely missing for the following struct:

EscrowLib.sol: L12-19

struct EscrowState {
    address line;
    address[] collateralTokens;
    /// if lenders allow token as collateral. ensures uniqueness in collateralTokens
    mapping(address => bool) enabled;
    /// tokens used as collateral (must be able to value with oracle)
    mapping(address => IEscrow.Deposit) deposited;
}

NatSpec is similarly missing for the following struct:

SpigotLib.sol: L7-17

---

NatSpec is completely missing for the following event:

CreditLib.sol: L16-21

    event AddCredit(
        address indexed lender,
        address indexed token,
        uint256 indexed deposit,
        bytes32 id
    );

NatSpec is similarly missing for the following events:

EscrowLib.sol

L221, L223, L225, L227

SpigotLib.sol

L241-244, L246, L248, L250-252, L255-260

L262-266, L270, L272, L274

SpigotedLineLib.sol

L30-34

MutualConsent.sol

L21-23

LineFactoryLib.sol

L8-14, L16-21, L23-28

---

---

Missing require error message

A require message should be included to enable users to understand the reason for failure.

Use Custom Errors to add an explanatory message to each of the require statements referenced below

LineOfCredit.sol: L112

        require(uint(status) >= uint( LineLib.STATUS.ACTIVE));

Similarly for the following require statements:

LineOfCredit.sol

L241, L259, L326

SpigotedLine.sol

L62, L143, L160, L239

EscrowedLine.sol

L64, L90

EscrowLib.sol

L91, L105, L161, L198, L216

SpigotLib.sol

L96, L128, L130, L155

L180, L189, L201

SpigotedLineLib.sol

L147

---

---