depositforlongtermrental() does not validate the sent_amount

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

depositforlongtermrental() does not validate the sent_amount. It doesn't

• check if amount sent in is enough to pay for rent for the rental's minimum_stay.
• deduct the protocol fee percentage from the amount sent in. The protocol earns no fees from a setreservationforlongterm() + depositforlongtermrental() action.

Proof Of Concept

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

    pub fn depositforlongtermrental(
        &self,
        deps: DepsMut,
        info: MessageInfo,
        token_id: String,
        renting_period: Vec<String>,
    ) -> Result<Response<C>, ContractError> {
        let mut token = self.tokens.load(deps.storage, &token_id)?;

        let mut position: i32 = -1;
        let sent_amount = info.funds[0].amount;
        if info.funds[0].denom != token.longterm_rental.denom {
            return Err(ContractError::InvalidDeposit {});
        }

        if sent_amount > Uint128::new(0)
        {
            for (i, item) in token.rentals.iter().enumerate() {
                if item.address == Some(info.sender.clone()) && item.renting_period[0].to_string() == renting_period[0]
                && item.renting_period[1].to_string() == renting_period[1]
                {
                   position = i as i32;
                }
            }
    
            if position == -1 {
                return Err(ContractError::NotReserved {});
            }
    
            // let fee_percentage = self.get_fee(deps.storage)?;
    
            token.rentals[position as usize].deposit_amount += 
            Uint128::from(sent_amount); 
            //@audit no calculation for if the sent_amount is suffificent enough to cover the rent amount required for the renting period  
            //@audit contract recieves money but doesnt track it? it is commented out? how will the owner withdraw fee amount, why is fee percentage not collected here?
            // self.increase_balance(deps.storage, info.funds[0].denom.clone(), Uint128::new((u128::from(sent_amount) * u128::from(fee_percentage)) / 10000))?;
            self.tokens.save(deps.storage, &token_id, &token)?;
        }
        else {
            return Err(ContractError::InsufficientDeposit {});
        }



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

depositforlongtermrental() is meant to be called after setreservationforlongterm() by the tenant to make rent payment for the property to be rented. Each long term rental property's token struct has a token.longterm_rental.price_per_month value, this value is set by the token owner via setlistforlongtermrental(). This token.longterm_rental.price_per_month value in combination with the renting_period ought to be used to calculate/validate if the amount sent in by the user is enough to cover the rent for the renting period.

But this is not done here, this means the depositforlongtermrental() will allow a tenant to pay lesser than the minimum price to rent. info.funds[0].amount is not checked to be equal to or greater than token.longterm_rental.price_per_month * renting_period[1] - renting_period[0] / (86400*30). Where renting_period[1] is the checkOut, renting_period[0] is the checkIn and 86400*30 represents a month in timestamp.

Also, per logic in setreservationforshortterm(), the protocol collects fees from the traveller anytime a reservation is made. But since setreservationforlongterm() handles no money and passes that responsibilty on to depositforlongtermrental(), depositforlongtermrental takes money but doesnt deduct or check that the amount sent in contains the rent + protocol fees charged for each reservation. There is no math for that.

Recommened Mitigation

• check that the sent in amount is sufficient for protocol fees for reservation + rent
• check that the amount for rent is sufficient enough for the landlord as specified by the landlord in the token.longterm_rental.price_per_month value.

Assessed type

Context

[c4-judge]

OpenCoreCH marked the issue as primary issue

[OpenCoreCH]

This seems to be intended in the current design. A call to depositforlongtermrental increases the deposit amount for a particular rental. Because it does not trigger any additional state changes (e.g., mark the rent as paid), there is no assumption about the amount in there. It is also not really clear what the benefit of the proposed mitigation would be. This would prevent some scenarios (e.g. payment of 50% in two installations), while not really improving the security (a user could still perform no deposits and never call the function, so there are no additional guarantees by that).

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory:
Insufficient proof

[blockchainstar12]

This seems to be intended in the current design. A call to depositforlongtermrental increases the deposit amount for a particular rental. Because it does not trigger any additional state changes (e.g., mark the rent as paid), there is no assumption about the amount in there. It is also not really clear what the benefit of the proposed mitigation would be. This would prevent some scenarios (e.g. payment of 50% in two installations), while not really improving the security (a user could still perform no deposits and never call the function, so there are no additional guarantees by that).

You take the point!