Debt DAO contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

A borrower can close a credit without repaying the debt to the lender. The lender will be left with a bad debt and the borrower will keep the borrowed amount and the collateral.

Proof of Concept

The close function of LineOfCredit doesn't check whether a credit exists or not. As a result, the count variable is decreased in the internal _close function when the close function is called with an non-existent credit ID: LineOfCredit.sol#L388:

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

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:

// contracts/tests/LineOfCredit.t.sol
function testCloseWithoutRepaying_AUDIT() public {
    assertEq(supportedToken1.balanceOf(address(line)), 0, "Line balance should be 0");
    assertEq(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial mint balance");
      
    _addCredit(address(supportedToken1), 1 ether);

    bytes32 id = line.ids(0);
    assert(id != bytes32(0));

    assertEq(supportedToken1.balanceOf(lender), mintAmount - 1 ether, "Lender should have initial balance less lent amount");
    
    hoax(borrower);
    line.borrow(id, 1 ether);
    assertEq(supportedToken1.balanceOf(borrower), mintAmount + 1 ether, "Borrower should have initial balance + loan");
    
    // The credit hasn't been repaid.
    // hoax(borrower);
    // line.depositAndRepay(1 ether);
    
    hoax(borrower);
    // Closing with a non-existent credit ID.
    line.close(bytes32(uint256(31337)));

    // The debt hasn't been repaid but the status is REPAID.
    assertEq(uint(line.status()), uint(LineLib.STATUS.REPAID));

    // Lender's balance is still reduced by the borrow amount.
    assertEq(supportedToken1.balanceOf(lender), mintAmount - 1 ether);

    // Borrower's balance still includes the borrowed amount.
    assertEq(supportedToken1.balanceOf(borrower), mintAmount + 1 ether);
}

Tools Used

Manual review

Recommended Mitigation Steps

In the close function of LineOfCredit, consider ensuring that a credit with the user-supplied ID exists, before closing it.

Lines of code

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

Vulnerability details

Impact

Borrowers and lenders can be impacted by consent that wasn't executed. Consider this scenario:

1. A borrower and a lender have agreed on rates.
2. The borrower calls setRates and records their consent.
3. Before the lender has executed the function, the borrower and the lender have agreed on new, lower, rates.
4. setRates was executed with the lower rates after getting mutual consent. However, the previous consent from the borrower is still in the contract.
5. After some time (e.g. after the borrower has borrowed a substantially bigger amount), the lender executes setRates with the higher rates they agreed on initially. This won't require a consent from the borrower because their consent will already be stored in the contract.

Proof of Concept

Only the current consent is removed when a call is executed (MutualConsent.sol#L38-L60):

function _mutualConsent(address _signerOne, address _signerTwo) internal returns(bool) {
    if(msg.sender != _signerOne && msg.sender != _signerTwo) { revert Unauthorized(); }

    address nonCaller = _getNonCaller(_signerOne, _signerTwo);

    // The consent hash is defined by the hash of the transaction call data and sender of msg,
    // which uniquely identifies the function, arguments, and sender.
    bytes32 expectedHash = keccak256(abi.encodePacked(msg.data, nonCaller));

    if (!mutualConsents[expectedHash]) {
        bytes32 newHash = keccak256(abi.encodePacked(msg.data, msg.sender));

        mutualConsents[newHash] = true;

        emit MutualConsentRegistered(newHash);

        return false;
    }

    delete mutualConsents[expectedHash];

    return true;
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider allowing parties to remove any consent they gave at any moment before a call was executed. Or, consider removing all previous consents (per function signature and signer's address) when executing a call that requires mutual consent.

Lines of code

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

Vulnerability details

Impact

A borrower can create a revenue contract that implements a function that's not whitelisted by an arbiter but which selector matches one of the whitelisted functions. Such function can withdraw funds from the revenue contract or transfer ownership, an arbiter will notice that and won't allow executing the function to the operator. However, the name of the function will be crafted in such a way that its selector will match one of the allowed functions. Thus, the operator will be able to call the function.

Proof of Concept

The whitelistedFunctions variable maintains a list of function selectors (4 byte function identifiers) (SpigotLib.sol#L210):

function updateWhitelistedFunction(SpigotState storage self, bytes4 func, bool allowed) external returns (bool) {
    if(msg.sender != self.owner) { revert CallerAccessDenied(); }
    self.whitelistedFunctions[func] = allowed;
    emit UpdateWhitelistFunction(func, allowed);
    return true;
}

Operator is allowed to execute only functions with the selectors from whitelistedFunctions (SpigotLib.sol#L67):

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

Being only 4 byte sequences, functions selectors are subject to collisions. For example, these selectors of these functions collide with the selector of the ERC20 transfer function (0xa9059cbb):

• join_tg_invmru_haha_fd06787(address,bool);
• func_2093253501(bytes);
• transfer(bytes4[9],bytes5[6],int48[11]);
• many_msg_babbage(bytes1).

(You can check known function selectors in this database: https://www.4byte.directory/)

The computational cost of generating a colliding function selector with meaningful name and arguments is low because, again, selectors are only 4 byte sequences.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing whitelistedFunctions because it cannot guarantee that a non-whitelisted function is not called.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L96 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L164

Vulnerability details

Impact

The Escrow contract doesn't support rebase tokens (one of the most popular rebase token is stETH), which can have different impacts:

1. in the case of stETH, the accrued balance will always be locked in the escrow contract indefinitely since, when releasing collateral, one cannot release more than the amount that was deposited (EscrowLib.sol#L163-L164);
2. in case of a rebase token with increasing balance, borrowers won't benefit from the increasing balance (e.g. stETH interest won't get accrued in collateral value);
3. in case of rebase tokens with decreasing balance, calculated collateral value can be higher than the actual one (in the collateral value calculation, the initially deposited amount is used, not the current balance: EscrowLib.sol#L61)

Proof of Concept

When using a rebase tokens as a collateral token, the current balance will be different from the one that was deposited. However, the deposited amount is recorded and used in all calculations that involve current collateral balance.

When adding collateral, the added amount is recorded as the actual amount of tokens added (EscrowLib.sol#L87-L101):

function addCollateral(EscrowState storage self, address oracle, uint256 amount, address token)
    external
    returns (uint256)
{
    require(amount > 0);
    if(!self.enabled[token])  { revert InvalidCollateral(); }

    LineLib.receiveTokenOrETH(token, msg.sender, amount);

    self.deposited[token].amount += amount;

    emit AddCollateral(token, amount);

    return _getLatestCollateralRatio(self, oracle);
}

When calculating collateral value, the recorded amounts are used instead of actual balances (EscrowLib.sol#L61):

function _getCollateralValue(EscrowState storage self, address oracle) public returns (uint256) {
    uint256 collateralValue;
    // gas savings
    uint256 length = self.collateralTokens.length;
    IOracle o = IOracle(oracle); 
    IEscrow.Deposit memory d;
    for (uint256 i; i < length; ++i) {
        address token = self.collateralTokens[i];
        d = self.deposited[token];
          // new var so we don't override original deposit amount for 4626 tokens
        uint256 deposit = d.amount;
        if (deposit != 0) {
            if (d.isERC4626) {
                // this conversion could shift, hence it is best to get it each time
                (bool success, bytes memory assetAmount) = token.call(
                    abi.encodeWithSignature(
                        "previewRedeem(uint256)",
                        deposit
                    )
                );
                if (!success) continue;
                deposit = abi.decode(assetAmount, (uint256));
            }

            collateralValue += CreditLib.calculateValue(
              o.getLatestAnswer(d.asset),
              deposit,
              d.assetDecimals
            );
        }
    }

    return collateralValue;
}

When releasing collateral, no more than the recorded amount can be removed (EscrowLib.sol#L163-L164):

function releaseCollateral(
    EscrowState storage self,
    address borrower,
    address oracle,
    uint256 minimumCollateralRatio,
    uint256 amount,
    address token,
    address to
) external returns (uint256) {
    require(amount > 0);
    if(msg.sender != borrower) { revert CallerAccessDenied(); }
    if(self.deposited[token].amount < amount) { revert InvalidCollateral(); }
    self.deposited[token].amount -= amount;
    
    LineLib.sendOutTokenOrETH(token, to, amount);

    uint256 cratio = _getLatestCollateralRatio(self, oracle);
    // fail if reduces cratio below min 
    // but allow borrower to always withdraw if fully repaid
    if(
      cratio < minimumCollateralRatio &&         // if undercollateralized, revert;
      ILineOfCredit(self.line).status() != LineLib.STATUS.REPAID // if repaid, skip;
    ) { revert UnderCollateralized(); }
    
    emit RemoveCollateral(token, amount);

    return cratio;
}

Tools Used

Manual review

Recommended Mitigation Steps

In all operations with collateral balances, consider checking actual token balances instead of using the value that was recorded when collateral was added.

Lines of code

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

Vulnerability details

Impact

If a lender is a contract, it can disable receiving of ETH (by reverting in the receive function) or an ERC777 (by reverting in the hook). As a result, the borrower won't be able to close a debt and will be forced by the lender into paying the facility fee indefinitely.

Proof of Concept

The _close function pushes payments to a lender, which passes the execution control to the lender contract (if it's a contract) and which allows the lender contract to revert and cause the closing of a credit to fail (LineOfCredit.sol#L487-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( // @audition passes execution control to the lender
            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;
}

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider using pull over push when releasing remained lender's funds. For example, when closing a credit, keep the record of remaining lender's funds and let the lender withdraw them in a separate call.