Withdraw can be DoSed by an attacker sending debt tokens to user's address

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/RiskLibrary.sol#L146

Vulnerability details

Impact

withdraw can be blocked and users will not be able to withdraw their tokens from the protocol.

Proof of Concept

Users use withdraw method to withdraw their tokens incase they want to exit the protocol:

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

The issue occur in validateUserIsNotBelowOpeningLimitBorrowCR

let's take a look at the method:

function validateUserIsNotBelowOpeningLimitBorrowCR(State storage state, address account) external view {
        uint256 openingLimitBorrowCR = Math.max(
            state.riskConfig.crOpening, //crOpening: 1.5e18
            state.data.users[account].openingLimitBorrowCR // 0 by default, or user-defined if SetUserConfiguration has been used
        );
        if (collateralRatio(state, account) < openingLimitBorrowCR) {
            revert Errors.CR_BELOW_OPENING_LIMIT_BORROW_CR(
                account, collateralRatio(state, account), openingLimitBorrowCR
            );
        }
    }

You can see in above code there is a check for collateralRatio, as it should, the condition is :

if (collateralRatio(state, account) < openingLimitBorrowCR)

The returned value of collateralRatio should be greater than openingLimitBorrowCR to pass validateUserIsNotBelowOpeningLimitBorrowCR correctly, otherwise a revert is done.

Now let's take a look at collateralRatio method:

function collateralRatio(State storage state, address account) public view returns (uint256) {
        uint256 collateral = state.data.collateralToken.balanceOf(account);
        uint256 debt = state.data.debtToken.balanceOf(account);
        uint256 debtWad = Math.amountToWad(debt, state.data.underlyingBorrowToken.decimals());
        uint256 price = state.oracle.priceFeed.getPrice();

        if (debt != 0) { 
            return Math.mulDivDown(collateral, price, debtWad);
        } else {
            return type(uint256).max;
        } 

    }

The math that used in the method is correct which is:

Math.mulDivDown(collateral, price, debtWad)

but the issue is when debt variable is calculated by the balance of the user's account:

uint256 debt = state.data.debtToken.balanceOf(account);

This can be problematic, because an attacker can front-run the user's withdraw TX and send debtToken to the user's address.

What happens in this case is when the debtWad increase the result of the math will decrease, therefor the returned value of collateralRatio can get smaller and will be less than openingLimitBorrowCR.

let's take this simple scenario:

• Bob execute a TX to withdraw his 1e18 of WETH
• Alice observe the TX and front-run Bob by sending debtToken to his address.
• Bob's address debtToken increased.
• Bob's withdraw TX fails because validateUserIsNotBelowOpeningLimitBorrowCRv revert at collateralRatio condition.
• It revert since state.data.debtToken.balanceOf(account) increased, and debtWad (scaled debtToken) affected the collateralRatio math.
• collateralRatio will return smaller value since:

  return Math.mulDivDown(collateral, price, debtWad);

• Result: the more the debtWad increase the less value of collateralRatio is returned.

An attacker can send a value of debtToken to user's address to cause the collateralRatio of the account to be less than openingLimitBorrowCR and cause a revert, blocking the user from achieving the withdraw as code below

if (collateralRatio(state, account) < openingLimitBorrowCR) {
            revert Errors.CR_BELOW_OPENING_LIMIT_BORROW_CR(
                account, collateralRatio(state, account), openingLimitBorrowCR
            );

Tools Used

Manual review

Recommended Mitigation Steps

Try to rely on user debt as storage variable (e.g. hashmap usersDebtTokens), not on the user address.

Assessed type

DoS

[evokid]

Hey @hansfriese, could you please look at this issue. It's possible to manipulate collateralRatio, by sending debtToken to to user's address. please kindly look at the flow:

let's take this simple scenario:

Bob execute a TX to withdraw his 1e18 of WETH Alice observe the TX and front-run Bob by sending debtToken to his address. Bob's address debtToken increased. Bob's withdraw TX fails because validateUserIsNotBelowOpeningLimitBorrowCRv revert at collateralRatio condition. It revert since state.data.debtToken.balanceOf(account) increased, and debtWad (scaled debtToken) affected the collateralRatio math. collateralRatio will return smaller value since: return Math.mulDivDown(collateral, price, debtWad);

also the math that collateralRatio value relying on: Math.mulDivDown(collateral, price, debtWad);

[hansfriese]

Alice observe the TX and front-run Bob by sending debtToken to his address. - it's impossible. debtToken can be transferred by the owner(Size protocol) only.

[evokid]

What you saying is indeed correct, thanks for noticing that 👍