Flawed validation in short-term rental reservations allows overpayment

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-10-coded-estate/blob/main/contracts/codedestate/src/execute.rs#L860-L864

Vulnerability details

Impact

This bug allows renters to overpay for their short-term rental due to a flaw in the validation logic. The contract's price check does not prevent overpayment if the rental price changes between the time the renter sends the transaction and when it is executed. This can result in financial loss and an unintended overcharge for renters.

Description

The setreservationforshortterm function allows users to reserve a short-term rental by sending funds equal to or greater than the calculated rental price plus a protocol fee. However, there is a flaw in how the contract checks the deposited amount because the validation logic allows an overpayment.

In a scenario where the renter sends a transaction with the intended price, and the property owner also try to lower the price before the renter's transaction is executed, the renter ends up paying more than the new rental price.

This behavior is problematic because the renter's intent is to pay the price at the moment they send the transaction. If the price changes during the transaction's pending period, the contract should revert the transaction to avoid unintended overpayment.

Code Snippet:

pub fn setreservationforshortterm(
    // function arguments
) {
    ...
    ...snipped...
    ...

    let sent_amount = info.funds[0].amount;
    let fee_percentage = self.get_fee(deps.storage)?;
    let rent_amount = token.shortterm_rental.price_per_day
        * (new_checkout_timestamp - new_checkin_timestamp) / (86400);
    // @c4-contest: Allow overpayment
    if sent_amount < Uint128::from(rent_amount) + Uint128::new((u128::from(rent_amount) * u128::from(fee_percentage)) / 10000) {
        return Err(ContractError::InsufficientDeposit {});
    }
    // @c4-contest: Excess funds are recorded as protocol fee
    self.increase_balance(deps.storage, info.funds[0].denom.clone(), sent_amount - Uint128::from(rent_amount))?;
    ...
    ...snipped...
}

Example Scenario:

1. A renter initiates a transaction to reserve a rental property at $100 per day.
2. The property owner also send a transaction to reduces the rental price to $80 per day while the renter's transaction is pending.
3. The renter's transaction is delayed or stuck due to gas price fluctuations or other network conditions.
4. The renter’s transaction is executed, but they are still charged $100 (plus fees), causing an overpayment.
5. The contract does not reject the transaction, leading to an unintended financial loss for the renter.

Rationale for severity

The severity is set to Medium because:

• It can cause financial losses to users, but only in limited scenarios.
• The impact is restricted to the difference between the original and reduced price.

Proof-of-Concept

The following test demonstrate the described scenario.

Boilerplate for PoC: https://gist.github.com/nnez/c76b1a867dd8dc441dbe552e048b796e

Steps

1. Replace everything in contracts/codedestate/src/multi_tests.rs with boilerplate from above secret gist.
2. Insert below test:

#[test]
fn m1_reserve_the_same_block_as_update() {
    let (mut app, contract_addr) = mock_app_init_contract();
    
    // Minter mints a token
    execute_mint(&mut app, &contract_addr, MINTER, TOKEN_ID);
    // Asserts that token is minted
    assert_eq!(query_token_count(&app, &contract_addr.to_string()), 1);
    
    // Minter list a short-term rental, at a price of 1000 per day
    let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetListForShortTermRental { 
        token_id: TOKEN_ID.to_string(), 
        denom: USDC.to_string(), 
        price_per_day: 1000, 
        auto_approve: true, 
        available_period: vec![], 
        minimum_stay: 1, 
        cancellation: vec![]
    };
    let res = app.execute_contract(
        Addr::unchecked(MINTER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &[],
    );
    assert!(res.is_ok());

    advance_blocks(&mut app, 10);

    // Case = Minter reduces price on the same block as the renter's reservation 
    // and renter's transaction is somehow delayed due to network conditions,
    // so Minter's transaction is executed first
    
    // Minter reduces price to 10 per day (a bit extreme, but to prove a point)
    let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetListForShortTermRental { 
        token_id: TOKEN_ID.to_string(), 
        denom: USDC.to_string(), 
        price_per_day: 10, 
        auto_approve: true, 
        available_period: vec![], 
        minimum_stay: 1, 
        cancellation: vec![]
    };
    let res = app.execute_contract(
        Addr::unchecked(MINTER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &[],
    );
    assert!(res.is_ok());

    // Renter's transaction is executed after Minter's transaction
    // The reserve transaction expects the price of 1000 per day
    init_usdc_balance(&mut app, TRAVELER, 1000);
    let tmr = app.block_info().time.plus_days(1);
    let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetReservationForShortTerm { 
        token_id: TOKEN_ID.to_string(), 
        renting_period: vec![tmr.seconds().to_string(), tmr.plus_days(1).seconds().to_string()], 
        guests: 1
    };
    let res = app.execute_contract(
        Addr::unchecked(TRAVELER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &vec![Coin {
            denom: USDC.to_string(),
            amount: Uint128::new(1000),
        }],
    );
    assert!(res.is_ok());

    // Funds are deposited into the contract for 1000 but the excess payment of (1000-10) = 900 are considered as platform fee
    // In other words, Renter overpaid by 900
    assert_eq!(query_usdc_balance(&app, &contract_addr.to_string()), 1000);
    assert_eq!(query_fee_balance(&app, &contract_addr.to_string()), 990);
}

3. Run cargo test m1_reserve_the_same_block_as_update -- --nocapture
4. Observe that the test passes, indicating that the contract allows overpayment by renter, causing unncessary loss of funds of renter.

Recommended Mitigations

There are two available options:

• Only allow paying an exact amount needed
• Allow overpayment but send excess amount back to caller

Assessed type

Context

[c4-judge]

OpenCoreCH marked the issue as primary issue

[OpenCoreCH]

It is not really clear why this is a financial loss. The user is fine with paying the price at the time of tx submission and conceptually, it is changed later for them, even if their particular tx is executed after the price change submission. Sure, this situation can be annoying for a user. Similarly, it would be very annoying if the price is lowered directly after the tx was executed or 1 minute later on. If the design were changed to refund the user, you could make a very similar argument that this is a financial loss for the owner because this price reduction was not even advertised to the user.

Moreover, there is no incentive for the owner to frontrun a users transaction and reduce the price. So this could only happen by a very unlucky coincidence.

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[OpenCoreCH]

Grade A together with other QA reports of the user

[c4-judge]

OpenCoreCH marked the issue as grade-a