Lines of code

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

Vulnerability details

Bug Description

For simplicity I will be discussing the buyCreditMarket and sellCreditMarket functions in the context of simple lending and borrowing operations.

During buyCreditMarket calls, the lender (caller) is fulfilling a borrower's borrowOffer and the amount of assets pulled from the lender will be equal to the borrowed assets sent to the borrower and the fees, which are sent to the feeRecipient:

Size::buyCreditMarket

178:    function buyCreditMarket(BuyCreditMarketParams calldata params) external payable override(ISize) whenNotPaused {
179:        state.validateBuyCreditMarket(params);
180:        uint256 amount = state.executeBuyCreditMarket(params); // @audit: amount of assets pulled from lender 
181:        if (params.creditPositionId == RESERVED_ID) {
182:            state.validateUserIsNotBelowOpeningLimitBorrowCR(params.borrower);
183:        }
184:        state.validateVariablePoolHasEnoughLiquidity(amount); // @audit: checking liquidity against actual amount of assets pulled

BuyCreditMarket::executeBuyCreditMarket

121:    function executeBuyCreditMarket(State storage state, BuyCreditMarketParams memory params)
122:        external
123:        returns (uint256 cashAmountIn) // @audit: cashAmountIn is equal to total assets pulled from lender 
124:    {
...
195:        state.data.borrowAToken.transferFrom(msg.sender, borrower, cashAmountIn - fees); // @audit: assets, excluding fees, sent to borrower
196:        state.data.borrowAToken.transferFrom(msg.sender, state.feeConfig.feeRecipient, fees); // @audit: fees sent to feeRecipient

As shown above, the cashAmountIn value includes the borrowed assets and the fees. Therefore, cashAmountIn is equal to the amount of assets that were pulled from the lender. This value is then used on line 184 of Size.sol in order to check available liquidity. As long as there is at least cashAmountIn of available liquidity, the borrower and feeRecipient should be able to withdraw their received assets.

Conversely, during sellCreditMarket calls, the borrower (caller) is fulfilling a lender's loanOffer and the amount of assets pulled from the lender should also be equal to the borrowed assets and the fees. However, we will notice that the amount of pulled assets is greater than expected:

Size::sellCreditMarket

188:    function sellCreditMarket(SellCreditMarketParams memory params) external payable override(ISize) whenNotPaused {
189:        state.validateSellCreditMarket(params);
190:        uint256 amount = state.executeSellCreditMarket(params); // @audit: amount of assets pulled from lender, excluding extra fees amount pulled
191:        if (params.creditPositionId == RESERVED_ID) {
192:            state.validateUserIsNotBelowOpeningLimitBorrowCR(msg.sender);
193:        }
194:        state.validateVariablePoolHasEnoughLiquidity(amount); // @audit: checking liquidity against lesser value

SellCreditMarket::executeSellCreditMarket

127:    function executeSellCreditMarket(State storage state, SellCreditMarketParams calldata params)
128:        external
129:        returns (uint256 cashAmountOut) // @audit: cashAmountOut is less than total assets pulled from lender
130:    {
...    
201:        state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut); // @audit: assets, including fees, sent to borrower
202:        state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees); // @audit: fees sent to feeRecipient

As we can see above, cashAmountOut is sent to the borrower and the fees are sent to the feeRecipient. The assets sent to the borrower are including the fees amount, meaning an extra fees amount is being pulled from the lender. The cashAmountOut, which is less than the actual amount of assets pulled, is used to check for available liquidity. However, this means that sellCreditMarket is underestimating the available liquidity requirement and an edge case can arise in which the sellCreditMarket function call executes successfully, but there is not enough available liquidity for the borrower and the feeRecipient to withdraw all of their received assets.

Impact

Size overcharges lenders and overpays borrowers during sellCreditMarket calls. Lenders effectively pay the fee twice and borrowers receive more assets than expected (borrowed assets + fees). Another consequence of this issue is that there is an edge case in which Size underestimates the available liquidity requirement, resulting in the borrower or the feeRecipient being unable to withdraw their received assets immediately (only one party can withdraw all their received assets).

Proof of Concept

Place the following test inside of the test/ directory and run with forge test --mc POC_Test -vvv:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

import {BaseTest} from "@test/BaseTest.sol";
import {Vars} from "@test/BaseTest.sol";

import {YieldCurveHelper} from "@test/helpers/libraries/YieldCurveHelper.sol";

contract POC_Test is BaseTest {

    uint256 depositAmountCollateral = 100e18;
    uint256 depositAmountAssets = 100e6;
    uint256 tenor = 365 days;
    int256 rate = 0.03e18;
    uint256 cashAmount = 10e6;

    error ERC20InsufficientBalance(address, uint256, uint256);

    function test_BuyCreditMarket_exactAmountIn() public {
        // loan creation via Size::BuyCreditMarket
        _test_simple_loan_creation({viaBuyCreditMarket: true});
    }

    function test_SellCreditMarket_exactAmountOut() public {
        // loan creation via Size::SellCreditMarket
        _test_simple_loan_creation({viaBuyCreditMarket: false});
    }

    function _test_simple_loan_creation(bool viaBuyCreditMarket) internal {
        // alice is the borrower (selling credit)
        // bob is the lender (buying credit)

        _deposit(alice, weth, depositAmountCollateral); // borrower supplies collateral
        _deposit(bob, usdc, depositAmountAssets); // lender supplies assets

        // set liquidity equal to cashAmount (in/out) to mimic just enough available liquidity for operation to succeed
        deal(address(usdc), address(variablePool), cashAmount);

        Vars memory _before = _state();

        // loan creation
        if (viaBuyCreditMarket) {
            // alice creating borrow offer
            _sellCreditLimit(alice, rate, tenor);

            // bob fulfilling borrow offer
            _buyCreditMarket(bob, alice, cashAmount, tenor, true);
        } else {
            // bob creating loan offer
            _buyCreditLimit(bob, block.timestamp + tenor, YieldCurveHelper.pointCurve(tenor, rate));

            // alice fulfilling loan offer
            _sellCreditMarket(alice, bob, cashAmount, tenor, false);
        }

        Vars memory _after = _state();

        // total cash pulled from lender is borrowed assets for alice and fees for feeRecipient
        uint256 borrowedAssets = _after.alice.borrowATokenBalance - _before.alice.borrowATokenBalance;
        uint256 fees = _after.feeRecipient.borrowATokenBalance - _before.feeRecipient.borrowATokenBalance;

        if (viaBuyCreditMarket) {
            // total cash pulled from lender is equal to amount that lender (caller) specified
            assertEq(borrowedAssets + fees, cashAmount);

            // there is enough available liquidity for both borrower and feeRecipient to withdraw assets
            _withdraw(alice, usdc, borrowedAssets);

            _withdraw(feeRecipient, usdc, fees);
        } else {
            // total cash pulled from lender is more than the amount that borrower (caller) specified
            assertGt(borrowedAssets + fees, cashAmount);

            // there is not enough available liquidity for both borrower and feeRecpient to withdraw assets
            _withdraw(alice, usdc, borrowedAssets);

            vm.expectRevert(abi.encodeWithSelector(ERC20InsufficientBalance.selector, address(variablePool), 0, fees));
            _withdraw(feeRecipient, usdc, fees);
        }

        emit log_named_uint("borrowed assets", borrowedAssets);
        emit log_named_uint("fees", fees);
        emit log_named_uint("expected total cash pulled from lender", cashAmount);
        emit log_named_uint("actual total cash pulled from lender", borrowedAssets + fees);
    }
}

Output from logs:

[PASS] test_BuyCreditMarket_exactAmountIn() (gas: 1588555)
Logs:
  borrowed assets: 9950000
  fees: 50000
  expected total cash pulled from lender: 10000000
  actual total cash pulled from lender: 10000000

[PASS] test_SellCreditMarket_exactAmountOut() (gas: 1585613)
Logs:
  borrowed assets: 10000000
  fees: 50000
  expected total cash pulled from lender: 10000000
  actual total cash pulled from lender: 10050000

Tools Used

manual

Recommended Mitigation

Exclude the fees from the cashAmountOut of assets sent to the borrower during sellCreditMarket calls:

diff --git a/src/libraries/actions/SellCreditMarket.sol b/src/libraries/actions/SellCreditMarket.sol
index 4a95fb2..591661b 100644
--- a/src/libraries/actions/SellCreditMarket.sol
+++ b/src/libraries/actions/SellCreditMarket.sol
@@ -198,7 +198,8 @@ library SellCreditMarket {
             lender: params.lender,
             credit: creditAmountIn
         });
-        state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut);
+        state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut - fees);
         state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees);
     }
 }

Assessed type

Oracle

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

Size overcharges lender and overpays borrower during `sellCreditMarket` calls #204