Arbitrary call order to handle mutual consent can lead to unrecoverable native ETH

[code423n4]

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.

[c4-judge]

dmvt marked the issue as duplicate of #42

[c4-judge]

dmvt changed the severity to 3 (High Risk)

[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