howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/SellCreditMarket.sol#L127-L204

Vulnerability details

Impact

When the borrower accepts the loan offer and uses the sellCreditMarket function, the apr is calculated in the validateSellCreditMarket function and if the calculated apr is greater than the borrower's acceptable apr, the function will revert to protect the borrower.

uint256 apr = loanOffer.getAPRByTenor(
            VariablePoolBorrowRateParams({
                variablePoolBorrowRate: state.oracle.variablePoolBorrowRate,
                variablePoolBorrowRateUpdatedAt: state.oracle.variablePoolBorrowRateUpdatedAt,
                variablePoolBorrowRateStaleRateInterval: state.oracle.variablePoolBorrowRateStaleRateInterval
            }),
            tenor
        );
        if (apr > params.maxAPR) {
            revert Errors.APR_GREATER_THAN_MAX_APR(apr, params.maxAPR);
        }

The problem occurs when the lender wants to sell his credit position to another lender. When the lender sells his credit position to another lender, the current borrowing rate can be different than before, and this can harm borrowers because the APR validation won't protect the borrower because the lender can use any params.maxAPR.

Proof of Concept

Added marketRateMultipliers to the YieldCurveHelper.PointCurve() to see the effects of the borrowing rate.

function pointCurve(
        uint256 m1,
        int256 r1
    ) public pure returns (YieldCurve memory) {
        uint256[] memory tenors = new uint256[](1);
        int256[] memory aprs = new int256[](1);
        uint256[] memory marketRateMultipliers = new uint256[](1);
        marketRateMultipliers[0] = 1e18; //@audit !!

        aprs[0] = r1;

        tenors[0] = m1;

        return
            YieldCurve({
                tenors: tenors,
                aprs: aprs,
                marketRateMultipliers: marketRateMultipliers
            });
    }

SellCreditMarket.t.sol

function test_SellCreditMarket_testAPR() public {
        _deposit(alice, weth, 100e18);
        _deposit(alice, usdc, 100e6);
        _deposit(bob, weth, 100e18);
        _deposit(bob, usdc, 100e6);
        _deposit(candy, weth, 100e18);
        _deposit(candy, usdc, 100e6);
        _buyCreditLimit(
            alice,
            block.timestamp + 12 days,
            YieldCurveHelper.pointCurve(12 days, 0.03e18)
        );
        _buyCreditLimit(
            candy,
            block.timestamp + 12 days,
            YieldCurveHelper.pointCurve(12 days, 0.03e18)
        );

        _updateConfig("variablePoolBorrowRateStaleRateInterval", 1);
        uint256 debtPositionId = _sellCreditMarket(
            bob,
            alice,
            RESERVED_ID,
            60e6,
            12 days,
            false
        );

        uint256 creditPositionId = size.getCreditPositionIdsByDebtPositionId(
            debtPositionId
        )[0];
        _setVariablePoolBorrowRate(0.5e18);

        _sellCreditMarket(alice, candy, creditPositionId, 30e6, 12 days);
    }

When Bob borrows from Alice, Bob's ratePerTenor is equal to 9.863e14. (I emitted an event in the executeSellCreditMarket function to see the ratePerTenor)

emit SellCreditMarket(lender: alice: [0x0000000000000000000000000000000000010000], creditPositionId: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77], tenor: 1036800 [1.036e6], amount: 60000000 [6e7], dueDate: 1036800 [1.036e6], exactAmountIn: false)
    │   │   │   ├─ [4039] YieldCurveLibrary::2b8d122a(00000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000fd200000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000e0000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000fd2000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000006a94d74f43000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000de0b6b3a7640000) [delegatecall]
    │   │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000006a94d74f430000
    │   │   │   ├─ emit testAPR(: 986301369863013 [9.863e14]) //@audit Bobs first APR !!

Alice decides to sell his credit position to Candy, but Bob's APR will be higher than his first borrow due to the borrow rate change.

emit SellCreditMarket(lender: candy: [0x0000000000000000000000000000000000030000], creditPositionId: 57896044618658097711785492504343953926634992332820282019728792003956564819967 [5.789e76], tenor: 1036800 [1.036e6], amount: 30000000 [3e7], dueDate: 1036800 [1.036e6], exactAmountIn: true)
    │   │   │   ├─ [1528] LoanLibrary::e6541090(00000000000000000000000000000000000000000000000000000000000000007fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) [delegatecall]
    │   │   │   │   └─ ← 0xf4803e074bd026baaf6ed2e288c9515f68c72fb7216eebdd7cae1718a53ec375
    │   │   │   ├─ [646] LoanLibrary::3978289f(00000000000000000000000000000000000000000000000000000000000000007fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) [delegatecall]
    │   │   │   │   └─ ← 0xaa95a8fd52bb407cd8a970e5893e7e19b3e1e249cf8337d708031b145d4588f1
    │   │   │   ├─ [4039] YieldCurveLibrary::2b8d122a(00000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000006f05b59d3b200000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000fd200000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000e0000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000fd2000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000006a94d74f43000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000de0b6b3a7640000) [delegatecall]
    │   │   │   │   └─ ← 0x000000000000000000000000000000000000000000000000075af03122f50000
    │   │   │   ├─ emit testAPR(: 17424657534246575 [1.742e16]) //@audit Bobs new APR !!

Tools Used

Manual Review

Recommended Mitigation Steps

When a lender sells his credit position, the new position's APR shouldn't be greater than the previous one, so the borrower won't be in a bad position.

Assessed type

Invalid Validation

aviggiano commented 4 months ago

This is incorrect. maxAPR can be set by the taker to protect against MEV

hansfriese commented 4 months ago

The new position's APR has no relation to the original borrower. It's just to calculate a cash amount from the position's credit. (That cash should be charged from the new lender)

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Insufficient proof