Old borrowOffer lingering can affect all future newly acquired credit positions, leading to unintended transactions

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/SellCreditLimit.sol#L45 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/BuyCreditMarket.sol#L83-L88 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/LiquidateWithReplacement.sol#L71-L74

Vulnerability details

Impact

To put it in a nutshell, this issue will lead to unintended/unauthorized transactions on the credit positions someone may have just acquired. Let me give some examples:

Scenario #1:

1. Alice wants to borrow some money so she puts some deposits and calls sellCreditLimit with her desired terms of borrow offer.
2. Bob is Okay with Alice's terms and he lends Alice some money.
3. Then, somehow Bob no longer wants this creditPosition so he puts up a limit sell order with his desired terms, to sell the position to someone else.
4. James is Okay with Bob's terms, so he took over this creditPosition from Bob for the entire amount.
5. Now, Bob should have no business with Alice any more. Sometime later, a new person Candy wants to borrow some money, so she puts some deposits and calls sellCreditLimit with her desired terms.
6. Again, Bob is Okay with Candy's terms and he lends Candy some money
7. Now, Bob does not intend to sell this newly acquired credit position. However, anybody else can come to buy Bob's new creditPositionId again immediately after Bob acquired it, even though it's against Bob's will. This is because the very old borrow offer which Bob was meant to use on the Alice position is still associated with Bob's account and keeps impacting all newly acquired credit positions in the future.

Proof of Concept

Insert the test function below into the test/local/actions/BuyCreditMarket.t.sol and run it, it is the POC of the Scenario #1 I described above.

    function test_BuyCreditMarket_buyCreditMarket_unauthorizedsale() public {
        _deposit(alice, weth, 100e18);
        _deposit(alice, usdc, 100e6);
        _deposit(bob, weth, 100e18);
        _deposit(bob, usdc, 100e6);
        _deposit(james, weth, 1600e18);
        _deposit(james, usdc, 1000e6);
        _deposit(candy, weth, 1600e18);
        _deposit(candy, usdc, 1000e6);
        uint256 rate = 0.03e18;

        // Alice wants to borrow some money
        _sellCreditLimit(alice, int256(rate), 365 days);

        uint256 issuanceValue = 10e6;
        uint256 futureValue = Math.mulDivUp(issuanceValue, PERCENT + rate, PERCENT);
        uint256 tenor = 365 days;
        uint256 amountIn = Math.mulDivUp(futureValue, PERCENT, PERCENT + rate);

        // Bob is Okay with Alice's terms and he lends Alice some money
        uint256 debtPositionId1 = _buyCreditMarket(bob, alice, futureValue, tenor);
        console.log("Debt# : ", debtPositionId1);
        uint256 creditPositionId1 = size.getCreditPositionIdsByDebtPositionId(debtPositionId1)[0];
        console.log("Credit# : ", creditPositionId1);

        // Now Bob no longer wants this creditPosition so he put up a limit sell order with his desired terms
        _sellCreditLimit(bob, int256(rate), 365 days);

        // James is Okay with Bob's terms, so he took over this creditPosition from Bob
        uint256 debtPositionId2 = _buyCreditMarket(james, bob, creditPositionId1,futureValue, tenor, false);
        console.log("Debt# : ", debtPositionId2);
        uint256 creditPositionId2 = size.getCreditPositionIdsByDebtPositionId(debtPositionId2)[0];
        console.log("Credit# : ", creditPositionId2);

        // Now Bob should have no business with Alice any more.
        // Sometime later, a new person Candy wants to borrow some money
        _sellCreditLimit(candy, int256(rate), 365 days);

        // Bob is Okay with Candy's terms and he lends Candy some money
        uint256 debtPositionId3 = _buyCreditMarket(bob, candy, futureValue, tenor);
        console.log("Debt# : ", debtPositionId3);
        uint256 creditPositionId3 = size.getCreditPositionIdsByDebtPositionId(debtPositionId3)[0];
        console.log("Credit# : ", creditPositionId3);

        // Now, Bob does not intend to sell this newly acquired credit position creditPositionId3, 
        // so he doesn't sellCreditLimit again; however, the previous old YieldCurve terms are still associated with Bob's name
        // so, anybody else can come to buy Bob's new creditPositionId3 again immediately after Bob acquired it, even though it's against Bob's will
        uint256 debtPositionId4 = _buyCreditMarket(james, bob, creditPositionId3,futureValue, tenor, false);
        console.log("Debt# : ", debtPositionId4);
        uint256 creditPositionId4 = size.getCreditPositionIdsByDebtPositionId(debtPositionId4)[0];
        console.log("Credit# : ", creditPositionId4);

    }

The test passed, which means indeed Bob's newly acquired credit positions can be immediately taken away by someone else, albeit Bob may not want to sell this one at all.

Tools Used

Manual review.

Recommended Mitigation Steps

There could be several possible mitigations.

The most straight forward one would be, instead of linking the borrow offer terms with only someone's account, change it to be linked to someone's account + credit position, so that some terms which were meant for an old position shall not impact future new positions.

One more thing worth to note, DO NOT link the terms with only credit positions but ignored the account, because that can lead to another critical bug. For example, when James acquired the entire credit position from Bob, it won't create a new position, instead, it will just change the old position's lender from Bob to James. So, if the borrow terms is linked to credit position only, then James could again immediately lose his position to another arbitrary lender although James has no intentions to sell it.

Another mitigation could be that, when a new credit position is created, do not put forSale: true by default as it is currently in the code. Make forSale: false can help prevent the unintended transactions happening. However, this requires many other pieces of logic to change accordingly, so it's not the most straight forward mitigation.

Assessed type

Invalid Validation

[jes16jupyter]

Hi, I would raise this issue as it will lead to unintended/unauthorized transactions.

[hansfriese]

Intended design. The borrower can mitigate this by disabling the global forSale flag or the position-specific forSale flag at the time of purchase using a multicall.