BuyCreditMarket: Missing Fee Deduction from Buyer Payment (exactAmountIn=true)

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/BuyCreditMarket.sol#L195-L196 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/actions/SellCreditMarket.sol#L201-L203

Vulnerability details

Impact

1. SellCreditMarket applies the swap fee consistently

• When calculating cashAmountOut in both exactAmountIn and exactAmountOut scenarios, it considers the swap fee.
• It explicitly subtracts the calculated fees from the cashAmountOut transferred to the seller.

2. BuyCreditMarket lacks explicit fee handling for the buyer

• While it calculates fees in both exactAmountIn and exactAmountOut scenarios, it doesn't explicitly deduct these fees from the buyer's provided funds.
• The buyer sends cashAmountIn regardless of the fee, potentially overpaying.

Proof of Concept

1. Overpayment by Buyers // src\libraries\actions\BuyCreditMarket.sol

function executeBuyCreditMarket(...) external returns (uint256 cashAmountIn) {
    // ... (calculations for cashAmountIn, creditAmountOut, fees)

    // Funds transferred WITHOUT deducting fees:
    state.data.borrowAToken.transferFrom(msg.sender, borrower, cashAmountIn - fees); 
    state.data.borrowAToken.transferFrom(msg.sender, state.feeConfig.feeRecipient, fees);
}

• Example:

  • Alice wants to buy 100 USDC worth of credit.
  • The calculated swap fee is 1 USDC.
  • Alice approves the protocol to spend 100 USDC.
  • Due to the bug, Alice pays the full 100 USDC, but only receives 99 USDC worth of credit. The remaining 1 USDC is correctly allocated as a fee.

• Impact: Buyers are consistently overpaying for credit, eroding their returns and potentially discouraging participation in the protocol.

2. Inconsistent Fee Distribution // src\libraries\actions\SellCreditMarket.sol

function executeSellCreditMarket(...) external returns (uint256 cashAmountOut) {
    // ... (calculations for cashAmountOut, creditAmountIn, fees)

    // Fees deducted BEFORE transferring to the seller:
    state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut); 
    state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees);
}

Comparison:

• Sellers have their fees deducted before receiving their payout, ensuring they bear the cost.
• Buyers, on the other hand, pay the full amount upfront, even though a portion is allocated as a fee.

• Impact: This inconsistency creates an imbalance in the fee structure, unfairly disadvantaging buyers and potentially distorting the protocol's intended economics.

3. Further Considerations:

• Severity: The impact depends on the magnitude of the swap fee. A small fee might have a negligible effect, while a larger fee could significantly impact user returns.
• User Experience: Overpaying without clear indication could lead to confusion and frustration among buyers, potentially harming the protocol's reputation.

Initial

• Fund both Alice and Bob with sufficient borrow tokens.
• Set up a loan offer from Bob and a borrow offer from Alice.

Test Case 1: Buyer Overpayment in BuyCreditMarket

function testBuyCreditMarketOverpayment() public {
    // 1. Record initial balances
    uint256 aliceBalanceBefore = size.data.borrowAToken.balanceOf(alice);

    // 2. Define buy parameters with a non-zero swap fee
    uint256 creditAmount = 100e6; // 100 USDC
    uint256 tenor = 30 days;
    // Assuming a swap fee that results in 1 USDC fee for this example
    uint256 expectedFee = 1e6; 

    // 3. Execute BuyCreditMarket
    size.buyCreditMarket(
        BuyCreditMarketParams({
            borrower: alice,
            creditPositionId: RESERVED_ID,
            amount: creditAmount,
            tenor: tenor,
            deadline: block.timestamp + 1 hours,
            minAPR: 0,
            exactAmountIn: true 
        })
    );

    // 4. Calculate expected final balance
    uint256 aliceBalanceExpected = aliceBalanceBefore - creditAmount; 

    // 5. Assert: Alice's balance should be reduced by the full creditAmount, NOT creditAmount - fee
    assertEq(size.data.borrowAToken.balanceOf(alice), aliceBalanceExpected, "Buyer overpaid!");
} 

Test Case 2: Correct Fee Deduction in SellCreditMarket

function testSellCreditMarketFeeDeduction() public {
    // 1. Record initial balances
    uint256 bobBalanceBefore = size.data.borrowAToken.balanceOf(bob);

    // 2. Define sell parameters (use same creditAmount and tenor as before)
    // ...

    // 3. Execute SellCreditMarket
    size.sellCreditMarket(
        // ... sell parameters
    );

    // 4. Calculate expected final balance (fee deducted)
    uint256 bobBalanceExpected = bobBalanceBefore + creditAmount - expectedFee;

    // 5. Assert: Bob's balance reflects the fee deduction
    assertEq(size.data.borrowAToken.balanceOf(bob), bobBalanceExpected, "Fee not deducted correctly!");
}

Expected Results & Analysis

• Test Case 1: If the bug exists, this test case will fail, indicating that the buyer's balance was not reduced by the fee amount.
• Test Case 2: This test case should pass, demonstrating that the SellCreditMarket function correctly deducts fees.

Tools Used

Vs

Recommended Mitigation Steps

If the omission is unintentional, modify the BuyCreditMarket function to deduct the calculated fees from the cashAmountIn before transferring funds.

Assessed type

Error

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope

[c4-judge]

hansfriese marked the issue as not a duplicate

[hansfriese]

Invalid

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid