Debt DAO contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

Borrower debt can be increased due to overflow error. Malicious lender or borrower by mistake can increase debt of borrower to very huge amount.

Proof of Concept

SpigotedLine.useAndRepay function allows to repay credit using unsued amount of credit tokens that is controlled by SpigotedLine.

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L137-L151

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

Note, that there is no any check that amount provided by caller is less or equal to credit.principal + credit.interests. That means that borrower or lender can provide it by mistake or maliciously. Then function LineOfCredit._repay will be called that will just pass amount to the CreditLib.repay.

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/CreditLib.sol#L168-L195

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

As you can see this function uses unchecked for calculation. That's why in case when the provided value is bigger than credit.principal + credit.interestAccrued we will have overflow here.

As a result now, users debt became super big, while it should be 0.

How this can be used? First of all by lender. He can call this function and provide needed amount for overflow. But there is one condition that should be met. Unused amount of tokens should be bigger then credit.principal + credit.interestAccrued.

This is the test that will show how it works. Copy it to SpigotedLine.t.sol.

function test_principal_become_very_huge() 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);

      uint256 principal;
      uint256 interestAccrued;
      (, principal, interestAccrued,,,,) = line.credits(line.ids(0));
      //amount is bigger than principal and interestAccrued to make oferflow
      uint256 amountThatShouldBeUnused = principal + interestAccrued + 1;
      bytes memory tradeData = abi.encodeWithSignature(
        'trade(address,address,uint256,uint256)',
        address(revenueToken),
        Denominations.ETH,
        1 gwei,
        amountThatShouldBeUnused
      );

      hoax(borrower);
      line.claimAndTrade(address(revenueToken), tradeData);
      uint256 unusedAmount = line.unused(address(revenueToken));
      //unused amount should be bigger then amount we provide to useAndRepay
      console.log("unused:", unusedAmount);

      (, principal, interestAccrued,,,,) = line.credits(line.ids(0));
      //now principal is lentAmount
      console.log("principal:", principal);
      console.log("interest:", interestAccrued);

      vm.prank(lender); // prank lender
      
      //here is the attack
      line.useAndRepay(amountThatShouldBeUnused);

      (, principal, interestAccrued,,,,) = line.credits(line.ids(0));
      //principal is too big now
      console.log("princ:", principal);
      console.log("interest:", interestAccrued);
    }

Tools Used

VsCode

Recommended Mitigation Steps

Do not pass amount that is bigger than credit.principal + credit.interestAccrued to _repay function.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L223-L244 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L59-L74

Vulnerability details

Impact

Borrower can by mistake add own money to credit if credit is in ETH.

Proof of Concept

Function LineOfCredit.addCredit is used to create new credit. It can be called only after contest of another party.

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

LineLib.receiveTokenOrETH(token, lender, amount) is responsible for getting payment. https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L59-L74

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

As you can see in case of native token payment, sender is not checked to be msg.sender, so this makes it's possible that borrower can by mistake pay instead of lender. It sounds funny, but it's possible. What is needed is that lender call addCredit first and then borrower calls addCredit and provides value.

Tools Used

VsCode

Recommended Mitigation Steps

Check that if payment in ETH then lender == msg.sender in addCredit function.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/MutualConsent.sol#L38-L60 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/MutualConsent.sol#L31-L36

Vulnerability details

Impact

Borrower or Lender can't remove their consent from MutualConsent if they need.

Proof of Concept

MutualConsent has mutualConsent modifier which is used to protect functions that need to have consent of 2 parties before call. Example of such function is LineOfCredit.addCredit.

This is how mutual consent checking works https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/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;
    }

As you can see the function store consent of first caller and then waits when another caller will call same function. There is no any ability to remove this contest.

How this can be used? For example at t1 lender and borrower aggreed on some credit for amount of tokens and specific fee rates. Then borrower calls createCredit and his consent is stored. But lender do not call createCredit. After some time smth happened and rates changed, so it's not good credit conditions for a borrower anymore, but very good for lender. And lender calls createCredit. As borrower consent already stored, this credit will be created and fees will start accumulate.

Tools Used

VsCode

Recommended Mitigation Steps

Add ability to remove consent, or think about some expiration time for consents.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L223-L244 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L59-L74

Vulnerability details

Impact

Function LineOfCredit.addCredit do not return overpaid in ETH amount and do not add it to credit. As a result if lender overpaid by mistake, he loses those funds.

Proof of Concept

Function LineOfCredit.addCredit allows to create new credit and fund it with ERC20 token or ETHER. LineLib.receiveTokenOrETH function is used to receive payment.

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

This function do not return overpaid amount to lender and also this overpaid amount is not added to credit line(this is correct). As result lender can lose funds by mistake.

Tools Used

VsCode

Recommended Mitigation Steps

Return overpaid amount back to lender.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L61-L80

Vulnerability details

Impact

Currently SpigotLib stores whitelisted functions per all revenue contracts, but it should store them for each revenue contract separately as whitelisted function in one revenue contract can be malicious in another.

Proof of Concept

When Spigot is deployed for the line, then borrower with approve of arbiter can add revenue contract to Spigot. This revenue contract should have some functions to work with it through the Spigot(transferOwnership, claimRevenue). It's not allowed to use all functions of revenue contract, because of security reasons(as it can transfer ownership or claim revenue). That's why there is ability to provide whitelisted functions using SpigotedLine.updateWhitelist function.

The problem is that when whitelisted function selector is stored it is stored for all revenue contracts, not only for specific one. https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L208-L213

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

And then when SpigotLib.operate is called by operator, no matter which revenue contract he wants to call, function just should be whitelisted. If it is whitelisted then call is possible to any revenue contract.

How attacker can use it. 1.Attacker creates simple revenue contract with function that is called doSomeJob(). 2.Arbiter checks that revenue contract contains transferOwnership ,claimRevenue functions that are not harmful. 3.They call addSpigot and add this revenue contract to Spigot. 4.Operator asks Arbiter to include doSomeJob() function as it is needed to do revenue. 5.After the check arbiter calls updateWhitelist and now function updateWhitelist is possible to call for every revenue contract. 6.Some time later attacker creates another contract with all valid functions and doSomeJob() function which does smth harmful(like claim revenue and mess up all calculations or transfer ownership to attacker). 7.Arbiter checks that new revenue contract contains transferOwnership ,claimRevenue functions that are not harmful. 8.They call addSpigot and add this revenue contract to Spigot. 9.Now attacker is possible to call this function on new revenue contract to break things.

Also it's possible that harmful function is provided in the first revenue contract and it's allowance to call will be whitelisted with the second revenue contract which implements that function without bad actions, just to get it whitelisted. Then when function is whitelisted, attacker calls it on first revenue contract.

Tools Used

VsCode

Recommended Mitigation Steps

Consider to have whitelisted functions per revenue contract.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L223-L244 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L59-L74

Vulnerability details

Impact

Function LineOfCredit.addCredit do not support non standard ERC20 tokens. This leads to incorrect calculations and lose of funds.

Proof of Concept

Function LineOfCredit.addCredit allows to create new credit and fund it with ERC20 token or ETHER.

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

This is how transfer is handled. LineLib.receiveTokenOrETH function is used to receive payment.

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

As you can see amount was just transferred without check of balance of contract before and after. Later the amount provided in addCredit is set to the credit info. In case of non standard erc20 token the provided amount will be wrong and all interest calculations will be also incorrect.

That means that current implementation can't handle different non standard tokens, like fee on transfer, rebase tokens.

Tools Used

VsCode

Recommended Mitigation Steps

Calculate transferred amount by checking balance before and after the transfer.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L34-L51

Vulnerability details

Impact

LineLib.sendOutTokenOrETH function use transfer to send ether. If receiver is not EOA then the call can revert because of not enough amount of gas.

Proof of Concept

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

If payment should be done in ether, then transfer is called which provide 2300 amount of gas to the call. If receiver is not EOA this amount can be not enough and this will cause all payments to revert and user will not be able to get his funds.

Actually all loan repayment will fail if all conditions are met.

Tools Used

VsCode

Recommended Mitigation Steps

Use call instead of `transfer.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SecuredLine.sol#L48-L66

Vulnerability details

Impact

SecuredLine.rollover function allows user to transfer the Escrow and Spigot from repaid secured line to another line. In case if line is CreditOfLine which do not know how to work with Spigot, ownersip of Spigot will be lost.

Proof of Concept

SecuredLine.rollover function changes line for the Escrow and transfers ownership of Spigot to the new line. https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SecuredLine.sol#L48-L66

  function rollover(address newLine)
    external
    onlyBorrower
    override
    returns(bool)
  {
    // require all debt successfully paid already
    if(status != LineLib.STATUS.REPAID) { revert DebtOwed(); }
    // require new line isn't activated yet
    if(ILineOfCredit(newLine).status() != LineLib.STATUS.UNINITIALIZED) { revert BadNewLine(); }
    // we dont check borrower is same on both lines because borrower might want new address managing new line
    EscrowedLine._rollover(newLine);
    SpigotedLineLib.rollover(address(spigot), newLine);


    // ensure that line we are sending can accept them. There is no recovery option.
    if(ILineOfCredit(newLine).init() != LineLib.STATUS.ACTIVE) { revert BadRollover(); }


    return true;
  }

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotedLineLib.sol#L146-L149

    function rollover(address spigot, address newLine) external returns(bool) {
      require(ISpigot(spigot).updateOwner(newLine));
      return true;
    }

As result ownership of Spigot will be completely lost and can't be recovered as LineOfCredit do not have such function. Also all functions that rely on owner will not possible to call.

Regarding to the Escrow it also can make some problems. After rollover Escrow will have new line and this line will never be changed, because LineOfCredit do not have such function.

In case if rollover was called for SpigotedLine then only Escrow will have problems. Borrower will not be able to withdraw his collateral funds while credit is not repaid. And in case if this SpigotedLine will become liquidateable and borrower didn't withdraw collateral before, then he will not be able to withdraw collateral anymore, even if SpigotedLine is not suppose to use any Escrow.

This is simple test that shows, that you can rollover to CreditOfLine.

function test_rollover_to_credit_of_line() public {
      _addCredit(address(supportedToken1), 1 ether);
      bytes32 id = line.ids(0);
      hoax(borrower);
      line.borrow(id, 1 ether);

      hoax(borrower);
      line.depositAndClose();

      LineOfCredit lineOfCredit = new LineOfCredit(
        address(oracle),
        arbiter,
        borrower,
        150 days);

      hoax(borrower);
      line.rollover(address(lineOfCredit));

      assertEq(address(lineOfCredit), spigot.owner());
      assertEq(address(lineOfCredit), escrow.line());
    }

Tools Used

VsCode

Recommended Mitigation Steps

You could add some function that indicates that line is SecuredLine and then allow rollover only to such function.