Insufficient validation before state change

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

This code is intended to allow users to buy credit on the market. However, there is a logic vulnerability in the code. The main problem of the vulnerability lies in the order of checking the validation conditions and executing the state changes. Specifically, the state change operation executeBuyCreditMarket is executed before the last two validation operations validateUserIsNotBelowOpeningLimitBorrowCR and validateVariablePoolHasEnoughLiquidity.

Proof of Concept

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

    function buyCreditMarket(BuyCreditMarketParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateBuyCreditMarket(params);
        uint256 amount = state.executeBuyCreditMarket(params);
        if (params.creditPositionId == RESERVED_ID) {
            state.validateUserIsNotBelowOpeningLimitBorrowCR(params.borrower);
        }
        state.validateVariablePoolHasEnoughLiquidity(amount);
    }

The vulnerability in this code is that the operation of buying the credit market (state.executeBuyCreditMarket(params)) is performed before all necessary validations are completed. This may lead to unexpected behavior and state changes if the last two validations fail.

1. The attacker first prepares a normal transaction, setting himself as params.borrower and setting params.creditPositionId to RESERVED_ID.
2. The attacker then sends this transaction to the contract by calling the contract's buyCreditMarket function.
3. The validateBuyCreditMarket(params) function is first called to validate the parameters, which pass the validation.
4. The next call to executeBuyCreditMarket(params) makes the relevant state changes and changes required to buy credit.
5. After that, the validateUserIsNotBelowOpeningLimitBorrowCR(params.borrower) function is called. If this function fails and throws an error due to a condition related to borrower (such as the borrower exceeds a certain preset borrowing threshold), the execution will stop.
6. However, at this point the state change via executeBuyCreditMarket(params) has already occurred without checking the borrower's credit limit or whether the contract has sufficient liquidity. Therefore, the attacker's credit limit has been successfully purchased even though their borrowing ratio is illegal.
7. As a result, the attacker could abuse the borrower parameter to exploit this vulnerability, ignoring borrowing limits and deceiving the contract.

   Tools Used

   Manual review

   Recommended Mitigation Steps

   The executeBuyCreditMarket(params) function that performs state-changing operations should only be executed after all necessary validation checks have been successfully completed. This modification ensures that all preconditions are met before any state-changing transactions and operations are executed. Therefore, the order of function calls will be adjusted as follows:

    function buyCreditMarket(BuyCreditMarketParams calldata params) external payable override(ISize) whenNotPaused {
        state.validateBuyCreditMarket(params);
    if (params.creditPositionId == RESERVED_ID) {
        state.validateUserIsNotBelowOpeningLimitBorrowCR(params.borrower);
    }
        uint256 amount = state.getBuyCreditMarketAmount(params);
        state.validateVariablePoolHasEnoughLiquidity(amount);
        state.executeBuyCreditMarket(params);
}

Assessed type

Invalid Validation