code-423n4 / 2024-06-size-validation

1 stars 0 forks source link

Overdue loans can't be partially repaid using `compensate` #438

Closed c4-bot-10 closed 4 months ago

c4-bot-10 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Compensate.sol#L51-L53

Vulnerability details

Impact

The protocol allows users to "split" their debt into smaller debt positions, so users can partially repay their loan, reducing the liquidation risk. This is done by calling Size::compensate, and passing creditPositionToCompensateId as RESERVED_ID. However, validateCompensate is checking if the loan is "active":

if (state.getLoanStatus(params.creditPositionWithDebtToRepayId) != LoanStatus.ACTIVE) {
    revert Errors.LOAN_NOT_ACTIVE(params.creditPositionWithDebtToRepayId);
}

This blocks users with overdue loans to compensate and partially repay their debts.

Users with overdue loans will be subject to full liquidation even if they were willing/able to repay part of their debt to "rescue" some of their collateral.

As a result, users will overdue loans will be vulnerable to full liquidation, without giving them the option to repay part of it.

Proof of Concept

function test_cant_compensate_overdue_loans() public {
    _deposit(bob, weth, 5e18);
    _deposit(alice, usdc, 1_000e6);

    int256[] memory aprs = new int256[](1);
    uint256[] memory tenors = new uint256[](1);
    uint256[] memory marketRateMultipliers = new uint256[](1);

    aprs[0] = 0.2e18;
    tenors[0] = 365 days;
    marketRateMultipliers[0] = 0;

    // Alice creates a limit order
    vm.prank(alice);
    size.buyCreditLimit(
        BuyCreditLimitParams({
            curveRelativeTime: YieldCurve({
                tenors: tenors,
                marketRateMultipliers: marketRateMultipliers,
                aprs: aprs
            }),
            maxDueDate: block.timestamp + 365 days
        })
    );

    // Bob sells credit to Alice (lends from Alice)
    vm.prank(bob);
    size.sellCreditMarket(
        SellCreditMarketParams({
            lender: alice,
            creditPositionId: type(uint256).max,
            tenor: 365 days,
            amount: 100e6,
            exactAmountIn: false,
            deadline: block.timestamp,
            maxAPR: type(uint256).max
        })
    );

    uint256 creditPositionId = type(uint256).max / 2;

    // Loan is overdue
    vm.warp(block.timestamp + 365 days + 1);

    // Bob can't compensate the loan, to partially repay the debt
    vm.prank(bob);
    vm.expectRevert(
        abi.encodeWithSelector(
            Errors.LOAN_NOT_ACTIVE.selector,
            creditPositionId
        )
    );
    size.compensate(
        CompensateParams({
            creditPositionWithDebtToRepayId: creditPositionId,
            creditPositionToCompensateId: type(uint256).max,
            amount: 50e6
        })
    );
}

Tools Used

Manual review

Recommended Mitigation Steps

For overdue loans, allow users to compensate their debt while passing creditPositionToCompensateId as RESERVED_ID, incentivizing users to repay their overdue loans partially.

Assessed type

Invalid Validation

aviggiano commented 3 months ago

@hansfriese response from the team

Response from the team:

so atm we do not support partial repayment in cash since accounting-wise it is not easy to track when N > 1 cashflows enter the protocol and therefore are deposited into Aave and start accruing the variable yield and account for them properly (it can be done but not in scope V1)

So the form of partial repayment we support does not involve a present cashflow but a future cashflow i.e. a credit, this means it can be done via compensate()

The compensate() allows to replace an existing credit with a new credit linked to a certain debt related to a borrower (skipping all the details since I believe everybody here is familiar with that)

The criteria for a credit credit1 to be eligible to compensate a given credit credit2 is that credit1.dueDate <= credit2.dueDate

Since in case the credit1.dueDate > credit2.dueDate would be allowed, then the compensator would be able to steal the time of value of money of the deltaT = credit1.dueDate - credit2.dueDate and could this continuously

Anyway, if the compensator only owns credit1.dueDate > credit2.dueDate he can still use the former to compensate the latter running this strategy: sell credit1.dueDate and get cash and use it, adding the difference if necessary, to buy a `credit11 such that credit11.dueDate <= credit2.dueDate

Now if the debt credit2 refers to is overdue then it means credit2.dueDate < block.timestamp which means it could only be compensated with other overdue credit but one of the eligibility criteria for a credit to be tradable is that it is not overdue so it won't be possible for the borrower to compensate an overdue loan this way.

Even in the case where RESERVED_ID is passed as creditPositionToCompensateId, he will have to specify a due date for the credit-debt pair he mints this way and this dueDate should be <= the overdue debt dueDate which is in the past, therefore it would be an overdue credit at creation time and so not tradeable, on top of this, if the borrower does that he could be liquidated immediately on this new overdue debt he creates

What is already designed, but decided not to be in scope for V1, so it will be for V2, is the partial repayment in cash with compensate() by chaining these already existing operations in the same TX

  • mint a new credit-debt pair with dueDate <= now and
  • repay immediately On top of this we will relax the eligibility condition for credit trading not to be overdue, meaning overdue credit will be tradeable, this means the above minted credit can be used to make partial repayment in cash and it will be a natural extension of the current accounting system (meaning no profound redesign) allowing for proper tracking of the variable rate yield So this will also cover naturally the case of partial repayment of overdue loans . As you can see it is already designed but we wanted to limit the scope of V1 a bit for security reasons since we already have quite a lot of mechanisms
0xAlix2 commented 3 months ago

Hello @hansfriese, thanks for your time.

Usually, liquidatable users would still have the ability to minimize their liquidation loss as much as possible usually through partial repayments. In this case, if a borrower has some overdue loan and has in cash 99% of the debt he won't be able to save his collateral (as the protocol only allows full debt repayment), knowing that he has the power (cash) to save part of it, forcing him to unjustly lose his collateral. On the other side, underwater users can partially repay their debt and save a part of their collateral. So why allow underwater users to save a part of the collateral and not users with overdue loans? Knowing that being underwater is more harmful to the protocol.

I believe users should be able to save as much collateral as possible and other than that incentivize them to raise their CR.

From the team's response

So this will also cover naturally the case of partial repayment of overdue loans. As you can see it is already designed but we wanted to limit the scope of V1 a bit for security reasons since we already have quite a lot of mechanisms

So the team sees this as an issue and plans to fix/introduce the mechanism in the V2 version of size.

As a result, I believe this is a valid issue.

Thanks!

hansfriese commented 3 months ago

I still believe it's an intended design of the current protocol, although the mechanism might change later. Additionally, the impact is low since the lender can compensate after liquidating the overdue loan first.