dacian - Lender can take borrower's collateral before first payment due

[sherlock-admin]

dacian

high

Lender can take borrower's collateral before first payment due

Summary

For PaymentCycleType.Seconds if PaymentDefault < PaymentCycle, Lender can take Borrower's collateral before first payment is due. If PaymentDefault > 0 but very small, Lender can do this almost immediately after accepting borrower's bid. This is especially bad as the Market Operator who controls these parameters can also be the Lender.

Vulnerability Detail

Lender calls CollateralManager.withdraw() L254, which calls TellerV2.isLoanDefaulted() L930, which bypasses the 1 day grace period & doesn't take into account when first payment is due.

Impact

Borrower loses their collateral before they can even make their first repayment, almost instantly if PaymentDefault > 0 but very small.

Code Snippet

Put this test in TellerV2_Test.sol:

function test_LenderQuicklyTakesCollateral() public {
	MarketRegistry mReg = (MarketRegistry)(payable(address(tellerV2.marketRegistry())));

	// payment cycle 3600 seconds, payment default 1 second
	// payment will be in default almost immediately upon being
	// accepted, even though the first payment is not due for much longer
	// than the default time
	uint32 PAYMENT_CYCLE_SEC   = 3600;
	uint32 PAYMENT_DEFAULT_SEC = 1;

	vm.startPrank(address(marketOwner));
	mReg.setPaymentCycle(marketId1, PaymentCycleType.Seconds, PAYMENT_CYCLE_SEC);
	mReg.setPaymentDefaultDuration(marketId1, PAYMENT_DEFAULT_SEC);
	vm.stopPrank();

	//Submit bid as borrower
	uint256 bidId = submitCollateralBid();
	// Accept bid as lender
	acceptBid(bidId);

	// almost immediately take the collateral as the lender, even though
	// the first payment wasn't due for much later
	ICollateralManager cMgr = tellerV2.collateralManager();
	skip(PAYMENT_DEFAULT_SEC+1);
	cMgr.withdraw(bidId);
	// try again to verify the collateral has been taken
	vm.expectRevert("No collateral balance for asset");
	cMgr.withdraw(bidId);
}

Tool used

Manual Review

Recommendation

Change the calculations done as a consequence of calling TellerV2.isLoanDefaulted() to take into account when first payment is due; see similar code which does this TellerV2.calculateNextDueDate() L886-L899. Lender should only be able to take Borrower's collateral after the Borrower has missed their first payment deadline by PaymentDefault seconds.

Consider enforcing sensible minimums for PaymentDefault. If PaymentDefault = 0 no liquidations will ever be possible as TellerV2._canLiquidateLoan() L963 will always return false, so perhaps it shouldn't be possible to set PaymentDefault = 0.

[devdacian]

Escalate for 10 USDC

In previous audit contests, the issue of the Borrower being prematurely liquidated/having their collateral seized has been judged as a High finding:

1. High - Loan can be written off by anybody before overdue delay expires - Union Finance, Sherlock
2. High - Users can be liquidated prematurely because calculation understates value of underlying position Blueberry, Sherlock
3. High - Lender is able to seize the collateral by changing the loan parameters Abra, C4
4. High - Users may be liquidated right after taking maximal debt Papr, C4

Please explain the objective standard of truth that when applied to these previous submissions where Borrowers are prematurely liquidated/have their collateral seized judges them all as High, but when applied to my submission where Borrowers are prematurely liquidated/have their collateral seized judges it only medium.

Alternatively, please judge my submission as a High finding in line with the historical judging in both Sherlock & C4.

[sherlock-admin]

Escalate for 10 USDC

In previous audit contests, the issue of the Borrower being prematurely liquidated/having their collateral seized has been judged as a High finding:

1. High - Loan can be written off by anybody before overdue delay expires - Union Finance, Sherlock
2. High - Users can be liquidated prematurely because calculation understates value of underlying position Blueberry, Sherlock
3. High - Lender is able to seize the collateral by changing the loan parameters Abra, C4
4. High - Users may be liquidated right after taking maximal debt Papr, C4

Please explain the objective standard of truth that when applied to these previous submissions where Borrowers are prematurely liquidated/have their collateral seized judges them all as High, but when applied to my submission where Borrowers are prematurely liquidated/have their collateral seized judges it only medium.

Alternatively, please judge my submission as a High finding in line with the historical judging in both Sherlock & C4.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Trumpero]

This scenario only occurs when the PaymentDefaultDuration variable has been set too low by the admin/owner (1 in the POC), so I believe it is an unlikely occurrence, and should be a medium issue. It is not similar to the other cases mentioned by Watson in the past contests, as they do not require any manipulations/mistakes from the admin.

[hrishibhat]

Escalation rejected

Valid medium
This requires an admin error/malice to set a low default duration to cause loss of funds.
As opposed to the other past issues mentioned in the escalation have a logical flaw in the code to cause loss of funds.

[sherlock-admin]

Escalation rejected

Valid medium
This requires an admin error/malice to set a low default duration to cause loss of funds.
As opposed to the other past issues mentioned in the escalation have a logical flaw in the code to cause loss of funds.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[ethereumdegen]

This is a non-issue because borrowers will be able to validate the default duration of a loan before agreeing to it. If they agree to a very short default duration, that is their choice. It does make sense to enforce a default duration longer than 1 payment cycle for ux purposes but i see that as an optional 'nice to have.' it will just be important to clearly communicate the default duration to borrowers.

[IAm0x52]

Sponsor has acknowledged this risk