Debt DAO contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

Any excess ETH sent is unaccounted for. It will be locked up in the contract.

Proof of Concept

receiveTokenOrETH() only reverts is msg.value is less than amount. It allows msg.value to be greater than amount.

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

Usage of receiveTokenOrETH() where the excess amount is unaccounted for:

• https://github.com/debtdao/Line-of-Credit/blob/master/contracts/utils/EscrowLib.sol#L94
• https://github.com/debtdao/Line-of-Credit/blob/master/contracts/modules/credit/LineOfCredit.sol#L237
• https://github.com/debtdao/Line-of-Credit/blob/master/contracts/modules/credit/LineOfCredit.sol#L280
• https://github.com/debtdao/Line-of-Credit/blob/master/contracts/modules/credit/LineOfCredit.sol#L305
• https://github.com/debtdao/Line-of-Credit/blob/master/contracts/modules/credit/LineOfCredit.sol#L330
• https://github.com/debtdao/Line-of-Credit/blob/master/contracts/modules/credit/LineOfCredit.sol#L401

Tools Used

none

Recommended Mitigation Steps

There's no scenario where the user is expected to send more than necessary. Thus, it shouldn't be allowed. Change receiveTokenOrETH() to:

if(msg.value != amount) { revert TransferFailed(); }

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/master/contracts/utils/SpigotLib.sol#L61-L80

Vulnerability details

Impact

The operator role in the Spigot contract can call whitelisted functions in operate(). But, functions are whitelisted across contracts. If two contracts share the same function signature, the operator will be able to call it. This can cause unforeseen consequences with a loss of funds being likely.

For example, let's say your revenue contract has a function transfer(address to, uint amount). The operator explains to the owner why that function is necessary for their contract's usage. The owner audits the function and agrees it's necessary while there's no risk to their loan. They whitelist the function. Because function signatures are whitelisted across contracts, the same transfer(address, uint) function can be called on any ERC20 contract. Thus, the operator is able to drain funds stored in the Spigot contract.

The vulnerability depends on external requirements (owner whitelisting the function). But, I think this is quite likely to happen. I don't think anybody would expect the whitelist to work across contracts. You have to read the Spigot contract itself to figure that out.

Proof of Concept

Operator can call operate() to execute a call to a user-provided contract with user-provided calldata:

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

The function only verifies that the called function's signature is whitelisted. It doesn't verify which contract is called with it.

Tools Used

none

Recommended Mitigation Steps

Operator should only be allowed to call a specific contract for which that function was whitelisted.

Lines of code

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

Vulnerability details

Impact

When fee-on-transfer tokens are used, the LineLib.receiveTokenOrETH() won't account for that. The internal bookkeeping will show a higher balance than the contract actually owns.

In the LineOfCredit contract that might cause a credit line not to be closable.

Proof of Concept

receiveTokenOrETH() doesn't verify the number of tokens it got:

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

Let's take the most basic example. There's a LineOfCredit contract with a single credit line. There's no liquidity left and the borrower wants to repay and close the credit.

They call LineOfCredit.depositAndClose():

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

The function calculates the total owed amount (principal + interest) and pulls those tokens from the user:

        uint256 totalOwed = credit.principal + credit.interestAccrued;
        LineLib.receiveTokenOrETH(credit.token, msg.sender, totalOwed);

If the credit token takes fees on transfer, totalOwed will be larger than the actual amount of tokens the contract received.

Then, it uses thetotalOwed amount to call _repay() and _close():

        _close(_repay(credit, id, totalOwed), id);

In repay(), the credit data is updated.

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

We reach the following state:

principal = 0
deposit = x
interest = y
balance = b, where b < x + y

Now, _close() is called:

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

The contract tries to send out the credit tokens to the lender. That's exactly deposit + interestRepaid. But, because of the fees when the borrower sent the tokens to the contract, it doesn't have enough tokens. The transfer fails and the credit can't be closed.

Tools Used

none

Recommended Mitigation Steps

LineLib.receiveTokenOrETH() should handle fee-on-transfer tokens properly by checking the balance after receiving the tokens.