Debt DAO contest - Trust's results

Lines of code

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

Vulnerability details

Description

The addCredit() function transfers money from lender to a LineOfCredit contract, and opens a credit account. increaseCredit() transfers additional funds to an existing credit account contract. Both functions are payable and guarded by mutualConsent(), which guarantees that both borrower and lender approve the execution.

It is not critical to understand how the mutualConsent() modifier works, but it is important to realize both borrower and lender call this function once (the order does not matter), and in the second execution the function body is actually executed.

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

The issue is that these two functions have to verify that lender calls after borrower. In the case of ETH credit token, when lender calls first, the message value is sent to the line without bookkeeping. At this point, it doesn't even matter if borrower will call the function as well or not, in both scenarios the previous msg.value will be lost:

function addCredit(
    uint128 drate,
    uint128 frate,
    uint256 amount,
    address token,
    address lender
)
    external
    payable
    override
    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;
}

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
        // ---------- ISSUE --------- IF BORROWER CALLS SECOND msg.value will be 0
        if(msg.value < amount) { revert TransferFailed(); }
    }
    return true;
}

Note that this is different from the other addCredit/increaseCredit submission (borrower leaks funds) - here the root cause is the order of transactions, whereas in the other submission it's the payability of borrower.

There is no way (to my knowledge) of retrieving funds that are not accounted for in bookkeeping. Even the sweeping function requires the sweeped funds to be in the unusedTokens array:

function sweep(address to, address token) external nonReentrant returns (uint256) {
    uint256 amount = unusedTokens[token];
    delete unusedTokens[token];
    bool success = SpigotedLineLib.sweep(
      to,
      token,
      amount,
      _updateStatus(_healthcheck()),
      borrower,
      arbiter
    );
    return success ? amount : 0;
}

Impact

When lender consents before borrower in ETH credit token, all the lent funds are permanently lost.

Tools Used

Manual audit

Recommended Mitigation Steps

Two options:

1. Add a borrowerBeforeLender modifier which will guarantee correct chain of events - the downside is that the order is now forced

2. If lender consents before borrower, keep their funds in escrow and allow them to cancel the consent.

Lines of code

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

Vulnerability details

Description

LineOfCredit manages an array of open credit line identifiers called ids. Many interactions with the Line operate on ids[0], which is presumed to be the oldest borrow which has non zero principal. For example, borrowers must first deposit and repay to ids[0] before other credit lines. 

The list is managed by several functions:

1. CreditListLib.removePosition - deletes parameter id in the ids array
2. CreditListLib.stepQ - rotates all ids members one to the left, with the leftmost becoming the last element
3. _sortIntoQ - most complex function, finds the smallest index which can swap identifiers with the parameter id, which satisfies the conditions:
   1. target index is not empty
   2. there is no principal owed for the target index's credit

The idea I had is that if we could corrupt the ids array so that ids[0] would be zero, but after it there would be some other active borrows, it would be a very severe situation. The whileBorrowing() modifier assumes if the first element has no principal, borrower is not borrowing. 

modifier whileBorrowing() {
    if(count == 0 || credits[ids[0]].principal == 0) { revert NotBorrowing(); }
    _;
}

It turns out there is a simple sequence of calls which allows borrowing while ids[0] is deleted, and does not re-arrange the new borrow into ids[0]!

1. id1 = addCredit() - add a new credit line, a new id is pushed to the end of ids array.
2. id2 = addCredit() - called again, ids.length = 2
3. close(id1) - calls removePosition() on id1, now ids array is [0x000000000000000000000000, id2 ]
4. borrow(id2) - will borrow from id2 and call _sortIntoQ. The sorting loop will not find another index other than id2's existing index (id == bytes32(0) is true).

From this sequence, we achieve a borrow while ids[0] is 0! Therefore, credits[ids[0]].principal = credits[0].principal = 0, and whileBorrowing() reverts.

The impact is massive - the following functions are disabled:

• SecureLine::liquidate()
• LineOfCredit::depositAndClose()
• LineOfCredit::depositAndRepay()
• LineOfCredit::claimAndRepay()
• LineOfCredit::claimAndTrade()

Impact

Borrower can craft a borrow that cannot be liquidated, even by arbiter. Alternatively, functionality may be completely impaired through no fault of users.

Proof of Concept

Copy the following code into LineOfCredit.t.sol

function _addCreditLender2(address token, uint256 amount) public {
    // Prepare lender 2 operations, does same as mintAndApprove()
    address lender2 = address(21);
    deal(lender2, mintAmount);
    supportedToken1.mint(lender2, mintAmount);
    supportedToken2.mint(lender2, mintAmount);
    unsupportedToken.mint(lender2, mintAmount);
    vm.startPrank(lender2);
    supportedToken1.approve(address(line), MAX_INT);
    supportedToken2.approve(address(line), MAX_INT);
    unsupportedToken.approve(address(line), MAX_INT);
    vm.stopPrank();
    // addCredit logic
    vm.prank(borrower);
    line.addCredit(dRate, fRate, amount, token, lender2);
    vm.stopPrank();
    vm.prank(lender2);
    line.addCredit(dRate, fRate, amount, token, lender2);
    vm.stopPrank();
}
function test_attackUnliquidatable() public {
    bytes32 id_1;
    bytes32 id_2;
    _addCredit(address(supportedToken1), 1 ether);
    _addCreditLender2(address(supportedToken1), 1 ether);
    id_1 =  line.ids(0);
    id_2 =  line.ids(1);
    hoax(borrower);
    line.close(id_1);
    hoax(borrower);
    line.borrow(id_2, 1 ether);
    id_1 =  line.ids(0);
    id_2 = line.ids(1);
    console.log("id1 : ", uint256(id_1));
    console.log("id2 : ", uint256(id_2));
    vm.warp(ttl+1);
    assert(line.healthcheck() == LineLib.STATUS.LIQUIDATABLE);
    vm.expectRevert(ILineOfCredit.NotBorrowing.selector);
    bool isSolvent = line.declareInsolvent();
}

Tools Used

Manual audit

Recommended Mitigation Steps

When sorting new borrows into the ids queue, do not skip any elements.

Lines of code

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

Vulnerability details

Description

CreditLib's repay() function is the actual accounting of repayments in a LineOfCredit:

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

Note that the entire function has no overflow protection. It is assumed that the caller of repay() will perform the necessary validation:

require(amount <= credit.principal + credit.interestAccrued);

If this is not the case, the following line will overflow:

  credit.principal -= principalPayment;

Unfortunately, there is one instance where amount is not checked, in useAndRepay():

/// see ISpigotedLine.useAndRepay
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;
}

When borrower tries to repay using unusedTokens, if they request a repay amount smaller than the unusedTokens total, but larger than the maximum they owe to lender, it will overflow the principal field. At this point, borrower will owe close to 2^256 tokens to lender.

Note that it is very easy for user to enter a larger amount than currently owed, as interest is ever accruing and they may estimate it to grow faster than it did in practice until the block is executed.

Impact

When borrower repays, it can overflow and make them owe 2^256 tokens to lender.

Proof of Concept

Copy the following code to SpigotedLine.t.sol:

function test_lender_can_use_and_repay_overflow() public {
  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);
  _borrow(id, lentAmount);
  bytes memory tradeData = abi.encodeWithSignature(
    'trade(address,address,uint256,uint256)',
    address(revenueToken),
    Denominations.ETH,
    2 gwei,
    lentAmount + 1 ether
  );
  uint256 new_tokens;
  hoax(borrower);
  new_tokens = line.claimAndTrade(address(revenueToken), tradeData);
  console.log(new_tokens);
  vm.prank(lender); // prank lender
  line.useAndRepay(lentAmount + 1 ether);
  (, uint256 principal,,,,,) = line.credits(line.ids(0));
  console.log("Principal is", principal);
  //assertEq(principal, 0);
}

Tools Used

Manual audit

Recommended Mitigation Steps

It is recommended to check for overflow in repay() function, as there is always a risk of introducing threats when adding more calls to repay().

Lines of code

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

Vulnerability details

Description

The receiveTokenOrETH() utility function is used multiple times the LineOfCredit contract:

• addCredit
• increaseCredit
• depositAndClose
• depositAndRepay
• close

Payment is accepted in ETH from the contract call, or ERC20 transfer:

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

The issue is that whenever the caller sends more msg.value than amount to be received, the difference would not be refunded back to them and be permanently stuck in the contract.

Usually leaks of value are treated as Med severity, but in this instance the leak is guaranteed in the close() and depositAndClose() function. This is because user does not specify an amount, only that they wish to close the credit line. The amount is calculated in real time, using the latest accrued interest. Therefore, user cannot know the accurate amount of msg.value to send and will either send too little, making the transaction revert, or send too much, and leak the difference. Because the leak is unavoidable, I believe the finding to be of high severity.

Impact

When using ETH as credit token, there will be a guaranteed leak of value.

Tools Used

Manual audit

Recommended Mitigation Steps

receiveTokenOrETH() should receive a boolean refundSpare which will send spare ETH to the msg.sender. It should only be false if the caller wants to perform several receives and wishes to perform accounting in the calling function.

Lines of code

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

Vulnerability details

Description

SpigotedLineLib's claimAndTrade function allows borrower to choose when to swap their revenue tokens to credit tokens.

function claimAndTrade(
    address claimToken,
    address targetToken,
    address payable swapTarget,
    address spigot,
    uint256 unused,
    bytes calldata zeroExTradeData
)
    external 
    returns(uint256, uint256)
{
    // can't trade into same token. causes double count for unused tokens
    if(claimToken == targetToken) { revert BadTradingPair(); }
    // snapshot token balances now to diff after trade executes
    uint256 oldClaimTokens = LineLib.getBalance(claimToken);
    uint256 oldTargetTokens = LineLib.getBalance(targetToken);
    
    // @dev claim has to be called after we get balance
    // reverts if there are no tokens to claim
    uint256 claimed = ISpigot(spigot).claimEscrow(claimToken);
    trade(
        claimed + unused,
        claimToken,
        swapTarget,
        zeroExTradeData
    );
    
    // underflow revert ensures we have more tokens than we started with
    uint256 tokensBought = LineLib.getBalance(targetToken) - oldTargetTokens;
    if(tokensBought == 0) { revert TradeFailed(); } // ensure tokens bought
    uint256 newClaimTokens = LineLib.getBalance(claimToken);
    // ideally we could use oracle to calculate # of tokens to receive
    // but sellToken might not have oracle. buyToken must have oracle
    emit TradeSpigotRevenue(
        claimToken,
        claimed,
        targetToken,
        tokensBought
    );
    // used reserve revenue to repay debt
    if(oldClaimTokens > newClaimTokens) {
      uint256 diff = oldClaimTokens - newClaimTokens;
      // used more tokens than we had in revenue reserves.
      // prevent borrower from pulling idle lender funds to repay other lenders
      if(diff > unused) revert UsedExcessTokens(claimToken,  unused); 
      // reduce reserves by consumed amount
      else return (
        tokensBought,
        unused - diff
      );
    } else { unchecked {
      // excess revenue in trade. store in reserves
      return (
        tokensBought,
        unused + (newClaimTokens - oldClaimTokens)
      );
    } }
}

Borrower passes calldata which is sent to 0x DEX via the trade() call:

function trade(
    uint256 amount,
    address sellToken,
    address payable swapTarget,
    bytes calldata zeroExTradeData
) 
    public
    returns(bool)
{
    if (sellToken == Denominations.ETH) {
        // if claiming/trading eth send as msg.value to dex
        (bool success, ) = swapTarget.call{value: amount}(zeroExTradeData);
        if(!success) { revert TradeFailed(); }
    } else {
        IERC20(sellToken).approve(swapTarget, amount);
        (bool success, ) = swapTarget.call(zeroExTradeData);
        if(!success) { revert TradeFailed(); }
    }
    return true;
}

The critical issue is that borrower may use this functionality to exfiltrate their locked escrow funds out of the contract. Borrower has complete freedom on how to craft 0x DEX calldata. They have the entire escrow balance + unused balance to pass to 0x.

The only limitation is that tokensBought (credit tokens) must increase:

// underflow revert ensures we have more tokens than we started with
uint256 tokensBought = LineLib.getBalance(targetToken) - oldTargetTokens;
if(tokensBought == 0) { revert TradeFailed(); } // ensure tokens bought

There are many different ways to structure the 0x call to exfiltrate the money. The simplest is probably the fillLimitOrder handler in 0x

function fillLimitOrder(
    LibNativeOrder.LimitOrder memory order,
    LibSignature.Signature memory signature,
    uint128 takerTokenFillAmount
) public payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount) {
    FillNativeOrderResults memory results = _fillLimitOrderPrivate(
        FillLimitOrderPrivateParams({
            order: order,
            signature: signature,
            takerTokenFillAmount: takerTokenFillAmount,
            taker: msg.sender,
            sender: msg.sender
        })
    );
    LibNativeOrder.refundExcessProtocolFeeToSender(results.ethProtocolFeePaid);
    (takerTokenFilledAmount, makerTokenFilledAmount) = (
        results.takerTokenFilledAmount,
        results.makerTokenFilledAmount
    );
}

User specifies this struct for processing:

struct LimitOrder {
    IERC20TokenV06 makerToken;
    IERC20TokenV06 takerToken;
    uint128 makerAmount;
    uint128 takerAmount;
    uint128 takerTokenFeeAmount;
    address maker;
    address taker;
    address sender;
    address feeRecipient;
    bytes32 pool;
    uint64 expiry;
    uint256 salt;
}

Therefore, it is easy for borrower to prepare a trade as maker, which accepts a ton of revenue tokens, and returns a single credit token. From the trade() call, borrower will fulfill that order as a taker, and still pass the balance check performed later.

There are many more clever and devious ways to exfiltrate the money once borrower has 0x arbitrary call primitive, for example playing with the feeRecipient comes to mind.

Impact

Borrower can bypass all revenue protection for lender, essentially borrowing for free.

Tools Used

Manual audit

Recommended Mitigation Steps

If it is desired to allow borrower to arbitrarily call 0x, it must be locked down very hard. Need to run balance checks on all revenue tokens, check slippage is acceptable and so on.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/factories/LineFactory.sol#L135

Vulnerability details

Description

LineFactory is in charge of creating new lines. deploySecuredLineWithModules() creates a new secure line using an existing spigot and escrow, and new CoreLineParams parameters. There is a dev note hinting to this issue:

*   @dev   The `cratio` in the CoreParams are not used, due to the fact
*          they're passed in when the Escrow is created separately.
*/

The problem is that caller passes a cratio in CoreLineParams, but it is completely ignored because the one in the existing escrow is used. There is no verification that the intended cratio == mSpigot's cratio. User could easily think the new ratio will be applied, but actually, the old one would be reused.

Since they are not even warned about this at any stage, they may suffer a surprising liquidation when their collateral does not suffice for the re-used cratio.

Impact

Borrowers may unknowingly use a previous collateral ratio when deploying a secured line with existing modules.

Tools Used

Manual audit

Recommended Mitigation Steps

add a requirement:

require(coreParams.cratio == Escrow(mEscrow).minimumCollateralRatio())