Debt DAO contest - ReyAdmirado's results

QA

1. typo in comments

priviliges --> privileges

• LineOfCredit.sol#L45

• SpigotedLine.sol#L46

• SpigotedLine.sol#L85

• SpigotedLine.sol#L180

• EscrowedLine.sol#L45

• EscrowedLine.sol#L74

• EscrowedLine.sol#L85

• SpigotedLineLib.sol#L44

accross --> across

• LineOfCredit.sol#L37

diferent --> different

• LineOfCredit.sol#L107

demoninated --> denominated

• LineOfCredit.sol#L213

• CreditLib.sol#L250

prinicpal --> principal

• LineOfCredit.sol#L395

diferent --> different

• LineOfCredit.sol#L416

availble --> available

• LineOfCredit.sol#L510

interfer --> interfere

• SpigotedLine.sol#L38

itselgf --> itself

• SpigotedLine.sol#L84

fuly --> fully

• SecuredLine.sol#L110

SecuredlLine.liquidate --> SecuredLine.liquidate

• EscrowedLine.sol#L43

SecuredlLine.rollover --> SecuredLine.rollover

• EscrowedLine.sol#L83

swithc --> switch

• EscrowedLine.sol#L84

repyment --> repayment

• EscrowedLine.sol#L84

revenuContract --> revenueContract

• Spigot.sol#L130

paramteter --> parameter

• Spigot.sol#L130

thdeposits --> the deposits

• Escrow.sol#L94

forlenders --> for lenders

• Escrow.sol#L95

acrrued --> accrued

• InterestRateCredit.sol#L17

adddress --> address

• SpigotedLineLib.sol#L48

interset --> interest

• CreditLib.sol#L123
• CreditLib.sol#L237

bwithdrawn --> withdrawn

• CreditLib.sol#L200

seeting --> setting

• CreditLib.sol#L221

2. use of floating pragma

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

• LineOfCredit.sol
• SpigotedLine.sol
• SecuredLine.sol
• InterestRateCredit.sol

3. event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it’s not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

• SpigotLib.sol#L255
• SpigotLib.sol#L262

4. require()/revert() statements should have descriptive reason strings or custom error

• LineOfCredit.sol#L112

• LineOfCredit.sol#L241

• LineOfCredit.sol#L259

• LineOfCredit.sol#L326

• SpigotedLine.sol#L62

• SpigotedLine.sol#L143

• SpigotedLine.sol#L160

• SpigotedLine.sol#L239

• EscrowedLine.sol#L64

• EscrowedLine.sol#L90

• SpigotLib.sol#L96

• SpigotLib.sol#L128

• SpigotLib.sol#L130

• SpigotLib.sol#L155

• SpigotLib.sol#L180

• SpigotLib.sol#L189

• SpigotLib.sol#L201

• SpigotedLineLib.sol#L147

• EscrowLib.sol#L91

• EscrowLib.sol#L105

• EscrowLib.sol#L161

• EscrowLib.sol#L198

• EscrowLib.sol#L216

5. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

• SpigotLib.sol#L90

6. inconsistent spacing

The instances below point out the usages that don’t follow the majority, within each file

returns( should be returns (

• LineOfCredit.sol#L64

• LineOfCredit.sol#L69

• LineOfCredit.sol#L143

• LineOfCredit.sol#L157

• LineOfCredit.sol#L200

• LineOfCredit.sol#L218

• LineOfCredit.sol#L421

• SpigotedLine.sol#L73

• SpigotedLine.sol#L88

• SpigotedLine.sol#L137

• SecuredLine.sol#L37

• SecuredLine.sol#L52

• SecuredLine.sol#L86

• SecuredLine.sol#L97

• SecuredLine.sol#L116

• EscrowedLine.sol#L25

• EscrowedLine.sol#L34

• EscrowedLine.sol#L61

• EscrowedLine.sol#L77

• EscrowedLine.sol#L89

• Spigot.sol#L145

• Spigot.sol#L210

• Spigot.sol#L216

• Escrow.sol#L57

• Escrow.sol#L65

• Escrow.sol#L74

a lot of if statements have this problem

18 instance in LineOfCredit.sol

1 instance in

• SpigotedLine.sol#L74

7 instances here

• SecuredLine.sol

3 instances here

• EscrowedLine.sol

7. Outdated compiler version

Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs

8. Unused/empty receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds

• SpigotedLine.sol#L272

• Spigot.sol#L221

9. Missing event emitting

Event is never emitted, consider adding emit in the position intended

• LineFactoryLib.sol#L16
• LineFactoryLib.sol#L23

Gas

1. Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if statement

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a } if(a <= b); x = b - a => if(a <= b); unchecked { x = b - a } this will stop the check for overflow and underflow so it will save gas

• SpigotedLine.sol#L122

• SpigotedLine.sol#L125

• SpigotLib.sol#L96

• SpigotedLineLib.sol#L101

• SpigotedLineLib.sol#L109

• CreditLib.sol#L183

2. <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Using the addition operator instead of plus-equals saves gas

• SpigotedLine.sol#L125

• SpigotedLine.sol#L172

• SpigotedLine.sol#L122

• SpigotedLine.sol#L144

• EscrowLib.sol#L96

• EscrowLib.sol#L164

• EscrowLib.sol#L202

3. not using the named return variables when a function returns, wastes deployment gas

• LineOfCredit.sol#L441

• Spigot.sol#L86

• SpigotLib.sol#L31

• SpigotLib.sol#L85

• SpigotLib.sol#L107

• CreditLib.sol#L80

• CreditLib.sol#L133

4. can make the variable outside the loop to save gas

• EscrowLib.sol#L58
• EscrowLib.sol#L61

5. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for-loop and while-loops

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

• LineOfCredit.sol#L179

• LineOfCredit.sol#L203

• LineOfCredit.sol#L520

• CreditLib.sol#L23

• CreditLib.sol#L51

• EscrowLib.sol#L57

6. require()/revert() strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

• InterestRateCredit.sol#L26

7. state variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

self.owner

• SpigotLib.sol#L105-L122

8. use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

9. using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

• Spigot.sol#L123

• SpigotLib.sol#L125

10. internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

_accrueInterest

• InterestRateCredit.sol#L42

_getNonCaller

• MutualConsent.sol#L65

11. usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is 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. Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

• LineOfCredit.sol#L224

• LineOfCredit.sol#L225

• LineOfCredit.sol#L249

• LineOfCredit.sol#L250

• SpigotedLine.sol#L32

• Spigot.sol#L143

• Escrow.sol#L24

• InterestRateCredit.sol#L76

• InterestRateCredit.sol#L77

12. public library function should be made private/internal

Changing from public will remove the compiler-introduced checks for msg.value and decrease the contract’s method ID table size

_claimRevenue

• SpigotLib.sol#L29

trade

• SpigotedLineLib.sol#L120

calculateValue

• CreditLib.sol#L109

accrue

• CreditLib.sol#L239

_getLatestCollateralRatio

• EscrowLib.sol#L34

_getCollateralValue

• EscrowLib.sol#L51