finalizelongtermrental() refunds money to the token owner/ property manager instead of the tenant

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-10-coded-estate/blob/97efb35fd3734676f33598e6dff70119e41c7032/contracts/codedestate/src/execute.rs#L1728

Vulnerability details

Impact

finalizelongtermrental() sends money to the token owner/ property manager instead of the tenant when the rental reservation has been cancelled.

Proof Of Concept

https://github.com/code-423n4/2024-10-coded-estate/blob/97efb35fd3734676f33598e6dff70119e41c7032/contracts/codedestate/src/execute.rs#L1727-L1731

      for (i, item) in token.rentals.iter().enumerate() {
            if item.address == Some(Addr::unchecked(tenant.clone()))
                && item.renting_period[0].to_string() == renting_period[0]
                && item.renting_period[1].to_string() == renting_period[1]
            {
                position = i as i32;
                amount = item.deposit_amount;

                if item.cancelled {
                    target = token.owner.to_string();
                    let fee_percentage = self.get_fee(deps.storage)?;
                    self.increase_balance(deps.storage, token.longterm_rental.denom.clone(), Uint128::new((u128::from(amount) * u128::from(fee_percentage)) / 10000))?;

                    amount -= Uint128::new((u128::from(amount) * u128::from(fee_percentage)) / 10000);
                }

                    
                    ....

        if amount > Uint128::new(0) {
            Ok(Response::new()
                .add_attribute("action", "finalizelongtermrental")
                .add_attribute("sender", info.sender)
                .add_attribute("token_id", token_id)
                .add_message(BankMsg::Send {
                    to_address: target.clone(),
                    amount: vec![Coin {
                        denom: token.longterm_rental.denom,
                        amount: amount,
                    }],
                }))            
        } 

finalizelongtermrental() iterates through the token.rentals array to find a specific rental. If this rental was cancelled by the tenant or token owner, the deposit_amount which is the rent should be refunded back to the tenant, the address which provided the rent amount/made reservation in the first place. But in the logic above, if item.cancelled == true, target is set to the token owner, the token owner is the manager/landlord of the rental property. This means that even if a reservation is cancelled, the deposit_amount still goes to the token owner. item.cancelled is set to true in cancelreservationafterapprovalforlongterm().

So if tenant makes reservation via setreservationforlongterm() and then proceeds to cancel after approval via cancelreservationafterapprovalforlongterm(), the deposit_amount which can be rent paid will not be refunded to the tenant when finalizelongtermrental() is called. It will go to the token owner.

The correct logic should be that if reservation item is marked cancelled, deposit amount should be refunded to tenant. If not marked cancelled then the deposit amount can go to token owner.

Reservation is cancelled, no value is provided but the no refund for the tenant who payed rent.

Recommened Mitigation

if item.cancelled == true make the target to be the tenant or item.address

Assessed type

Context

[OpenCoreCH]

Similar issue as #26 (wrong handling of cancelled reservations when finalizing), but somewhat different causes. Leaving them unduped for now

[c4-judge]

OpenCoreCH marked the issue as primary issue

[blockchainstar12]

This is intended logic

[OpenCoreCH]

For #26 and this, the underlying issue is that the cancellation vector of long term rentals is ignored.

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)

[c4-judge]

OpenCoreCH marked the issue as duplicate of #26

[c4-judge]

OpenCoreCH marked the issue as satisfactory