withdraw() users may can't withdraw underlyingBorrowToken properly

[c4-bot-9]

Lines of code

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

Vulnerability details

Vulnerability details

We can withdraw underlyingCollateralToken and underlyingBorrowToken by withdraw()

    function withdraw(WithdrawParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateWithdraw(params);
        state.executeWithdraw(params);
@>      state.validateUserIsNotBelowOpeningLimitBorrowCR(msg.sender);
    }

    function executeWithdraw(State storage state, WithdrawParams calldata params) public {
        uint256 amount;
        if (params.token == address(state.data.underlyingBorrowToken)) {
            amount = Math.min(params.amount, state.data.borrowAToken.balanceOf(msg.sender));
            if (amount > 0) {
@>              state.withdrawUnderlyingTokenFromVariablePool(msg.sender, params.to, amount);
            }
        } else {
            amount = Math.min(params.amount, state.data.collateralToken.balanceOf(msg.sender));
            if (amount > 0) {
                state.withdrawUnderlyingCollateralToken(msg.sender, params.to, amount);
            }
        }

        emit Events.Withdraw(params.token, params.to, amount);
    }

From the code above we know that whether we take underlyingCollateralToken or underlyingBorrowToken
will check validateUserIsNotBelowOpeningLimitBorrowCR() ==> collateralRatio() > openingLimitBorrowCR.

This makes sense for taking underlyingCollateralToken, but not for taking underlyingBorrowToken.

Because.

1. taking the underlyingBorrowToken does not affect the collateralRatio.
2. the user has already borrowed the funds (with interest accrued and collateralized), it is the user's asset, and should be able to be withdrawn at will, even if it may be liquidated.
3. openingLimitBorrowCR is still far from being liquidated, and should not restrict the user from withdrawing the borrowed token.

Impact

If the token is already borrowed, just stored in Size and not yet taken, but due to a slight price fluctuation, validateUserIsNotBelowOpeningLimitBorrowCR() fails but is still far from being liquidated, the user may not be able to take the borrowed token.
resulting in the possibility that the user may not be able to withdraw the borrowed funds

Recommended Mitigation

    function withdraw(WithdrawParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateWithdraw(params);
        state.executeWithdraw(params);
+       if (params.token != address(state.data.underlyingBorrowToken)  {
+             state.validateUserIsNotBelowOpeningLimitBorrowCR(msg.sender);
+       }
    }

Assessed type

Context

[aviggiano]

This is a valid bug and the report is good

Fixed in SizeCredit/size-solidity#124

[hansfriese]

Nice catch!

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as primary issue

[c4-judge]

hansfriese marked the issue as selected for report

[coffiasd]

code-423n4/2024-06-size-validation#248. I think this submission describe the same issue @hansfriese

[hansfriese]

@coffiasd
Thanks for your update.
I am checking the validation repo right now and I see some other duplicates also.

[samuraii77]

Hi, this issue should be of high severity. The likelihood is very high, imagine a user borrows $1000 and gives $1500 as collateral, his collateral ratio is now 1.5e18. If the price of ETH drops by just 1 cent (or even less), his borrowed funds will be locked. The impact is also high as well.

[hansfriese]

I still believe Medium is appropriate for the following reasons:

• The user can withdraw the borrowed funds at the time of borrowing using a multicall.
• The user can still use the borrowed funds to repay their loan.
• There is no fund loss; the funds are simply locked for a while.