malicious rental owner can rob bidders of the currency amounts sent in via older bids.

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

malicious rental owner can rob bidders of the currency amounts sent in via older bids.

Proof Of Concept

• user adds a bid via setbidtobuy() with token.sell.denom for currency A
• in the middle of bidding, the token owner calls setlistforsell() and changes token.sell.denom to currency B
• user attempts to add another bid via setbidtobuy() with token.sell.denom as currency B
• because user had a previously acvtive bid before, the logic runs a for loop to iterate through previous bids and find the old bid.
  https://github.com/code-423n4/2024-10-coded-estate/blob/97efb35fd3734676f33598e6dff70119e41c7032/contracts/codedestate/src/execute.rs#L657-L664

       for (i, item) in token.bids.iter().enumerate() {
           if item.address == info.sender.to_string()
           {
               position = i as i32;
               amount = item.offer.into();
               break;
           }
       }

this old bid is found and position is set to the index value. This means the if poisition == -1 if block does not execute/is skipped but the else block which deletes the older bid is executed

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

        else {
            // update the approval list (remove any for the same spender before adding)
            token.bids.retain(|item| item.address != info.sender);
        }

This is then saved into the token storage and now because position is not equal to -1, money is attempted to be sent back to the bidder as refund for its previous bid. See below

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

        if position != -1 && (amount > Uint128::from(0u64)) {
            Ok(Response::new()
            .add_attribute("action", "setbidtobuy")
            .add_attribute("sender", info.sender.clone())
            .add_attribute("token_id", token_id)
            .add_message(BankMsg::Send {
                to_address: info.sender.to_string(),
                amount: vec![Coin {
                    denom: token.sell.denom,
                    amount: amount,
                }],
            }))
        }

This send action will send the currency amount for older bid while the denom is currency B. Like instead of sending back 10 units of currency A for example, it will send back 10 units of currency B. Even though both currencies may not be of the same value!

This can be exploited by a malicious rental owner to accept bids, change currency and rob bidders of their value used in the older bids.

Recommened Mitigation

ensure that the refund is being sent with the correct currency that the old bid was made with, and not the new recently changed to currency.

Assessed type

Context

[c4-judge]

OpenCoreCH marked the issue as duplicate of #5

[c4-judge]

OpenCoreCH changed the severity to 3 (High Risk)

[c4-judge]

OpenCoreCH marked the issue as satisfactory