code-423n4 / 2024-06-size-validation

1 stars 0 forks source link

A malicious borrower user can cause loss of funds for lender #446

Closed c4-bot-3 closed 4 months ago

c4-bot-3 commented 4 months ago

Lines of code

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

Vulnerability details

Impact

Lenders are being charged fees when borrowers fill up a lending order, thus malicious borrowers can spam borrow transactions to lenders causing fees being charged multple times from the lenders when they're basically not doing anything thus lenders incurring losses.

Proof of Concept

The protocol operates on a basis that lenders/borrowers can fill up borrowing/lending orders. The vulnerability occurs when tokens are being transferred from lender to borrower when a borrower fills an order and its completed (link is pasted above)

state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut); state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees);

As we can see the cashAmountOut (amount to be borrowed) being sent from the lender to the borrower(msg.sender), then fees are still sent from the lender to the fees reciepent.

I tweaked a function in the test file SellCreditMarket.t.sol so just run this test function there ` function test_consolefees() public { _deposit(alice, usdc, 200000e6); _deposit(bob, weth, 200e18); _deposit(candy, weth, 200e18); _deposit(james, weth, 200e18); _buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(365 days, 0.03e18)); Vars memory test1 = _state(); console.log("bob 1st eth balance after borrow order ", test1.bob.collateralTokenBalance); console.log("bob 1st usdc balance after borrow order ", test1.bob.borrowATokenBalance); console.log("james 1st usdc balance after borrow order ", test1.james.borrowATokenBalance); console.log("candy 1st usdc balance after borrow order ", test1.candy.borrowATokenBalance); console.log("alice 1st usdc balance after borrow order", test1.alice.borrowATokenBalance);

    uint256 amount = 50000e6;
    uint256 tenor = 365 days;

    uint256 futureValue = Math.mulDivUp(amount, (PERCENT + 0.03e18), PERCENT);
    uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
  Vars memory test2 = _state();
   console.log("bob 2nd eth balance",test2.bob.collateralTokenBalance);
     console.log("bob 2nd usdc balance",test2.bob.borrowATokenBalance);
    console.log("alice 2nd usdc balance",test2.alice.borrowATokenBalance);
    console.log("feereciepentbalance", test2.feeRecipient.borrowATokenBalance);
      _sellCreditMarket(james, alice, RESERVED_ID, amount, tenor, false);

Vars memory test3 = _state(); console.log("james usdc balance",test3.james.borrowATokenBalance); console.log("alice 3rd usdc balance",test3.alice.borrowATokenBalance); console.log("feereciepentbalance", test3.feeRecipient.borrowATokenBalance);

       _sellCreditMarket(candy, alice, RESERVED_ID, amount, tenor, false);

Vars memory test4 = _state(); console.log("candy usdc balance",test4.candy.borrowATokenBalance); console.log("alice 4th usdc balance",test4.alice.borrowATokenBalance); console.log("feereciepentbalance", test4.feeRecipient.borrowATokenBalance);`

Apologise if the above test function is messy, as we can see for every borrow transaction called, fees are taken from the lender(alice) and sent to the feerecipient instead of the person calling it, thus her position can be spammed out and I don't know how high the developers might set the fees so this loss can be very high and the likelihood of this happening is high.

If this is a design choice it was not stated and it should be added to the docs.

Tools Used

Manual review Foundry

Recommended Mitigation Steps

- state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees); + state.data.borrowAToken.transferFrom(msg.sender, state.feeConfig.feeRecipient, fees);

Assessed type

Token-Transfer

Topmark1 commented 3 months ago

No comment is left to understand why this was not accepted, I believe this Issue is a valid issue and it also show POCs to prove this, would implore Judge to please have a look thanks

hansfriese commented 3 months ago

Low spamming and incorrect perceptions