_sortIntoQ() Wrong sequencing logic may lead to locked funds

[code423n4]

Lines of code

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

Vulnerability details

Impact

LineOfCredit#_sortIntoQ() moving ID into the next availble FIFO position in the repayment queue,but it can not move when ids[0]=bytes32(0), resulting in the inability to #depositAndClose() and #liquidate(), the amount is locked

Proof of Concept

Steps:

1. Assume there are two Credits
   ids[0]={id:id1,token:tokenA,principal:0}
   ids[1]={id:id2,token:tokenB,principal:0}

2. After #depositAndClose() borrowing and fully repaying for ids[0], the result:
   ids[0]={id:id2,token:tokenB,principal:0}
   ids[1]={id:0,token:0,principal:0}

3. afer #addCredit() Add a Credit, which becomes:
   ids[0]={id:id2,token:tokenB,principal:0}
   ids[1]={id:0,token:0,principal:0}
   ids[2]={id:id3,token:tokenC,principal:0}

4. after #depositAndClose() borrowing and full repayment for ids[0]
   ids[0]={id:0,token:0,principal:0}
   ids[1]={id:id3,token:tokenC,principal:0}
   ids[2]={id:0,token:0,principal:0}

5. #borrow(ids[1]) Borrow 100 for ids[1], since _sortIntoQ() ignores ids[0]==bytes32(0), resulting ids[0] unchanged
   ids[0]={id:0,token:0,principal:0} //****always first ****/
   ids[1]={id:id3,token:tokenC,principal:100}
   ids[2]={id:0,token:0,principal:0}

6. After that it will not be possible to #depositAndClose() and #liquidate(), the amount is locked, because modifier whileBorrowing() will revert NotBorrowing();

    modifier whileBorrowing() {
        if(count == 0 || credits[ids[0]].principal == 0) { revert NotBorrowing(); } /***@audit principal==0****/
        _;
    }

  function liquidate(
    uint256 amount,
    address targetToken
  )
    external
    whileBorrowing //@audit whileBorrowing**/
    returns(uint256)


    function depositAndClose()
        external
        payable
        override
        whileBorrowing //@audit whileBorrowing**/
        onlyBorrower
        returns (bool)
    {

test example code:

contract SecuredLineTest is Test {
...

+    function test_not_liquidate() public {
+        _addCredit(address(supportedToken1), 1 ether);
+        _addCredit(address(supportedToken2), 1 ether);
+        vm.startPrank(borrower);
+        line.borrow(line.ids(0), 1 ether);        
+        line.depositAndClose();
+        vm.stopPrank(); 
+        _addCredit(address(supportedToken1), 1 ether); 
+        vm.startPrank(borrower);                       
+        line.borrow(line.ids(0), 1 ether);        
+        line.depositAndClose();
        
+        line.borrow(line.ids(1), 1 ether);
+        for(uint256 i;i<3;++i){
+            console.logBytes32(line.ids(i));
+        }
+        line.depositAndClose(); //***@audit whileBorrowing() will revert NotBorrowing() ****/
+        vm.stopPrank();        
+    }

forge test --match test_not_liquidate  -vvv

Tools Used

Recommended Mitigation Steps

remove id == bytes32(0)

    function _sortIntoQ(bytes32 p) internal returns (bool) {
        uint256 lastSpot = ids.length - 1;
        uint256 nextQSpot = lastSpot;
        bytes32 id;
        for (uint256 i; i <= lastSpot; ++i) {
            id = ids[i];
            if (p != id) {
                if (
-                 id == bytes32(0) ||       // deleted element. In the middle of the q because it was closed.
                  nextQSpot != lastSpot ||  // position already found. skip to find `p` asap
                  credits[id].principal > 0 //`id` should be placed before `p` 
                ) continue;
                nextQSpot = i;              // index of first undrawn line found
            } else {
                ...
            }
          
        }
    }

[c4-judge]

dmvt marked the issue as duplicate of #69

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as duplicate of #354

[c4-judge]

dmvt marked the issue as nullified

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as duplicate of #421

[c4-judge]

dmvt marked the issue as not nullified

[c4-judge]

dmvt marked the issue as satisfactory