`Size.buyCreditMarket(BuyCreditMarketParams)` race condition can force many transaction to revert in normal circumstances

[c4-bot-4]

Lines of code

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

Vulnerability details

Description

The way lenders buy credit on market is specifying the amount of credit to buy, the tenor and the interest rate. However, this function can be frontrunned by a any other lender. If the sum of both lenders amount is greater than the credit offer, the second lender will revert the transaction.

This is an issue because the amount of the first lender may not fulfill the credit offer, forcing the second lender transaction to revert. This could happen again in normal cirmustances, by a third lender, and so on, leading to many honest lender transaction to revert as well.

Impact

Race condition of Size.buyCreditMarket(BuyCreditMarketParams) can force many honest transactions to revert in normal circumstances

POC

For next example ignore fees and interest rates.

Alice has a collateral of 1 ETH valued at 4,000 USDC, she wants to take a loan of 1,000 USDC for 1 month. She put a credit offer on the market with this conditions.

Bob, Eve, and Charlie are lenders, Bob has 500 USDC, Eve 700 USDC and Charlie 400 USDC willing to lend.

1. The three lenders calls Size.buyCreditMarket(BuyCreditMarketParams) specifying that they want to lend their USDC to Alice, Eve transaction goes first, then Bob's and Charlie's. Bob and Charlie transactions will revert because after Eve partially fulfill the credit offer, the remaining credit offer is just 300 USDC.
2. Now, also Lenny is willing to lend 100 USDC to Alice, so he also call Size.buyCreditMarket(BuyCreditMarketParams), Bob and Charlie do the same with 300 USDC. Lenny's transaction goes first, then Charlie and Bob's transaction revert again because credit offer remaining is not enough to fulfill their amount.
3. Finally Bob and Charlie insist with lending 200 USDC, only Bob transaction goes through, Charlie's transaction revert again because the credit offer is already fulfilled.

The reason why this happen is because Size.buyCreditMarket(BuyCreditMarketParams) assumes the function caller is smart enough to specify and amount lower or equal to the remaining credit offer, but it does not consider the implicit race condition of this function.

Coded POC

```solidity // Add to BuyCreditMarket.t.sol function test_BuyCreditMarket_buyCreditMarket_allow_partial_fulfill() public { address eve = address(uint160(uint256(keccak256("EVE")))); vm.label(eve, "EVE"); _setPrice(4000e18); // 1 ETH = 4000 USDC uint256[] memory tenors = new uint256[](1); uint256[] memory marketRateMultipliers = new uint256[](1); int256[] memory aprs = new int256[](1); tenors[0] = 30 days; marketRateMultipliers[0] = 1e18; aprs[0] = 0.1e18; SellCreditLimitParams memory sellCreditLimitParams = SellCreditLimitParams({ curveRelativeTime: YieldCurve({ tenors: tenors, marketRateMultipliers: marketRateMultipliers, aprs: aprs }) }); _deposit(alice, weth, 1e18); _sellCreditLimit(alice, sellCreditLimitParams.curveRelativeTime); uint256[] memory creditPositionIds = new uint256[](0); _setUserConfiguration( alice, 4e18, true, false, creditPositionIds ); _deposit(bob, usdc, 500e6); _deposit(eve, usdc, 700e6); _updateConfig("variablePoolBorrowRateStaleRateInterval", 1 hours); _setVariablePoolBorrowRate(0.1e18); // Eve buys credit from Alice _buyCreditMarket( eve, alice, RESERVED_ID, 700e6, 30 days, false ); vm.expectRevert(); // Erase this line once code has been modified _buyCreditMarket( bob, alice, RESERVED_ID, 500e6, 30 days, false ); // Add check to ensure min between remaing credit and bob amount has been fulfilled for a more robust test } ```

Recommended mitigation

If the amount of the lender is greater than the remaining credit offer, update the amount to lend to the remaining credit offer. In this way, the amount to lend acts like a slippage. Also, consider that the remaining amount should always be greater/equal state.riskConfig.minimumCreditBorrowAToken to prevent create small loan without enough incentive to be liquidated.

Assessed type

Other

[carlitox477]

Firstly, I would like to express my disagreement with label insufficient quality report given the provided coded POC

Secondly, this is a real issue that can affect normal behaviour of protocol users, enforcing paying gas for transactions that will finally reverting (consider that error is triggered after uint256 amount = state.executeBuyCreditMarket(params);, which execute many opcodes).

Thirdly, another way to use this bug to harm protocol users is coding a bot that front-run every attempt to fulfil a credit offer by just buying the minimum, forcing the second transaction to revert. Although it could be argue that this is gas intense, and that it would not harm users in more than the gas for reverting transaction, if this event happen constantly the use of buyCreditMarket will be discouraged (why a user would use a function that always reverts?). If this function is simply not used, then there is no reason to sell credit with a limit order (no one would fulfil it). This means, that this bug opens a vector attack that can be used to simply discourage the use of the protocol by anyone. The attack can be performed by an enough wealthy actor.

This bug is also present in sellCreditMarket and was reported a separate issue given that it is another part of the code, tbh I don't know if this should be considered as a separate issue for this reason or not, leaving the judge the final decision.

[hansfriese]

Front-running is an anticipated behavior on the order book, and the severity is considered low as long as the user does not lose significant funds, apart from gas fees.