Halborn 2022-12-20 Audit - v0.1 CHANGELOG from Halborn + Code4rena fixes since 2022-08-03

• Scope
• Additions
• Modifications
• Spec
• Commit Freeze
• v0.1
• IDLE Spigot Migration
• Final Report
• High Severity Issues
• Medium Severity Issues
• Acknowledged Severity Issues
• Mitigations

Scope

Additions

1. SecuredLine declareInsolvent() -
• allows us to enter a final negative end state if borrower still has debt after liquidating all assets
• calls EscrowedLine and SpigotedLine _declareInsolvent()
• Files: https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/modules/credit/SecuredLine.sol#L95-L97
• Tests: https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/tests/SecuredLine.t.sol#L408-L498
2. Oracle checks
• ensures no reverts on malicious/toxic tokens DoSing Line or Escrow contract during collateral/debt value calculations (one bad token means we can’t liquidate a borrower)
• Files: https://github.com/debtdao/Line-of-Credit/blob/develop/contracts/modules/oracle/Oracle.sol
• Tests: https://github.com/debtdao/Line-of-Credit/blob/develop/contracts/tests/Oracle.t.sol
3. credit.isOpen
• unify UX around lender payments
• prevent griefing by lender by not sending tokens directly in close/depositAndClose DoSing borrower from repaying.
• close/depositAndClose add repaid amount to credit position and mark position closed. Lender is then require to call withdraw() to get tokens
• Files: https://github.com/debtdao/Line-of-Credit/pull/172/files
• Tests:
• https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/tests/LineOfCredit.t.sol#L541-L643
4. Revoking consent
• Added ability to revoke consent to prevent replay attacks
• Files: https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/utils/MutualConsent.sol#L48-L79
• Tests: https://github.com/debtdao/Line-of-Credit/blob/develop/contracts/tests/MutualConsent.t.sol
5. ReservesChanged event
• track all unused revenue + credit tokens in SpigotedLine so we have better accounting when creating claimAndTrade/Repay functions which mandate using all unusedTokens revenue tokens when being called.
• Files:https://github.com/debtdao/Line-of-Credit/pull/194/files
• Tests: https://github.com/debtdao/Line-of-Credit/pull/194/files
6. LineFactory.sol + ModuleFactory.sol
• super simple. just check params being passed down are in right order basically
• File: https://github.com/debtdao/Line-of-Credit/blob/develop/contracts/modules/factories/LineFactory.sol
• Tests: https://github.com/debtdao/Line-of-Credit/blob/develop/contracts/tests/LineFactory.t.sol
7. spigot-migrations repo + IdleFinanceMigration.sol
• looked over our spec and ours + their contracts independently before to double check our due dilligence that they were compatible with our Spigot and all the potential attack vectors
• This is the specific script contract that will be executed by their onchain governance in prod
• private repo. give @Untitled you github handles to add
• README: https://github.com/debtdao/spigot-migrations/blob/main/migrations/idle-finance/docs.md
• Files: https://github.com/debtdao/spigot-migrations/blob/main/migrations/idle-finance/Migration.sol
• Tests: https://github.com/debtdao/spigot-migrations/blob/main/migrations/idle-finance/Migration.t.sol
8. View functions for accessing data needed to for interacting with Lines and debt.
1. Files:
2. Tests: N/A

Modifications

1. Repayment Queue - stepQ logic
• doesnt cycle all elements in array, just finds first non-null element.
• Moved stepQ() to _close() instead of _repay()
• File:
• https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/modules/credit/LineOfCredit.sol#L471
• https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/utils/CreditListLib.sol#L43-L65
• Tests: https://github.com/debtdao/Line-of-Credit/blob/develop/contracts/tests/Queue.t.sol
2. SpigotedLine.unusedTokens math calculations after trades
• Files:
• https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/modules/credit/SpigotedLine.sol#L112-L119
• https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/utils/SpigotedLineLib.sol#L87-L101
• Tests:
• https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/tests/SpigotedLine.t.sol#L180-L434
3. Remove Spigot.treasury
1. Code4rena report - could DoS claimRevenue by changing treasury to an address that doesnt accept ETH
2. Files: https://github.com/debtdao/Line-of-Credit/pull/169/files
3. Tests:
4. Spigot now allows multiple tokens per revenue contract
1. removed token from spigot settings struct and pass it in as arg to claimRevenue()
2. Files: https://github.com/debtdao/Line-of-Credit/pull/144/files
3. Tests: https://github.com/debtdao/Line-of-Credit/pull/144/files#diff-aa8cd0983ad44fdaca2c50eefa42f1f2d22ab7fa088cd2f499ae08a1d0373aaa
4. mitigations to calling arbitrary contracts (anyone can call claimRevenue and pass in any token address)
1. nonReentrant modifier and external calls all happen before revenue is account (have to)
2. toxic tokens only matter if they can steal other tokens or themselves from the spigot somehow. We never approve any external address including revenue tokens or revenue contracts to spend any ERC20 incuding the revenue token itself. So toxic claimToken shouldnt be able to steal any tokens in getBalance call
3. Rev contract shouldnt have exploit for customized toxic revenue token bc that should be found in rev contract due diligence (rev contract would have to give special priviliges somewhere)
4. random toxic tokens wouldnt be considered as “real” revenue/collateral anyway.
5. whileNoUnclaimedRevenue modifier was removed (was added after 1st Halborn audit) because it only protected push payments and now any token can be claimed from any revenue contract so push payment tokens cant be stuck (yes we know this means different ownerSplits can be exploited on push payments but owner can only allow 1 push payment setting. In our case all rev contracts should always have the same split so this isn’t a big deal for our LoC but could be for other apps)
5. remove directly depositing native ETH into our system via LineOfCredit and Escrow as collateral and credit tokens (still support native ETH on Spigot + SpigotedLine as a revenue token)
• kept payable modifier on functions because we check that `msg.value = 0` in LineLib.receiveTokenOrETH
• hardcode WETH address into LineLIib + use swap WETH for Denominations.ETH when making oracle price requests
• Files: https://github.com/debtdao/Line-of-Credit/pull/189/files
• Tests: (adds tests for removing native ETH support + mainnet tests for oracle and 0x integrations) https://github.com/debtdao/Line-of-Credit/pull/189/files#diff-d9f4a7e55bfcfb2bb235762c51d99b425eac787e18a33869159f71994ec65e79
6. cleaned up _close() and close() functions. Store lender funds in contract on close instead of immediately sending out.
• File: https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/modules/credit/LineOfCredit.sol#L295-L297
• Tests: https://github.com/debtdao/Line-of-Credit/blob/315740773138070481daacff501b7ac727030338/contracts/tests/LineOfCredit.t.sol#L541-L643

Spec

Commit Freeze

v0.1

GitHub - debtdao/Line-of-Credit at c9d296a995bfa8e58aca9bdf6e765f1d2d1e0879

We have comprehensive docs on our site https://docs.debtdao.finance/developers/architecture We track remote remotes like Foundry and Chainlink via submodules so you will need to install those in addition to our repo itself If you have forge installed already you can run forge install Alternatively using just git When cloning you can run git clone --recurse-submodules Or if you already have repo installed you can run git pull --recurse-submodules We have deployed contracts to Gõrli testnet.

github.com

IDLE Spigot Migration

Final Report

High Severity Issues

Medium Severity Issues

Acknowledged Severity Issues

Mitigations