Debt DAO contest - PaludoX0's results

Lines of code

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

Vulnerability details

Impact

If a Credit is closed the id is removed from ids array, but array lenght doesn't change. When a credit is closed, internal function _close in LineOfCredit.sol is called. This function call ids.removePosition(id) from CreditListLib. CreditListLib.removePosition use EVM "delete" function to remove the id but array length doesn't change. See for reference https://blog.finxter.com/how-to-delete-an-element-from-an-array-in-solidity/#:~:text=We%20can%20create%20a%20function,using%20the%20pop()%20method.

Proof of Concept

This situation could lead to many problematic scenarios. I've tested the following:

1. lender and borrower addCredit for supportedToken1
2. borrower borrows half supportedToken1
3. lender and borrower addCredit for supportedToken2
4. borrower borrows half supportedToken1 and full supportedToken2
5. borrower depositAndClose supportedToken1
6. lender and borrower addCredit for supportedToken1
7. borrower borrows full supportedToken1
8. borrower depositAndClose supportedToken2
9. borrower tries to depositAndClose supportedToken1 but reverts due to modifier "whileBorrowing" because credits[ids[0]].principal == 0 modifier whileBorrowing() { if(count == 0 || credits[ids[0]].principal == 0)

In LineOfCredit.t.sol copy this test function:

function test_return_lender_funds_on_depositAndClose_and_addCredit_again() public {
    assertEq(supportedToken1.balanceOf(address(line)), 0, "Line balance should be 0");
    assertEq(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial mint balance");
    _addCredit(address(supportedToken1), 1 ether); // deposit 1eth token 1
    bytes32 id = line.ids(0);
    assert(id != bytes32(0));
    assertEq(supportedToken1.balanceOf(lender), mintAmount - 1 ether, "Lender should have initial balance less lent amount");
    vm.startPrank(borrower);  
    line.borrow(id, 0.5 ether); //borrow 0.5eth token 1
    vm.stopPrank();
    vm.warp(10 days);
    _addCredit(address(supportedToken2), 1 ether); //deposit 1eth token 2
    bytes32 id2 = line.ids(1);
    assert(id2 != bytes32(0));
    vm.startPrank(borrower);  
    line.borrow(id, 0.5 ether); //borrow 0.5eth token 1
    line.borrow(id2, 1 ether); //borrow 1eth token 2
    vm.warp(10 days);
    line.depositAndClose(); //Close token 1 collateral
    vm.stopPrank();
    assertLe(supportedToken1.balanceOf(borrower),mintAmount, "Borrower should have initial balance loan");
    assertGe(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial balance after depositAndClose");
    assert(id2 == line.ids(0)); //now token2 is correctly in position 0 of array
    vm.warp(10 days);
    _addCredit(address(supportedToken1), 1 ether); // deposit 1eth of token 1 again
    vm.startPrank(borrower);  
    line.borrow(id, 1 ether);//borrow 1eth token 1
    vm.warp(10 days);
    line.depositAndClose(); //now borrower wants to close both lines of credit and close first token 2 credit
    assertEq(uint(line.status()), uint(LineLib.STATUS.ACTIVE), "Line not repaid"); //line is still active
    assertLe(supportedToken2.balanceOf(borrower),mintAmount, "Borrower should have initial balance loan");
    assertGe(supportedToken2.balanceOf(lender), mintAmount, "Lender should have initial balance after depositAndClose");
    assertGe(supportedToken1.balanceOf(borrower),mintAmount, "Borrower should have initial balance loan");
    assertLe(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial balance after depositAndClose");
    assert(bytes32(0) == line.ids(0)); // now line.ids(0) should be == id (token1 hash) but is equal to 0
    line.depositAndClose(); //tries to close token 1 credit but reverts
}

This will revert because credits[ids[0]].principal == 0

Tools Used

Forge

Recommended Mitigation Steps

In CredistListLib.sol rewrite the function as follows

function removePosition(bytes32[] storage ids, bytes32 id) external returns(bool) { uint256 len = ids.length; uint index; //search for id position in array for(uint256 i; i < len; ++i) { if(ids[i] == id) { index = i; } } //move id in last position and pop for (uint i = index; i<ids.length-1; i++){ ids[i] = ids[i+1]; }
ids.pop(); return true; }

Try the following test

function test_return_lender_funds_on_depositAndClose_and_addCredit_again() public {
    assertEq(supportedToken1.balanceOf(address(line)), 0, "Line balance should be 0");
    assertEq(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial mint balance");
    _addCredit(address(supportedToken1), 1 ether); // deposit 1eth token 1
    bytes32 id = line.ids(0);
    assert(id != bytes32(0));
    assertEq(supportedToken1.balanceOf(lender), mintAmount - 1 ether, "Lender should have initial balance less lent amount");
    vm.startPrank(borrower);  
    line.borrow(id, 0.5 ether); //borrow 0.5eth token 1
    vm.stopPrank();
    vm.warp(10 days);
    _addCredit(address(supportedToken2), 1 ether); //deposit 1eth token 2
    bytes32 id2 = line.ids(1);
    assert(id2 != bytes32(0));
    vm.startPrank(borrower);  
    line.borrow(id, 0.5 ether); //borrow 0.5eth token 1
    line.borrow(id2, 1 ether); //borrow 1eth token 2
    vm.warp(10 days);
    line.depositAndClose(); //Close token 1 collateral
    vm.stopPrank();
    assertLe(supportedToken1.balanceOf(borrower),mintAmount, "Borrower should have initial balance loan");
    assertGe(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial balance after depositAndClose");
    assert(id2 == line.ids(0)); //now token2 is correctly in position 0 of array
    vm.warp(10 days);
    _addCredit(address(supportedToken1), 1 ether); // deposit 1eth of token 1 again
    vm.startPrank(borrower);  
    line.borrow(id, 1 ether);//borrow 1eth token 1
    vm.warp(10 days);
    line.depositAndClose(); //now borrower wants to close both lines of credit and close first token 2 credit
    line.depositAndClose(); //close token 1 credit
    vm.stopPrank();
    assertEq(uint(line.status()), uint(LineLib.STATUS.REPAID), "Line not repaid");
    assertLe(supportedToken2.balanceOf(borrower),mintAmount, "Borrower should have initial balance loan");
    assertGe(supportedToken2.balanceOf(lender), mintAmount, "Lender should have initial balance after depositAndClose");
    assertLe(supportedToken1.balanceOf(borrower),mintAmount, "Borrower should have initial balance loan");
    assertGe(supportedToken1.balanceOf(lender), mintAmount, "Lender should have initial balance after depositAndClose");
}