Function `settleWithBuyout()` incorrectly calculate the main lender because single lender can have multiple tranches in a loan

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/AuctionWithBuyoutLoanLiquidator.sol#L69

Vulnerability details

Impact

The settleWithBuyout() function is used to settle an auction with a buyout from the main lender. It calculates the main lender by looping through all the tranches and selecting the lender of the largest tranche as the main lender.

uint256 largestTrancheIdx;
uint256 largestPrincipal;
for (uint256 i = 0; i < _loan.tranche.length;) {
    if (_loan.tranche[i].principalAmount > largestPrincipal) {
        largestPrincipal = _loan.tranche[i].principalAmount;
        largestTrancheIdx = i;
    }
    unchecked {
        ++i;
    }
}

However, this is incorrect as a single lender could have multiple tranches. This could be abused by a borrower to manipulate the selection of the main lender.

Proof of Concept

Consider the following scenario:

1. Alice (lender) offers to lend 40 eth
2. Bob (another lender) offers to lend 20 eth
3. Caleb (borrower) wants to allow Bob, not Alice, to buyout his NFT but still wants to borrow all 40 eth from Alice. He can take partial loan from Alice like this:

tranche = [
  0: 20 eth from Bob
  1: 20 eth from Alice
  2: 0.01 eth from Caleb
  3: 20 eth from Alice
]

As you can see, even though Alice lend 40 eth to Caleb, Bob will be the main lender. And also since the tranche of Alice is not consecutive (Caleb intentionally put 0.01 eth in between), they cannot be merged as well.

Tools Used

Manual Review

Recommended Mitigation Steps

To get the correct main lender, sum up the total principal in different tranches from the same lender.

Assessed type

Other

[c4-judge]

0xA5DF marked the issue as primary issue

[0xend]

I think this is highly unlikely given tranches have seniority. Looks like I forgot to check for _getMinTranchePrincipal (will be addressed in the mitigation review) which would also set a cost for caleb.

[0xend]

I'd say this is def low risk

[0xend]

Lastly, lender could actually refinance the small tranche from the borrower an merge all tranches.

[0xA5DF]

I think that selecting the lender with the biggest tranche rather than the biggest share is a reasonable design considering that finding the lender with the biggest share would be much more complicated and expensive to do on chain. And it isn't like the lender with the biggest share is losing their debt payment. Regarding manipulation, as long as it happens at the beginning of the loan, far from the starting time of the auction, I don't think it's significant.

Marking as low

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

Moved to #70

[c4-judge]

0xA5DF marked the issue as grade-c