| Bug Desc | Primary Ticket # | Severity | Status | Reason For Status | Mitigation Plan | Assign |
|---|---|---|---|---|---|---|
No oracle response validation | 3 | Not Valid Bug | Oracles excluded from audit | |||
basic solidity stuff | 4 | Not Valid Bug | stupid shit | Untitled | ||
dividing before multiplying | 5 | Not Valid Bug | test file | Untitled | ||
mutualConsent side effects | 6 | Not Valid Bug | Kinda the point. Only a change state in mutualConsent not any other contract. | Untitled | ||
Missing events | 9 | Not Valid Bug | Do have events just not in that contract. everything indexed in our subgraph already | Untitled | ||
Code cleanup | 9 | Not Valid Bug | bad examples | Untitled | ||
code optimizations | 10 | Not Valid Bug | most suggestions make code harder to read, have extremely minor effects or are based on innacurate assumptions of code flows | Untitled | ||
optimizations | 11 | Not Valid Bug | bleh | Untitled | ||
QA | 12 | Not Valid Bug | bleh | Untitled | ||
Bypass MutualConsent for infinite lenders | 13 | Not Valid Bug | assumes borrower attacks the loc. Would mean griefing lender and preventing borrower from borrowing anymore (but if spigot is installed lender repaying themselves doesnt suffer from this attack) so lender can still be repaid (assuming spigot) but borrow wont be able to borrow anymore | Untitled | ||
use call() instead of transfer() | 14 | Pending Review - Confirmed | ||||
Bypass MutualConsent for infinite lenders | 15 | Not Valid Bug | blocks is wrong metric, we need time. Timestamp manipulation is bounded and does not pose significant risk regardless, especially over time.
If we did then the merge would have broken our contracts (interest would be charged at wrong rate) | Untitled | ||
Borrower can send 3rd party collateral to arbitrary address | 16 | Not Valid Bug | They deposit into borrowers account, it is now borrowers token. Functioning as expected | Untitled | ||
Stale oracle data | 17 | Not Valid Bug | valid but out of scope | Untitled | ||
Gas opti | 18 | Not Valid Bug | meh | Untitled | ||
Gas opti | 19 | Not Valid Bug | meh | Untitled | ||
QA | 20 | Not Valid Bug | meh | Untitled | ||
Stale oracle data | 21 | Not Valid Bug | valid but out of scope | Untitled | ||
QA | 22 | Mitigated Planned | valid but out of scope | Untitled | ||
Stale oracle data | 23 | Not Valid Bug | valid but out of scope | Untitled | ||
borrower can send ETH instead of lender + lender can send too much ETH | 24 | if(msg.value < amount) { revert TransferFailed(); }
into
if(msg.sender != sender) { revert WrongETHSender(); }
if(msg.value < amount) { revert TransferFailed(); }
+
if(msg.value > amount) { sender.call(msg.value - amount) // also ensures lender can accept ETH preventing reverts later} | ||||
25 | Duplicate-24 | |||||
addCredit does not support non-standard ERC20s | 26 | Pending Review - Not Valid | Not our responsibility to support non-standard ERC20s. We cant prevent lenders and borrowers from using them but we have already said that we dont support these types of tokens.
We are already using SafeERC20 which is industry standard. | I guess we could check balances before/after but that increases gas costs a lot. I’m pretty sure its known that fee-on-transfer and rebase tokens arent supported by most protocols (e.g. uni v2, set protocol ,literally everything unless they write custom contracts for that specific token e.g. aAMPL) | ||
QA | 31 | Mitigated Planned | valid but out of scope | Untitled | ||
33 | 33 | Pending Review - Confirmed | Not super serious imo, if they agreed to the terms, they agreed to the terms but i guess we should have some expiry. I think we should have a 1 hash cache for non-borrower called to mutualConsent. | So remove mutualConsent modifier from MutualConsent to LoC (leave internal func there)
add consentCache(address ⇒ bytes32) mapping in LoC
could also make it a double mapping with bytes4 per function but I think 1 at a time is good so they can use a stale increaseCredit with a new setRate (makes automation/multi-tx a lot harder tho)
in mutualConsent and mutualConsentById check if caller is borrower. if not, overwrite consentCache for caller with new hash they generated (or expect from borrower, idk which is best for user and dev UX)
| ||
lender loses ETH if calling addCredit but Borrower doesnt | 42 | Pending Review - Confirmed | remove raw ETH. support. only WETH.
need to hardcode WETH address in LineLib and have a branch for oracle if its WETH address then use Denominations.ETH ffor chainlink oracle price fetch | |||
63 | 63 | Not Valid Bug | Working as expected. defaultSplit becomes schelling point and since both parties agree to terms in the first place there’s no reason to change it (and default is the amount weset for borrower to derisk loan so lenders + borrower know that this should be the rate for all contracts)
Also reported + acknowledged during Halborn audit. | |||
Call to declareInsolvent() would revert when contract status reaches liquidation | 69 | Pending Review - Confirmed | Borrower can manipulate the credit lines to block functionality | The issue mentions the whileBorrowing() modifier, but the logic in _sortIntoQ appears to be at fault. See issues, #421, #329, #354 for details | ||
Calling Spigot.claimRevenue function with non-existent revenue contract address sends all pushed revenue to treasury | 70 | Pending Review - Confirmed | Borrow can divert all revenue to their treasury by passing in an empty/invalid revenue contract | Check for valid revenue contract that’s passed into claimRevenue() | ||
SpigotLib should store whitelisted functions per revenue contract | 71 | Pending Review - Confirmed | It is possible to call a malicious function via operate on a revenue contract, though the owner should check for this | Store whitelisted functions per revenue contract | ||
Insufficient checks result in stolen revenue | 75 | Pending Review - Not Valid | Obv using insecure tokens is insecure. Warden seemed to miss the exploit in their face that they could just transfer the tokens to any arbitrary address not the Spigot in their example. | Token should be linked to a revenue stream | ||
SecuredLine.rollover function allows to rollover to CreditOfLine. Ownersip of Spigot will be lost ferover | 77 | Pending Review - Accepted Risk | Custom line won’t show up on the market place because not deployed via factory | |||
Malicious Escrow can be passed to the line with SecuredLine.rollover function | 78 | Pending Review - Accepted Risk | Custom line won’t show up on marketplace because not deployed via factory | |||
Borrower debt can be increased to huge number due to overflow error | 82 | Pending Review - Confirmed | Not checking amount passed when repaying can cause over/underflow | should validate amount passed when repaying | ||
Denial of Service in LineOfCredit.depositAndRepay function | 83 | Pending Review - Confirmed | Potentially annoying UX for users | Don’t revert if amount is over, pay back difference | ||
Lenders can prevent closing the debt | 85 | Pending Review - Confirmed | @Untitled pls review - the borrower should do due diligence and check that hte lender address is not a contract with a malicious receive function, however the mitigation suggestion about not sending Eth when clothing debt feels valid | Either preventing native Eth transactiosns, or changing debt closing push payments to pull payments | ||
Lender can intentionally make bad trades and cause loss of funds for borrower | 88 | Pending Review - Confirmed | @Untitled pls review - not sure where you rank this on seriousness. A malicious lender could set an unreasonably high gasPrice via the Ox API when getting a quote | Mitigation suggestion is to prevent the ledner from calling claimAndRepay, but not sure if this is the intended UX | ||
Funds can be added and not accrue as collateral | 89 | Pending Review - Confirmed | It’s possible to send Eth and an ERC20 and not account for both as collatoral | check msg.value == 0 when receiving an ERC20 in receiveTokenOrEth() | ||
119 | 119 | |||||
use wrong token decimal in escrow | 120 | Pending Review - Not Valid | > On the other hand, d.assetDecimals
is the number of decimals of the shares.
WRONG! We store underlying asset decimal not the 4626 token decimal
Here we store the asset as the underlying token and the decimal as the underlying decimal if it is a 4626 token so the call to the oracle is in underlying (for 4626 we price based off underlying)
https://github.com/debtdao/Line-of-Credit/blob/6987988fe39901cad9a8e5ebb2c6aa719590873d/contracts/utils/EscrowLib.sol#L122-L138 | |||
accrueInterest floor to 0 | 121 | Pending Review - Not Valid | Have already considered this. Attack does not take into account gas costs. And you must accrue interest on all positions, not just you’re own.
Economically not a viable attack even with todays low gas and ETH prices.
The WBTC example given would lose the lender $560 USD but requires calling accrueInterest on N positions *every single block*. even if the only lender, gas costs on mainnet for 1 month of attacks ( ~100k gas per accrueInterest() * 216,000 (blocks per month) * 15 gwei * $1,200 (ETH price 2022-11-18)) = $38,880 by my calculations far outweighing the profits even in the worst possible case given (every block with 2 decimal token)
At bigger debt values, the rounding error is smaller also mitigating the bug. | |||
134 | 134 | Pending Review - Not Valid | If Line becomes liquidatable, ownerSplit auto switches to 100% no longer sending funds to treasury, completely negating the bug | |||
147 | 147 | Pending Review - Not Valid | Making things updatable is also an attack vector.
We use Chainlink feed registry which is upgradable and only controllable by their team. Our existing solution is better than proposed fix.
Also our lines have deadlines, typically 3-12 months. While oracles could be deprecated in that time frame, it would not affect line operations at all unless there was a minimumCollateralRatio > 0 and then it would only block borrowing. Borrower could still use any credit they already have and pay back at their leisure (although would be liquidatable, arbiter would not). | |||
Reentrancy on _close | 160 | Is technically valid but i dont like their reasons/example. Should still delete credit[id] before sending tokens.
Have had this up for a bit
| ||||
169 | 169 | Incorrect understanding of contracts. Lenders don’t own specific revenue contracts. The LoC owns Spigot (thus all revenue) and decides which lender gets access to it.
We also literally have a useAndRepay function that lets lender #2 use unused revenue. Or Lender #1 repays all their debt and then Lender #2 becomes Lender #1
Their just describing our repayment queue feature, no bug. | ||||
176 | 176 | Deuplicate-160 | ||||
240 | Not Valid Bug | Its a library not a contract. Does not become inherited into the SpigotedLine interface, would need to make a new function on the contract to specifically expose SpigotedLineLib.trade() | ||||
258 | 258 | Pending Review - Confirmed | Add if(credit.lender == address(0)) { revert CloseFailedWithPrincipal(); } to beginning of _close() | |||
413 | 413 | Not Valid Bug | Working as expected. If line is not REPAID after deadline, then status becomes LIQUIDATABLE. | |||
467 | 467 | Pending Review - Not Valid | ERC777 token example is not valid since borrower must approve all tokens used in the line.
Lender MUST hold ETH to call addCredit meaning that ETH transfers cant be rejected either.
Moreover the borrower is not negatively affected by this situation. All collateral and Spigot is recoverable via the Arbiter if Line is LIQUIDATABLE and can be returned to borrower if lender is found to be malicious. | |||
472 | 472 | Not Valid Bug | Working as expected.
Being able to close positions is not bug. If positions were closed ** without repayment ** thats a bug. If all lenders are made whole when Line is marked REPAID then everything is functioning as expected.
ERC777 reentrancy is impossible because caller to close() or depositAndClose() must be borrower so unless the token extension used for reentrancy is also the borrower which would be highly unusual this wouldnt work anyway. | |||
