Malicious lender can take over other lender's position

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/AccountingLibrary.sol#L81

Vulnerability details

Impact

Malicious lender can take over the position of another lender and possibly profit in case of profitable self liquidation

Proof of Concept

The size protocol allow user to perform variety of things, including lending, borrowing and also lending and borrowing at the same time, this is all achieved through the process of order book , where lenders can post there lending position either as a limit order (buyCreditLimit) or as a market order (buyCreditMarket) and vise versa for borrowers.

Maker

function buyCreditLimit(BuyCreditLimitParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateBuyCreditLimit(params);
        state.executeBuyCreditLimit(params);
    }

Taker

 function buyCreditMarket(BuyCreditMarketParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateBuyCreditMarket(params);
        uint256 amount = state.executeBuyCreditMarket(params);
        if (params.creditPositionId == RESERVED_ID) {
            state.validateUserIsNotBelowOpeningLimitBorrowCR(params.borrower);
        }
        state.validateVariablePoolHasEnoughLiquidity(amount);
    }

But there exist a bug that allow a lender to take over another lenders position without his intention

Consider this:

• bob, a lender and a borrower, initiated his borrow offer via the sellfCreditLimit waiting for a taker.
• alice, a borrower also initiated his borrow offer via the same func.
• bob, quickly fill this other and become the lender (of alice)
• now another lender james, notices this and quickly initiated a buyCreditMarket order taking over bob's position

malicious james can do this over and over again to make sure bob never have a stable position

This is due to :

creditPosition = CreditPosition({
            lender: lender,
            credit: debtPosition.futureValue,
            debtPositionId: debtPositionId,
            forSale: true
        });

where the createDebtAndCreditPosition was called with the forSale param set as true

And also, we take advantage of this part of code from the validateBuyCreditmarket function, which is related to the first borrow offer submitted by bob

BorrowOffer memory borrowOffer = state.data.users[borrower].borrowOffer;

this wouldn't have been possible had there been no borrow offer submitted by bob, the action would have revert due to this line of code 👇

// validate borrower
        if (borrowOffer.isNull()) {
            revert Errors.INVALID_BORROW_OFFER(borrower);
        }

Coded POC

paste it in BuyCreditMarket.t.sol

function test_BuyCreditMarket_buyCreditMarket_exactAmountIns() public {
        _deposit(alice, weth, 100e18);
        _deposit(bob, usdc, 100e6);
        // _deposit(candy, weth, 100e18);
        _deposit(james, usdc, 100e6);

        // bob's borrow order ( maker)
        _sellCreditLimit(bob, 0.03e18, 365 days);

        // alice's borrow order (maker)
        _sellCreditLimit(alice, 0.03e18, 365 days);

        uint256 amountIn = 10e6;
        uint256 tenor = 365 days;

        // bob fills alice's order and become lender
        _buyCreditMarket(bob, alice, amountIn, tenor, true);

        // malicious james now take over bob's position as the lender by specifying the creditPositionId of bob's order with alice
        _buyCreditMarket(
            james,
            address(0),
            57896044618658097711785492504343953926634992332820282019728792003956564819967,
            amountIn,
            tenor,
            true
        );

        // this is to see whos the lender after the takeOver
        size.getCreditPosition(57896044618658097711785492504343953926634992332820282019728792003956564819967);

        // compare it with james address ( assertEQ didn't seem to work :))
        vm.prank(james);
    }

Tools Used

Manual review

Foundry

Recommended Mitigation Steps

This can be handled better by allowing lenders to specify whether the position is forSale during its creation.

Assessed type

Context

[ABDuullahi]

Hello @hansfriese , great efforts put for the judging.

I believe this to be a dupp of this or a unique on its own(with its duplicate)

The issue here is talking about a position can be immediately taken over by another lender without the intention of the original lender.

As per the POC, its possible due to this line of code

BorrowOffer memory borrowOffer = state.data.users[borrower].borrowOffer;

where when the function gets call with a creditPositionId thats != to the RESERVED_ID, the borrower gets updated to the

borrower = creditPosition.lender;

where the creditPosition.lender == bob address, making the function call to pass, and of course , the forSale flag being true from the beginning of its creation without letting the lender specify whether its for sale or not.

i hope this helps, Thanks.

[hansfriese]

I believe this is neither a duplicate of that issue nor a valid Medium risk for the following reasons:

• The lender can disable the sale of his credit positions using a global forSale flag.
• The lender can use a multicall to disable the forSale flag of the credit position at the time of purchase.

[ABDuullahi]

Hello @hansfriese , thanks for taking time reviewing my comment.

Actually, i tried implementing that via the multiCall as u said, but its close to impossible, because, in order to call the setUserConfiguration that will allow u to set the forsale flag to false , you will need to put the creditPositionId

struct SetUserConfigurationParams {
    // The opening limit borrow CR
    uint256 openingLimitBorrowCR;
    // Whether all credit positions for sale are disabled
    bool allCreditPositionsForSaleDisabled;
    // Whether credit position IDs array are for sale
 >   bool creditPositionIdsForSale;
    // The credit position IDs array
 >   uint256[] creditPositionIds;
}

which can only be confirm/known after the call to the buyCreditMarket , meaning multiCall will not work for this.

I believe the must suitable solution is to allow lender to specify during the call to purchase the position thanks.

[hansfriese]

Thanks for your comment but I still stand by my decision.

[ABDuullahi]

Just to add more clarity here.

The function in question is not sellCreditMarket where the creditPositionId is known prior to its call.

The function in question is buyCreditMarket which when bob(as per the POC) calls will provide the Reserved_ID

struct BuyCreditMarketParams {
    // The borrower
    // If creditPositionId is not RESERVED_ID, this value is ignored and the owner of the existing credit is used
    address borrower;
    // The credit position ID to buy
    // If RESERVED_ID, a new credit position will be created
    uint256 creditPositionId;
    // The amount of credit to buy
    uint256 amount;
    // The tenor of the loan
    // If creditPositionId is not RESERVED_ID, this value is ignored and the tenor of the existing loan is used
    uint256 tenor;
    // The deadline for the transaction
    uint256 deadline;
    // The minimum APR for the loan
    uint256 minAPR;
    // Whether amount means cash or credit
    bool exactAmountIn;
}

as its creditPositionId specifying that bob is trying to create a new credit position. only then a new creditPositionId will be created here :

AccountingLibrary.Sol

85  uint256 creditPositionId = state.data.nextCreditPositionId++;
86        state.data.creditPositions[creditPositionId] = creditPosition;
87        state.validateMinimumCreditOpening(creditPosition.credit);
88       state.validateTenor(dueDate - block.timestamp);

and that will serve as the position id from now on.

which means during multiCall, i believe it will be impossible to also call the setUserConfiguration that will allow lender to set a forSale flag, since the creditPositionId is not even returned as a function argument during the size.buyCreditMarket call for it to be captured.

And as for the global allCreditPositionsForSaleDisabled , it would be a suicide for someone with many lending positions and want to sell them.

This should serve as a last comment, thanks.

[hansfriese]

Thanks for your comment. I agree that using a multicall may not be feasible, but there are other methods to prevent unintended selling for the honest user. According to the sponsor's feedback on other issues, it is intended behavior to use the forSale flag set to true by default. This would not be considered a valid risk unless the report demonstrates severe impacts that are unavoidable for the user.