Debt DAO contest - lukris02's results

QA Report for Debt DAO contest

Overview

During the audit, 9 non-critical issues were found.

Non-Critical Risk Findings(9)

NC-1. Order of Functions

Description

According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:

1. constructor
2. receive function (if exists)
3. fallback function (if exists)
4. external
5. public
6. internal
7. private

Instances

receive() function at the end but should be after constructor:

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L272

external functions between internal:

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SecuredLine.sol#L48-L104

public functions before external:

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/EscrowLib.sol#L34-L84
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L29-L58

Recommendation

Reorder functions where possible.

NC-2. Order of Layout

Description

According to Order of Layout, inside each contract, library or interface, use the following order:

1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

Instances

Modifiers after constructor:

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L78
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/interest-rate/InterestRateCredit.sol#L25

Recommendation

Place modifiers before constructor.

NC-3. No space between the control structures

Description

According to Style Guide, there should be a single space between the control structures if, while, and for and the parenthetic block representing the conditional.

Instances

• for(uint256 i; i < len; ++i) {
• for(uint i = 1; i < len; ++i) {
• and 85 instances of if(

Recommendation

Change:

if(...) {
    ...
}

to:

if (...) {
    ...
}

NC-4. Open TODOs

Instances

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/factories/LineFactory.sol#L140
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/factories/LineFactory.sol#L145

Recommendation

Resolve issues.

NC-5. Open question

Instances

• [Bob - consider renaming to newIds
• * Bob - consider renaming this function removeId()
• self.escrowed[token] = 0; // keep 1 in escrow for recurring call gas optimizations?

NC-6. Typos

Instances

• // Line Financials aggregated accross all existing Credit => across
• * @param arbiter_ - A neutral party with some special priviliges on behalf of Borrower and Lender. => privileges
• * @dev - updates `status` variable in storage if current status is diferent from existing status => different
• @notice - accrues token demoninated interest on a lender's position. => denominated
• // ensure all money owed is accounted for. Accrue facility fee since prinicpal was paid off => principal
• * @notice - updates `status` variable in storage if current status is diferent from existing status. => different
• * @notice - Insert `p` into the next availble FIFO position in the repayment queue => available
• * @param arbiter_ - neutral party with some special priviliges on behalf of borrower and lender => privileges
• * @notice requires Spigot contract itselgf to be transfered to Arbiter and sold off to a 3rd party before declaring insolvent => itself
• * @notice requires Spigot contract itselgf to be transfered to Arbiter and sold off to a 3rd party before declaring insolvent => transferred
• * @dev - priviliged internal function => privileged
• * @return isInsolvent - if the entire Line including all collateral sources is fuly insolvent. => fully
• * @notice helper function to allow borrower to easily swithc collateral to a new Line after repyment => switch
• * @notice helper function to allow borrower to easily swithc collateral to a new Line after repyment => repayment
• * @dev - revenuContract's transfer func MUST only accept one paramteter which is the new owner. => parameter
• * @dev - Used if we setup Escrow before Line exists. Line has no way to interface with this function so once transfered `line` is set forever => transferred
• * @notice - allows the lines arbiter to enable thdeposits of an asset => the deposits
• * - gives better risk segmentation forlenders => for lenders
• * @notice Interest rate / acrrued interest calculation contract for Line of Credit contracts => accrued
• * @param credit - The lender position that is being bwithdrawn from => withdrawn
• // emit events before seeting to 0 => setting
• * @param interest - interset rate contract used by line that will calculate interest owed => interest
• // get token demoninated interest accrued => denominated
• * @dev priviliged internal function => privileged
• * @param spigot - The Spigot to claim from. Must be owned by adddress(this) => address
• * @notice - Transfers ownership of the entire Spigot and its revenuw streams from its then Owner to either => revenue

NC-7. Maximum line length exceeded

Description

According to Style Guide, maximum suggested line length is 120 characters.

Instances

More than 50 instances.

Recommendation

Make the lines shorter.

NC-8. Missing NatSpec

Description

NatSpec is missing for more than 50 functions.

Recommendation

Add NatSpec for all functions.

NC-9. No error message in require

Instances

• require(uint(status) >= uint( LineLib.STATUS.ACTIVE));
• require(interestRate.setRate(id, drate, frate));
• require(interestRate.setRate(id, drate, frate));
• require(amount <= credit.principal + credit.interestAccrued);
• require(defaultRevenueSplit_ <= SpigotedLineLib.MAX_SPLIT);
• require(amount <= unusedTokens[credit.token]);
• require(msg.sender == borrower);
• require(msg.sender == arbiter);
• require(escrow_.liquidate(amount, targetToken, to));
• require(escrow.updateLine(newLine));
• require(amount > 0);
• require(msg.sender == ILineOfCredit(self.line).arbiter());
• require(amount > 0);
• require(amount > 0);
• require(msg.sender == self.line);
• require(LineLib.sendOutTokenOrETH(token, self.treasury, claimed - escrowedAmount));
• require(revenueContract != address(this));
• require(self.settings[revenueContract].transferOwnerFunction == bytes4(0));
• require(success);
• require(newOwner != address(0));
• require(newOperator != address(0));
• require(newTreasury != address(0));
• require(ISpigot(spigot).updateOwner(newLine));

Recommendation

Add error messages.

Gas Optimizations Report for Debt DAO contest

Overview

During the audit, 3 gas issues were found.
Total savings ~800.

Gas Optimizations Findings(3)

G-1. Use calldata instead of memory for read-only arguments

Description

Since Solidity v0.6.9, memory and calldata are allowed in all functions regardless of their visibility type (See "Calldata Variables" section here).
When function arguments should not be modified, it is cheaper to use calldata.

Instances

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L218
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L483
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/spigot/Spigot.sol#L125
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/CreditLib.sol#L74
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L125

Recommendation

Consider using calldata where possible.

Saved

This saves at least 60 gas per iteration.
So, ~60*5 = 300

G-2. Use unchecked blocks for incrementing i

Description

In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. In the loops, "i" will not overflow because the loop will run out of gas before that.

Instances

• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L179
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L203
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L520
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/CreditListLib.sol#L23
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/CreditListLib.sol#L51
• https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/EscrowLib.sol#L57

Recommendation

Change:

for (uint256 i; i < n; ++i) {
 // ...
}

to:

for (uint256 i; i < n;) { 
 // ...
 unchecked { ++i; }
}

Saved

This saves ~30-40 gas per iteration.
So, ~35*6 = 210

G-3. Use unchecked blocks for subtractions where underflow is impossible

Description

In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. When an overflow or underflow isn’t possible (after require or if-statement), some gas can be saved by using unchecked blocks.

Instances

• repaid - newTokens
• newTokens - repaid
• unusedTokens[credit.token] -= amount
• ids[i - 1] = ids[i]
• self.deposited[token].amount -= amount
• self.deposited[token].amount -= amount
• claimed - escrowedAmount
• diff = oldClaimTokens - newClaimTokens;
• unused - diff

Saved

This saves ~35.
So, ~35*9 = 315