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

[code423n4]

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.

[c4-judge]

dmvt marked the issue as duplicate of #42

[trust1995]

Believe I have provided much more detail than the selected-for-report duplicate:
#125

[c4-judge]

dmvt marked the issue as satisfactory

[C4-Staff]

liveactionllama marked the issue as not a duplicate

[C4-Staff]

liveactionllama marked the issue as duplicate of #125