Incorrect cashAmount check for variable pool ability to fulfill withdrawal

[howlbot-integration]

Lines of code

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

Vulnerability details

Impact

the validateVariablePoolHasEnoughLiquidity function was meant as a check to know if the variable pool balance of the borrow token exceeds the amount of the cash being received by the receiver. The issue here is that in the Size:buyCreditMarket the parameter being passed is the mount returned from the executeBuyCreditMarket but amount is actually the combination of the amount to be received + the fees for the receiver. Which is the wrong implementation as the amount received is the only one to be checked, if this implementation is used it will cause the buyCreditMarket to revert even though the protocol has enough to pay the cash receiver.

Proof of Concept

Inside the executeBuyCreditMarket function as shown below the amount being returned is the cashAmount + fees which is later deducted before transfer

inside the function cashAmountIn is the amount being transferred, in both exactAmountIn situation

if (params.exactAmountIn) {
            cashAmountIn = params.amount;
            (creditAmountOut, fees) = state.getCreditAmountOut({
                cashAmountIn: cashAmountIn,
                maxCashAmountIn: params.creditPositionId == RESERVED_ID
                    ? cashAmountIn
                    : Math.mulDivUp(creditPosition.credit, PERCENT, PERCENT + ratePerTenor),
                maxCredit: params.creditPositionId == RESERVED_ID
                    ? Math.mulDivDown(cashAmountIn, PERCENT + ratePerTenor, PERCENT)
                    : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        } else {
            creditAmountOut = params.amount;
            (cashAmountIn, fees) = state.getCashAmountIn({
                creditAmountOut: creditAmountOut,
                maxCredit: params.creditPositionId == RESERVED_ID ? creditAmountOut : creditPosition.credit,
                ratePerTenor: ratePerTenor,
                tenor: tenor
            });
        }

Then the amount is subtracted before transfer shown in the snippet below

state.data.borrowAToken.transferFrom(msg.sender, borrower, cashAmountIn - fees);
        state.data.borrowAToken.transferFrom(msg.sender, state.feeConfig.feeRecipient, fees);

But the amount being used being for the validateVariablePoolHasEnoughLiquidity call is the cashAmountIn which will lead to a DOS even though the variable pool has enough for the fee receiver.

Note: There is an inconsistent implementation of the same type of function with different logic from the protocol for example

• sellCreditMarket::executeSellCreditMarket only checks if the amount of cash being received is present in the variable pool

state.validateVariablePoolHasEnoughLiquidity(amount);

• Size::liqudateWithReplacement also only checks if the amount being transferred is present in the variable pool

state.validateVariablePoolHasEnoughLiquidity(amount);

One of this logics is wrong and the protocol should act to fix the logic

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor the code to fit with the already established logic for checking if the variable pool can pay the amount the receiver is getting or a new system can be developed by the protocol.

Assessed type

Other

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by hansfriese

[c4-judge]

hansfriese marked the issue as duplicate of #15

[c4-judge]

hansfriese marked the issue as satisfactory