drawDebt use inverted new LUP limit check, making user-specified LUP limit control void

Summary

BorrowerActions's drawDebt() LUP limit check is now inverted, nullifying LUP level control that limitIndex_ argument is designed to provide to a user.

Vulnerability Detail

ERC20Pool and ERC721Pool versions of drawDebt() allow a borrower to specify the limitIndex_ that should limit the resulting LUP in the terms of its index, which is positively correlated with the LUP price. Now this check is incorrectly applied, reverting good draws and allowing for bad ones, which increase the probability of user's position liquidation given that the user who supplied the limit and had the call succeeded assumes that the resulting debt is far from liquidation while it's otherwise.

LUP price being too low is a dangerous state for a borrower, as it increases the probability of liquidation and raises Neutral price level (as NP = ... * TP / LUP), making it more profitable to kick such a loan. So a borrower may naturally want to ensure that NewLUPIndex > limitIndex_, i.e. to control that LUP is not too low as a result of drawDebt() that pushes LUP lower by increasing the utilization.

Now the check is inverted, reverting the call when vars.lupId > limitIndex_, i.e. when it is the desirable situation for the borrower. More importantly, when vars.lupId <= limitIndex_ the call will not be reverted, so the LUP level control of the borrower is rendered void.

Impact

When a borrower specify limitIndex_ they will be sure that as the call succeeded the resulting LUP isn't dangerously low. As it is in fact inverted the borrower is closer to liquidation than desired, while the protocol has signalled otherwise by accepting the call with such a limit.

This increases probability of the liquidation, signalling otherwise, and makes way for the associated asset losses for a user.

Given this and the absence of low probability prerequisites as drawDebt() with limitIndex_ set is one of the base operations for the protocol, setting the severity to be high.

Code Snippet

drawDebt() incorrectly treats higher LUP and lupId as a dangerous thing for a borrower, while it is vice versa, i.e. the check to be if (vars.lupId < limitIndex_) instead of the current if (vars.lupId > limitIndex_) revert LimitIndexReached() as it is the low LUP that brings the risk of liquidation to the borrower:

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/libraries/external/BorrowerActions.sol#L161-L180

        // borrow against pledged collateral
        // check both values to enable an intentional 0 borrow loan call to update borrower's loan state
        if (amountToBorrow_ != 0 || limitIndex_ != 0) {
            // only intended recipient can borrow quote
            if (borrowerAddress_ != msg.sender) revert BorrowerNotSender();

            // add origination fee to the amount to borrow and add to borrower's debt
            vars.debtChange = Maths.wmul(amountToBorrow_, _feeRate(poolState_.rate) + Maths.WAD);

            vars.borrowerDebt += vars.debtChange;

            // check that drawing debt doesn't leave borrower debt under min debt amount
            _revertOnMinDebt(loans_, result_.poolDebt, vars.borrowerDebt, poolState_.quoteDustLimit);

            // add debt change to pool's debt
            result_.poolDebt += vars.debtChange;

            // determine new lup index and revert if borrow happens at a price higher than the specified limit (lower index than lup index)
            vars.lupId = _lupIndex(deposits_, result_.poolDebt);
            if (vars.lupId > limitIndex_) revert LimitIndexReached();

Also, limitIndex_ = 0 (or limitIndex_ = eps) should correspond to allowing any LUP price scenario (it's the analogue of the deliberate absense of the slippage check), but now zero limitIndex_ will lead to always reverting the drawDebt() call via if (vars.lupId > limitIndex_) revert LimitIndexReached().

limitIndex_ is a user defined argument of ERC20Pool's and ERC721Pool's drawDebt():

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC20Pool.sol#L125-L146

    function drawDebt(
        address borrowerAddress_,
        uint256 amountToBorrow_,
        uint256 limitIndex_,
        uint256 collateralToPledge_
    ) external nonReentrant {
        ...

        DrawDebtResult memory result = BorrowerActions.drawDebt(
            auctions,
            buckets,
            deposits,
            loans,
            poolState,
            borrowerAddress_,
            amountToBorrow_,
            limitIndex_,
            collateralToPledge_
        );

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC721Pool.sol#L134-L152

    function drawDebt(
        address borrowerAddress_,
        uint256 amountToBorrow_,
        uint256 limitIndex_,
        uint256[] calldata tokenIdsToPledge_
    ) external nonReentrant {
        ...

        DrawDebtResult memory result = BorrowerActions.drawDebt(
            auctions,
            buckets,
            deposits,
            loans,
            poolState,
            borrowerAddress_,
            amountToBorrow_,
            limitIndex_,
            Maths.wad(tokenIdsToPledge_.length)
        );

Tool used

Manual Review

Recommendation

Consider using the vars.lupId < limitIndex_ or vars.lupId <= limitIndex_ check, for example:

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/libraries/external/BorrowerActions.sol#L161-L180

        // borrow against pledged collateral
        // check both values to enable an intentional 0 borrow loan call to update borrower's loan state
        if (amountToBorrow_ != 0 || limitIndex_ != 0) {
            ...

            // determine new lup index and revert if borrow happens at a price higher than the specified limit (lower index than lup index)
            vars.lupId = _lupIndex(deposits_, result_.poolDebt);
-           if (vars.lupId > limitIndex_) revert LimitIndexReached();
+           if (vars.lupId <= limitIndex_) revert LimitIndexReached();

The limit usually is inclusive, but LimitIndexReached error imply otherwise, so some version of the approach to be chosen.

grandizzy commented 1 year ago

If lupId > limitIndex_, it means LUP has a lower price than the limit, that is borrower is stating "I do not want this borrow to succeed if it pushes LUP below this price." That implies a min price or a max index.

hrishibhat commented 1 year ago

Closing based on Sponsor comment

sherlock-audit / 2023-01-ajna-judging

hyh - drawDebt use inverted new LUP limit check, making user-specified LUP limit control void #158