Debt DAO contest - berndartmueller's results

Lines of code

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

Vulnerability details

The Spigot.claimRevenue function allows (anyone) to claim revenue tokens from the spigot (push and pull payments) and escrows them for the owner to withdraw later.

The revenue is automatically split between the treasury and escrow according to the settings in SpigotState.settings[revenueContract].ownerSplit.

However, the SpigotLib.claimRevenue function does not check whether the revenueContract is a valid revenue contract address.

Impact

Anyone (e.g. the borrower or the SpigotState.treasury owner) can call Spigot.claimRevenue with an arbitrary revenueContract address, forcing a revenue stream split of 0% to the escrow and 100% to the treasury.

Proof of Concept

modules/spigot/Spigot.sol#L74

function claimRevenue(address revenueContract, address token, bytes calldata data)
    external nonReentrant
    returns (uint256 claimed)
{
    return state.claimRevenue(revenueContract, token, data);
}

utils/SpigotLib.sol#L90

If the Spigot contract receives its revenue via push payments and the SpigotLib.claimRevenue function is called with an arbitrary revenueContract address, 0% of the revenue stream will be escrowed and **100%**will be immediately transferred to the treasury address.

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;
}

Tools Used

Manual review

Recommended mitigation steps

Consider asserting in the SpigotLib.claimRevenue function that the revenueContract address is a valid revenue contract.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L31-L36 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L234 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L270

Vulnerability details

Creating new credits and increasing the credit deposit requires both parties, the lender and the borrower, to agree. This is implemented by having both call the same function with the same call data.

However, as it's possible to use native ETH as a credit token, this can lead to a situation where the lender is the first to call the function and sends native ETH with it. The function is not executed at this time, as the borrower has not yet called the function. The already transferred native ETH is now locked and unrecoverable.

Impact

If the lender is the first to call the LineOfCredit.addCredit or LineOfCredit.increaseCredit functions, native ETH transferred with the call will be locked as the function is only executed after the second and final signer has called the function.

Proof of Concept

utils/MutualConsent.sol#L31-L36

The MutualConsent.mutualConsent modifier only allows the function to be called in case both signers have called the function with the same call data.

modifier mutualConsent(address _signerOne, address _signerTwo) {
  if(_mutualConsent(_signerOne, _signerTwo))  {
    // Run whatever code needed 2/2 consent
    _;
  }
}

modules/credit/LineOfCredit.sol#L234

The LineOfCredit.addCredit function uses the mutualConsent modifier to add a new credit. Native ETH can be sent to the function as the deposit. However, if the lender is the first caller of the function and sends native ETH, the function will not be executed due to the missing second call of the borrower. ETH funds will be unrecoverable.

function addCredit(
    uint128 drate,
    uint128 frate,
    uint256 amount,
    address token,
    address lender
)
    external
    payable
    whileActive
    mutualConsent(lender, borrower)
    returns (bytes32)
{
    LineLib.receiveTokenOrETH(token, lender, amount);

    bytes32 id = _createCredit(lender, token, amount);

    require(interestRate.setRate(id, drate, frate));

    return id;
}

modules/credit/LineOfCredit.sol#L270

Similarly, the LineOfCredit.increaseCredit function uses the .LineOfCreditmutualConsentById modifier (which internally uses the MutualConsent.mutualConsent modifier). The same issue with native-sent ETH applies. If the lender is the first caller of the function and sends native ETH, the function will not be executed due to the missing second call of the borrower. ETH funds will be unrecoverable.

function increaseCredit(bytes32 id, uint256 amount)
  external
  payable
  override
  whileActive
  mutualConsentById(id)
  returns (bool)
{
    Credit memory credit = credits[id];
    credit = _accrue(credit, id);

    credit.deposit += amount;

    credits[id] = credit;

    LineLib.receiveTokenOrETH(credit.token, credit.lender, amount);

    emit IncreaseCredit(id, amount);

    return true;
}

Tools Used

Manual review

Recommended mitigation steps

Consider enforcing the function call order to ensure that the party providing the native ETH funds is the second and last caller of the function.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L388-L410 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L292-L311

Vulnerability details

Closing a single credit line can be reentered and allows decrementing count as many times as needed to reach the value 0. As soon as the value is 0, the status of all credit lines will be updated to LineLib.STATUS.REPAID.

Reentrancy is possible by the borrower in case the used credit token credit.token is an ERC-777 token with sender/receiver hooks and if the borrower is a smart contract (which is both possible).

Impact

The overall status of the line of credit facility can be updated to LineLib.STATUS.REPAID by reentering a single credit line close repeatedly. This allows the borrower to sweep all revenue or credit tokens with the SpigotedLine.sweep function.

Proof of Concept

modules/credit/LineOfCredit.sol#L500

The LineOfCredit._close function decrements the amount of positions count by 1. Additionally, it uses the unchecked statement to allow underflows.

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;
}

modules/credit/LineOfCredit.sol#L388-L410

The LineOfCredit.close(bytes32 id) function can possibly be reentered in case the used credit.token is an ERC-777 token with sender/receiver hooks. In the specific case that accrued interest is repaid, the borrower can reenter the LineOfCredit.close(bytes32 id) function. The LineOfCredit._close function decrements count by 1. If count is 0, it will underflow due to the unchecked statement.

The function can be reentered infinitely. On each reentry, count is decremented. If count reaches 0, the status is updated to LineLib.STATUS.REPAID.

function close(bytes32 id) external payable override returns (bool) {
    Credit memory credit = credits[id];
    address b = borrower; // gas savings
    if(msg.sender != credit.lender && msg.sender != b) {
      revert CallerAccessDenied();
    }

    // ensure all money owed is accounted for. Accrue facility fee since prinicpal was paid off
    credit = _accrue(credit, id);
    uint256 facilityFee = credit.interestAccrued;

    if(facilityFee > 0) {
      // only allow repaying interest since they are skipping repayment queue.
      // If principal still owed, _close() MUST fail
      LineLib.receiveTokenOrETH(credit.token, b, facilityFee);

      credit = _repay(credit, id, facilityFee);
    }

    _close(credit, id); // deleted; no need to save to storage

    return true;
}

modules/credit/LineOfCredit.sol#L292-L311

The same applies to the LineOfCredit.depositAndClose function. The borrower msg.sender can reenter the function and ultimately decrement count to 0.

function depositAndClose()
    external
    payable
    override
    whileBorrowing
    onlyBorrower
    returns (bool)
{
    bytes32 id = ids[0];
    Credit memory credit = _accrue(credits[id], id);

    // Borrower deposits the outstanding balance not already repaid
    uint256 totalOwed = credit.principal + credit.interestAccrued;
    LineLib.receiveTokenOrETH(credit.token, msg.sender, totalOwed);

    // Borrower clears the debt then closes and deletes the credit line
    _close(_repay(credit, id, totalOwed), id);

    return true;
}

Tools Used

Manual review

Recommended mitigation steps

Consider using a reentrancy guard (same for LineOfCredit.depositAndClose)

Lines of code

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

Vulnerability details

A borrower can repay (parts) of a credit line with the SpigotedLine.useAndRepay function. This function will use amount of unusedTokens[credit.token] as a repayment. However, if amount exceeds the principal and the accrued interest, credit.principal will underflow without an error and set the principal value to a very large number.

This a problem because a borrower can unknowingly provide a larger than necessary amount to the SpigotedLine.useAndRepay function to make sure enough funds are used to fully repay the principal and the remaining interest.

Additionally, a lender can do the same thing as the lender can call this function.

Impact

The credit.principal underflows without an error and will be set to a very large number. This will force a secured line immediately into liquidation. Additionally, having a principal value close to 2^256 - 1 will make it hugely expensive to repay the credit line.

Proof of Concept

utils/CreditLib.sol#L186

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; // @audit-info potential underflow without an error due to the unchecked block
        credit.interestRepaid += interest;
        credit.interestAccrued = 0;

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

        return credit;
    }
} }

To demonstrate the issue, copy the following test case and paste it into the SpigotedLine.t.sol test file. Then run forge test --match-test "test_lender_use_and_repay_underflow".

Following scenario causes the repayment to underflow:

1. Borrower borrows 1 ether of revenueToken
2. 2 ether worth of revenueToken is claimed and traded from the revenue contract
3. Use all of the previously claimed funds (2 ether) to repay the line of credit (= 1 ether)
4. credit.principal underflows due to principalPayment is larger than credit.principal

function test_lender_use_and_repay_underflow() public {
    uint256 largeRevenueAmount = lentAmount * 2;

    deal(address(lender), lentAmount + 1 ether);
    deal(address(revenueToken), MAX_REVENUE);
    address revenueC = address(0xbeef); // need new spigot for testing
    bytes32 id = _createCredit(address(revenueToken), Denominations.ETH, revenueC);

    // 1. Borrow lentAmount = 1 ether
    _borrow(id, lentAmount);

    // 2. Claim and trade largeRevenueAmount = 2 ether (revenue)
    bytes memory tradeData = abi.encodeWithSignature(
      'trade(address,address,uint256,uint256)',
      address(revenueToken),
      Denominations.ETH,
      1 gwei,
      largeRevenueAmount
    );

    hoax(borrower);
    line.claimAndTrade(address(revenueToken), tradeData);

    (, uint256 principalBeforeRepaying,,,,,) = line.credits(line.ids(0));
    assertEq(principalBeforeRepaying, lentAmount);

    // 3. Use and repay debt with previously claimed and traded revenue (largeRevenueAmount = 2 ether)
    vm.prank(lender);
    line.useAndRepay(largeRevenueAmount);
    (, uint256 _principal,,,,,) = line.credits(line.ids(0));

    uint256 underflowedPrincipal = principalBeforeRepaying;

    unchecked {
      underflowedPrincipal -= (largeRevenueAmount);
    }

    // 4. Principal underflowed
    assertEq(_principal, underflowedPrincipal);
  }

Tools Used

Manual review

Recommended mitigation steps

Consider asserting amount is less or equal than credit.principal + credit.interestAccrued (require(amount <= credit.principal + credit.interestAccrued);). Similar as how it is done in LineOfCredit.depositAndRepay()

Lines of code

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

Vulnerability details

The LineLib.receiveTokenOrETH helper function is used across the codebase to take care of receiving native ETH or ERC-20 tokens. Additionally, this function asserts that the received native ETH amount is equal to or greater than the expected amount. Otherwise, it reverts.

However, if msg.value is greater than amount, the amount value stays the same, even though more ETH was received.

Impact

Overpayment of native ETH is not accounted for and leads to a loss of funds for the ETH sender.

Proof of Concept

utils/LineLib.sol#L71

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;
}

Tools Used

Manual review

Recommended mitigation steps

Consider updating amount to msg.value if token == Denominations.ETH.

Lines of code

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

Vulnerability details

The Spigot.operate function allows the operator to call whitelisted functions on revenue contracts to maintain their product while still allowing the spigot owner to receive its revenue stream.

Only specific function signatures are allowed to be called on the revenue contract. However, the revenueContract address is not checked for existence in the SpigotLib.settings mapping.

Impact

Depending on the whitelisted functions in the SpigotLib.whitelistedFunctions mapping, it can be used maliciously.

For instance, if a legitimate revenueContract has a function transfer(address,uint256) whitelisted (for whatever legitimate reason), the operator can reuse this whitelisted function signature to call transfer on any arbitrary ERC-20 token address (= revenueContract). This allows the operator to steal any ERC-20 token from the deployed spigot contract.

Proof of Concept

utils/SpigotLib.sol#L76

function operate(SpigotState storage self, address revenueContract, bytes calldata data) external returns (bool) {
    if(msg.sender != self.operator) { revert CallerAccessDenied(); }

    // extract function signature from tx data and check whitelist
    bytes4 func = bytes4(data);

    if(!self.whitelistedFunctions[func]) { revert BadFunction(); }

    // cant claim revenue via operate() because that fucks up accounting logic. Owner shouldn't whitelist it anyway but just in case
    // also can't transfer ownership so Owner retains control of revenue contract
    if(
      func == self.settings[revenueContract].claimFunction ||
      func == self.settings[revenueContract].transferOwnerFunction
    ) { revert BadFunction(); }

    (bool success,) = revenueContract.call(data);
    if(!success) { revert BadFunction(); }

    return true;
}

Tools Used

Manual review

Recommended mitigation steps

Consider adding a check to ensure that the revenueContract is a valid revenue contract address.

Lines of code

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

Vulnerability details

A credit line can be closed by using the LineOfCredit.depositAndClose() or LineOfCredit.close. The remaining funds deposited by the lender (credit.deposit) and the accumulated and paid interest are transferred to the lender.

However, if the used credit token credit.token is native ETH (or an ERC-777 token with receiver hooks, and under the assumption that the oracle supports this asset in the first place), the lender can reject the closing of the credit by reverting the token transfer.

Impact

The lender can prevent the borrower from closing the credit line. This leads to the following consequences:

• Migrating (rollover) to a new line is not possible (it requires all credits to be closed, see SecuredLine.sol#L55)
• Releasing a spigot and transferring ownership to the borrower is not possible (see SpigotedLineLib.sol#L195)
• Sweeping remaining tokens (e.g. revenue tokens) in the Spigot to the borrower is not possible (see (SpigotedLineLib.sol#L220)[https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/SpigotedLineLib.sol#L220])

Proof of Concept

modules/credit/LineOfCredit.sol#L489-L493

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;
}

Tools Used

Manual review

Recommended mitigation steps

Consider using a pull-based pattern to allow the lender to withdraw the funds instead of sending them back directly.