Sandwich attack on loan fulfillment will temporarily prevent users from accessing their borrowed funds

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The protocol does not remove borrowed token liquidity from the aave pool once a loan is initiated. It only checks if there is enough available liquidity via the validateVariablePoolHasEnoughLiquidity() function. A user is expected to withdraw their funds after the loan is created. This means that a malicios user can sandwich loans that exceed the available liquidity in aave by depositing liquidity in a frontrun transaction and removing it in a backrun transaction. This way a user that takes out a loan will not be able to remove their funds until there is enough available liquidity in the pool.

Also note that liqudity may become unavailable via other factors, like external users removing liquidity from aave.

Proof of Concept

The sellCreditMarket() function does not remove liquidity from aave, but only checks if there is enough liqudity available via a call to validateVariablePoolHasEnoughLiquidity().

https://github.com/code-423n4/2024-06-size/blob/main/src/Size.sol#L194

    function sellCreditMarket(SellCreditMarketParams memory params) external payable override(ISize) whenNotPaused {
        state.validateSellCreditMarket(params);
        uint256 amount = state.executeSellCreditMarket(params);
        if (params.creditPositionId == RESERVED_ID) {
            state.validateUserIsNotBelowOpeningLimitBorrowCR(msg.sender);
        }
        // @audit validate liquidity after execution??
        state.validateVariablePoolHasEnoughLiquidity(amount);
    }

Also, the executeSellCreditMarket() function transfers the size protocol accounting borrowAToken to the borrower, instead of the underlying token itself, leaving the user to withdraw their borrowed funds in a seprate transaction.

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

    function validateSellCreditMarket(State storage state, SellCreditMarketParams calldata params) external view {
        ...
        state.data.borrowAToken.transferFrom(params.lender, msg.sender, cashAmountOut);
        state.data.borrowAToken.transferFrom(params.lender, state.feeConfig.feeRecipient, fees);
    }

The validateVariablePoolHasEnoughLiquidity() function only checks the balance of the variable pool, and does not remove liquidity:

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/CapsLibrary.sol#L67

    function validateVariablePoolHasEnoughLiquidity(State storage state, uint256 amount) public view {
        uint256 liquidity = state.data.underlyingBorrowToken.balanceOf(address(state.data.variablePool));
        if (liquidity < amount) {
            revert Errors.NOT_ENOUGH_BORROW_ATOKEN_LIQUIDITY(liquidity, amount);
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Remove the liqudity from the pool when the loan is created and still let the user pull their funds in a separate transaction to remain compliant with pull over push pattern.

Assessed type

Other

[aviggiano]

We acknowledge this issue, but we understand that MEV attacks like frontrunning, sandwich attacks, etc., do not belong to the protocol layer. For example, the on-chain Uniswap logic can't do anything against sandwich attacks.

Also, this can be mitigated with a multicall.

I am not sure how Code4rena judges will weigh in on this issue, but since it was not explicitly stated on the list of Known Limitations, we have decided to make it more explicit in our documentation: https://github.com/SizeCredit/size-solidity/pull/135/files#diff-0373daa6ef71068fa3fa5a28763665721100171f5bd190af4a53dd1c926ed5d0

[hansfriese]

Will keep as Medium since it accurately highlights the irrationality of the current validation process.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report

[jorgect207]

Hey @hansfriese i think my issue [121] in the validation repo should be duplicate of this, or at least another findings apart of this one.

[jorgect207]

hey @hansfriese let some comments in the validation [121]