SecuredLine.rollover function allows to rollover to CreditOfLine. Ownersip of Spigot will be lost ferover.

[code423n4]

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.

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

kibagateaux marked the issue as sponsor acknowledged

[c4-sponsor]

kibagateaux marked the issue as disagree with severity

[kibagateaux]

Technically not possible since we only deploy SecuredLine in our system, if someone was deploying contracts on their own, of course they could misconfigure it like any contract but impossible on our deployed system.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-b