Debt DAO contest - TomJ's results

Table of Contents

Low Risk Issues

• Missing Zero Address Check
• Admin Address Change should be a 2-Step Process
• Use fixed compiler versions instead of floating version
• Unused receive and fallback function

Non-critical Issues

• Require Statements without Descriptive Revert Strings
• Define Magic Numbers to Constant
• Naming Convention for Constants
• Event is Missing Indexed Fields

 

Low Risk Issues

Missing Zero Address Check

Issue

I recommend adding check of 0-address for input validation of critical address parameters. Not doing so might lead to non-functional contract and have to redeploy the contract, when it is updated to 0-address accidentally.

PoC

Total of 13 instances found.

1. SpigotedLine.sol:constructor(): spigot address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L64

2. SpigotedLine.sol:constructor(): swapTarget address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L66

3. LineOfCredit.sol:constructor(): borrower address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L57

4. LineOfCredit.sol:constructor(): arbiter address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L56

5. LineOfCredit.sol:constructor(): oracle address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L55

6. EscrowedLine.sol:constructor(): escrow address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/EscrowedLine.sol#L17

7. Spigot.sol:constructor(): state.owner address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/spigot/Spigot.sol#L36

8. Spigot.sol:constructor(): state.operator address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/spigot/Spigot.sol#L37

9. Spigot.sol:constructor(): state.treasury address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/spigot/Spigot.sol#L38

10. Escrow.sol:constructor(): oracle address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/escrow/Escrow.sol#L49

11. Escrow.sol:constructor(): borrower address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/escrow/Escrow.sol#L50

12. Escrow.sol:constructor(): state.line address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/escrow/Escrow.sol#L51

13. LineFactory.sol:constructor(): factory address https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/factories/LineFactory.sol#L26

Mitigation

Add 0-address check for above addresses.

 

Admin Address Change should be a 2-Step Process

Issue

High privilege account such as admin / owner is changed with only single process. This can be a concern since an admin / owner role has a high privilege in the contract and mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

1. owner privilege of Spigot.sol

    function updateOwner(SpigotState storage self, address newOwner) external returns (bool) {
        if(msg.sender != self.owner) { revert CallerAccessDenied(); }
        require(newOwner != address(0));
        self.owner = newOwner;
        emit UpdateOwner(newOwner);
        return true;
    }

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/spigot/Spigot.sol#L159-L161 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/SpigotLib.sol#L178-L184

Mitigation

This can be fixed by implementing 2-step process. We can do this by following. First make the function approve a new address as a pending admin/owner. Next that pending admin/owner has to claim the ownership in a separate transaction to be a new admin/owner.

 

Use fixed compiler versions instead of floating version

Issue

It is best practice to lock your pragma instead of using floating pragma. The use of floating pragma has a risk of accidentally get deployed using latest complier which may have higher risk of undiscovered bugs. Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

Total of 5 instances found.

./Oracle.sol:2:pragma solidity ^0.8.9;
./SpigotedLine.sol:1:pragma solidity ^0.8.9;
./SecuredLine.sol:1:pragma solidity ^0.8.9;
./LineOfCredit.sol:1:pragma solidity ^0.8.9;
./InterestRateCredit.sol:1:pragma solidity ^0.8.9;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Unused receive and fallback function

Issue

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert to prevent user's ETH getting locked up in the contract.

PoC

Total of 2 instances found.

./Spigot.sol:227:    receive() external payable {
./SpigotedLine.sol:272:    receive() external payable {}

 

Non-critical Issues

Require Statements without Descriptive Revert Strings

Issue

It is best practice to include descriptive revert strings for require statement for readability and auditing.

PoC

Total of 23 instances found.

./SpigotedLineLib.sol:147:      require(ISpigot(spigot).updateOwner(newLine));
./EscrowedLine.sol:64:    require(escrow_.liquidate(amount, targetToken, to));
./EscrowedLine.sol:90:    require(escrow.updateLine(newLine));
./SpigotedLine.sol:62:        require(defaultRevenueSplit_ <= SpigotedLineLib.MAX_SPLIT);
./SpigotedLine.sol:143:      require(amount <= unusedTokens[credit.token]);
./SpigotedLine.sol:160:        require(msg.sender == borrower);
./SpigotedLine.sol:239:        require(msg.sender == arbiter);
./EscrowLib.sol:91:        require(amount > 0);
./EscrowLib.sol:105:        require(msg.sender == ILineOfCredit(self.line).arbiter());
./EscrowLib.sol:161:        require(amount > 0);
./EscrowLib.sol:198:        require(amount > 0);
./EscrowLib.sol:216:      require(msg.sender == self.line);
./SpigotLib.sol:96:            require(LineLib.sendOutTokenOrETH(token, self.treasury, claimed - escrowedAmount));
./SpigotLib.sol:128:        require(revenueContract != address(this));
./SpigotLib.sol:130:        require(self.settings[revenueContract].transferOwnerFunction == bytes4(0));
./SpigotLib.sol:155:        require(success);
./SpigotLib.sol:180:        require(newOwner != address(0));
./SpigotLib.sol:189:        require(newOperator != address(0));
./SpigotLib.sol:201:        require(newTreasury != address(0));
./LineOfCredit.sol:112:        require(uint(status) >= uint( LineLib.STATUS.ACTIVE));
./LineOfCredit.sol:241:        require(interestRate.setRate(id, drate, frate));
./LineOfCredit.sol:259:        require(interestRate.setRate(id, drate, frate));
./LineOfCredit.sol:326:        require(amount <= credit.principal + credit.interestAccrued);

Mitigation

Add descriptive revert strings to easier understand what the code is trying to do.

 

Define Magic Numbers to Constant

Issue

It is best practice to define magic numbers to constant rather than just using as a magic number. This improves code readability and maintainability.

PoC

1. Magic Number: 100

./SpigotLib.sol:90:        uint256 escrowedAmount = claimed * self.settings[revenueContract].ownerSplit / 100;

Mitigation

Define magic numbers to constant.

 

Naming Convention for Constants

Issue

Constants should be named with all capital letters with underscores separating words.

Solidity documentation: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#constants

PoC

Below 8 constant variable should follow constants naming convention best practice.

./LineFactory.sol:12:    uint8 constant defaultRevenueSplit = 90; // 90% to debt repayment
./LineFactory.sol:14:    uint32 constant defaultMinCRatio = 3000; // 30.00% minimum collateral ratio

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

Total of 4 instances found.

./MutualConsent.sol:21:    event MutualConsentRegistered(
                               bytes32 _consentHash
                           );
./SpigotLib.sol:241:    event AddSpigot(
                            address indexed revenueContract,
                            uint256 ownerSplit
                        );
./SpigotLib.sol:255:    event ClaimRevenue(
                            address indexed token,
                            uint256 indexed amount,
                            uint256 escrowed,
                            address revenueContract
                        );
./SpigotLib.sol:262:    event ClaimEscrow(
                            address indexed token,
                            uint256 indexed amount,
                            address owner
                        );

Mitigation

Add up to 3 indexed fields when possible.

 

Table of Contents

• Duplicate require() Checks Should be a Modifier or a Function
• X = X + Y costs less gass than X += Y
• Internal Function Called Only Once can be Inlined
• Use Calldata instead of Memory for Read Only Function Parameters
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Empty Blocks Should Emit Something or be Removed
• Keep The Revert Strings of Error Messages within 32 Bytes
• Use Custom Errors to Save Gas

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

Total of 2 instances found.

./EscrowLib.sol:91:        require(amount > 0);
./EscrowLib.sol:161:        require(amount > 0);
./EscrowLib.sol:198:        require(amount > 0);

./LineOfCredit.sol:241:        require(interestRate.setRate(id, drate, frate));
./LineOfCredit.sol:259:        require(interestRate.setRate(id, drate, frate));

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

X = X + Y costs less gass than X += Y

Issue

X = X + Y costs less gass than X += Y for state variables

PoC

1. SpigotedLine.sol:claimAndRepay() https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L125

./SpigotedLine.sol:122:            unusedTokens[credit.token] -= repaid - newTokens;
./SpigotedLine.sol:125:            unusedTokens[credit.token] += newTokens - repaid;

Change to

unusedTokens[credit.token] = unusedTokens[credit.token] - repaid - newTokens;
unusedTokens[credit.token] = unusedTokens[credit.token] + newTokens - repaid;

Before avg: 90215 After avg: 86139 Save 4076 gas

2. SpigotedLine.sol:claimAndTrade() https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L172

./SpigotedLine.sol:172:        unusedTokens[targetToken] += newTokens;

Change to

unusedTokens[targetToken] = unusedTokens[targetToken] + newTokens;

Before avg: 110350 After avg: 109902 Save 448 gas

3. LineOfCredit.sol:increaseCredit() https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L276

./LineOfCredit.sol:276:        credit.deposit += amount;

Change to

credit.deposit = credit.deposit + amount;

Before avg: 17555 After avg: 17553 Save 2 gas

 

Internal Function Called Only Once Can be Inlined

Issue

Certain function is defined even though it is called only once. Inline it instead to where it is called to avoid usage of extra gas.

PoC

Total of 4 instances found.

1. _rollover() of EscrowedLine.sol This function called only once at line 59 of SecuredLine.sol. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/EscrowedLine.sol#L89 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SecuredLine.sol#L59

2. _getNonCaller() of MutualConsent.sol This function called only once at line 41 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L65 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L41

3. _sortIntoQ() of LineOfCredit.sol This function called only once at line 364 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L516 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L364

4. _accrueInterest() of InterestRateCredit.sol This function called only once at line 39 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/interest-rate/InterestRateCredit.sol#L42 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/interest-rate/InterestRateCredit.sol#L39

Mitigation

I recommend to not define above functions and instead inline it at place it is called.

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

Total of 5 instances found.

./Spigot.sol:125:    function addSpigot(address revenueContract, Setting memory setting) external returns (bool) {
./SpigotLib.sol:125:    function addSpigot(SpigotState storage self, address revenueContract, ISpigot.Setting memory setting) external returns (bool) {
./CreditLib.sol:74:    ILineOfCredit.Credit memory credit,
./CreditLib.sol:169:    ILineOfCredit.Credit memory credit,
./CreditLib.sol:203:    ILineOfCredit.Credit memory credit,

Mitigation

Change memory to calldata

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations in order to reduce the elements from 32 bytes to specified size. Therefore it is more gas saving to use 32 bytes unless it is used in a struct or state variable in order to reduce storage slot.

Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 29 instances found.

./LineFactoryLib.sol:51:        uint8 split = SecuredLine(oldLine).defaultRevenueSplit();
./LineFactoryLib.sol:85:        uint8 revenueSplit
./SpigotedLineLib.sol:13:    uint8 constant MAX_SPLIT = 100;
./SpigotedLineLib.sol:169:    function updateSplit(address spigot, address revenueContract, LineLib.STATUS status, uint8 defaultSplit) external returns (bool) {
./SpigotedLineLib.sol:170:        (uint8 split,, bytes4 transferFunc) = ISpigot(spigot).getSetting(revenueContract);
./Spigot.sol:146:    function updateOwnerSplit(address revenueContract, uint8 ownerSplit)
./Spigot.sol:222:        returns(uint8, bytes4, bytes4)
./Escrow.sol:24:    uint32 public immutable minimumCollateralRatio;
./Escrow.sol:43:        uint32 _minimumCollateralRatio,
./SpigotedLine.sol:32:    uint8 public immutable defaultRevenueSplit;
./SpigotedLine.sol:60:        uint8 defaultRevenueSplit_
./LineFactory.sol:12:    uint8 constant defaultRevenueSplit = 90; // 90% to debt repayment
./LineFactory.sol:13:    uint8 constant MAX_SPLIT = 100; // max % to take
./LineFactory.sol:14:    uint32 constant defaultMinCRatio = 3000; // 30.00% minimum collateral ratio
./LineFactory.sol:43:        uint32 minCRatio,
./LineFactory.sol:71:        uint8 split = defaultRevenueSplit; // gas savings
./SecuredLine.sol:21:        uint8 defaultSplit_
./SpigotLib.sol:25:    uint8 constant MAX_SPLIT = 100;
./SpigotLib.sol:164:    function updateOwnerSplit(SpigotState storage self, address revenueContract, uint8 ownerSplit)
./SpigotLib.sol:229:        returns(uint8, bytes4, bytes4)
./SpigotLib.sol:252:        uint8 indexed split
./CreditLib.sol:112:    uint8 decimals
./CreditLib.sol:138:      uint8 decimals;
./LineOfCredit.sol:224:        uint128 drate,
./LineOfCredit.sol:225:        uint128 frate,
./LineOfCredit.sol:249:        uint128 drate,
./LineOfCredit.sol:250:        uint128 frate
./InterestRateCredit.sol:76:        uint128 dRate,
./InterestRateCredit.sol:77:        uint128 fRate

Mitigation

I suggest using uint256 instead of anything smaller and downcast where needed.

 

Empty Blocks Should Emit Something or be Removed

Issue

There are function with empty blocks. These should do something like emit an event or reverting. If not it should be removed from the contract.

PoC

Total of 1 instance found.

./SpigotedLine.sol:272:    receive() external payable {}

Mitigation

Add emit or revert in the function block.

 

Keep The Revert Strings of Error Messages within 32 Bytes

Issue

Since each storage slot is size of 32 bytes, error messages that is longer than this will need extra storage slot leading to more gas cost.

PoC

Total of 1 instance found.

InterestRateCredit.sol
26:        require(
27:            msg.sender == lineContract,
28:            "InterestRateCred: only line contract."
29:        );

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/interest-rate/InterestRateCredit.sol#L26-L29

Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

Total of 1 instance found.

InterestRateCredit.sol
26:        require(
27:            msg.sender == lineContract,
28:            "InterestRateCred: only line contract."
29:        );

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/interest-rate/InterestRateCredit.sol#L26-L29

Mitigation

I suggest implementing custom errors to save gas.