Attakers can steal the funds from long-term reservation

[c4-bot-10]

Lines of code

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

Vulnerability details

Description

In this protocol NFT owner can set the NFT in sale even if it is still under active rent by triggering execute.rs#setlistforsell() which could set token.sell.auto_approve to a true value (means anyone can directly be approved and this will open multiple doors for attackers)

Users can call execute.rs#setbidtobuy() and send the necessary amount to gain approval of this NFT

File: execute.rs#setbidtobuy()

675:             if token.sell.auto_approve {
676:                 // update the approval list (remove any for the same spender before adding)
677:                 let expires = Expiration::Never {  };
678:                 token.approvals.retain(|apr| apr.spender != info.sender);
679:                 let approval = Approval {
680:                     spender: info.sender.clone(),
681:                     expires,
682:                 };
683:                 token.approvals.push(approval);
684:                 
685:             }

Using the same function setbidtobuy() any address that has an existing bid in the NFT can cancel its bid and receive back all the initial funds (no fees in this function).

On the other side, the owner or any approved address can invoke execute.rs#withdrawtolandlord() and specify the receiver of the withdrawal funds (this function gives the homeowners the ability to withdraw a part of the funds even before the rent end, this is only for longterm rentals )

File: execute.rs

1787:     pub fn withdrawtolandlord(
/**CODE**/
1796:         address:String
1797:     ) -> Result<Response<C>, ContractError> {
/**CODE**/
1848:             .add_message(BankMsg::Send {
1849:                 to_address: address,
1850:                 amount: vec![Coin {
1851:                     denom: token.longterm_rental.denom,
1852:                     amount: Uint128::from(amount) - Uint128::new((u128::from(amount) * u128::from(fee_percentage)) / 10000),

However, the Attacker can create a sophisticated attack using withdrawtolandlord() and setbidtobuy()
1- Choose an NFT that has a token.sell.auto_approve == true and an active long-term rental
2- Call setbidtobuy() this will give him the necessary approval to finish the attack, he also need to transfer the asked funds .
3- Trigger withdrawtolandlord() and transfer the maximum amount of tokens

File: execute.rs#withdrawtolandlord()

1832:                 if item.deposit_amount - Uint128::from(token.longterm_rental.price_per_month) < Uint128::from(amount)  {
1833:                     return Err(ContractError::UnavailableAmount {  });
1834:                 }

4- Invoke setbidtobuy() to receive his original deposited funds.

Impact

Steal the funds from long-term reservations using setbidtobuy()

Tools Used

Manual Review

Recommended Mitigation Steps

File: execute.rs
1787:     pub fn withdrawtolandlord(
1788:         &self,
1789:         deps: DepsMut,
1790:         env: Env,
1791:         info: MessageInfo,
1792:         token_id: String,
1793:         tenant: String,
1794:         renting_period: Vec<String>,
1795:         amount:u64,
1796:         address:String
1797:     ) -> Result<Response<C>, ContractError> {
1798:         let mut token = self.tokens.load(deps.storage, &token_id)?;
1799: 
-1800:         self.check_can_send(deps.as_ref(), &env, &info, &token)?;
+1800:         self.check_can_approve(deps.as_ref(), &env, &info, &token)?;

Assessed type

Invalid Validation

[OpenCoreCH]

Same attack pattern / underlying issue as #6 / #12

[c4-judge]

OpenCoreCH marked the issue as duplicate of #12

[c4-judge]

OpenCoreCH marked the issue as satisfactory

[nnez]

Hey there @OpenCoreCH
I missed this one. Should it be deduped from both #12 and #6 ?

I think the root cause here is the incorrect use of check_can_send where it should be check_can_approve instead.

[OpenCoreCH]

Hey there @OpenCoreCH I missed this one. Should it be deduped from both #12 and #6 ?

I think the root cause here is the incorrect use of check_can_send where it should be check_can_approve instead.

Good point thanks, first thought about duplicating it with #6 now because both are about auto approvals and cancelling later on. But the fix for #6 would not help here, the cancellation happens after the withdraw call. So even if approvals were removed, this attack would still work.

[c4-judge]

OpenCoreCH marked the issue as not a duplicate

[c4-judge]

OpenCoreCH marked the issue as selected for report

[c4-sponsor]

@blockchainstar12 Sponsors are not allowed to close, reopen, or assign issues or pull requests.

[blockchainstar12]

We remove setting approvals from setbidtobuy() function